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 #9573

Closed
wants to merge 1 commit into from

Conversation

mkamakura
Copy link
Contributor

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

test

Description of change

Use port 0 instead of common.PORT.

Use port 0 instead of common.PORT.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 12, 2016
@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Nov 12, 2016
@santigimeno
Copy link
Member

The changes look good, but we have to make sure that there's at least another test that checks that listening on a specific port on a tls.Server object works. If that's the case LGTM, otherwise we will have to keep this test so we have this specific code path covered. See this text from the writing_tests guide:

If the test doesn't depend on a specific port number then always use 0 instead of an arbitrary value, as it allows tests to be run in parallel safely, as the operating system will assign a random port. If the test requires a specific port, for example if the test checks that assigning a specific port works as expected, then it is ok to assign a specific port number.

@shigeki
Copy link
Contributor

shigeki commented Nov 12, 2016

we have to make sure that there's at least another test that checks that listening on a specific port on a tls.Server object works.

I think that test-net-server-bind.js seems to be an appropriate one. It checks 3 static ports incrementally. We can mark it as a test of static ports in the comment.

@santigimeno
Copy link
Member

I think that test-net-server-bind.js seems to be an appropriate one. It checks 3 static ports incrementally. We can mark it as a test of static ports in the comment.

I'm fine with that but I wonder if having a test that actually covers a tls.Server instance calling listen on a specific port makes also sense. Or considering that tls.Server inherits from net.Server test-net-server-bind is enough. If the latter, LGTM.

@targos
Copy link
Member

targos commented Nov 27, 2016

targos pushed a commit to targos/node that referenced this pull request Nov 27, 2016
Use port 0 instead of common.PORT.

PR-URL: nodejs#9573
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@targos
Copy link
Member

targos commented Nov 27, 2016

Landed in 31d1a3f. Thank you!

@targos targos closed this Nov 27, 2016
addaleax pushed a commit that referenced this pull request Dec 5, 2016
Use port 0 instead of common.PORT.

PR-URL: #9573
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@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.

PR-URL: nodejs#9573
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
Use port 0 instead of common.PORT.

PR-URL: #9573
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
Use port 0 instead of common.PORT.

PR-URL: #9573
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
Use port 0 instead of common.PORT.

PR-URL: #9573
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
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
test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants