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 tests for setBroadcast() #6750

Closed
wants to merge 2 commits into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented May 13, 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)

dgram test

Description of change

The only tests for setBroadcast() (from the dgram module) were in
test/internet which means they almost never get run. This adds a
minimal test that can check JS-land functionality in test/parallel.

Since I was doing the necessary git spelunking anyway, I took the time
to add the YAML information into the docs about when setBroadcast()
first appeared in its current form.

I also expanded a comment and did some minor formatting on the existing
test/internet test. If there were an easy and reliable way to check
for the BROADCAST flag on an interface, it's possible that a version of
the test could be moved to test/sequential or test/parallel once it
was modified to only use internal networks.

@Trott Trott added dgram Issues and PRs related to the dgram subsystem / UDP. test Issues and PRs related to the tests. labels May 13, 2016
@@ -283,6 +283,9 @@ not work because the packet will get silently dropped without informing the
source that the data did not reach its intended recipient.

### socket.setBroadcast(flag)
<!-- YAML
added: v0.6.9
Copy link
Member

Choose a reason for hiding this comment

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

I'm fairly sure it goes all the way back to v0.2.0. It was certainly in v0.4.x because I wrote the libuv replacement during the v0.5.x development cycle.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bnoordhuis I got v0.6.9 from f749338.

In v0.6.8, the function existed, but this was the implementation:

Socket.prototype.setBroadcast = function(arg) {
  throw new Error('not yet implemented');
};

My interpretation was that the function existed in earlier releases, but went away for a while, and then came back in 0.6.9. Am I mistaken? Was that function never really run and there was a version in dgram_legacy.js or something that was actually the implementation for 0.6.8?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I suppose it's possible I wrote the libuv code but forgot to hook it up in node... let's pretend this conversation never happened.

@bnoordhuis
Copy link
Member

LGTM with a comment.

@jasnell
Copy link
Member

jasnell commented May 16, 2016

Would prefer the doc change to be in a separate commit. Otherwise LGTM with @bnoordhuis' nit addressed.

Trott added 2 commits May 16, 2016 13:22
Since I was doing the necessary git spelunking anyway, I took the time
to add the YAML information into the docs about when `setBroadcast()`
first appeared in its current form.
The only tests for `setBroadcast()` (from the `dgram` module) were in
`test/internet` which means they almost never get run. This adds a
minimal test that can check JS-land functionality in `test/parallel`.

I also expanded a comment and did some minor formatting on the existing
`test/internet` test. If there were an easy and reliable way to check
for the BROADCAST flag on an interface, it's possible that a version of
the test could be moved to `test/sequential` or `test/parallel` once it
was modified to only use internal networks.
@Trott
Copy link
Member Author

Trott commented May 16, 2016

Moved the doc change into its own commit per @jasnell.

@Trott
Copy link
Member Author

Trott commented May 16, 2016

@jasnell
Copy link
Member

jasnell commented May 16, 2016

Thank you @Trott !

@Trott
Copy link
Member Author

Trott commented May 16, 2016

Only failure in CI is a Windows build failure. Will wait a short time to land to see if we can get consensus on the YAML comment. If not, I'll remove that commit before landing.

@Trott
Copy link
Member Author

Trott commented May 16, 2016

Just compiled Node.js v0.6.8 and did this:

$ node -v
v0.6.8
$ node
> var dgram = require('dgram');
undefined
> var foo = dgram.createSocket('udp4');
undefined
> foo.bind();
undefined
> foo.setBroadcast(true);
Error: not yet implemented
    at Socket.setBroadcast (dgram.js:226:9)
    at repl:1:5
    at REPLServer.eval (repl.js:80:21)
    at repl.js:190:20
    at REPLServer.eval (repl.js:87:5)
    at Interface.<anonymous> (repl.js:182:12)
    at Interface.emit (events.js:67:17)
    at Interface._onLine (readline.js:162:10)
    at Interface._line (readline.js:426:8)
    at Interface._ttyWrite (readline.js:603:14)
>

Then did the same with Node.js 0.6.9:

$ node -v
v0.6.9
$ node
> var dgram = require('dgram');
undefined
> var foo = dgram.createSocket('udp4');
undefined
> foo.bind();
undefined
> foo.setBroadcast(true);
undefined
> 

This makes me feel comfortable enough to land this with 0.6.9 as the version specified in the YAML comment. So I'm going to go do that now...

Trott added a commit to Trott/io.js that referenced this pull request May 16, 2016
Since I was doing the necessary git spelunking anyway, I took the time
to add the YAML information into the docs about when `setBroadcast()`
first appeared in its current form.

PR-URL: nodejs#6750
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Trott added a commit to Trott/io.js that referenced this pull request May 16, 2016
The only tests for `setBroadcast()` (from the `dgram` module) were in
`test/internet` which means they almost never get run. This adds a
minimal test that can check JS-land functionality in `test/parallel`.

I also expanded a comment and did some minor formatting on the existing
`test/internet` test. If there were an easy and reliable way to check
for the BROADCAST flag on an interface, it's possible that a version of
the test could be moved to `test/sequential` or `test/parallel` once it
was modified to only use internal networks.

PR-URL: nodejs#6750
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented May 16, 2016

Landed in 5f31b7e and 78520fa

@Trott Trott closed this May 16, 2016
evanlucas pushed a commit that referenced this pull request May 17, 2016
The only tests for `setBroadcast()` (from the `dgram` module) were in
`test/internet` which means they almost never get run. This adds a
minimal test that can check JS-land functionality in `test/parallel`.

I also expanded a comment and did some minor formatting on the existing
`test/internet` test. If there were an easy and reliable way to check
for the BROADCAST flag on an interface, it's possible that a version of
the test could be moved to `test/sequential` or `test/parallel` once it
was modified to only use internal networks.

PR-URL: #6750
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 17, 2016
Since I was doing the necessary git spelunking anyway, I took the time
to add the YAML information into the docs about when `setBroadcast()`
first appeared in its current form.

PR-URL: #6750
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott Trott deleted the larry branch January 13, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dgram Issues and PRs related to the dgram subsystem / UDP. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants