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: remove common.PORT from parallel tests #17410

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Dec 1, 2017

common.PORT should not be used in parallel tests because another test
may experience a collision with common.PORT when using port 0 to get
an open port. This has been observed to result in test failures in CI.

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

test

@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. labels Dec 1, 2017
Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

LGTM as long as CI passes

@@ -38,7 +38,7 @@ const {

const client = net.connect({
host: addresses.INVALID_HOST,
port: common.PORT,
port: 80, // port number doesn't matter because hosst name is invalid
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in comment.

It might be a good idea to add a common.mustNotCall() for the 'connect' event.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM once @mscdex comments are addressed

`common.PORT` should not be used in parallel tests because another test
may experience a collision with `common.PORT` when using port 0 to get
an open port. This has been observed to result in test failures in CI.
@Trott Trott force-pushed the no-port-in-parallel branch from a691006 to 9062864 Compare December 4, 2017 22:14
@Trott
Copy link
Member Author

Trott commented Dec 4, 2017

Nits addressed. (Thanks, @mscdex!)

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

Trott added a commit to Trott/io.js that referenced this pull request Dec 5, 2017
`common.PORT` should not be used in parallel tests because another test
may experience a collision with `common.PORT` when using port 0 to get
an open port. This has been observed to result in test failures in CI.

PR-URL: nodejs#17410
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@Trott
Copy link
Member Author

Trott commented Dec 5, 2017

Landed in e183900

@Trott Trott closed this Dec 5, 2017
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
`common.PORT` should not be used in parallel tests because another test
may experience a collision with `common.PORT` when using port 0 to get
an open port. This has been observed to result in test failures in CI.

PR-URL: #17410
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
`common.PORT` should not be used in parallel tests because another test
may experience a collision with `common.PORT` when using port 0 to get
an open port. This has been observed to result in test failures in CI.

PR-URL: #17410
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
@gibfahn
Copy link
Member

gibfahn commented Dec 20, 2017

Should land cleanly once #17296 lands

gibfahn pushed a commit that referenced this pull request Apr 12, 2018
`common.PORT` should not be used in parallel tests because another test
may experience a collision with `common.PORT` when using port 0 to get
an open port. This has been observed to result in test failures in CI.

PR-URL: #17410
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@gibfahn
Copy link
Member

gibfahn commented Apr 12, 2018

@MylesBorins I've just landed this on v8.x-staging (conflicts were resolved due to @joyeecheung 's backport of #17296).

This fixes test failures like:

not ok 1781 async-hooks/test-graph.tcp
  ---
  duration_ms: 0.682
  severity: fail
  stack: |-
    /home/iojs/build/workspace/node-test-commit-arm/nodes/ubuntu1604-arm64/test/async-hooks/test-graph.tcp.js:20
    net.connect({ port: server.address().port, host: '::1' },
                                        ^
    
    TypeError: Cannot read property 'port' of null
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-arm/nodes/ubuntu1604-arm64/test/async-hooks/test-graph.tcp.js:20:37)
        at Module._compile (module.js:652:30)
        at Object.Module._extensions..js (module.js:663:10)
        at Module.load (module.js:565:32)
        at tryModuleLoad (module.js:505:12)
        at Function.Module._load (module.js:497:3)
        at Function.Module.runMain (module.js:693:10)
        at startup (bootstrap_node.js:184:16)
        at bootstrap_node.js:605:3
  ...
ok 1782 async-hooks/test-embedder.api.async-resource.improper-order
  ---
  duration_ms: 1.155
  ...

So if you start seeing those you may want to prioritise getting this landed on 6.x.

@MylesBorins MylesBorins mentioned this pull request May 2, 2018
@Trott Trott deleted the no-port-in-parallel branch January 13, 2022 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.