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.

Sign up for GitHub

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

Jump to bottom

test: use port 0 instead of common.PORT #9572

Closed
wants to merge 1 commit into from
Closed

test: use port 0 instead of common.PORT #9572

wants to merge 1 commit into from

Conversation

saitoxu
Copy link
Contributor

@saitoxu saitoxu commented Nov 12, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

Use port 0 instead of common.PORT to follow writing test guideline.
This is a part of Code And Learn at NodeFest 2016 Challenge in Tokyo.

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 12, 2016
@shigeki
Copy link
Contributor

shigeki commented Nov 12, 2016

Thanks. CI is running on https://ci.nodejs.org/job/node-test-pull-request/4827/ .

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Nov 12, 2016
cjihrig
cjihrig requested changes Nov 12, 2016
View reviewed changes
test/parallel/test-http-status-reason-invalid-chars.js
}).listen(common.PORT, common.mustCall(() => {
const url = `http://localhost:${common.PORT}`;
}).listen(0, common.mustCall(() => {
const url = `http://localhost:${server.address().port}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use server.address().address too?

@saitoxu
Copy link
Contributor Author

saitoxu commented Nov 13, 2016

@cjihrig Thank you for your feedback. I committed some changes, could you see again?

lpinca
lpinca reviewed Nov 13, 2016
View reviewed changes
test/parallel/test-http-status-reason-invalid-chars.js
const url = `http://localhost:${common.PORT}`;
}).listen(0, common.mustCall(() => {
const addr = server.address().address;
const host = net.isIPv6(addr) ? `[${addr}]` : addr;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can you rename this to hostname?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I changed the name.

cjihrig
cjihrig approved these changes Nov 13, 2016
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

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

@Trott
Copy link
Member

Trott commented Nov 13, 2016

CI failure seems relevant....

@jbergstroem
Copy link
Member

New CI: https://ci.nodejs.org/job/node-test-commit/6032/

shigeki
shigeki approved these changes Nov 18, 2016
View reviewed changes
Copy link
Contributor

@shigeki shigeki left a comment

Choose a reason for hiding this comment

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

CI is green. LGTM.

@shigeki
Copy link
Contributor

shigeki commented Nov 18, 2016

@saitoxu Could you squash these commits into one? The commit title is like test: fix test-http-status-reason-invalid-chars.js or more better sentence within 50 chars as you like and put all your current comment messages into after third line. I can rebase your commits but it is the course of code and learn. I hope you can make a good commit message.

@saitoxu
test: fix test-http-status-reason-invalid-chars.js
00597b3 
Use port 0 instead of common.PORT, and use server address
instead of localhost to follow writing test guideline.
This is a part of Code And Learn at NodeFest 2016 Challenge in Tokyo.

PR-URL: nodejs#9572
@targos
Copy link
Member

targos commented Nov 27, 2016

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

targos pushed a commit that referenced this pull request Nov 27, 2016
@saitoxu @targos
test: fix test-http-status-reason-invalid-chars
c9509ac 
Use port 0 instead of common.PORT, and use server address
instead of localhost to follow writing test guideline.
This is a part of Code And Learn at NodeFest 2016 Challenge in Tokyo.

PR-URL: #9572
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos
Copy link
Member

targos commented Nov 27, 2016

CI failures are unrelated. Landed in c9509ac.
Thanks for your contribution!

@targos targos closed this Nov 27, 2016
@saitoxu saitoxu deleted the change-port-number branch November 28, 2016 16:26
addaleax pushed a commit that referenced this pull request Dec 5, 2016
@saitoxu @addaleax
test: fix test-http-status-reason-invalid-chars
d51c856 
Use port 0 instead of common.PORT, and use server address
instead of localhost to follow writing test guideline.
This is a part of Code And Learn at NodeFest 2016 Challenge in Tokyo.

PR-URL: #9572
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Fishrock123 Fishrock123 mentioned this pull request Dec 5, 2016
Merged
2 tasks
addaleax pushed a commit to addaleax/node that referenced this pull request Dec 8, 2016
@saitoxu @addaleax
test: fix test-http-status-reason-invalid-chars
6105c91 
Use port 0 instead of common.PORT, and use server address
instead of localhost to follow writing test guideline.
This is a part of Code And Learn at NodeFest 2016 Challenge in Tokyo.

PR-URL: nodejs#9572
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@saitoxu @MylesBorins
test: fix test-http-status-reason-invalid-chars
420e7f1 
Use port 0 instead of common.PORT, and use server address
instead of localhost to follow writing test guideline.
This is a part of Code And Learn at NodeFest 2016 Challenge in Tokyo.

PR-URL: #9572
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@saitoxu @MylesBorins
test: fix test-http-status-reason-invalid-chars
72c9830 
Use port 0 instead of common.PORT, and use server address
instead of localhost to follow writing test guideline.
This is a part of Code And Learn at NodeFest 2016 Challenge in Tokyo.

PR-URL: #9572
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@saitoxu @MylesBorins
test: fix test-http-status-reason-invalid-chars
47cffbc 
Use port 0 instead of common.PORT, and use server address
instead of localhost to follow writing test guideline.
This is a part of Code And Learn at NodeFest 2016 Challenge in Tokyo.

PR-URL: #9572
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Dec 21, 2016
Merged
Merged
@gibfahn gibfahn mentioned this pull request Jan 17, 2017
Closed
3 tasks
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

@targos targos targos approved these changes

@santigimeno santigimeno santigimeno approved these changes

@shigeki shigeki shigeki approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.