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: use 443, not common.PORT #14928

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
10 participants
@maclover7
Member

maclover7 commented Aug 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@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

This comment has been minimized.

Show comment
Hide comment
@maclover7

maclover7 Aug 18, 2017

Member

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? 😬

Member

maclover7 commented Aug 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 19, 2017

Member

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.

Member

Trott commented Aug 19, 2017

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

This comment has been minimized.

Show comment
Hide comment
@maclover7

maclover7 Aug 19, 2017

Member

@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.

Member

maclover7 commented Aug 19, 2017

@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.

@maclover7 maclover7 changed the title from test: use 0, not common.PORT to test: use 443, not common.PORT Aug 19, 2017

@Trott

Trott approved these changes Aug 19, 2017

LGTM

@Trott

This comment has been minimized.

Show comment
Hide comment
@refack

refack approved these changes Aug 19, 2017

@lpinca

lpinca approved these changes Aug 21, 2017

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 23, 2017

Member

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

Member

jasnell commented Aug 23, 2017

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

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 23, 2017

Member

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);
Member

Trott commented Aug 23, 2017

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

This comment has been minimized.

Show comment
Hide comment
@maclover7

maclover7 Aug 24, 2017

Member

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 😬 )

Member

maclover7 commented Aug 24, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 24, 2017

Member

(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.

Member

Trott commented Aug 24, 2017

(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

This comment has been minimized.

Show comment
Hide comment

jasnell added a commit that referenced this pull request Aug 24, 2017

test: simplify test-tls-client-default-ciphers
PR-URL: #14928
Ref: #12376
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 24, 2017

Member

Landed in d86eb5c

Member

jasnell commented Aug 24, 2017

Landed in d86eb5c

@jasnell jasnell closed this Aug 24, 2017

@maclover7 maclover7 deleted the maclover7:jm-port branch Aug 25, 2017

addaleax added a commit to addaleax/ayo that referenced this pull request Aug 25, 2017

test: simplify test-tls-client-default-ciphers
PR-URL: nodejs/node#14928
Ref: nodejs/node#12376
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

addaleax added a commit to ayojs/ayo that referenced this pull request Aug 28, 2017

test: simplify test-tls-client-default-ciphers
PR-URL: nodejs/node#14928
Ref: nodejs/node#12376
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 10, 2017

test: simplify test-tls-client-default-ciphers
PR-URL: #14928
Ref: #12376
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

@MylesBorins MylesBorins referenced this pull request Sep 10, 2017

Merged

v8.5.0 proposal #15308

MylesBorins added a commit that referenced this pull request Sep 12, 2017

test: simplify test-tls-client-default-ciphers
PR-URL: #14928
Ref: #12376
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 20, 2017

test: simplify test-tls-client-default-ciphers
PR-URL: #14928
Ref: #12376
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

@MylesBorins MylesBorins referenced this pull request Sep 20, 2017

Merged

v6.11.4 proposal #15506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment