test: use random ports where possible #7045
Conversation
|
Apparently the diff is too big to review through GH's web interface. Reviewing it locally, it mostly LGTM but we'll still need tests that exercise binding to a specific port. I suppose that's covered by tests outside test/parallel. |
|
CI again to give the arm nodes a chance: |
|
/cc @nodejs/testing |
mscdex
force-pushed
the
test-use-random-ports
branch
from
3edaaaa
to
83dfaa0
Compare
|
The idea of using random ports makes me a bit nervous, in that it could introduce new non-deterministic failures. IMO a better way to deal with the |
|
@orangemocha The problem is with the parallel tests, not the sequential tests |
mscdex
force-pushed
the
test-use-random-ports
branch
from
83dfaa0
to
c5f310d
Compare
|
/cc @nodejs/collaborators Thoughts? |
|
Happy with it as long as some tests are retained to use a default port. We need to test that as well. |
|
@bnoordhuis @mcollina FWIW there are tests in |
|
LGTM (it's impossible to review completely) if CI passes. What's the problem with ARM? |
|
@mcollina Just CI infrastructure issues I guess. Another CI run for the heck of it: https://ci.nodejs.org/job/node-test-pull-request/2945/ |
|
that run errored badly:/ |
|
@mcollina Yeah, an issue with a recently merged commit. New CI: |
mscdex
force-pushed
the
test-use-random-ports
branch
from
c5f310d
to
fcba0d3
Compare
|
CI is still green except for a CI infrastructure failure on Windows. Retry on Windows is green: https://ci.nodejs.org/job/node-test-commit-windows-fanned/2886/ |
|
LGTM |
|
Even taking into account @orangemocha's concerns, I'd love to have everything move to randomized ports as it would give us the ability to do some hardware reuse in CI. An example situation where this might be handy is if we were to migrate away from Jenkins, or even try to upgrade to Jenkins v2, we could be running many (not all, some don't have capacity) of our machines attached to both old and new CI master. this lgtm me anyway |
|
@orangemocha Since this is affecting only |
|
Yes, go ahead. Thank you. |
This helps to prevent issues where a failed test can keep a bound socket open long enough to cause other tests to fail with EADDRINUSE because the same port number is used. PR-URL: nodejs#7045 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
mscdex
force-pushed
the
test-use-random-ports
branch
from
fcba0d3
to
2bc7841
Compare
This helps to prevent issues where a failed test can keep a bound socket open long enough to cause other tests to fail with EADDRINUSE because the same port number is used. PR-URL: #7045 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
|
@mscdex would you be willing to backport this to v4.x? |
This helps to prevent issues where a failed test can keep a bound socket open long enough to cause other tests to fail with EADDRINUSE because the same port number is used. PR-URL: nodejs#7045 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
This helps to prevent issues where a failed test can keep a bound socket open long enough to cause other tests to fail with EADDRINUSE because the same port number is used. PR-URL: #7045 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
This helps to prevent issues where a failed test can keep a bound socket open long enough to cause other tests to fail with EADDRINUSE because the same port number is used. PR-URL: #7045 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
This helps to prevent issues where a failed test can keep a bound socket open long enough to cause other tests to fail with EADDRINUSE because the same port number is used. PR-URL: #7045 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
Checklist
Affected core subsystem(s)
Description of change
This helps to prevent issues where a failed test can keep a bound socket open long enough to cause other tests to fail with
EADDRINUSEbecause the same port number is used.I should note that this commit still leaves some tests unmodified (mostly
cluster-related anddebugger-related tests), because either #7043 needs to land first or the solution is a little trickier than with typical tests. There are also some tests that usecommon.PORTbut they do not pass it to.bind()or.listen(), so those should be safe to keep as-is.