-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
changed var to const, added strict equality checks #8762
Conversation
f8c67e8
to
7e4c1b3
Compare
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.
This looks pretty good so far, thank you! I find this test kind of hard to read, so if you have any ideas for other improvements/explaining comments/whatever, please feel free to add that here!
check++; | ||
setTimeout(checkFunction, 100); | ||
return; | ||
} | ||
assert.equal(0, openCount, 'no leaked file descriptors using ' + | ||
assert.strictEqual(0, openCount, 'no leaked file descriptors using ' + | ||
endFn + '() (got ' + openCount + ')'); |
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.
Could you try to indent the second line here so that the arguments line up 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.
Oh, and just a suggestion, but maybe this would be better using a template string here? (If you agree that that might be more readable, that is.)
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.
Template string was a great idea. I've followed up with a commit - how do you like the look of it?
LGTM with @addaleax's comments and if CI has no surprises |
LGTM 🎉🎉🎉 |
Changed var to const where appropriate. Substituted assert.strictEqual for assert.equal for better type checks.
878f9b7
to
72e2def
Compare
@thealphanerd squasht! |
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!
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
LGTM |
Changed var to const where appropriate. Substituted assert.strictEqual for assert.equal for better type checks. PR-URL: #8762 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Landed in 52b6cfb |
Changed var to const where appropriate. Substituted assert.strictEqual for assert.equal for better type checks. PR-URL: #8762 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Changed var to const where appropriate. Substituted assert.strictEqual for assert.equal for better type checks. PR-URL: #8762 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
test fs
Description of change
Changed var to const where appropriate. Substituted
assert.strictEqual for assert.equal for better type checks.