test: use common.mustCall in test-tls-connect-given-socket.js#12592
test: use common.mustCall in test-tls-connect-given-socket.js#12592vperezma wants to merge 2 commits intonodejs:masterfrom
Conversation
| // 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 <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Landed in 72e3dda. |
PR-URL: nodejs#12592 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #12592 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #12592 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
7 participants
remove process block and add common.mustCall
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test