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 dgram tests #12623

Closed
wants to merge 1 commit into from
Closed

test: dynamic port in dgram tests #12623

wants to merge 1 commit into from

Conversation

sebastianplesciuc
Copy link

Removed common.PORT from:

  • test-dgram-close-in-listening
  • test-dgram-close-is-not-callback
  • test-dgram-close
  • test-dgram-exclusive-implicit-bind
  • test-dgram-oob-buffer

This is required in order to eliminate the possibility of port collision in parallel tests.

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

Removed common.PORT from test-dgram-close-in-listening,
test-dgram-close-is-not-callback, test-dgram-close,
test-dgram-exclusive-implicit-bind and test-dgram-oob-buffer
in order to eliminate the possibility of port collision.

Refs: #12376
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 24, 2017
@sebastianplesciuc
Copy link
Author

sebastianplesciuc commented Apr 24, 2017

I don't know if this is the most elegant solution. My reasoning was that if we actually send data on UDP we might as well send it to a port we own in that specific test. Or, if we need to send it to a closed port, to call send() in the callback of another socket's close().

Thoughts?

@vsemozhetbyt vsemozhetbyt added the dgram Issues and PRs related to the dgram subsystem / UDP. label Apr 24, 2017
@vsemozhetbyt
Copy link
Contributor

@addaleax
Copy link
Member

Landed in d289678, 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-dgram-close-in-listening,
test-dgram-close-is-not-callback, test-dgram-close,
test-dgram-exclusive-implicit-bind and test-dgram-oob-buffer
in order to eliminate the possibility of port collision.

Refs: #12376
PR-URL: #12623
Reviewed-By: Anna Henningsen <[email protected]>
@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@sebastianplesciuc
Copy link
Author

@gibfahn PR: #13792

Let me know if it's ok. If this shouldn't be backported then I'll close the PR.

gibfahn pushed a commit that referenced this pull request Jun 20, 2017
Removed common.PORT from test-dgram-close-in-listening,
test-dgram-close-is-not-callback, test-dgram-close,
test-dgram-exclusive-implicit-bind and test-dgram-oob-buffer
in order to eliminate the possibility of port collision.

Refs: #12376
PR-URL: #12623
Backport-PR-URL: #13792
Reviewed-By: Anna Henningsen <[email protected]>
@gibfahn
Copy link
Member

gibfahn commented Jun 20, 2017

Let me know if it's ok. If this shouldn't be backported then I'll close the PR.

Looks good to me, thanks for doing it!

MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
Removed common.PORT from test-dgram-close-in-listening,
test-dgram-close-is-not-callback, test-dgram-close,
test-dgram-exclusive-implicit-bind and test-dgram-oob-buffer
in order to eliminate the possibility of port collision.

Refs: #12376
PR-URL: #12623
Backport-PR-URL: #13792
Reviewed-By: Anna Henningsen <[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
dgram Issues and PRs related to the dgram subsystem / UDP. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants