-
Notifications
You must be signed in to change notification settings - Fork 30k
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-dgram-pingpong #5125
Conversation
assert.equal('PONG', msg.toString('ascii')); | ||
|
||
count += 1; | ||
client.on('message', function(msg) { |
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.
Perhaps the callback here should be wrapped with common.mustCall(..., N)
?
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 could go either way on it. I left it without common.mustCall()
and used the assert.strictEqual(pongsReceived, N);
instead so that it would be readily apparent that it's the analogous check to assert(pingsReceived >= N);
.
Stress test with this fix: Stress test on current master (should show lots of failures): |
So, looking at the stress test results, these changes greatly reduce the flakiness but don't eliminate it... More to be done... |
OK, greatly revised. @mscdex, PTAL. |
Stress test passed. CI failure is unrelated test. |
LGTM |
console.log('done'); | ||
}); | ||
const server = pingPongTest(common.PORT, 'localhost'); | ||
server.on('close', common.mustCall(pingPongTest.bind(this, common.PORT))); |
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.
Binding this
suggests it's somehow important but it's not, is it? It's always undefined, if I read the diff right.
LGTM with a nit. |
There is no guarantee UDP messages will be received. Accommodate the occasional dropped message. This is a functionality test, not a performance benchmark. Speed up the test by not sending 1500 messages across three ports. Fixes: nodejs#4526
Nit addressed. CI again: https://ci.nodejs.org/job/node-test-pull-request/1579/ |
LGTM |
There is no guarantee UDP messages will be received. Accommodate the occasional dropped message. This is a functionality test, not a performance benchmark. Speed up the test by not sending 1500 messages across three ports. Fixes: #4526 PR-URL: #5125 Reviewed-By: Brian White <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
Landed in 987e9e3 |
There is no guarantee UDP messages will be received. Accommodate the occasional dropped message. This is a functionality test, not a performance benchmark. Speed up the test by not sending 1500 messages across three ports. Fixes: #4526 PR-URL: #5125 Reviewed-By: Brian White <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
There is no guarantee UDP messages will be received. Accommodate the occasional dropped message. This is a functionality test, not a performance benchmark. Speed up the test by not sending 1500 messages across three ports. Fixes: #4526 PR-URL: #5125 Reviewed-By: Brian White <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
There is no guarantee UDP messages will be received. Accommodate the occasional dropped message. This is a functionality test, not a performance benchmark. Speed up the test by not sending 1500 messages across three ports. Fixes: #4526 PR-URL: #5125 Reviewed-By: Brian White <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
There is no guarantee UDP messages will be received. Accommodate the occasional dropped message. This is a functionality test, not a performance benchmark. Speed up the test by not sending 1500 messages across three ports. Fixes: #4526 PR-URL: #5125 Reviewed-By: Brian White <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
There is no guarantee UDP messages will be received. Accommodate the occasional dropped message. This is a functionality test, not a performance benchmark. Speed up the test by not sending 1500 messages across three ports. Fixes: #4526 PR-URL: #5125 Reviewed-By: Brian White <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
There is no guarantee UDP messages will be received. Accommodate the occasional dropped message. This is a functionality test, not a performance benchmark. Speed up the test by not sending 1500 messages across three ports. Fixes: nodejs#4526 PR-URL: nodejs#5125 Reviewed-By: Brian White <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
There is no guarantee UDP messages will be received. Accommodate the
occasional dropped message.
This is a functionality test, not a performance benchmark. Speed up the
test by sending 5 messages per server rather than 500.
Fixes: #4526