test: use common.mustCall in test-tls-connect-given-socket.js#12592
Uh oh!
There was an error while loading. Please reload this page.
Conversation
vsemozhetbyt commented Apr 22, 2017
| // Immediate death socket | ||
| constimmediateDeath=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]>
Trott commented May 15, 2017
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]>
7 participants
remove process block and add common.mustCall
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test