test: use 443, not common.PORT

[maclover7]

Ref: #12376

cc @Trott

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

test

[tniessen]

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

[Trott]

Hmmm.... Using 0 for connect() doesn't make semantic sense. It's more likely to be a distraction than anything else. These things are throwing/emitting errors before they connect, right to anything, right? Using 0 runs the risk of it appearing that port 0 is the error-causing thing in the test and not the invalid host or whatever. For the TLS client, maybe just use port 443 because that's the usual port for such things so it shouldn't raise any attention? For the net test, any valid port should be fine. common.PORT is A-OK, or you can change it to 80 or 443 or 8080 or whatever.

[maclover7]

For the TLS client, maybe just use port 443 because that's the usual port for such things so it shouldn't raise any attention?

Agree, will do

For the net test, any valid port should be fine. common.PORT is A-OK, or you can change it to 80 or 443 or 8080 or whatever.

Isn't the point here to migrate away from common.PORT? 😬

[Trott]

Isn't the point here to migrate away from common.PORT? 😬

Almost, but not quite. The point here is to avoid port collisions between tests that use port 0 and tests that use common.PORT.^*

The easiest way to do that is to avoid listening to or connecting to common.PORT in tests that are in test/parallel.

If I understand these tests correctly, in both of these cases, the connect() fails before it actually establishes a connection. So the use of common.PORT is simply a placeholder. It could be any valid port. (We don't want to use 0 because it's invalid in connect(), so the connection might fail for that reason rather than the reason it's supposed to fail in the test.)

So, since any valid port will do as a placeholder and no actual connection occurs in these function calls (because they throw/emit errors), common.PORT is probably fine. So is any other valid value.
¯\(ツ)/¯

^* For example, if one test uses listen(0) and the OS gives it port 12346, and then a few milliseconds later, another unrelated test (running in parallel with that first test) tries listen(common.PORT) and common.PORT is also set to 12346, then the second test will fail with EADDRINUSE. This seems like it would be unlikely/infrequent, but we've seen it on CI more than once. It happens a lot less now that there's a lot less common.PORT in parallel.

[maclover7]

@Trott updated -- thank you for your time walking me through this and for the great discussion! Was a little bit hard to understand the tests at first. I removed test-net-connect-immediate-finish from my commit, since it's fine that it's using common.PORT. I updated the commit message/PR title to reflect what the change is actually doing now.

[Trott]

LGTM

[Trott]

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

[jasnell]

Actually... thinking twice about this one... should the test use a commonly used hardcoded port?

[Trott]

Actually... thinking twice about this one... should the test use a commonly used hardcoded port?

@jasnell I think you're right. I think lines 40-44 can all be replaced with a single line:

  assert.throws(tls.connect, Done);

[maclover7]

Good idea, updated @Trott @jasnell

(might be something for a later PR, but is it necessary to assert that Done is raised? seems like that's testing how tls.createSecureContext internals work, and it stuff were to break then the ciphers assertion would also break 😬 )

[Trott]

(might be something for a later PR, but is it necessary to assert that Done is raised? seems like that's testing how tls.createSecureContext internals work, and it stuff were to break then the ciphers assertion would also break 😬 )

I think it's still necessary because that makes sure that the function threw because of tls.createSecureContext() and not some other reason.

[Trott]

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

[jasnell]

Landed in d86eb5c