-
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: replace var with const/let and use common.mustCall for cb #9959
Conversation
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.
Merge commits as single commit!
92d4a6e
to
fff2a4a
Compare
@princejwesley I merged them. |
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 one small nit.
var common = require('../common'); | ||
var assert = require('assert'); | ||
const common = require('../common'); | ||
const assert = require('assert'); |
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.
Can you move this line below the hasCrypto
check?
remove process.on('exit') because all callbacks are wrapped by common.mustCall.
fff2a4a
to
1c38ce2
Compare
As @cjihrig said, I moved the require statement. |
Thanks! Landed with slightly trimmed commit message in 4913acd. |
remove process.on('exit') because all callbacks are wrapped by common.mustCall. PR-URL: #9959 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Roman Reiss <[email protected]>
@silverwind Thanks! |
remove process.on('exit') because all callbacks are wrapped by common.mustCall. PR-URL: #9959 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Roman Reiss <[email protected]>
remove process.on('exit') because all callbacks are wrapped by common.mustCall. PR-URL: #9959 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Roman Reiss <[email protected]>
remove process.on('exit') because all callbacks are wrapped by common.mustCall. PR-URL: #9959 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Roman Reiss <[email protected]>
remove process.on('exit') because all callbacks are wrapped by common.mustCall. PR-URL: #9959 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Roman Reiss <[email protected]>
remove process.on('exit') because all callbacks are wrapped by common.mustCall. PR-URL: #9959 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Roman Reiss <[email protected]>
remove process.on('exit') because all callbacks are wrapped by common.mustCall. PR-URL: #9959 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Roman Reiss <[email protected]>
remove process.on('exit') because all callbacks are wrapped by common.mustCall. PR-URL: #9959 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Roman Reiss <[email protected]>
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
Description of change
common.mustCall
for callbacksprocess.on('exit')
because all callbacks are wrapped bycommon.mustCall