FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: use port 0 instead of common.PORT by saitoxu · Pull Request #9572 · nodejs/node · GitHub

/ node Public

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

Closed
saitoxu wants to merge 1 commit into
nodejs:masterfrom
saitoxu:change-port-number
ConversationCommits (1)ChecksFiles changed
Closed

test: use port 0 instead of common.PORT#9572
saitoxu wants to merge 1 commit into
nodejs:masterfrom
saitoxu:change-port-number

Conversation

saitoxu commented Nov 12, 2016

Copy link
Copy Markdown
Contributor
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 added the test Issues and PRs related to the tests. label Nov 12, 2016

shigeki commented Nov 12, 2016

Copy link
Copy Markdown
Contributor

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

mscdex added the http Issues or PRs related to the http subsystem. label Nov 12, 2016
cjihrig requested changes Nov 12, 2016
View reviewed changes
Comment thread test/parallel/test-http-status-reason-invalid-chars.js Outdated

cjihrig Nov 12, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

saitoxu commented Nov 13, 2016

Copy link
Copy Markdown
Contributor Author

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

lpinca reviewed Nov 13, 2016
View reviewed changes
Comment thread test/parallel/test-http-status-reason-invalid-chars.js Outdated

lpinca Nov 13, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit: can you rename this to hostname?

saitoxu Nov 13, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

OK, I changed the name.

cjihrig approved these changes Nov 13, 2016
View reviewed changes

cjihrig left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Trott commented Nov 13, 2016

Copy link
Copy Markdown
Member

CI failure seems relevant....

jbergstroem commented Nov 17, 2016

Copy link
Copy Markdown
Member

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

shigeki approved these changes Nov 18, 2016
View reviewed changes

shigeki left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

CI is green. LGTM.

shigeki commented Nov 18, 2016

Copy link
Copy Markdown
Contributor

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

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 commented Nov 27, 2016

Copy link
Copy Markdown
Member

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

targos pushed a commit that referenced this pull request Nov 27, 2016
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 commented Nov 27, 2016

Copy link
Copy Markdown
Member

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

targos closed this Nov 27, 2016
saitoxu deleted the change-port-number branch November 28, 2016 16:26
addaleax pushed a commit that referenced this pull request Dec 5, 2016
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 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
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
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
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
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 mentioned this pull request Jan 17, 2017
Closed
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lpinca lpinca approved these changes
jasnell jasnell approved these changes
santigimeno santigimeno approved these changes
cjihrig cjihrig approved these changes
+2 more reviewers
shigeki shigeki approved these changes
targos targos 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.

Back | FazBrowse Home | New Git URL