-
Notifications
You must be signed in to change notification settings - Fork 30k
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: updated test-domain-exit-dispose-again #10003
Conversation
setTimeout at 49:5 requires two arguments, so I added TIMEOUT_DURATION as the second argument. On lines 72 and 73 changed assert.equal() to assert.strictEqual().
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.
LGTM with green CI
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.
CI failures are unrelated. |
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.
The timeout duration for a timer that should never run should be set to the minimum IMO, which is 1
.
@@ -51,7 +51,7 @@ setTimeout(function firstTimer() { | |||
'a domain that should be disposed.'); | |||
disposalFailed = true; | |||
process.exit(1); | |||
}); | |||
}, TIMEOUT_DURATION); |
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.
I think this should be 1
instead of TIMEOUT_DURATION
.
Landing. Will fix the one nit while doing so. |
setTimeout at 49:5 requires two arguments. On lines 72 and 73 changed assert.equal() to assert.strictEqual(). PR-URL: nodejs#10003 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Landed in ac78812. |
setTimeout at 49:5 requires two arguments. On lines 72 and 73 changed assert.equal() to assert.strictEqual(). PR-URL: #10003 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
setTimeout at 49:5 requires two arguments. On lines 72 and 73 changed assert.equal() to assert.strictEqual(). PR-URL: nodejs#10003 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
setTimeout at 49:5 requires two arguments. On lines 72 and 73 changed assert.equal() to assert.strictEqual(). PR-URL: #10003 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
setTimeout at 49:5 requires two arguments. On lines 72 and 73 changed assert.equal() to assert.strictEqual(). PR-URL: #10003 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
setTimeout at 49:5 requires two arguments. On lines 72 and 73 changed assert.equal() to assert.strictEqual(). PR-URL: #10003 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
test
Description of change
setTimeout at 49:5 requires two arguments, so I added
TIMEOUT_DURATION as the second argument. On lines 72
and 73 changed assert.equal() to assert.strictEqual().