Skip to content

Conversation

@sebastianplesciuc
Copy link

Removed common.PORT from test-regress-GH-5051 and
test-regress-GH-5727 in order to eliminate the possibility
of port collision.

Refs: #12376

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 25, 2017
@mscdex mscdex added the net Issues and PRs related to the net subsystem. label Apr 25, 2017
Copy link
Member

Choose a reason for hiding this comment

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

${0} can just be 0

@Trott
Copy link
Member

Trott commented Apr 25, 2017

For test/parallel/test-regress-GH-5051.js, do we actually need to do all this? It creates a request but it never actually runs the request, does it? (Honest question; I'm actually not sure.) If it never tries to connect to a port or anything, then we can just hard-code it to 80 or 8080 or whatever and not do all the net stuff.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

I'm pretty sure the 5051 test never actually runs a network request, so we can dispense with net and hardcode any port we like. (I propose 8080.) If you believe differently, by all means, fill me in.

@sebastianplesciuc
Copy link
Author

@Trott You're right. I changed it to 8080 and fixed the ${0} issue. Thank you!

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green. Might want to consider adding a comment where 8080 is being hardcoded explaining that there's no network connection that actually happens; it's a request that gets created but never used. But I'm fine with it as-is too.

Removed common.PORT from test-regress-GH-5051 and
test-regress-GH-5727 in order to eliminate the possibility
of port collision.

Refs: #12376
@sebastianplesciuc
Copy link
Author

@Trott Added comment :)

@Trott
Copy link
Member

Trott commented Apr 27, 2017

@addaleax
Copy link
Member

Landed in e927809, thanks for the PR!

@addaleax addaleax closed this Apr 27, 2017
addaleax pushed a commit that referenced this pull request Apr 27, 2017
Removed common.PORT from test-regress-GH-5051 and
test-regress-GH-5727 in order to eliminate the possibility
of port collision.

Refs: #12376
PR-URL: #12639
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
gibfahn pushed a commit that referenced this pull request Jun 18, 2017
Removed common.PORT from test-regress-GH-5051 and
test-regress-GH-5727 in order to eliminate the possibility
of port collision.

Refs: #12376
PR-URL: #12639
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
Removed common.PORT from test-regress-GH-5051 and
test-regress-GH-5727 in order to eliminate the possibility
of port collision.

Refs: #12376
PR-URL: #12639
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
Removed common.PORT from test-regress-GH-5051 and
test-regress-GH-5727 in order to eliminate the possibility
of port collision.

Refs: #12376
PR-URL: #12639
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants