dgram,test: add addMembership/dropMembership tests by Trott · Pull Request #6753 · nodejs/node · GitHub

Checklist

tests and code linting passes
a test and/or benchmark is included
documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

test dgram

Description of change

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.

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.

I also did some minor tidying on the existing test/internet test.

These should likely be done as a separate commit

Done!

Couple of minor nits. Otherwise LGTM if CI is green.

Is this something that could be moved to common?

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."

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

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.

LGTM with a comment. I also second @jasnell's nits.

Per nits from @jasnell and @cjihrig:

Moved doc metadata update into its own commit
Omitted removal of lib/dgram.js comment

LGTM, thank you

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

Missing socket.close()?

@bnoordhuis Yes, good catch. Added socket.close(), rebased against current master, force pushed.

Rebased against master, fixed a nit from @bnoordhuis, time to run CI again: https://ci.nodejs.org/job/node-test-pull-request/2685/

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

@bnoordhuis wrote:

parallel/test-dgram-membership and the documentation changes LGTM but the style fixes to the other test don't really belong, IMO.

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.

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

CI is green. Landing...

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 <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

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 <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Landed in 1ef4916 and f94ebb0

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 <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

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 <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

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 <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

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 <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

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 <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

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 <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Trott added dgram Issues and PRs related to the dgram subsystem / UDP. test Issues and PRs related to the tests. labels May 14, 2016

jasnell reviewed May 15, 2016
View reviewed changes

cjihrig reviewed May 15, 2016
View reviewed changes

Trott force-pushed the 80s branch from 4939da2 to 4b6c02c Compare May 16, 2016 22:15

bnoordhuis reviewed May 17, 2016
View reviewed changes

Trott force-pushed the 80s branch from 4b6c02c to 789b904 Compare May 18, 2016 05:08

Trott added 2 commits May 18, 2016 22:21

doc: add added: info for dgram.*Membership()

963fce8

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.

Trott force-pushed the 80s branch from 789b904 to 57b62f5 Compare May 19, 2016 05:23

Trott closed this May 19, 2016

MylesBorins mentioned this pull request Jun 24, 2016

v4.4.7 proposal #7395

Merged

MylesBorins mentioned this pull request Jul 12, 2016

v4.5.0 proposal #7688

Merged

indutny mentioned this pull request Aug 9, 2016

Node.js Foundation Core Technical Committee (CTC) Meeting 2016-08-10 #8030

Closed

MylesBorins mentioned this pull request Oct 26, 2016

V4.6.2 proposal #9298

Closed

Trott deleted the 80s branch January 13, 2022 22:43

dgram,test: add addMembership/dropMembership tests by Trott · Pull Request #6753 · nodejs/node · GitHub

Uh oh!

Uh oh!

dgram,test: add addMembership/dropMembership tests - #6753

dgram,test: add addMembership/dropMembership tests#6753Trott wants to merge 2 commits intonodejs:masternodejs/node:masterfrom Trott:80sTrott/io.js:80sCopy head branch name to clipboard

Conversation

Trott commented May 14, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

jasnell May 15, 2016

Choose a reason for hiding this comment

Uh oh!

Trott May 16, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented May 15, 2016

Uh oh!

cjihrig May 15, 2016

Choose a reason for hiding this comment

Uh oh!

Trott May 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott May 17, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis May 17, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrig commented May 15, 2016

Uh oh!

Trott commented May 16, 2016

Uh oh!

jasnell commented May 16, 2016

Uh oh!

Trott commented May 17, 2016

Uh oh!

bnoordhuis May 17, 2016

Choose a reason for hiding this comment

Uh oh!

Trott May 18, 2016

Choose a reason for hiding this comment

Uh oh!

Trott commented May 18, 2016

Uh oh!

bnoordhuis commented May 18, 2016

Uh oh!

Trott commented May 19, 2016

Uh oh!

Trott commented May 19, 2016

Uh oh!

Trott commented May 19, 2016

Uh oh!

Trott commented May 19, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dgram,test: add addMembership/dropMembership tests#6753
Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:80s

Trott May 16, 2016 •
edited
Loading