test: remove modifcation to common.PORT #6990

Trott · 2016-05-26T06:14:27Z

Checklist

tests and code linting passes
the commit message follows commit guidelines

Affected core subsystem(s)

test debugger

Description of change

A possibly-buggy fixture server uses common.PORT+1000 for its port
rather than common.PORT. That could result in it clashing with other
ports if tests are run in parallel. The test runner increments
common.PORT by 100 for each running instance for tests. Change to use
common.PORT and have the tests that use the fixture start with
common.PORT+1 for anything they need.

Refs: #6989

A possibly-buggy fixture server uses `common.PORT+1000` for its port rather than `common.PORT`. That could result in it clashing with other ports if tests are run in parallel. The test runner increments `common.PORT` by 100 for each running instance for tests. Change to use common.PORT and have the tests that use the fixture start with common.PORT+1 for anything they need. Refs: nodejs#6989

Trott · 2016-05-26T06:14:42Z

/cc @rvagg

MylesBorins · 2016-05-26T06:57:17Z

@Trott LGTM

It seems like there are a few other instances... specifically in the debugger tests... would you be open to changing all of these instances in this PR?

bnoordhuis · 2016-05-26T07:40:51Z

My grep fu finds these:

$ git grep -P 'PORT\s*\+\s*\d\d+' test/
test/debugger/helper-debugger-repl.js:var port = common.PORT + 1337;
test/debugger/test-debugger-client.js:var debugPort = common.PORT + 1337;
test/fixtures/clustered-server/app.js:  server.listen(common.PORT+1000);
test/parallel/test-debug-signal-cluster.js:var port = common.PORT + 42;

There are several more that add a single digit number but those look Mostly Harmless.

The changes in this PR LGTM although comments should preferably be capitalized and punctuated and have two spaces before the //.

santigimeno · 2016-05-26T08:01:37Z

LGTM

mscdex · 2016-05-26T12:40:05Z

Why not take this opportunity to just use 0 for the port and let the OS assign one? Death to common.PORT!

cjihrig · 2016-05-26T13:47:04Z

LGTM

Trott · 2016-05-26T17:48:12Z

Why not take this opportunity to just use 0 for the port and let the OS assign one? Death to common.PORT!

If that works, that would also solve the problem where one test doesn't clean up after itself and subsequent tests start getting EADDRINUSE errors.

bnoordhuis · 2016-05-26T18:50:58Z

That won't work for debugger tests until #5025 lands. Dusting off that PR is on my TODO list, though.

mhdawson · 2016-05-27T15:00:25Z

LGTM

Fishrock123 · 2016-05-27T18:32:03Z

test/parallel/test-debug-port-cluster.js

@@ -3,7 +3,7 @@ const common = require('../common');
 const assert = require('assert');
 const spawn = require('child_process').spawn;

-const PORT_MIN = common.PORT;
+const PORT_MIN = common.PORT + 1; // fixture uses common.PORT


Would be less confusing to assign to a special port in fixtures?

I thought so too at first, but it turns out that the fixture uses a single port, while this uses use multiple ports. Less weird IMO to give the common.PORT to the fixture and let this use special ports rather than have both using special ports and needing to make sure that the special ports don't collide.

Trott · 2016-05-28T21:06:25Z

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

Trott · 2016-05-29T00:01:41Z

CI again, although fail on Raspberry Pi is certainly unrelated: https://ci.nodejs.org/job/node-test-pull-request/2843/

A possibly-buggy fixture server uses `common.PORT+1000` for its port rather than `common.PORT`. That could result in it clashing with other ports if tests are run in parallel. The test runner increments `common.PORT` by 100 for each running instance for tests. Change to use common.PORT and have the tests that use the fixture start with common.PORT+1 for anything they need. PR-URL: nodejs#6990 Refs: nodejs#6989 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Trott · 2016-05-29T20:30:29Z

Landed in bddfe63

@thealphanerd @bnoordhuis I'll pick off the remaining instances in a subsequent PR.

Remove uses of `common.PORT + 1337` where `common.PORT` suffices. Refs: nodejs#6990

A possibly-buggy fixture server uses `common.PORT+1000` for its port rather than `common.PORT`. That could result in it clashing with other ports if tests are run in parallel. The test runner increments `common.PORT` by 100 for each running instance for tests. Change to use common.PORT and have the tests that use the fixture start with common.PORT+1 for anything they need. PR-URL: nodejs#6990 Refs: nodejs#6989 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Remove uses of `common.PORT + 1337` where `common.PORT` suffices. PR-URL: nodejs#7055 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Refs: nodejs#6990

A possibly-buggy fixture server uses `common.PORT+1000` for its port rather than `common.PORT`. That could result in it clashing with other ports if tests are run in parallel. The test runner increments `common.PORT` by 100 for each running instance for tests. Change to use common.PORT and have the tests that use the fixture start with common.PORT+1 for anything they need. PR-URL: #6990 Refs: #6989 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>