-
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: fix flaky test-inspector #9727
Conversation
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. Interesting that it's the opposite to what @santigimeno suggested in #5386 (comment).
Is there a reason you only ran the test 100 times? Don't we normally run 9999 times to check for flakyness?
Normally, yes, but ~93% failure rate makes it seem like 100 is enough. But hey, let's go big: https://ci.nodejs.org/job/node-stress-single-test/1068/nodes=win10/console UPDATE: 9999 runs and it's all ✅ green. |
CI is ✅ green! |
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.
Thanks!
Using `socket.destroy()` instead of `socket.end()` fixes more-than-intermittent ECONNRESET issues on Windows. Fixes: nodejs#8804
e1366de
to
dd78cc7
Compare
Made a small change (removed the flaky designation for the test in |
Using `socket.destroy()` instead of `socket.end()` fixes more-than-intermittent ECONNRESET issues on Windows. PR-URL: nodejs#9727 Fixes: nodejs#8804 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>
Landed in 2486273. Welcome back, green CI. Please stay a while, will ya? |
Using `socket.destroy()` instead of `socket.end()` fixes more-than-intermittent ECONNRESET issues on Windows. PR-URL: #9727 Fixes: #8804 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>
Using `socket.destroy()` instead of `socket.end()` fixes more-than-intermittent ECONNRESET issues on Windows. PR-URL: nodejs#9727 Fixes: nodejs#8804 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>
Using `socket.destroy()` instead of `socket.end()` fixes more-than-intermittent ECONNRESET issues on Windows. PR-URL: #9727 Fixes: #8804 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
test V8_inspector
Description of change
Using
socket.destroy()
instead ofsocket.end()
fixesmore-than-intermittent ECONNRESET issues on Windows.
Fixes: #8804
/cc @eugeneo @gibfahn
Stress test on master showing 93 failures in 100 runs on Windows 10: https://ci.nodejs.org/job/node-stress-single-test/1063/nodes=win10/console
Stress test with this change showing 0 failures in 100 runs on Windows 10:
https://ci.nodejs.org/job/node-stress-single-test/1066/nodes=win10/console