Skip to content

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

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
Closed
saitoxu wants to merge 1 commit into from
Closed

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

saitoxu 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 Outdated
Copy link
Contributor

@cjihrig cjihrig Nov 12, 2016

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 Outdated
Copy link
Member

@lpinca lpinca Nov 13, 2016

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

@saitoxu saitoxu Nov 13, 2016

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

jbergstroem commented Nov 17, 2016

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

@jasnell jasnell jasnell approved these changes

@santigimeno santigimeno santigimeno approved these changes

@targos targos targos approved these changes

@cjihrig cjihrig cjihrig approved these changes

+1 more reviewer

@shigeki shigeki shigeki approved these changes

Reviewers whose approvals may not affect merge requirements

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.