Skip to content

test,dgram: harden test-dgram-bind-shared-ports.js#13100

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.

Sign up for GitHub

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

Jump to bottom
Merged
refack merged 0 commits into from
Jun 9, 2017
Merged

test,dgram: harden test-dgram-bind-shared-ports.js #13100

refack merged 0 commits into from
Jun 9, 2017

Conversation

@refack
Copy link
Contributor

@refackrefack commented May 18, 2017•
edited
Loading

  • add mustCall and mustNotCall to all callbacks
  • exit the processes instead of kill

Ref: #13055
Ref: #12999
Ref: #13526

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)

dgram,cluster,test

@refackrefack added dgram Issues and PRs related to the dgram subsystem / UDP. wip Issues and PRs that are still a work in progress. net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests. labels May 18, 2017
@refackrefack self-assigned this May 18, 2017
@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label May 18, 2017
@refack
Copy link
ContributorAuthor

refack commented May 18, 2017

Stress: https://ci.nodejs.org/job/node-stress-single-test/nodes=freebsd10-64/1229/

@gibfahn
Copy link
Member

gibfahn commented May 18, 2017

FWIW you can run node-test-commit or node-stress-single-test on a branch of your fork of Node if you want.

@refack
Copy link
ContributorAuthor

refack commented May 18, 2017

FWIW you can run node-test-commit or node-stress-single-test on a branch of your fork of Node if you want.

Thanks, I was vaguely aware of that, but was not sure...
Anyway this might mature into a real PR (maybe).

Trott
Trott reviewed May 18, 2017
View reviewed changes
test/sequential/test-dgram-bind-shared-ports.js Outdated
Copy link
Member

@TrottTrottMay 18, 2017•
edited
Loading

Choose a reason for hiding this comment

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

FWIW, I think this will only fire in the second worker. The first worker doesn't get an error on socket2 (and it's the first worker we care about for socket2 because it actually binds to the port).

Starting to wonder if whatever complexity is required to make socket2.close() work isn't any greater than complexity that might be required to make port: 0 work for this test. :-|

@refack
Copy link
ContributorAuthor

refack commented May 18, 2017

ork for this test. :-|

Is that a wink?

@refackrefackforce-pushed the 13064 branch 3 times, most recently from b0381e0 to 45323f0CompareMay 18, 2017 22:41
@refackrefack changed the title [WIP] test balloon for 13064test,dgram: harden test-dgram-bind-shared-ports.jsMay 18, 2017
@refackrefack added cluster Issues and PRs related to the cluster subsystem. and removed wip Issues and PRs that are still a work in progress. net Issues and PRs related to the net subsystem. labels May 18, 2017
@refack
Copy link
ContributorAuthor

refack commented May 18, 2017

CI: https://ci.nodejs.org/job/node-test-commit/9990/

This was referenced May 19, 2017
Closed
Closed
@refackrefack added the wip Issues and PRs that are still a work in progress. label May 19, 2017
@refack
Copy link
ContributorAuthor

refack commented May 19, 2017

Found a bug when workers bind for port 0. Blocking until I fix it.

@refackrefack added the blocked PRs that are blocked by other issues or PRs. label May 19, 2017
@refackrefack mentioned this pull request May 20, 2017
Closed
3 tasks
@fhinkel
Copy link
Member

fhinkel commented Jun 7, 2017

ping @refack can this be landed now that #13064 is closed?

@refackrefack mentioned this pull request Jun 7, 2017
Closed
@refackrefack changed the title test,dgram: harden test-dgram-bind-shared-ports.jscluster: enable cluster to share sockets when worker bind to port 0Jun 7, 2017
@refack
Copy link
ContributorAuthor

refack commented Jun 8, 2017

Dry-run CI: https://ci.nodejs.org/job/node-test-pull-request/8551/
(might cancel some platforms due to outage)

@refackrefack closed this Jun 9, 2017
@refack
Copy link
ContributorAuthor

refack commented Jun 9, 2017

Landed in c9d45c4

@refackrefack merged commit c9d45c4 into nodejs:masterJun 9, 2017
@refack
Copy link
ContributorAuthor

refack commented Jun 9, 2017

Extra sanity of master: https://ci.nodejs.org/job/node-test-commit-linuxone/6521/

@refackrefack deleted the 13064 branch June 9, 2017 21:23
addaleax pushed a commit that referenced this pull request Jun 10, 2017
@refack@addaleax
test: harden test-dgram-bind-shared-ports
3da56ac
* add `mustCall` and `mustNotCall` to all callbacks * added `known_issue` for port binding PR-URL: #13100 Refs: #13055 Refs: #12999 Refs: #13526 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@addaleaxaddaleax mentioned this pull request Jun 10, 2017
Merged
@refackrefack removed their assignment Jun 12, 2017
@gibfahngibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@MylesBorins
Copy link
Contributor

MylesBorins commented Jul 17, 2017

@refack could you please backport to v6.x?

refack added a commit to refack/node that referenced this pull request Jul 17, 2017
@refack
test: harden test-dgram-bind-shared-ports
5967e4b
* add `mustCall` and `mustNotCall` to all callbacks * added `known_issue` for port binding PR-URL: nodejs#13100 Refs: nodejs#13055 Refs: nodejs#12999 Refs: nodejs#13526 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@refackrefack mentioned this pull request Jul 17, 2017
Closed
3 tasks
@refack
Copy link
ContributorAuthor

refack commented Jul 17, 2017

@refack could you please backport to v6.x?

#14327

MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
@refack@MylesBorins
test: harden test-dgram-bind-shared-ports
1c340c7
* add `mustCall` and `mustNotCall` to all callbacks * added `known_issue` for port binding PR-URL: #13100 Refs: #13055 Refs: #12999 Refs: #13526 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
@refack@MylesBorins
test: harden test-dgram-bind-shared-ports
ac21bfd
* add `mustCall` and `mustNotCall` to all callbacks * added `known_issue` for port binding Backport-PR-URL: #14327 PR-URL: #13100 Refs: #13055 Refs: #12999 Refs: #13526 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
@refack@MylesBorins
test: harden test-dgram-bind-shared-ports
189dc42
* add `mustCall` and `mustNotCall` to all callbacks * added `known_issue` for port binding Backport-PR-URL: #14327 PR-URL: #13100 Refs: #13055 Refs: #12999 Refs: #13526 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@refack@MylesBorins
test: harden test-dgram-bind-shared-ports
a1cef1f
* add `mustCall` and `mustNotCall` to all callbacks * added `known_issue` for port binding Backport-PR-URL: #14327 PR-URL: #13100 Refs: #13055 Refs: #12999 Refs: #13526 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Aug 16, 2017
Merged
This was referenced Sep 5, 2017
Closed
Closed
Closed
Closed
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes

@TrottTrottTrott left review comments

@jasnelljasnelljasnell approved these changes

Assignees

No one assigned

Labels

clusterIssues and PRs related to the cluster subsystem.dgramIssues and PRs related to the dgram subsystem / UDP.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@refack@gibfahn@fhinkel@Trott@MylesBorins@mcollina@jasnell@nodejs-github-bot