FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: remove common.PORT from parallel tests by Trott · Pull Request #17410 · nodejs/node · GitHub

/ node Public

test: remove common.PORT from parallel tests - #17410

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:no-port-in-parallel
ConversationCommits (1)ChecksFiles changed
Closed

test: remove common.PORT from parallel tests#17410
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:no-port-in-parallel

Conversation

Trott commented Dec 1, 2017

Copy link
Copy Markdown
Member

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 added async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. labels Dec 1, 2017
maclover7 approved these changes Dec 1, 2017
View reviewed changes

maclover7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM as long as CI passes

mscdex reviewed Dec 1, 2017
View reviewed changes
Comment thread test/parallel/test-net-connect-immediate-finish.js Outdated

mscdex Dec 1, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Typo in comment.

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

mhdawson approved these changes Dec 4, 2017
View reviewed changes

mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM once @mscdex comments are addressed

test: remove common.PORT from parallel tests
9062864 
`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 force-pushed the no-port-in-parallel branch from a691006 to 9062864 Compare December 4, 2017 22:14

Trott commented Dec 4, 2017

Copy link
Copy Markdown
Member Author

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
test: remove common.PORT from parallel tests
e183900 
`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 commented Dec 5, 2017

Copy link
Copy Markdown
Member Author

Landed in e183900

Trott closed this Dec 5, 2017
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
test: remove common.PORT from parallel tests
a73b8c2 
`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
test: remove common.PORT from parallel tests
8c81ba0 
`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 mentioned this pull request Dec 12, 2017
Merged

gibfahn commented Dec 20, 2017

Copy link
Copy Markdown
Member

Should land cleanly once #17296 lands

gibfahn pushed a commit that referenced this pull request Apr 12, 2018
test: remove common.PORT from parallel tests
53b702f 
`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 commented Apr 12, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

@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 mentioned this pull request May 2, 2018
Merged
Trott deleted the no-port-in-parallel branch January 13, 2022 22:48
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lpinca lpinca approved these changes
cjihrig cjihrig approved these changes
+5 more reviewers
mscdex mscdex left review comments
lance lance approved these changes
maclover7 maclover7 approved these changes
hiroppy hiroppy approved these changes
mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL