test: refactor test-beforeexit-event #10121
Conversation
cda4900 to
48691c1
Compare
There was a problem hiding this comment.
Same here, why is this necessary?
There was a problem hiding this comment.
I think we should just get rid of the revivals and deaths variables and just use common.mustCall() with the expected number of calls.
faa9ace to
0eddb09
Compare
There was a problem hiding this comment.
maybe better as before listener?
There was a problem hiding this comment.
I thought we were trying to remove all logging in tests unless required by the test?
There was a problem hiding this comment.
Maybe but it just gets swallowed anyways?
The console.log are definitely not required though so ¯\_(ツ)_/¯
There was a problem hiding this comment.
That's not how this works. :)
You will need to wrap the callback that you want to call before using it (as in, the variables at lines 9-15 already need the wrappers). Then the exit event listener can be removed.
cba9aff to
3e3fe42
Compare
|
@Fishrock123 requested changes have been completed, any additional feedback would be much appreciated. Thanks:-) |
There was a problem hiding this comment.
This can just be:
process.on('beforeExit', common.mustCall(() => {}), 4);There was a problem hiding this comment.
This 'error' handler can be removed since node will already throw if there are no 'error' handlers.
There was a problem hiding this comment.
What I had in mind instead of this is to wrap the enclosing function in common.mustCall(), for example:
setImmediate(common.mustCall(() => {
process.once('beforeExit', common.mustCall(tryTimer));
}));There was a problem hiding this comment.
Similarly, this would instead be:
setTimeout(common.mustCall(() => {
process.once('beforeExit', common.mustCall(tryListen));
}));There was a problem hiding this comment.
and this one would just be
.on('listening', common.mustCall(() => {
this.close();
}));There was a problem hiding this comment.
This needs to be:
process.once('beforeExit', common.mustCall(tryImmediate));29b7e97 to
8f8c63e
Compare
There was a problem hiding this comment.
These can probably just be () => {}
There was a problem hiding this comment.
Now that I think about it, I think we can actually get rid of this separate check entirely by just adding a
process.on('beforeExit', common.mustCall(() => {}));after the this.close() in tryListen().
8f8c63e to
5a9a898
Compare
|
LGTM |
|
CI clean except for failure on smartos which seems unrelated to this test. Opened #10166 to track that issue separately. |
|
@radelmann, one last thing before we land. We need your full name "Rob Adelmann" as the author for the commit. I could fix that as part of landing but probably better if you do this in your env and re-push. You should be able to change with: git commit --amend --author="Rob Adelmann adelmann@adobe.com" and if you can also update your git config so that future commits also have your full name that would be good. |
Just for clarity, feel free to use the name and email address you would like your commits to be credited to (or indicate that just “adelmann” is actually exactly what you prefer). |
- replaced var with const/let. - removed all console.log() statements. - removed deaths and revivals vars. - wrapped beforexit listener callbacks with common.mustCall(). - removed exit event listener.
5a9a898 to
4b254d8
Compare
|
Landed in 8960383, thanks for the contribution! |
- replaced var with const/let. - removed all console.log() statements. - removed deaths and revivals vars. - wrapped beforexit listener callbacks with common.mustCall(). - removed exit event listener. PR-URL: #10121 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
- replaced var with const/let. - removed all console.log() statements. - removed deaths and revivals vars. - wrapped beforexit listener callbacks with common.mustCall(). - removed exit event listener. PR-URL: #10121 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
- replaced var with const/let. - removed all console.log() statements. - removed deaths and revivals vars. - wrapped beforexit listener callbacks with common.mustCall(). - removed exit event listener. PR-URL: nodejs#10121 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
- replaced var with const/let. - removed all console.log() statements. - removed deaths and revivals vars. - wrapped beforexit listener callbacks with common.mustCall(). - removed exit event listener. PR-URL: #10121 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
- replaced var with const/let. - removed all console.log() statements. - removed deaths and revivals vars. - wrapped beforexit listener callbacks with common.mustCall(). - removed exit event listener. PR-URL: #10121 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
- replaced var with const/let. - removed all console.log() statements. - removed deaths and revivals vars. - wrapped beforexit listener callbacks with common.mustCall(). - removed exit event listener. PR-URL: #10121 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
test
Description of change
varwithconstorlet.console.log()statements.deathsandrevivalsvars.beforexitlistener callbacks withcommon.mustCall().exitevent listener.