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.

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

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

Closed
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
}).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?

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.

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.

@Trott
Copy link
Member

Trott commented Nov 13, 2016

CI failure seems relevant....

@jbergstroem
Copy link
Member

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.

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

targos pushed a commit that referenced this pull request Nov 27, 2016
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@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
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Dec 5, 2016
2 tasks
addaleax pushed a commit to addaleax/node that referenced this pull request Dec 8, 2016
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
This was referenced Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.