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

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

 });

-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 testMay 10, 2017

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

lpinca reviewed May 10, 2017
View reviewed changes

test/parallel/test-dgram-send-callback-buffer-empty-address.js Outdated
Copy link
Member
lpincaMay 10, 2017
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: missing new line at the end of file.

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

Uh oh!

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 Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Trott commented May 10, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented May 10, 2017

Uh oh!

arturgvieira-zz commented May 10, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arturgvieira-zz commented May 10, 2017

Uh oh!

lpincaMay 10, 2017

Choose a reason for hiding this comment

Uh oh!

lpinca commented May 10, 2017

Uh oh!

lpinca commented May 10, 2017

Uh oh!

arturgvieira-zz commented May 10, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arturgvieira-zz commented May 10, 2017

Uh oh!

lpinca commented May 11, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arturgvieira-zz commented May 11, 2017

Uh oh!

lpinca commented May 11, 2017

Uh oh!

arturgvieira-zz commented May 11, 2017

Uh oh!

cjihrig commented May 11, 2017

Uh oh!

lpinca commented May 11, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arturgvieira-zz commented May 11, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arturgvieira-zz commented May 11, 2017

Uh oh!

TrottMay 11, 2017

Choose a reason for hiding this comment

Uh oh!

cjihrigMay 11, 2017

Choose a reason for hiding this comment

Uh oh!

TrottMay 11, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpincaMay 11, 2017

Choose a reason for hiding this comment

Uh oh!

lpinca commented May 12, 2017

Uh oh!

lpinca commented May 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

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

TrottMay 11, 2017•
edited
Loading