-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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: retry on known SmartOS bug #5454
Conversation
Stress test for this version of the test, expected to show zero failures: https://ci.nodejs.org/job/node-stress-single-test/545/nodes=smartos14-32/console Stress test for current version on master, showing failures: https://ci.nodejs.org/job/node-stress-single-test/528/nodes=smartos14-32/console |
How is it done? |
@santigimeno wrote:
|
/cc @nodejs/testing |
Bump. @nodejs/collaborators |
Still fishing for an Maybe @mscdex who reported the problem? Or @jasnell who Or @indutny who reviewed a PR for a more generalized fix that helps other tests but not necessarily ones with lots of connections like this one? |
Why doesn't more generalized fix do not help here? |
socket.on('error', (e) => { | ||
// If SmartOS and ECONNREFUSED, then retry. See | ||
// https://github.com/nodejs/node/issues/2663. | ||
if (common.isSunOS && (e.code === 'ECONNREFUSED')) { |
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.
Please skip additional parens here.
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.
Please skip additional parens here.
Additional parens removed, branch rebased, force pushed. Will run CI one more time.
There's a build-in lint rule to catch extra parentheses, Alas, enabling it flags 315 instances in the code base. Probably not worth the churn. I might look more closely at it, though...
Anyway, if it helps - LGTM |
@indutny asked:
The more generalized fix retries the test one time if it's SmartOS and failed due to However, for tests that open dozens or hundreds of connections, one retry may not be enough. So, for this and for one other test, we ignore |
There is a known issue with SmartOS that is generally worked around in `tools/test.py`. However, a more robust workaround is required for some tests that open many network connections. `test-http-regr-nodejsgh-2928` is one such test. Fixes: nodejs#5445 Refs: nodejs#3941 PR-URL: nodejs#5454
Hopefully gratuitous CI: https://ci.nodejs.org/job/node-test-pull-request/1796/ |
There is a known issue with SmartOS that is generally worked around in `tools/test.py`. However, a more robust workaround is required for some tests that open many network connections. `test-http-regr-nodejsgh-2928` is one such test. Fixes: nodejs#5445 Refs: nodejs#3941 PR-URL: nodejs#5454 Reviewed-By: Fedor Indutny <fedor@indutny.com>
Landed in 8592697 |
There is a known issue with SmartOS that is generally worked around in `tools/test.py`. However, a more robust workaround is required for some tests that open many network connections. `test-http-regr-gh-2928` is one such test. Fixes: #5445 Refs: #3941 PR-URL: #5454 Reviewed-By: Fedor Indutny <fedor@indutny.com>
There is a known issue with SmartOS that is generally worked around in `tools/test.py`. However, a more robust workaround is required for some tests that open many network connections. `test-http-regr-gh-2928` is one such test. Fixes: #5445 Refs: #3941 PR-URL: #5454 Reviewed-By: Fedor Indutny <fedor@indutny.com>
There is a known issue with SmartOS that is generally worked around in `tools/test.py`. However, a more robust workaround is required for some tests that open many network connections. `test-http-regr-gh-2928` is one such test. Fixes: #5445 Refs: #3941 PR-URL: #5454 Reviewed-By: Fedor Indutny <fedor@indutny.com>
The flakiness issue for test-http-regr-nodejsgh-2928 on SmartOS was resolved in late February in nodejs#5454. This change removes its flaky designation in sequential.status.
The flakiness issue for test-http-regr-nodejsgh-2928 on SmartOS was resolved in late February in nodejs#5454. This change removes its flaky designation in sequential.status. PR-URL: nodejs#6540 Reviewed-By: James M Snell <jasnell@gmail.com>
There is a known issue with SmartOS that is generally worked around
in
tools/test.py
. However, a more robust workaround is required forsome tests that open many network connections.
test-http-regr-gh-2928
is one such test.Fixes: #5445
Refs: #3941