FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: fix flaky test-dgram-pingpong by Trott · Pull Request #5125 · nodejs/node · GitHub

/ node Public

test: fix flaky test-dgram-pingpong - #5125

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:pingpong
ConversationCommits (1)ChecksFiles changed
Closed

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

Conversation

Trott commented Feb 6, 2016

Copy link
Copy Markdown
Member

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 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 commented Feb 6, 2016

Copy link
Copy Markdown
Member Author

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

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

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

mscdex Feb 6, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Perhaps the callback here should be wrapped with common.mustCall(..., N)?

Trott Feb 6, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Feb 6, 2016

Copy link
Copy Markdown
Member Author

Stress test with this fix:

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

Trott commented Feb 6, 2016

Copy link
Copy Markdown
Member Author

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

Trott commented Feb 7, 2016

Copy link
Copy Markdown
Member Author

OK, greatly revised. @mscdex, PTAL.

Trott commented Feb 7, 2016

Copy link
Copy Markdown
Member Author

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 commented Feb 7, 2016

Copy link
Copy Markdown
Member Author

Stress test passed. CI failure is unrelated test.

mscdex commented Feb 7, 2016

Copy link
Copy Markdown
Contributor

LGTM

bnoordhuis reviewed Feb 7, 2016
View reviewed changes
Comment thread test/sequential/test-dgram-pingpong.js Outdated

bnoordhuis Feb 7, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Binding this suggests it's somehow important but it's not, is it? It's always undefined, if I read the diff right.

bnoordhuis commented Feb 7, 2016

Copy link
Copy Markdown
Member

LGTM with a nit.

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 commented Feb 7, 2016

Copy link
Copy Markdown
Member Author

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

jasnell commented Feb 7, 2016

Copy link
Copy Markdown
Member

LGTM

jasnell pushed a commit that referenced this pull request Feb 7, 2016
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 commented Feb 7, 2016

Copy link
Copy Markdown
Member

Landed in 987e9e3

jasnell closed this Feb 7, 2016
rvagg pushed a commit that referenced this pull request Feb 8, 2016
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 mentioned this pull request Feb 8, 2016
Closed
rvagg pushed a commit that referenced this pull request Feb 9, 2016
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 mentioned this pull request Feb 12, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
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
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 mentioned this pull request Feb 18, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
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
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 deleted the pingpong branch January 13, 2022 22:42
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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

Back | FazBrowse Home | New Git URL