-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Conversation
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
LGTM with a comment. |
Would prefer the doc change to be in a separate commit. Otherwise LGTM with @bnoordhuis' nit addressed. |
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.
Moved the doc change into its own commit per @jasnell. |
Thank you @Trott ! |
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. |
Just compiled Node.js v0.6.8 and did this:
Then did the same with Node.js 0.6.9:
This makes me feel comfortable enough to land this with |
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>
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>
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>
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>
Checklist
Affected core subsystem(s)
dgram test
Description of change
The only tests for
setBroadcast()
(from thedgram
module) were intest/internet
which means they almost never get run. This adds aminimal 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 checkfor the BROADCAST flag on an interface, it's possible that a version of
the test could be moved to
test/sequential
ortest/parallel
once itwas modified to only use internal networks.