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

test: replace port in dgram cd address empty test #12929

Closed
wants to merge 1 commit into from

Conversation

@arturgvieira
Copy link
Contributor

commented May 9, 2017

Replaced common.PORT in the following test.
test-dgram-send-callback-buffer-empty-address.js

Ref: #12376

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

test dgram

@jasnell
jasnell approved these changes May 9, 2017

@mscdex mscdex added the dgram label May 9, 2017

@Trott

This comment has been minimized.

Copy link
Member

commented May 10, 2017

Is the added complexity here better than just keeping the test simple and moving to sequential? I don't have an opinion either way, but I imagine others might. @nodejs/testing

Also: At least technically, I don't think this gets rid of the port collision issue that common.PORT has, although it does greatly narrow the window for collisions. Another Node.js test (running in parallel) could still open something on the port used here in the bit of time between this test closing and then sending. Since UDP is send-it-and-forget-it, that probably doesn't matter for this test, but it might matter for the other test that is listening. A reason to move to sequential instead? Or am I over-thinking things here? I'm fine either way, but again, I imagine others might have opinions. @nodejs/testing again

@Trott

This comment has been minimized.

Copy link
Member

commented May 10, 2017

(Also: I would expect make jslint to flag an error here involving a missing newline at the end of the file.)

@arturgvieira

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2017

I appreciate the comments, either way, is fine with me also. I followed the algorithm from another test which is also on your list, commit d289678 , test-dgram-close-in-listening.js the same modification.

@arturgvieira arturgvieira changed the title test: remove common.PORT from test test: remove common.PORT in dgram cb address test May 10, 2017

@arturgvieira arturgvieira changed the title test: remove common.PORT in dgram cb address test test: replace port in dgram cb address empty test May 10, 2017

@arturgvieira

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2017

@Trott Also corrected the missing newline at the end of the file. Nice catch.

test/parallel/test-dgram-send-callback-buffer-empty-address.js Outdated

portGetter.close();
}));

This comment has been minimized.

Copy link
@lpinca

lpinca May 10, 2017

Member

Nit: missing new line at the end of file.

@lpinca

This comment has been minimized.

Copy link
Member

commented May 10, 2017

I'm not sure either, as @Trott said this doesn't completely remove port collisions and makes the test harder to grok.
I would prefer to move these tests to sequential.

@lpinca

This comment has been minimized.

Copy link
Member

commented May 10, 2017

On second thought, the socket (client) has not been bound so it will pick a random port when calling send().

I think destination port doesn't matter so common.PORT is probably fine in this case and the original test doesn't need to be changed.

@arturgvieira

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2017

@lpinca Ok, I'll remove the changes and move the tests to sequential.

@arturgvieira arturgvieira referenced this pull request May 10, 2017
3 of 3 tasks complete

@arturgvieira arturgvieira changed the title test: replace port in dgram cb address empty test test: move to sequential dgram cd address empty test May 10, 2017

@arturgvieira

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2017

@lpinca @Trott All done, tests moved to sequential.

@lpinca

This comment has been minimized.

Copy link
Member

commented May 11, 2017

As I wrote in my last comment I think the test can be left as is. There is no need to move it sequential.

client.send(buf, common.PORT, onMessage);

The code above bind the socket on a random port. common.PORT here is the destination port and it can be any port, it doesn't matter.

@arturgvieira

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2017

I apologize, misunderstood your comment. I'll close the PR.

@arturgvieira arturgvieira deleted the arturgvieira:commonPort03-branch branch May 11, 2017

@lpinca

This comment has been minimized.

Copy link
Member

commented May 11, 2017

No problem. Also wait for other collaborators feedback, maybe I am missing something :)

@arturgvieira arturgvieira restored the arturgvieira:commonPort03-branch branch May 11, 2017

@arturgvieira

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2017

Sorry, I am super new. I'll reopen the PR.

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented May 11, 2017

It is possible for these dgram tests to bind to the port to prevent port collisions. The socket would end up sending to itself and just ignore the data. It adds pretty minimal complexity, and allows the tests to stay in parallel.

@lpinca

This comment has been minimized.

Copy link
Member

commented May 11, 2017

@arturgvieira, @cjihrig is suggesting something like this

diff --git a/test/parallel/test-dgram-send-callback-buffer-empty-address.js b/test/parallel/test-dgram-send-callback-buffer-empty-address.js
index 67e64d6bfe..5b3b1c41a1 100644
--- a/test/parallel/test-dgram-send-callback-buffer-empty-address.js
+++ b/test/parallel/test-dgram-send-callback-buffer-empty-address.js
@@ -14,4 +14,4 @@ const onMessage = common.mustCall(function(err, bytes) {
   client.close();
 });
 
-client.send(buf, common.PORT, onMessage);
+client.bind(0, () => client.send(buf, client.address().port, onMessage));
@arturgvieira

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2017

@lpinga Understood and it seems like a good way to go, I will go ahead and update the PRs making that change.

test: replace port in dgram cd address empty test
Replaced common.PORT in the following test.
test-dgram-send-callback-buffer-empty-address.js

Ref: #12376

@arturgvieira arturgvieira changed the title test: move to sequential dgram cd address empty test test: replace port in dgram cd address empty test May 11, 2017

@arturgvieira

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2017

@lpinca All done, updated the PRs with the change requested.

@@ -14,4 +14,4 @@ const onMessage = common.mustCall(function(err, bytes) {
client.close();
});

client.send(buf, common.PORT, onMessage);
client.bind(0, () => client.send(buf, client.address().port, onMessage));

This comment has been minimized.

Copy link
@Trott

Trott May 11, 2017

Member

Probably a good idea to wrap the callback in common.mustCall().

This comment has been minimized.

Copy link
@cjihrig

cjihrig May 11, 2017

Contributor

Not a bad idea. onMessage() is wrapped in a mustCall(), so in this case not strictly necessary. No strong preference.

This comment has been minimized.

Copy link
@Trott

Trott May 11, 2017

Member

Ah, yeah, if onMessage is already wrapped then the additional wrapping suggested by me above is actually not needed.

This comment has been minimized.

Copy link
@lpinca

lpinca May 11, 2017

Member

Yeah I also find common.mustCall() a bit redundant here which is the reason why I didn't suggest it in the first place.

@lpinca
lpinca approved these changes May 11, 2017
@lpinca

This comment has been minimized.

@lpinca

This comment has been minimized.

Copy link
Member

commented May 22, 2017

Landed in 4c84734.

@lpinca lpinca closed this May 22, 2017

lpinca added a commit that referenced this pull request May 22, 2017
test: replace common.PORT in dgram test
Replace common.PORT with available port assigned by the operating
system in test-dgram-send-callback-buffer-empty-address.

PR-URL: #12929
Refs: #12376
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Trott Trott referenced this pull request May 22, 2017
55 of 64 tasks complete
jasnell added a commit that referenced this pull request May 23, 2017
test: replace common.PORT in dgram test
Replace common.PORT with available port assigned by the operating
system in test-dgram-send-callback-buffer-empty-address.

PR-URL: #12929
Refs: #12376
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
jasnell added a commit that referenced this pull request May 23, 2017
test: replace common.PORT in dgram test
Replace common.PORT with available port assigned by the operating
system in test-dgram-send-callback-buffer-empty-address.

PR-URL: #12929
Refs: #12376
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@jasnell jasnell referenced this pull request May 28, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.