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-zzarturgvieira-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-botnodejs-github-bot added the test Issues and PRs related to the tests. label May 10, 2017
@mscdexmscdex 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

It would be great if your commit messages and PR titles could be less generic. Having multiple PRs with identical titles makes it really hard to track what's going on. Including the test name would be helpful.

@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-zzarturgvieira-zz changed the title test: remove common.PORT from testtest: replace port in dgram cb testMay 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 threadtest/parallel/test-dgram-send-callback-buffer.js Outdated

@cjihrigcjihrigMay 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-zzarturgvieira-zz changed the title test: replace port in dgram cb testtest: move to sequential dgram cb testMay 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-zzarturgvieira-zz changed the title test: move to sequential dgram cb testtest: replace port in dgram cb testMay 11, 2017
lpinca
lpinca reviewed May 12, 2017
View reviewed changes
Comment threadtest/parallel/test-dgram-send-callback-buffer.js Outdated

@lpincalpincaMay 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 threadtest/parallel/test-http-server-de-chunked-trailer.js Outdated

@lpincalpincaMay 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-zzarturgvieira-zzMay 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

@mhdawsonmhdawson 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

Landed in e00116d.

@lpincalpinca 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>
@TrottTrott 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>
@jasnelljasnell mentioned this pull request May 28, 2017
Closed
@gibfahngibfahn 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>
@MylesBorinsMylesBorins mentioned this pull request Jul 18, 2017
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lpincalpincalpinca approved these changes
@cjihrigcjihrigcjihrig approved these changes
+1 more reviewer
@mhdawsonmhdawsonmhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

dgramIssues and PRs related to the dgram subsystem / UDP.testIssues 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