Skip to content

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-botnodejs-github-bot added the test Issues and PRs related to the tests. label Nov 12, 2016
@mscdexmscdex 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

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.Servertest-net-server-bind is enough. If the latter, LGTM.

@targos
Copy link
Member

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
@targos
Copy link
Member

Landed in 31d1a3f. Thank you!

@targostargos 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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
@Fishrock123Fishrock123 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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
This was referenced Dec 21, 2016
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testIssues and PRs related to the tests.tlsIssues and PRs related to the tls subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@mkamakura@santigimeno@shigeki@targos@jasnell@lpinca@mscdex@MylesBorins@addaleax@nodejs-github-bot