dgram,test: add addMembership/dropMembership tests#6753
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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
Trott added dgram 
doc/api/dgram.md Outdated
Member
There was a problem hiding this comment.
These should likely be done as a separate commit
Member
jasnell commented May 15, 2016
Couple of minor nits. Otherwise LGTM if CI is green. |
Contributor
There was a problem hiding this comment.
Is this something that could be moved to common?
MemberAuthor
There was a problem hiding this comment.
I'm on the fence on that. I have a concern that putting that address in common will signal to people (like me) who don't fully understand multicast that it's OK for tests in parallel and sequential to communicate with that address. It's not, I don't think, and that's probably why the existing test that uses that address is in internet.
This test needs to have an address to test addMembership() and dropMembership(), which, now that I think about it, I guess means that this test will cause some IGMP traffic on the local network?
/cc @nodejs/build in the hopes someone who actually understands how multicast works can say "Yup, this test will send a packet" or "Nope, this test does not send a packet on the network."
MemberAuthor
There was a problem hiding this comment.
Hmmm, although 224.0.0.114 would be multicast on the local subnet only, so maybe it's OK in the context of this test? Like, it's not sending packets destined for the Internet or anything...
Member
There was a problem hiding this comment.
I can't say for sure it's true for all platforms but just joining or leaving a local multicast group (which all 224.0.0.xxx addresses are) should not generate any traffic, that's just bookkeeping to the kernel.
Contributor
cjihrig commented May 15, 2016
LGTM with a comment. I also second @jasnell's nits. |
MemberAuthor
Trott commented May 16, 2016
Member
jasnell commented May 16, 2016
LGTM, thank you |
MemberAuthor
Trott commented May 17, 2016
MemberAuthor
There was a problem hiding this comment.
@bnoordhuis Yes, good catch. Added socket.close(), rebased against current master, force pushed.
MemberAuthor
Trott commented May 18, 2016
Rebased against master, fixed a nit from @bnoordhuis, time to run CI again: https://ci.nodejs.org/job/node-test-pull-request/2685/ |
Member
bnoordhuis commented May 18, 2016
parallel/test-dgram-membership and the documentation changes LGTM but the style fixes to the other test don't really belong, IMO. Lots of failures on one of the smartos machines although I don't think they're related to this PR. The test itself passes: https://ci.nodejs.org/job/node-test-commit-smartos/2537/nodes=smartos14-64/console |
MemberAuthor
Trott commented May 19, 2016
@bnoordhuis wrote:
True. I'll remove those style changes and re-run CI. If no surprises, landing soon... |
Since I was doing the necessary git archaeology anyway, I took the time to add YAML information to the docs about when `addMembership()` and `dropMembership()` first appeared in their current forms.
The only tests for `addMembership()` and `dropMembership()` (from the `dgram` module) were in `test/internet` which means they almost never get run. This adds checks in `test/parallel`. I also did some minor tidying on the existing `test/internet` test.
MemberAuthor
Trott commented May 19, 2016
MemberAuthor
Trott commented May 19, 2016
CI is green. Landing... |
Trott added a commit to Trott/io.js that referenced this pull request May 19, 2016
Since I was doing the necessary git archaeology anyway, I took the time to add YAML information to the docs about when `addMembership()` and `dropMembership()` first appeared in their current forms. PR-URL: nodejs#6753 Ref: nodejs#6578 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request May 19, 2016
The only tests for `addMembership()` and `dropMembership()` (from the `dgram` module) were in `test/internet` which means they almost never get run. This adds checks in `test/parallel`. PR-URL: nodejs#6753 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
MemberAuthor
Trott commented May 19, 2016
saghul added a commit to saghul/node that referenced this pull request May 19, 2016
On OSX it's possible that the fd is replaced, so use the proper libuv API to get the correct fd. PR-URL: nodejs#6753 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request May 23, 2016
Since I was doing the necessary git archaeology anyway, I took the time to add YAML information to the docs about when `addMembership()` and `dropMembership()` first appeared in their current forms. PR-URL: #6753 Ref: #6578 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Fishrock123 pushed a commit that referenced this pull request May 23, 2016
The only tests for `addMembership()` and `dropMembership()` (from the `dgram` module) were in `test/internet` which means they almost never get run. This adds checks in `test/parallel`. PR-URL: #6753 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Fishrock123 pushed a commit that referenced this pull request May 23, 2016
On OSX it's possible that the fd is replaced, so use the proper libuv API to get the correct fd. PR-URL: #6753 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Jun 2, 2016
Since I was doing the necessary git archaeology anyway, I took the time to add YAML information to the docs about when `addMembership()` and `dropMembership()` first appeared in their current forms. PR-URL: #6753 Ref: #6578 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Merged
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
The only tests for `addMembership()` and `dropMembership()` (from the `dgram` module) were in `test/internet` which means they almost never get run. This adds checks in `test/parallel`. PR-URL: #6753 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
The only tests for `addMembership()` and `dropMembership()` (from the `dgram` module) were in `test/internet` which means they almost never get run. This adds checks in `test/parallel`. PR-URL: #6753 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
saghul added a commit to saghul/node that referenced this pull request Jul 11, 2016
On OSX it's possible that the fd is replaced, so use the proper libuv API to get the correct fd. PR-URL: nodejs#6753 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
On OSX it's possible that the fd is replaced, so use the proper libuv API to get the correct fd. PR-URL: #6753 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
On OSX it's possible that the fd is replaced, so use the proper libuv API to get the correct fd. PR-URL: #6753 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
On OSX it's possible that the fd is replaced, so use the proper libuv API to get the correct fd. PR-URL: #6753 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Merged
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
On OSX it's possible that the fd is replaced, so use the proper libuv API to get the correct fd. PR-URL: #6753 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
On OSX it's possible that the fd is replaced, so use the proper libuv API to get the correct fd. PR-URL: #6753 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request Aug 31, 2016
Since I was doing the necessary git archaeology anyway, I took the time to add YAML information to the docs about when `addMembership()` and `dropMembership()` first appeared in their current forms. PR-URL: nodejs#6753 Ref: nodejs#6578 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 4, 2016
Since I was doing the necessary git archaeology anyway, I took the time to add YAML information to the docs about when `addMembership()` and `dropMembership()` first appeared in their current forms. PR-URL: #6753 Ref: #6578 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 28, 2016
Since I was doing the necessary git archaeology anyway, I took the time to add YAML information to the docs about when `addMembership()` and `dropMembership()` first appeared in their current forms. PR-URL: #6753 Ref: #6578 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
Since I was doing the necessary git archaeology anyway, I took the time to add YAML information to the docs about when `addMembership()` and `dropMembership()` first appeared in their current forms. PR-URL: #6753 Ref: #6578 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
Since I was doing the necessary git archaeology anyway, I took the time to add YAML information to the docs about when `addMembership()` and `dropMembership()` first appeared in their current forms. PR-URL: #6753 Ref: #6578 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
Affected core subsystem(s)
test dgram
Description of change
The only tests for
addMembership()anddropMembership()(from thedgrammodule) were intest/internetwhich means they almost neverget run. This adds checks in
test/parallel.Since I was doing the necessary git archaeology anyway, I took the time
to add YAML information to the docs about when
addMembership()anddropMembership()first appeared in their current forms.I also did some minor tidying on the existing
test/internettest.