test: remove timers from test-tls-socket-close #53019
Conversation
nodejs-github-bot
added
needs-ci
lpinca
force-pushed
the
remove/timers-from-test-tls-socket-close
branch
from
c679e6f
to
5581681
Compare
github-actions
bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
github-actions
bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
| netSocket.destroy(); | ||
| assert.strictEqual(netSocket.destroyed, true); | ||
|
|
||
| setImmediate(() => { |
There was a problem hiding this comment.
We have separate tests for setImmediate().
There was a problem hiding this comment.
I don't really get what you mean, common.mustCall wouldn't be there to validate setImmediate behavior, but to ensure the assertion are actually run. Anyway, it doesn't really matter.
There was a problem hiding this comment.
We are testing that the callback of setImmediate() is called in other tests.
There was a problem hiding this comment.
It's not called if e.g. process.exit is called before it's picked up
There was a problem hiding this comment.
There is no process.exit() in the test.
There was a problem hiding this comment.
Adding common.mustCall would ensure we catch it if it was ever added
There was a problem hiding this comment.
I've added it only where it is needed given the current assumptions. If the logic changes there are other more places where it would be needed.
| @@ -15,6 +15,10 @@ const cert = fixtures.readKey('agent2-cert.pem'); | |||
| let serverTlsSocket; | |||
| const tlsServer = tls.createServer({ cert, key }, (socket) => { | |||
| serverTlsSocket = socket; | |||
| socket.on('data', (chunk) => { | |||
There was a problem hiding this comment.
No, only one byte is sent by the other peer.
aduh95
added
author ready
nodejs-github-bot
removed
the
commit-queue
|
Landed in a81d833 |
Fixes: #49902 PR-URL: #53019 Reviewed-By: Antoine du Hamel <[email protected]>
Fixes: nodejs#49902 PR-URL: nodejs#53019 Reviewed-By: Antoine du Hamel <[email protected]>
Fixes: nodejs#49902 PR-URL: nodejs#53019 Reviewed-By: Antoine du Hamel <[email protected]>
Fixes: #49902 PR-URL: #53019 Reviewed-By: Antoine du Hamel <[email protected]>
Fixes: #49902 PR-URL: #53019 Reviewed-By: Antoine du Hamel <[email protected]>
Fixes: #49902