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: recommended changes to test-cluster-send-handle-twice.js #10049

Closed
wants to merge 1 commit into from
Closed

test: recommended changes to test-cluster-send-handle-twice.js #10049

wants to merge 1 commit into from

Conversation

amarzavery
Copy link
Contributor

@amarzavery amarzavery commented Dec 1, 2016 •
edited
Loading

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change
  • var --> const as applicable
  • assert.equal --> assert.strictEqual
  • assert(false, ..) --> common.fail()
  • common.mustCall for functions that need to be called exactly once
  • modified an assert(!signal, 'Worker exited by a signal'); call to assert.strictEqual(signal, null); call as that made more sense

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016
@imyller imyller added cluster Issues and PRs related to the cluster subsystem. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. labels Dec 1, 2016
cjihrig
cjihrig reviewed Dec 3, 2016
View reviewed changes
test/parallel/test-cluster-send-handle-twice.js
var client = net.connect({ host: 'localhost', port: common.PORT });
client.on('close', function() { cluster.worker.disconnect(); });
const client = net.connect({ host: 'localhost', port: common.PORT });
client.on('close', common.mustCall(function() { cluster.worker.disconnect(); }));
Copy link
Contributor

Choose a reason for hiding this comment

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

This change isn't really necessary (although it doesn't hurt anything). Nothing is being asserted in the callback, so the test should fail if it's not called.

cjihrig
cjihrig reviewed Dec 3, 2016
View reviewed changes
test/parallel/test-cluster-send-handle-twice.js
const worker = cluster.fork();
worker.on('exit', common.mustCall(function(code, signal) {
assert.strictEqual(code, 0, 'Worker exited with an error code');
assert.strictEqual(signal, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

You could have left the 'Worker exited by a signal' as the third argument here.

Trott
Trott reviewed Dec 22, 2016
View reviewed changes
test/parallel/test-cluster-send-handle-twice.js
setTimeout(function() { client.end(); }, 50);
}).on('error', function(e) {
console.error(e);
assert(false, 'server.listen failed');
common.fail('server.listen failed');
Copy link
Member

Choose a reason for hiding this comment

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

This bug was in the original test, but the next line will never execute because common.fail() (and, in the existing test, assert(false, ...)) result in the termination of the program.

@Trott
Copy link
Member

Trott commented Dec 22, 2016

Maybe @cjihrig @mcollina or @bnoordhuis can give this a quick review? (Looping in the people suggested for cluster stuff in onboarding-extras.md.)

Trott
Trott approved these changes Dec 24, 2016
View reviewed changes
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

@Trott
Copy link
Member

Trott commented Dec 24, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/5569/

cjihrig
cjihrig approved these changes Dec 24, 2016
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig 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 the CI is happy.

Trott pushed a commit to Trott/io.js that referenced this pull request Dec 24, 2016
@amarzavery @Trott
test: refactor test-cluster-send-handle-twice.js
7e7062c 
- `var` --> `const` as applicable
- `assert.equal`  --> `assert.strictEqual`
- `assert(false, ..)` --> `common.fail()`
- `common.mustCall` for functions that need to be called exactly once
- modified an `assert(!signal, 'Worker exited by a signal');` call to
  `assert.strictEqual(signal, null);` call as that made more sense

PR-URL: nodejs#10049
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@Trott
Copy link
Member

Trott commented Dec 24, 2016

Landed in 7e7062c.
Thanks for the contribution! 🎉

@Trott Trott closed this Dec 24, 2016
targos pushed a commit that referenced this pull request Dec 26, 2016
@amarzavery @targos
test: refactor test-cluster-send-handle-twice.js
f9f2cda 
- `var` --> `const` as applicable
- `assert.equal`  --> `assert.strictEqual`
- `assert(false, ..)` --> `common.fail()`
- `common.mustCall` for functions that need to be called exactly once
- modified an `assert(!signal, 'Worker exited by a signal');` call to
  `assert.strictEqual(signal, null);` call as that made more sense

PR-URL: #10049
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 27, 2016
Merged
targos pushed a commit that referenced this pull request Dec 28, 2016
@amarzavery @targos
test: refactor test-cluster-send-handle-twice.js
1b77fd8 
- `var` --> `const` as applicable
- `assert.equal`  --> `assert.strictEqual`
- `assert(false, ..)` --> `common.fail()`
- `common.mustCall` for functions that need to be called exactly once
- modified an `assert(!signal, 'Worker exited by a signal');` call to
  `assert.strictEqual(signal, null);` call as that made more sense

PR-URL: #10049
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@evanlucas evanlucas mentioned this pull request Jan 3, 2017
Merged
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
@amarzavery @evanlucas
test: refactor test-cluster-send-handle-twice.js
c0db903 
- `var` --> `const` as applicable
- `assert.equal`  --> `assert.strictEqual`
- `assert(false, ..)` --> `common.fail()`
- `common.mustCall` for functions that need to be called exactly once
- modified an `assert(!signal, 'Worker exited by a signal');` call to
  `assert.strictEqual(signal, null);` call as that made more sense

PR-URL: #10049
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
@amarzavery @evanlucas
test: refactor test-cluster-send-handle-twice.js
bbe618d 
- `var` --> `const` as applicable
- `assert.equal`  --> `assert.strictEqual`
- `assert(false, ..)` --> `common.fail()`
- `common.mustCall` for functions that need to be called exactly once
- modified an `assert(!signal, 'Worker exited by a signal');` call to
  `assert.strictEqual(signal, null);` call as that made more sense

PR-URL: #10049
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@amarzavery @MylesBorins
test: refactor test-cluster-send-handle-twice.js
3a9b5b4 
- `var` --> `const` as applicable
- `assert.equal`  --> `assert.strictEqual`
- `assert(false, ..)` --> `common.fail()`
- `common.mustCall` for functions that need to be called exactly once
- modified an `assert(!signal, 'Worker exited by a signal');` call to
  `assert.strictEqual(signal, null);` call as that made more sense

PR-URL: #10049
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@amarzavery @MylesBorins
test: refactor test-cluster-send-handle-twice.js
771015f 
- `var` --> `const` as applicable
- `assert.equal`  --> `assert.strictEqual`
- `assert(false, ..)` --> `common.fail()`
- `common.mustCall` for functions that need to be called exactly once
- modified an `assert(!signal, 'Worker exited by a signal');` call to
  `assert.strictEqual(signal, null);` call as that made more sense

PR-URL: #10049
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
@amarzavery @MylesBorins
test: refactor test-cluster-send-handle-twice.js
6db76da 
- `var` --> `const` as applicable
- `assert.equal`  --> `assert.strictEqual`
- `assert(false, ..)` --> `common.fail()`
- `common.mustCall` for functions that need to be called exactly once
- modified an `assert(!signal, 'Worker exited by a signal');` call to
  `assert.strictEqual(signal, null);` call as that made more sense

PR-URL: #10049
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
cluster Issues and PRs related to the cluster subsystem. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@amarzavery @Trott @cjihrig @imyller @targos @nodejs-github-bot