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 number from env in tls socket test #3557

Conversation

stefanmb
Copy link
Contributor

Tests normally use common.PORT to allow the user to select which port
number to listen on. Hardcoding the port number will cause parallel
instances of the test to fail.

Tests normally use common.PORT to allow the user to select which port
number to listen on. Hardcoding the port number will cause parallel
instances of the test to fail.
@Trott
Copy link
Member

Trott commented Oct 28, 2015

LGTM if CI is happy. /cc @indutny

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

@indutny
Copy link
Member

indutny commented Oct 28, 2015

LGTM, if CI is green

@rvagg
Copy link
Member

rvagg commented Oct 28, 2015

pretty sure this can't live in parallel any more with it changed to common.PORT, needs to be moved to sequential

@Trott
Copy link
Member

Trott commented Oct 28, 2015

@rvagg common.js assigns different ports to each process. It should be safe to leave in parallel on CI. common.PORT is used in 348 tests in test/parallel already.

@stefanmb
Copy link
Contributor Author

@Trott Thanks for the clarification.

I should have used more precise language in my comment. The issue I have is with running multiple instances of the test harness in parallel (perhaps "concurrent" would have been a better term). To avoid collisions I'm using a distinct NODE_COMMON_PORT value for each instance, but this particular test does not respect the environment value, causing grief. The intent of the PR is to resolve socket errors in this admittedly peculiar, but annoying scenario.

@mscdex mscdex added tls Issues and PRs related to the tls subsystem. test Issues and PRs related to the tests. labels Oct 28, 2015
@jbergstroem
Copy link
Member

We don't run tests in parallel in CI anyway. The change LGTM.

bnoordhuis pushed a commit that referenced this pull request Oct 28, 2015
Tests normally use common.PORT to allow the user to select which port
number to listen on. Hardcoding the port number will cause parallel
instances of the test to fail.

PR-URL: #3557
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
@bnoordhuis
Copy link
Member

Landed in c9e682d, thanks.

@bnoordhuis bnoordhuis closed this Oct 28, 2015
@Trott
Copy link
Member

Trott commented Oct 28, 2015

@jbergstroem Oh, right, not on CI but on local machines when people use make test....

rvagg pushed a commit to rvagg/io.js that referenced this pull request Oct 29, 2015
Tests normally use common.PORT to allow the user to select which port
number to listen on. Hardcoding the port number will cause parallel
instances of the test to fail.

PR-URL: nodejs#3557
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
@rvagg rvagg mentioned this pull request Oct 29, 2015
jasnell pushed a commit that referenced this pull request Oct 29, 2015
Tests normally use common.PORT to allow the user to select which port
number to listen on. Hardcoding the port number will cause parallel
instances of the test to fail.

PR-URL: #3557
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 29, 2015
Tests normally use common.PORT to allow the user to select which port
number to listen on. Hardcoding the port number will cause parallel
instances of the test to fail.

PR-URL: #3557
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
@jasnell
Copy link
Member

jasnell commented Oct 29, 2015

Landed in v4.x-staging in 295c964

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.

8 participants