-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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: remove error message checking in test-worker-init-failure #34727
Conversation
@@ -35,7 +35,6 @@ if (process.argv[2] === 'child') { | |||
// (i.e. single cpu) `ulimit` may not lead to such an error. | |||
|
|||
worker.on('error', (e) => { | |||
assert.match(e.message, /EMFILE/); | |||
assert.ok(e.code === 'ERR_WORKER_INIT_FAILED' || e.code === 'EMFILE'); |
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.
I am not sure if this will work: as in the error site in the related failure incident, e.code is going to be ENOENT
?
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.
and worker did not fail in it's INIT
, instead at runtime, so the assertion at line 38 would fail anyways?
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.
I guess putting Fixes:
in the commit message was a bit hasty. I still think we should probably remove the message sniffing, even if it doesn't resolve the test flakiness.
In trying to test this, the test fails when run in parallel with itself. I can make it fail with |
@Trott - are you able to test (and cause it to fail) with this change to the tests's assertion? |
Unfortunately, no. The errors are varied.
|
Unfortunately, the test is sensitive to resource constraints and is unreliable on macOS in CI when in parallel. Fixes: nodejs#34727
@Trott - thanks. While the test did not expect these failures, with the (resource) constrained execution, these failures are absolutely meaningful. the first one ( the secone one ( IMO the first one can be accommodated in the test, while the second one should be fixed in the tracing agent. |
I removed "Fixes: " from the commit message. Landed in 9861962 |
Refs: nodejs#34727 (comment) PR-URL: nodejs#34769 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>
Let the check for the error code suffice and don't check for a particular string in the message. PR-URL: #34727 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: #34727 (comment) PR-URL: #34769 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>
Let the check for the error code suffice and don't check for a particular string in the message. PR-URL: #34727 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: #34727 (comment) PR-URL: #34769 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>
Let the check for the error code suffice and don't check for a particular string in the message. PR-URL: #34727 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: #34727 (comment) PR-URL: #34769 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>
Let the check for the error code suffice and don't check for a particular string in the message. PR-URL: #34727 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: #34727 (comment) PR-URL: #34769 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>
Let the check for the error code suffice and don't check for a
particular string in the message.
Fixes: #33759
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes