Skip to content
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.

Sign up for GitHub

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

Jump to bottom

test: eliminate multicast test FreeBSD flakiness #4042

Closed
wants to merge 12 commits into from
Closed

test: eliminate multicast test FreeBSD flakiness #4042

wants to merge 12 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 26, 2015

test-dgram-multicast-multi-process was flaky on FreeBSD and Raspeberry
Pi. This refactoring fixes the issue by eliminating a race condition.

Fixes: #2474

@Trott
Copy link
Member Author

Trott commented Nov 26, 2015

This change moves process.send({ listening: true }); after the binding of events for listenSocket. Because we're dealing with separate processes, the previous arrangement resulted in a race condition.

@Trott
Copy link
Member Author

Trott commented Nov 26, 2015

With this change, 9999 consecutive successful runs on FreeBSD:

Without this change, not so much:

@Trott Trott mentioned this pull request Nov 26, 2015
Closed
@Trott
Copy link
Member Author

Trott commented Nov 26, 2015

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

@Trott
Copy link
Member Author

Trott commented Nov 26, 2015

/cc @defunctzombie (to confirm that the test still does what it was written to do)

cjihrig
cjihrig reviewed Nov 26, 2015
View reviewed changes
test/internet/test-dgram-multicast-multi-process.js
new Buffer('Fourth message to send')
];
const common = require('../common'),
assert = require('assert'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're changing all of these lines, it would be awesome to split them into individual const declaration statements.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, done.

@mscdex mscdex added dgram Issues and PRs related to the dgram subsystem / UDP. test Issues and PRs related to the tests. freebsd Issues and PRs related to the FreeBSD platform. labels Nov 26, 2015
@jbergstroem
Copy link
Member

I think we want to skip if we're in a freebsd jail here.

Trott added 3 commits November 28, 2015 11:05
@Trott
test: eliminate multicast test FreeBSD flakiness
c623001 
test-dgram-multicast-multi-process was flaky on FreeBSD and Raspeberry
Pi. This refactoring fixes the issue by eliminating a race condition.

Fixes: nodejs#2474
@Trott
fixup
0f2782b
@Trott
fixup: skip if in a FreeBSD jail
8021d83
@Trott
Copy link
Member Author

Trott commented Nov 28, 2015

Added code to skip test if in a FreeBSD jail per suggestion from @jbergstroem

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

@Trott
Copy link
Member Author

Trott commented Nov 29, 2015

Bump. Anyone feel good enough about this for an LGTM?

@defunctzombie
Copy link
Contributor

I won't be able to take a look until next week but am sure others can
confirm that correct behavior is sill tested.

On Monday, November 30, 2015, Rich Trott notifications@github.com wrote:

Bump. Anyone feel good enough about this for an LGTM?

—
Reply to this email directly or view it on GitHub
#4042 (comment).

bnoordhuis
bnoordhuis reviewed Nov 30, 2015
View reviewed changes
test/internet/test-dgram-multicast-multi-process.js
const common = require('../common');
const assert = require('assert');
const dgram = require('dgram');
const Buffer = require('buffer').Buffer;
Copy link
Member

Choose a reason for hiding this comment

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

You can safely drop this, I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, done.

@Trott
Copy link
Member Author

Trott commented Nov 30, 2015

All nits un-nitted.

Stress test: https://ci.nodejs.org/job/node-stress-single-test/93/nodes=freebsd102-64/

CI: https://ci.nodejs.org/job/node-accept-pull-request/132/ https://ci.nodejs.org/job/node-test-pull-request/884/

(Clearly, I should get rid of the CI node-accept-pull-request bookmark...)

bnoordhuis
bnoordhuis reviewed Nov 30, 2015
View reviewed changes
test/internet/test-dgram-multicast-multi-process.js
if (dead === listeners) {
console.error('[PARENT] All workers have died.');
console.error('[PARENT] Fail');
process.exit(1);
Copy link
Member

Choose a reason for hiding this comment

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

Not a nit, more a stream-of-consciousness remark, but I would have thrown an exception here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really a response but getting all stream-of-consciousness over here too: FWIW, I agree and probably would have chosen to throw an exception too and would have also not included console.error() messages here and elsewhere in the test. But those are just my personal preferences and I only wanted to go so far in rewriting existing code. All this stuff was in the existing test. It only shows up as a change because the indentation changed and/or I moved it to a different location.

@bnoordhuis
Copy link
Member

LGTM

@Trott
Copy link
Member Author

Trott commented Dec 1, 2015

Landed in b3313aa

@Trott Trott closed this Dec 1, 2015
Trott added a commit to Trott/io.js that referenced this pull request Dec 1, 2015
@Trott
test: eliminate multicast test FreeBSD flakiness
b3313aa 
test-dgram-multicast-multi-process was flaky on FreeBSD and Raspeberry
Pi. This refactoring fixes the issue by eliminating a race condition.

Fixes: nodejs#2474
PR-URL: nodejs#4042
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Trott added a commit that referenced this pull request Dec 5, 2015
@Trott @rvagg
test: eliminate multicast test FreeBSD flakiness
22b0971 
test-dgram-multicast-multi-process was flaky on FreeBSD and Raspeberry
Pi. This refactoring fixes the issue by eliminating a race condition.

Fixes: #2474
PR-URL: #4042
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This was referenced Dec 8, 2015
Closed
Merged
@rvagg rvagg mentioned this pull request Dec 17, 2015
Closed
Trott added a commit that referenced this pull request Dec 29, 2015
@Trott
test: eliminate multicast test FreeBSD flakiness
24b6596 
test-dgram-multicast-multi-process was flaky on FreeBSD and Raspeberry
Pi. This refactoring fixes the issue by eliminating a race condition.

Fixes: #2474
PR-URL: #4042
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
@Trott
test: eliminate multicast test FreeBSD flakiness
22ba1b4 
test-dgram-multicast-multi-process was flaky on FreeBSD and Raspeberry
Pi. This refactoring fixes the issue by eliminating a race condition.

Fixes: #2474
PR-URL: #4042
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
Merged
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@Trott
test: eliminate multicast test FreeBSD flakiness
4f03018 
test-dgram-multicast-multi-process was flaky on FreeBSD and Raspeberry
Pi. This refactoring fixes the issue by eliminating a race condition.

Fixes: nodejs#2474
PR-URL: nodejs#4042
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@Trott Trott deleted the dgram-flaky branch January 13, 2022 22:30
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. freebsd Issues and PRs related to the FreeBSD platform. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Investigate flaky test test-dgram-multicast-multi-process
8 participants
@Trott @jbergstroem @defunctzombie @bnoordhuis @cjihrig @mscdex @jasnell @MylesBorins