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

arturgvieira-zz · 2017-05-09T19:25:23Z

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

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

Trott · 2017-05-10T05:21:27Z

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 · 2017-05-10T05:22:04Z

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

arturgvieira-zz · 2017-05-10T05:32:01Z

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-zz · 2017-05-10T06:58:46Z

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

lpinca · 2017-05-10T12:12:23Z

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

+
+    portGetter.close();
+  }));
+


Nit: missing new line at the end of file.

lpinca · 2017-05-10T12:27:36Z

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 · 2017-05-10T13:07:08Z

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-zz · 2017-05-10T22:35:13Z

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

arturgvieira-zz · 2017-05-10T23:35:13Z

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

lpinca · 2017-05-11T05:47:46Z

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-zz · 2017-05-11T05:50:38Z

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

lpinca · 2017-05-11T05:52:07Z

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

arturgvieira-zz · 2017-05-11T05:54:21Z

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

cjihrig · 2017-05-11T13:36:24Z

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 · 2017-05-11T13:45:33Z

@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-zz · 2017-05-11T17:21:11Z

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

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

arturgvieira-zz · 2017-05-11T18:17:40Z

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

Trott · 2017-05-11T18:19:58Z

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));


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

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

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

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 · 2017-05-12T07:59:52Z

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

lpinca · 2017-05-22T06:41:08Z

Landed in 4c84734.

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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

nodejs-github-bot added the test Issues and PRs related to the tests. label May 9, 2017

jasnell approved these changes May 9, 2017

View reviewed changes

mscdex added the dgram Issues and PRs related to the dgram subsystem / UDP. label May 9, 2017

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

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

lpinca reviewed May 10, 2017

View reviewed changes

arturgvieira-zz mentioned this pull request May 10, 2017

test: replace port in dgram cb test #12942

Closed

3 tasks

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

cjihrig approved these changes May 11, 2017

View reviewed changes

arturgvieira-zz closed this May 11, 2017

arturgvieira-zz deleted the commonPort03-branch branch May 11, 2017 05:50

arturgvieira-zz restored the commonPort03-branch branch May 11, 2017 05:54

arturgvieira-zz reopened this May 11, 2017

This was referenced May 11, 2017

test: replace port in dgram cb length test #12943

Closed

test: replace port in dgram cb address test #12944

Closed

test: replace port in dgram cd address empty test

459f793

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

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

Trott reviewed May 11, 2017

View reviewed changes

cjihrig approved these changes May 11, 2017

View reviewed changes

lpinca approved these changes May 11, 2017

View reviewed changes

lpinca closed this May 22, 2017

Trott mentioned this pull request May 22, 2017

Remove common.PORT usage from parallel tests #12376

Closed

64 tasks

jasnell mentioned this pull request May 28, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the dont-land-on-v6.x label Jul 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

arturgvieira-zz commented May 9, 2017 •

edited

Loading

Trott commented May 10, 2017 •

edited

Loading

Trott commented May 10, 2017

arturgvieira-zz commented May 10, 2017 •

edited

Loading

arturgvieira-zz commented May 10, 2017

lpinca May 10, 2017

lpinca commented May 10, 2017

lpinca commented May 10, 2017

arturgvieira-zz commented May 10, 2017 •

edited

Loading

arturgvieira-zz commented May 10, 2017

lpinca commented May 11, 2017 •

edited

Loading

arturgvieira-zz commented May 11, 2017

lpinca commented May 11, 2017

arturgvieira-zz commented May 11, 2017

cjihrig commented May 11, 2017

lpinca commented May 11, 2017 •

edited

Loading

arturgvieira-zz commented May 11, 2017 •

edited

Loading

arturgvieira-zz commented May 11, 2017

Trott May 11, 2017

cjihrig May 11, 2017

Trott May 11, 2017 •

edited

Loading

lpinca May 11, 2017

lpinca commented May 12, 2017

lpinca commented May 22, 2017

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

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

Conversation

arturgvieira-zz commented May 9, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Trott commented May 10, 2017 • edited Loading

Trott commented May 10, 2017

arturgvieira-zz commented May 10, 2017 • edited Loading

arturgvieira-zz commented May 10, 2017

lpinca May 10, 2017

Choose a reason for hiding this comment

lpinca commented May 10, 2017

lpinca commented May 10, 2017

arturgvieira-zz commented May 10, 2017 • edited Loading

arturgvieira-zz commented May 10, 2017

lpinca commented May 11, 2017 • edited Loading

arturgvieira-zz commented May 11, 2017

lpinca commented May 11, 2017

arturgvieira-zz commented May 11, 2017

cjihrig commented May 11, 2017

lpinca commented May 11, 2017 • edited Loading

arturgvieira-zz commented May 11, 2017 • edited Loading

arturgvieira-zz commented May 11, 2017

Trott May 11, 2017

Choose a reason for hiding this comment

cjihrig May 11, 2017

Choose a reason for hiding this comment

Trott May 11, 2017 • edited Loading

Choose a reason for hiding this comment

lpinca May 11, 2017

Choose a reason for hiding this comment

lpinca commented May 12, 2017

lpinca commented May 22, 2017

arturgvieira-zz commented May 9, 2017 •

edited

Loading

Trott commented May 10, 2017 •

edited

Loading

arturgvieira-zz commented May 10, 2017 •

edited

Loading

arturgvieira-zz commented May 10, 2017 •

edited

Loading

lpinca commented May 11, 2017 •

edited

Loading

lpinca commented May 11, 2017 •

edited

Loading

arturgvieira-zz commented May 11, 2017 •

edited

Loading

Trott May 11, 2017 •

edited

Loading