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.

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

dgram,test: add addMembership/dropMembership tests #6753

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
5 participants
@Trott
Copy link
Member

commented May 14, 2016

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.

@Trott Trott added dgram test labels May 14, 2016

@@ -71,6 +71,9 @@ socket.on('message', (msg, rinfo) => {
```

### socket.addMembership(multicastAddress[, multicastInterface])
<!-- YAML
added: v0.6.9
-->

This comment has been minimized.

Copy link
@jasnell

jasnell May 15, 2016

Member

These should likely be done as a separate commit

This comment has been minimized.

Copy link
@Trott

Trott May 16, 2016

Author Member

Done!

@jasnell

View changes

lib/dgram.js Outdated
@@ -509,7 +509,7 @@ Socket.prototype.dropMembership = function(multicastAddress,

Socket.prototype._healthCheck = function() {
if (!this._handle)
throw new Error('Not running'); // error message from dgram_legacy.js
throw new Error('Not running');

This comment has been minimized.

Copy link
@jasnell

jasnell May 15, 2016

Member

Not really necessary?

@jasnell

This comment has been minimized.

Copy link
Member

commented May 15, 2016

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

const common = require('../common');
const assert = require('assert');
const dgram = require('dgram');
const multicastAddress = '224.0.0.114';

This comment has been minimized.

Copy link
@cjihrig

cjihrig May 15, 2016

Contributor

Is this something that could be moved to common?

This comment has been minimized.

Copy link
@Trott

Trott May 16, 2016

Author Member

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

This comment has been minimized.

Copy link
@Trott

Trott May 17, 2016

Author Member

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

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis May 17, 2016

Member

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.

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented May 15, 2016

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

@Trott Trott force-pushed the Trott:80s branch May 16, 2016

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 16, 2016

Per nits from @jasnell and @cjihrig:

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

This comment has been minimized.

Copy link
Member

commented May 16, 2016

LGTM, thank you

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 17, 2016

socket.addMembership(multicastAddress);
socket.dropMembership(multicastAddress);
}
);

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis May 17, 2016

Member

Missing socket.close()?

This comment has been minimized.

Copy link
@Trott

Trott May 18, 2016

Author Member

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

@Trott Trott force-pushed the Trott:80s branch May 18, 2016

@Trott

This comment has been minimized.

Copy link
Member Author

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/

@bnoordhuis

This comment has been minimized.

Copy link
Member

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

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 19, 2016

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

Trott added some commits May 16, 2016

doc: add `added:` info for dgram.*Membership()
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.
dgram,test: add addMembership/dropMembership tests
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.

@Trott Trott force-pushed the Trott:80s branch to 57b62f5 May 19, 2016

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 19, 2016

@Trott

This comment has been minimized.

Copy link
Member Author

commented May 19, 2016

CI is green. Landing...

Trott added a commit to Trott/io.js that referenced this pull request May 19, 2016

doc: add `added:` info for dgram.*Membership()
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>

Trott added a commit to Trott/io.js that referenced this pull request May 19, 2016

dgram,test: add addMembership/dropMembership tests
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>
@Trott

This comment has been minimized.

Copy link
Member Author

commented May 19, 2016

Landed in 1ef4916 and f94ebb0

@Trott Trott closed this May 19, 2016

saghul added a commit to saghul/node that referenced this pull request May 19, 2016

unix,stream: fix getting the correct fd for a handle
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>

Fishrock123 added a commit that referenced this pull request May 23, 2016

doc: add `added:` info for dgram.*Membership()
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>

Fishrock123 added a commit that referenced this pull request May 23, 2016

dgram,test: add addMembership/dropMembership tests
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>

Fishrock123 added a commit that referenced this pull request May 23, 2016

unix,stream: fix getting the correct fd for a handle
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>

rvagg added a commit that referenced this pull request Jun 2, 2016

doc: add `added:` info for dgram.*Membership()
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>

@MylesBorins MylesBorins referenced this pull request Jun 24, 2016

Merged

v4.4.7 proposal #7395

MylesBorins added a commit that referenced this pull request Jun 24, 2016

dgram,test: add addMembership/dropMembership tests
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>

MylesBorins added a commit that referenced this pull request Jun 24, 2016

dgram,test: add addMembership/dropMembership tests
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>

saghul added a commit to saghul/node that referenced this pull request Jul 11, 2016

unix,stream: fix getting the correct fd for a handle
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>

MylesBorins added a commit that referenced this pull request Jul 11, 2016

unix,stream: fix getting the correct fd for a handle
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>

MylesBorins added a commit that referenced this pull request Jul 11, 2016

unix,stream: fix getting the correct fd for a handle
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>

MylesBorins added a commit that referenced this pull request Jul 12, 2016

unix,stream: fix getting the correct fd for a handle
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>

@MylesBorins MylesBorins referenced this pull request Jul 12, 2016

Merged

v4.5.0 proposal #7688

MylesBorins added a commit that referenced this pull request Jul 14, 2016

unix,stream: fix getting the correct fd for a handle
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>

MylesBorins added a commit that referenced this pull request Jul 14, 2016

unix,stream: fix getting the correct fd for a handle
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>

Trott added a commit to Trott/io.js that referenced this pull request Aug 31, 2016

doc: add `added:` info for dgram.*Membership()
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>

MylesBorins added a commit that referenced this pull request Sep 4, 2016

doc: add `added:` info for dgram.*Membership()
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>

MylesBorins added a commit that referenced this pull request Sep 28, 2016

doc: add `added:` info for dgram.*Membership()
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>

rvagg added a commit that referenced this pull request Oct 18, 2016

doc: add `added:` info for dgram.*Membership()
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>

MylesBorins added a commit that referenced this pull request Oct 26, 2016

doc: add `added:` info for dgram.*Membership()
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>

@MylesBorins MylesBorins referenced this pull request Oct 26, 2016

Closed

V4.6.2 proposal #9298

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.