test: refactor test-tls-timeout-server-2 #9876
Conversation
* Use `common.mustCall` for all callbacks * Use `const` instead of `var`
|
|
||
| server.listen(0, function() { | ||
| server.listen(0, common.mustCall(function() { |
There was a problem hiding this comment.
Unsure what the convention of using common.mustCall is. This one could be removed and the test would still work as expected, since line 18 also has a common.mustCall.
My instinct is to use common.mustCall as much as possible to avoid any problems in the future as this code is further refactored, but happy to change this based on what other people tend to do.
There was a problem hiding this comment.
common.mustCall() should be used to ensure that all assertions are executed.
| var server = tls.createServer(options, function(cleartext) { | ||
| var s = cleartext.setTimeout(50, function() { | ||
| const server = tls.createServer(options, common.mustCall(function(cleartext) { | ||
| const s = cleartext.setTimeout(50, common.mustCall(function() { |
There was a problem hiding this comment.
This change isn't needed so much, as there are no assertions inside. If this isn't called, the test should fail without common.mustCall().
There was a problem hiding this comment.
In that case, would you prefer we add an assertion inside as well? It seems to me that if this test exits prior to cleanup, that indicates that something is wrong.
There was a problem hiding this comment.
No, I don't think we need to add extra assertions unless there is something that should be tested that currently is not. We use common.mustCall() to make sure that the paths containing the assertions are hit. If there are no assertions in that path, then the test should fail naturally if the code paths are not run. If the test can pass without the code being run, then the code can probably just be removed :-)
There was a problem hiding this comment.
Don't we want to ensure that cleartext.setTimeout actually calls its callback? I get that we don't want to use common.mustCall when there are no assertions, so I'm thinking we want to actually add an assertion that the function is called (e.g., by using assert.ok(true) within the function body or whatever is most idiomatic).
I'm not entirely sure what the intention behind this test file is; is it really just to check the type of tls.createServer's argument? Given the name of the test file, I figured it was that the timeout actually gets called.
I took a look at the other test file for test-tls-timeout-server and it seems to just be testing the tlsClientError event.
Incidentally, on line 21 it does something similar to what I was doing in this file. Depending on the outcome of our conversation here, it seems as though that file should be changed as well?
There was a problem hiding this comment.
Sorry for the delay responding. I lost track of this in my email.
If the cleartext.setTimeout() callback is not called, then server.close() won't be called either, keeping the event loop open, and timing out the test.
There was a problem hiding this comment.
No worries, I'm sure there's been a lot of email with all of the Code & Learn commits going around.
Your response makes sense, so I just pushed a commit that removes this inner common.mustCall. For consistency, would you like me to submit a separate PR that also removes something very similar from test-tls-timeout-server?
There was a problem hiding this comment.
Looking at that test, it looks like common.mustCall() is required either where it currently is, or on the server.listen() callback. I don't see much point in changing that aspect of the test, but there are some vars that could be changed to const in that test.
imyller
added
the
code-and-learn
The `common.mustCall` isn’t necessary, since the test will fail from timing out if the callback is never called.
* Use `common.mustCall` for all callbacks * Use `const` instead of `var` PR-URL: #9876 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
|
Landed in 7a38e49. Thank you |
* Use `common.mustCall` for all callbacks * Use `const` instead of `var` PR-URL: #9876 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
* Use `common.mustCall` for all callbacks * Use `const` instead of `var` PR-URL: nodejs#9876 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
* Use `common.mustCall` for all callbacks * Use `const` instead of `var` PR-URL: nodejs#9876 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
* Use `common.mustCall` for all callbacks * Use `const` instead of `var` PR-URL: #9876 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
* Use `common.mustCall` for all callbacks * Use `const` instead of `var` PR-URL: #9876 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
* Use `common.mustCall` for all callbacks * Use `const` instead of `var` PR-URL: #9876 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
test
Description of change
common.mustCallfor all callbacksconstinstead ofvar