-
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: make test-async-wrap-getasyncid parallelizable #18245
Conversation
stress test: https://ci.nodejs.org/job/node-stress-single-test/1720/ (expect to fail until #18241 lands, but should fail with the flake instead of EACCES) |
(Ah, wait, the title is actually misleading, it should be "make test-async-wrap-getasyncid runnable by itself" but that's probably too long.. |
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. Optional idea: Perhaps it makes sense to put it in the block with common.PIPE
so they're a unit in case anyone ever refactors the file into multiple files or something like that?
The stress test is actually green. I tried it on my local alpine box and I think the flake cannot be reproduced by simply stressing this test... |
Oh, I tried a different configuration and this time the flake shows up: https://ci.nodejs.org/job/node-stress-single-test/1721/nodes=alpine34-container-x64/ |
bf543f8
to
97561c9
Compare
Went with @Trott 's suggestion and put the CI: https://ci.nodejs.org/job/node-test-pull-request/12617/ |
@nodejs/collaborators Does anyone oppose having this fast-tracked? I would like to land this relatively soon and then run the stress test against #18241 |
+1 to fast-tracking. |
Thanks everyone. Landed in bfe41fe |
PR-URL: #18245 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #18245 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #18245 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #18245 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: nodejs#18245 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Previously this test cannot be run with the node-stress-single-test job because the tmp dir is not created before
common.PIPE
is listened. See https://ci.nodejs.org/job/node-stress-single-test/1712/nodes=alpine34-container-x64/consoleChecklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
test