-
Notifications
You must be signed in to change notification settings - Fork 29.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: skip test that cannot pass under --node-builtin-modules-path #42834
test: skip test that cannot pass under --node-builtin-modules-path #42834
Conversation
@nodejs/testing |
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.
This test has also been failing for me locally, and I think skipping it make sense. Could it be a macOS specific limitation? I don't remember seeing this error when I was compiling on a Linux machine. If that's the case, we may want to add os.platform()==='darwin'
in the condition.
It fails in Linux too. I triggered a CI build for
That’s the only failing test. |
Commit Queue failed- Loading data for nodejs/node/pull/42834 ✔ Done loading data for nodejs/node/pull/42834 ----------------------------------- PR info ------------------------------------ Title test: skip test that cannot pass under --node-builtin-modules-path (#42834) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch GeoffreyBooth:builtin-modules-path-test -> nodejs:master Labels test, flaky-test, author ready, worker Commits 1 - test: skip test that cannot pass under --node-builtin-modules-path Committers 1 - Geoffrey Booth PR-URL: https://github.com/nodejs/node/pull/42834 Reviewed-By: Antoine du Hamel ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/42834 Reviewed-By: Antoine du Hamel -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 23 Apr 2022 04:42:56 GMT ✔ Approvals: 1 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/42834#pullrequestreview-950951284 ✖ This PR needs to wait 107 more hours to land (or 0 hours if there is one more approval) ✔ Last GitHub CI successful ℹ Last Full PR CI on 2022-04-24T00:45:56Z: https://ci.nodejs.org/job/node-test-pull-request/43657/ - Querying data for job/node-test-pull-request/43657/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/2221820838 |
The collaborator guide section “Waiting for approvals” says “Before landing pull requests, allow 48 hours for input from other collaborators.” Searching elsewhere in the document I see “At least two collaborators must approve a pull request before the pull request lands. One collaborator approval is enough if the pull request has been open for more than seven days.” So I guess that’s where the commit queue bot’s rules come from. I didn’t remember this second rule; it would make sense to mention it under “Waiting for approvals.” cc @Trott @aduh95 @targos |
Landed in f54bf28 |
PR-URL: #42834 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
PR-URL: #42834 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
PR-URL: #42834 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
PR-URL: #42834 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
PR-URL: #42834 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
PR-URL: nodejs/node#42834 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Fixes #40879.
The test
test-worker-init-failure.js
cannot pass when Node is built using--node-builtin-modules-path
, because the test intentionally lowers the limit of the number of files that can be concurrently opened and using builtin modules blows past this limit. See error in #40879 (comment).The test is already skipped in Windows environments. This PR makes it also get skipped when Node is built using
--node-builtin-modules-path
. This makes development easier, as now I can develop using--node-builtin-modules-path
and run the tests and expect all of them to pass (unless my new code broke something). cc @HarshithaKP @addaleax @Trott