test: replace port in dgram cb test by arturgvieira-zz · Pull Request #12942 · nodejs/node · GitHub
Skip to content

test: replace port in dgram cb test#12942

Closed
arturgvieira-zz wants to merge 1 commit into
nodejs:masterfrom
arturgvieira-zz:commonPort04-branch
Closed

test: replace port in dgram cb test#12942
arturgvieira-zz wants to merge 1 commit into
nodejs:masterfrom
arturgvieira-zz:commonPort04-branch

Conversation

@arturgvieira-zz

@arturgvieira-zz arturgvieira-zz commented May 10, 2017 •
edited
Loading

Copy link
Copy Markdown

Replaced common.PORT in the following test.
test-dgram-send-callback-buffer.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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 10, 2017
@mscdex mscdex added the dgram Issues and PRs related to the dgram subsystem / UDP. label May 10, 2017
@Trott

Trott commented May 10, 2017

Copy link
Copy Markdown
Member

@arturgvieira-zz

arturgvieira-zz commented May 10, 2017 •
edited
Loading

Copy link
Copy Markdown
Author

You got it, will change the commit messages and name of the PRs. Thanks

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

arturgvieira-zz commented May 10, 2017

Copy link
Copy Markdown
Author

@Trott updated the PR titles and commit message headings. Let me know your thoughts.

@Trott

Trott commented May 10, 2017

Copy link
Copy Markdown
Member

@Trott updated the PR titles and commit message headings. Let me know your thoughts.

Thanks. That's much easier to differentiate in notifications.

@arturgvieira-zz

arturgvieira-zz commented May 10, 2017

Copy link
Copy Markdown
Author

Thank you. I am still new and appreciate learning best practices. Thanks for taking the time to explain.

cjihrig
cjihrig requested changes May 10, 2017
View reviewed changes
Comment thread test/parallel/test-dgram-send-callback-buffer.js Outdated

@cjihrig cjihrig May 10, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You could just use the existing client socket here.

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

arturgvieira-zz commented May 11, 2017 •
edited
Loading

Copy link
Copy Markdown
Author

@cjihrig If you could show me how to use the client socket here instead I would appreciate it from an understanding perspective. It turns out that currently it is preferred to remove the change and instead keep the original tests. #12929

@cjihrig

cjihrig commented May 11, 2017

Copy link
Copy Markdown
Contributor

If you could show me how to use the client socket here instead I would appreciate it from an understanding perspective.

Call bind() on the existing client socket. After binding, use client.address() to get the port used by send().

@arturgvieira-zz

arturgvieira-zz commented May 11, 2017 •
edited
Loading

Copy link
Copy Markdown
Author

Oh, I see, thanks for the help.

@lpinca

lpinca commented May 11, 2017

Copy link
Copy Markdown
Member

For anyone reviewing, see discussion in #12929.

@arturgvieira-zz arturgvieira-zz changed the title test: move to sequential dgram cb test test: replace port in dgram cb test May 11, 2017
lpinca
lpinca reviewed May 12, 2017
View reviewed changes
Comment thread test/parallel/test-dgram-send-callback-buffer.js Outdated

@lpinca lpinca May 12, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This line is too long, the linter will not like it.

@arturgvieira-zz

arturgvieira-zz commented May 12, 2017

Copy link
Copy Markdown
Author

@lpinca All done, wrapped arguments to shorten line length.

lpinca
lpinca reviewed May 12, 2017
View reviewed changes
Comment thread test/parallel/test-http-server-de-chunked-trailer.js Outdated

@lpinca lpinca May 12, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Has this new test been added by mistake?

@arturgvieira-zz arturgvieira-zz May 12, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, I'm sorry my mistake, give me a second and I will correct that.

@lpinca

lpinca commented May 12, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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

@vieiraarturg
test: replace port in dgram cb test
b53b1ac 
Replaced common.PORT in the following test.
test-dgram-send-callback-buffer.js

Ref: #12376
mhdawson
mhdawson approved these changes May 12, 2017
View reviewed changes

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@lpinca

lpinca commented May 22, 2017

Copy link
Copy Markdown
Member

@lpinca lpinca closed this May 22, 2017
lpinca pushed a commit that referenced this pull request May 22, 2017
@vieiraarturg @lpinca
test: use dynamic port in test-dgram-send-callback-buffer
e00116d 
Replace common.PORT with available port assigned by the operating
system in test-dgram-send-callback-buffer.

PR-URL: #12942
Refs: #12376
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <mhdawson@ibm.com>
@Trott Trott mentioned this pull request May 22, 2017
Closed
64 tasks
jasnell pushed a commit that referenced this pull request May 23, 2017
@vieiraarturg @jasnell
test: use dynamic port in test-dgram-send-callback-buffer
4b489e7 
Replace common.PORT with available port assigned by the operating
system in test-dgram-send-callback-buffer.

PR-URL: #12942
Refs: #12376
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <mhdawson@ibm.com>
jasnell pushed a commit that referenced this pull request May 23, 2017
@vieiraarturg @jasnell
test: use dynamic port in test-dgram-send-callback-buffer
8ef4fe0 
Replace common.PORT with available port assigned by the operating
system in test-dgram-send-callback-buffer.

PR-URL: #12942
Refs: #12376
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <mhdawson@ibm.com>
@jasnell jasnell mentioned this pull request May 28, 2017
Closed
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
MylesBorins pushed a commit that referenced this pull request Jul 17, 2017
@vieiraarturg @MylesBorins
test: use dynamic port in test-dgram-send-callback-buffer
7909c6d 
Replace common.PORT with available port assigned by the operating
system in test-dgram-send-callback-buffer.

PR-URL: #12942
Refs: #12376
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <mhdawson@ibm.com>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lpinca lpinca lpinca approved these changes
@cjihrig cjihrig cjihrig approved these changes
+1 more reviewer
@mhdawson mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

dgram Issues and PRs related to the dgram subsystem / UDP. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@arturgvieira-zz @Trott @cjihrig @lpinca @mhdawson @mscdex @MylesBorins @nodejs-github-bot @vieiraarturg