Skip to content

test: fix flaky test-dgram-pingpong#5125

Closed
Trott wants to merge 1 commit intonodejs:masterfrom
Trott:pingpong
Closed

test: fix flaky test-dgram-pingpong#5125
Trott wants to merge 1 commit intonodejs:masterfrom
Trott:pingpong

Conversation

@Trott
Copy link
Member

@Trott Trott commented Feb 6, 2016

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

@Trott Trott added dgram Issues and PRs related to the dgram subsystem / UDP. test Issues and PRs related to the tests. lts-watch-v4.x labels Feb 6, 2016
@Trott
Copy link
Member Author

Trott commented Feb 6, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/1575/

mscdex
mscdex reviewed Feb 6, 2016
View reviewed changes
test/sequential/test-dgram-pingpong.js
assert.equal('PONG', msg.toString('ascii'));

count += 1;
client.on('message', function(msg) {
Copy link
Contributor

@mscdex mscdex Feb 6, 2016

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)?

Copy link
Member Author

@Trott Trott Feb 6, 2016

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);.

@Trott
Copy link
Member Author

Trott commented Feb 6, 2016

Stress test with this fix:

Stress test on current master (should show lots of failures):

@Trott
Copy link
Member Author

Trott commented Feb 6, 2016

So, looking at the stress test results, these changes greatly reduce the flakiness but don't eliminate it... More to be done...

@Trott
Copy link
Member Author

Trott commented Feb 7, 2016

OK, greatly revised. @mscdex, PTAL.

@Trott
Copy link
Member Author

Trott commented Feb 7, 2016

New CI: https://ci.nodejs.org/job/node-test-pull-request/1577/

New stress test: https://ci.nodejs.org/job/node-stress-single-test/441/nodes=pi2-raspbian-wheezy/console

@Trott
Copy link
Member Author

Trott commented Feb 7, 2016

Stress test passed. CI failure is unrelated test.

@mscdex
Copy link
Contributor

mscdex commented Feb 7, 2016

LGTM

bnoordhuis
bnoordhuis reviewed Feb 7, 2016
View reviewed changes
test/sequential/test-dgram-pingpong.js Outdated
Copy link
Member

@bnoordhuis bnoordhuis Feb 7, 2016

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.

@bnoordhuis
Copy link
Member

bnoordhuis commented Feb 7, 2016

LGTM with a nit.

@Trott
test: fix flaky test-dgram-pingpong
11b7436 
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
@Trott
Copy link
Member Author

Trott commented Feb 7, 2016

Nit addressed. CI again: https://ci.nodejs.org/job/node-test-pull-request/1579/

@jasnell
Copy link
Member

jasnell commented Feb 7, 2016

LGTM

jasnell pushed a commit that referenced this pull request Feb 7, 2016
@Trott @jasnell
test: fix flaky test-dgram-pingpong
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 <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Feb 7, 2016

Landed in 987e9e3

@jasnell jasnell closed this Feb 7, 2016
rvagg pushed a commit that referenced this pull request Feb 8, 2016
@Trott @rvagg
test: fix flaky test-dgram-pingpong
fc3c32b 
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 <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@rvagg rvagg mentioned this pull request Feb 8, 2016
Closed
rvagg pushed a commit that referenced this pull request Feb 9, 2016
@Trott @rvagg
test: fix flaky test-dgram-pingpong
cf3aa91 
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 <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 12, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
@Trott
test: fix flaky test-dgram-pingpong
bcd42d2 
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 <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
@Trott
test: fix flaky test-dgram-pingpong
a8417a2 
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 <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 18, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
@Trott
test: fix flaky test-dgram-pingpong
e3aa9b6 
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 <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@Trott
test: fix flaky test-dgram-pingpong
c62719d 
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 <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott Trott deleted the pingpong branch January 13, 2022 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

dgram Issues and PRs related to the dgram subsystem / UDP. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Trott @mscdex @bnoordhuis @jasnell @MylesBorins