GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test_runner: remove redundant bootstrap boolean by cjihrig · Pull Request #54013 · nodejs/node

Conversation

@cjihrig

@cjihrig cjihrig commented Jul 24, 2024

Copy link
Copy Markdown
Contributor

The test runner bootstrap process awaits a Promise and then sets a boolean flag. This commit consolidates the Promise and boolean into a single value. This commit also ensures that the globalRoot test is always assigned in createTestTree() in order to better consolidate the CLI/run() and non-CLI configuration.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss and mdrobny reacted with rocket emoji
All reactions
  • 🚀 2 reactions
@cjihrig
cjihrig requested review from MoLow and atlowChemi July 24, 2024 04:09
@nodejs-github-bot

nodejs-github-bot commented Jul 24, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Jul 24, 2024
atlowChemi
atlowChemi approved these changes Jul 24, 2024
View reviewed changes
anonrig
anonrig approved these changes Jul 24, 2024
View reviewed changes
MoLow
MoLow approved these changes Jul 24, 2024
View reviewed changes

@MoLow MoLow left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jul 24, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jul 24, 2024
@nodejs-github-bot

nodejs-github-bot commented Jul 24, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60583/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jul 24, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60589/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig
cjihrig force-pushed the bootstrap branch from b9a7184 to e8b2ca0 Compare July 24, 2024 13:52
@cjihrig cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jul 24, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jul 24, 2024
@nodejs-github-bot

nodejs-github-bot commented Jul 24, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60591/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jul 24, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jul 24, 2024
@nodejs-github-bot

nodejs-github-bot commented Jul 24, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60593/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig cjihrig added the commit-queue-squash PRs the Commit Queue should land as one squashed commit. label Jul 24, 2024
cjihrig added 3 commits July 25, 2024 09:47
@cjihrig
test_runner: remove redundant bootstrap boolean
8d6cd53 
The test runner bootstrap process awaits a Promise and then sets
a boolean flag. This commit consolidates the Promise and boolean
into a single value. This commit also ensures that the globalRoot
test is always assigned in createTestTree() in order to better
consolidate the CLI/run() and non-CLI configuration.
@cjihrig
hmm
2d775db
@cjihrig
ugh
383ec7f
@cjihrig
cjihrig force-pushed the bootstrap branch from db2a04c to 383ec7f Compare July 25, 2024 14:03
@nodejs-github-bot

nodejs-github-bot commented Jul 25, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60623/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig
Revert "ugh"
7ac0989 
This reverts commit 383ec7f.
@nodejs-github-bot

nodejs-github-bot commented Jul 25, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60626/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Jul 25, 2024

Copy link
Copy Markdown
Contributor Author

I've been trying to figure out how this seemingly very innocent change was introducing EPERM failures on Windows when trying to clean up the tmp directory. Then I saw the same failures in an unrelated CI run. So, I'm going to land this once I get a green CI.

It looks like the flaky test in question was introduced in #53866 (cc @mcollina).

Related to the EPERM issues - I only tried one run, but using fsPromises.rm() to perform the cleanup worked. I tried this after @richardlau called out the recent changes to rmSync(). Those changes in #53617 seem to have dropped code related to special handling of EPERM on Windows. That might be worth following up on.
MoLow reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MoLow
MoLow approved these changes Jul 25, 2024
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Jul 25, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60631/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig cjihrig added author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue PRs queued for automated landing through the Commit Queue. and removed needs-ci PRs that need a full CI run. labels Jul 26, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Jul 26, 2024
@nodejs-github-bot

nodejs-github-bot commented Jul 26, 2024

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/54013
✔  Done loading data for nodejs/node/pull/54013
----------------------------------- PR info ------------------------------------
Title      test_runner: remove redundant bootstrap boolean (#54013)
Author     Colin Ihrig <[email protected]> (@cjihrig)
Branch     cjihrig:bootstrap -> nodejs:main
Labels     author ready, commit-queue-squash, test_runner
Commits    4
 - test_runner: remove redundant bootstrap boolean
 - hmm
 - ugh
 - Revert "ugh"
Committers 1
 - cjihrig <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/54013
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54013
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 24 Jul 2024 04:09:00 GMT
   ✔  Approvals: 3
   ✔  - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/54013#pullrequestreview-2195620030
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/54013#pullrequestreview-2196305917
   ✔  - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/54013#pullrequestreview-2200309148
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2024-07-25T22:32:06Z: https://ci.nodejs.org/job/node-test-pull-request/60631/
- Querying data for job/node-test-pull-request/60631/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/10105569552
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig approved these changes Jul 26, 2024
View reviewed changes
@mcollina

mcollina commented Jul 26, 2024

Copy link
Copy Markdown
Member

@cjihrig this commit cannot land atm because GHA did not run for this.
cjihrig reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jul 26, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60648/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jakecastelli

jakecastelli commented Jul 27, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Green CI + 3 approvals, is this PR ready to land?

because GHA did not run for this.

What is GHA? I tried to find it in glossary but not there yet 😄

edit: Oh is it Github Action?
cjihrig and atlowChemi reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig cjihrig added commit-queue PRs queued for automated landing through the Commit Queue. and removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. labels Jul 27, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jul 27, 2024
@nodejs-github-bot
nodejs-github-bot merged commit 14e7eb3 into nodejs:main Jul 27, 2024
@nodejs-github-bot

nodejs-github-bot commented Jul 27, 2024

Copy link
Copy Markdown
Collaborator

Landed in 14e7eb3
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig
cjihrig deleted the bootstrap branch July 27, 2024 13:45
targos pushed a commit that referenced this pull request Jul 28, 2024
@cjihrig @targos
test_runner: remove redundant bootstrap boolean
bbd1ee0 
The test runner bootstrap process awaits a Promise and then sets
a boolean flag. This commit consolidates the Promise and boolean
into a single value. This commit also ensures that the globalRoot
test is always assigned in createTestTree() in order to better
consolidate the CLI/run() and non-CLI configuration.

PR-URL: #54013
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jul 30, 2024
v22.6.0 proposal #54123
Merged
RafaelGSS pushed a commit that referenced this pull request Aug 5, 2024
@cjihrig @RafaelGSS
test_runner: remove redundant bootstrap boolean
3fb97a9 
The test runner bootstrap process awaits a Promise and then sets
a boolean flag. This commit consolidates the Promise and boolean
into a single value. This commit also ensures that the globalRoot
test is always assigned in createTestTree() in order to better
consolidate the CLI/run() and non-CLI configuration.

PR-URL: #54013
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@targos targos added the dont-land-on-v20.x label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonrig anonrig anonrig approved these changes
@MoLow MoLow MoLow approved these changes
@atlowChemi atlowChemi atlowChemi approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. test_runner Issues and PRs related to the test runner subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@cjihrig @nodejs-github-bot @mcollina @jakecastelli @anonrig @MoLow @atlowChemi @targos
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.