test: use common.mustCall in test-tls-connect-given-socket.js #12592
Conversation
| @@ -72,14 +66,14 @@ const server = tls.createServer(options, (socket) => { | |||
|
|
|||
| // Immediate death socket | |||
| const immediateDeath = net.connect(port); | |||
| establish(immediateDeath).destroy(); | |||
| establish(immediateDeath, 0).destroy(); | |||
There was a problem hiding this comment.
I don't think it makes sense to pass 0 to common.mustCall() as the expected argument. I would expect any common.mustCall() to be passed a value of 1 (or undefined) or higher. If the intention is that it should never be called, common.mustNotCall() should be used instead.
There was a problem hiding this comment.
undefined will result in the default of 1. I think either we need to be OK with sending 0 to common.mustCall() (I am, at least in this case) or else add some logic to switch out whether common.mustCall() or common.mustNotCall() gets called. That works too, although might be a bit much. I'm fine with either.
There was a problem hiding this comment.
This one was a bit of let's get it working. establish() is called multiple times with different expected results each time and this was the easiest to get things going. The test as written, however, could use a bit of a bigger refactoring.
jasnell
changed the title
jasnell
added
the
code-and-learn
PR-URL: nodejs#12592 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
|
Landed in 72e3dda. |
PR-URL: nodejs#12592 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #12592 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #12592 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
remove process block and add common.mustCall
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test