-
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: Refactors variables to ES6 compatible and newer assertions #10023
Conversation
- changes var to const/let - changes assert.equal to assert.strictEqual **Note: `process.on('exit')` wasn't able to change the handler to `common.mustCall` because `common.mustCall` actually uses `process.on('exit')` internally**
- changes var to const/let - change assert.equal to assert.strictEqual **Note: `process.on('exit')` wasn't able to change the handler to `common.mustCall` because `common.mustCall` actually uses `process.on('exit')` internally**
assert.notEqual(serverLog[i], serverLog[i + 1]); | ||
assert.equal(ticketLog[i], ticketLog[i + 1]); | ||
assert.strictEqual(ticketLog[i], ticketLog[i + 1]); | ||
|
||
// 2nd connection should have different ticket | ||
assert.notEqual(ticketLog[i], ticketLog[i + naturalServers.length]); |
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.
Couldn't this also use assert.notStrictEqual()
?
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.
Ah, I'll go ahead and make changes tonight. I was initially just checking for 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, but there are probably other improvements that could be made.
- changes `notEqual` to `notStrictEqual`
@cjihrig went ahead and made the changes. Ready to be reviewed again. |
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
Merging now |
Landed in 0c4c225. |
- changes var to const/let - changes assert.equal to assert.strictEqual - changes `notEqual` to `notStrictEqual` PR-URL: #10023 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
- changes var to const/let - changes assert.equal to assert.strictEqual - changes `notEqual` to `notStrictEqual` PR-URL: #10023 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
- changes var to const/let - changes assert.equal to assert.strictEqual - changes `notEqual` to `notStrictEqual` PR-URL: nodejs#10023 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
- changes var to const/let - changes assert.equal to assert.strictEqual - changes `notEqual` to `notStrictEqual` PR-URL: #10023 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
- changes var to const/let - changes assert.equal to assert.strictEqual - changes `notEqual` to `notStrictEqual` PR-URL: #10023 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
- changes var to const/let - changes assert.equal to assert.strictEqual - changes `notEqual` to `notStrictEqual` PR-URL: #10023 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
test
Description of change
var
to eitherconst
orlet
where applicableassert.equal
toassert.strictEqual
for more "strict" checkNote:- even though ticket was asking to change
process.on('exit')
handler tocommon.mustCall
- this was omitted as this kept breaking the test (my guess and from discussion with others was that it's becausecommon.mustCall
hasprocess.on('exit')
internally which never firesprocess.on('exit')
in the test