test: refactor and deflake test-tls-sni-server-client #27426
Conversation
| const fixtures = require('../common/fixtures'); | ||
| const { strictEqual } = require('assert'); | ||
| const { connect, createServer } = require('tls'); | ||
| const { readKey } = require('../common/fixtures'); |
There was a problem hiding this comment.
Not blocking, but why the mass-introduction of destructuring? To me, it makes the subsequent code harder to understand. Later on, if I see fixtures.readKey(), I know exactly what that means. But if I just see readKey()...I'm less sure. Doubly so for connect() on the line above.
Again, not blocking because it's ultimately a style choice, but I'm genuinely curious if there's a motivation/advantage I'm not seeing that I should be seeing.
There was a problem hiding this comment.
No particular reason, in most cases it is obvious and shorter. For example assert.strictEqual why not only strictEqual and the same for createServer, mustCall, mustNotCall. The others are for consistency.
I will revert this to make the diff smaller.
There was a problem hiding this comment.
When I was working in python (which js's import syntax seems to be modelled on), we forbade this usage in our coding standards. Functions were always called with their leading module name, tls.connect not connect (which could be net.connect). We found it makes code harder to understand when we couldn't see where a function came from. It seems to me that the js community, or perhaps node, is going the opposite way. Its a stylistic choice, I don't feel strongly enough about it to try to change it, but I'm in agreement with @Trott that eliding the module name makes subsequent code harder to read.
There was a problem hiding this comment.
I partially agree but it in my opinion it depends. On a large file it makes sense. When there are just few lines of code, it's easy to see where it's coming from.
One thing is for sure though, I don't like to mix things, so it's always or never for me :)
|
(Needs a rebase.) |
- Run all tests in parallel. - Move `socket.end()` call to client. - Use `common.mustCall()` and `common.mustNotCall()`. Fixes: nodejs#27219 Refs: nodejs#27300
lpinca
force-pushed
the
deflake/test-tls-sni-server-client
branch
from
e61b13c
to
8d63f4c
Compare
|
Landed in bc0a330 |
- Run all tests in parallel. - Move `socket.end()` call to client. - Use `common.mustCall()` and `common.mustNotCall()`. Fixes: nodejs#27219 Refs: nodejs#27300 PR-URL: nodejs#27426 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
socket.end()call to client.common.mustCall()andcommon.mustNotCall().Fixes: #27219
Refs: #27300
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes