Skip to content

test: eliminate multicast test FreeBSD flakiness#4042

Closed
Trott wants to merge 12 commits intonodejs:masterfrom
Trott:dgram-flaky
Closed

test: eliminate multicast test FreeBSD flakiness#4042
Trott wants to merge 12 commits intonodejs:masterfrom
Trott:dgram-flaky

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 Outdated
Copy link
Contributor

@cjihrig cjihrig Nov 26, 2015

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

@Trott Trott Nov 27, 2015

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

jbergstroem commented Nov 28, 2015

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

defunctzombie commented Nov 29, 2015

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 Outdated
Copy link
Member

@bnoordhuis bnoordhuis Nov 30, 2015

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

@Trott Trott Nov 30, 2015

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
Copy link
Member

@bnoordhuis bnoordhuis Nov 30, 2015

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

@Trott Trott Dec 1, 2015

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

bnoordhuis commented Nov 30, 2015

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