-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: check for error on Windows #2035
Conversation
|
||
function noop() {} | ||
|
||
if (cluster.isMaster) { | ||
var worker1 = cluster.fork(); | ||
|
||
if (isWindows) { | ||
worker1.on('error', function(er) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe wrap the callback in common.mustCall(...)
?
LGTM with a suggestion. Can you remove the console.log statements? |
// test | ||
if (process.platform == 'win32') | ||
process.exit(0); | ||
var isWindows = process.platform === 'win32'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use common.isWindows
here.
I agree about the |
Instead of not running the dgram-bind-shared-ports on Windows, check that it gets ENOTSUP.
Per feedback from @bnoordhuis and @cjihrig:
New CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/71/ |
Yet another CI to confirm that the smartOS failure was a flaky test and not something in this PR: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/72/ |
Yea, this PR only changes a test, so unless |
worker1.kill(); | ||
}; | ||
|
||
worker1.on('error', common.mustCall(checkErrType, 1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 is the default, you can just write it as worker1.on('error', common.mustCall(function(err) {
(although it's not wrong this way.)
LGTM |
Third time is the charm on the CI run. All green: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/73/ |
Instead of not running the dgram-bind-shared-ports on Windows, check that it gets ENOTSUP. PR-URL: #2035 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Merged in 8e9089a |
Instead of not running the dgram-bind-shared-ports on Windows, check that it gets ENOTSUP. PR-URL: nodejs#2035 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Instead of not running the dgram-bind-shared-ports
on Windows, check that it gets ENOTSUP.
CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/70/