Skip to content
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: dynamic port in parallel regress tests #12639

Closed
wants to merge 1 commit into from
Closed

test: dynamic port in parallel regress tests #12639

wants to merge 1 commit into from

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
const address = this.address();
const key = `${address.family.slice(-1)}:${address.address}:${common.PORT}`;
const key = `${address.family.slice(-1)}:${address.address}:${0}`;
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