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.

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

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

Merged
merged 0 commits into from
Jun 9, 2017
Merged

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

merged 0 commits into from
Jun 9, 2017

Conversation

refack
Copy link
Contributor

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

@refack refack 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
@refack refack self-assigned this May 18, 2017
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 18, 2017
@refack
Copy link
Contributor Author

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
Contributor Author

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
// an error is expected on the second worker
process.send(`socket2:${err.code}`);
});
socket2.close(common.mustCall(() => {
Copy link
Member

@Trott Trott May 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
Contributor Author

refack commented May 18, 2017

ork for this test. :-|

Is that a wink?

@refack refack force-pushed the 13064 branch 3 times, most recently from b0381e0 to 45323f0 Compare May 18, 2017 22:41
@refack refack changed the title [WIP] test balloon for 13064 test,dgram: harden test-dgram-bind-shared-ports.js May 18, 2017
@refack refack 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
Contributor Author

refack commented May 18, 2017

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

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

refack commented May 19, 2017

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

@refack refack added the blocked PRs that are blocked by other issues or PRs. label May 19, 2017
@refack refack 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?

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

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)

@refack refack closed this Jun 9, 2017
@refack
Copy link
Contributor Author

refack commented Jun 9, 2017

Landed in c9d45c4

@refack refack merged commit c9d45c4 into nodejs:master Jun 9, 2017
@refack
Copy link
Contributor Author

refack commented Jun 9, 2017

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

@refack refack 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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@addaleax addaleax mentioned this pull request Jun 10, 2017
Merged
@refack refack removed their assignment Jun 12, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@MylesBorins
Copy link
Contributor

@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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@refack refack mentioned this pull request Jul 17, 2017
Closed
3 tasks
@refack
Copy link
Contributor Author

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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
Merged
This was referenced Sep 5, 2017
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott left review comments

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
cluster Issues and PRs related to the cluster subsystem. dgram Issues and PRs related to the dgram subsystem / UDP. test Issues 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