-
Notifications
You must be signed in to change notification settings - Fork 29k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: refactor parallel/test-tls-delayed-attach #19421
Conversation
test: refactor parallel/test-tls-delayed-attach Added description to the test, replace function with arrow function and implemented common.mustCall
setTimeout(function() { | ||
const s = new tls.TLSSocket(c, { | ||
isServer: true, | ||
secureContext: tls.createSecureContext(options) | ||
}); | ||
|
||
s.on('data', function(chunk) { | ||
s.on('data', common.mustCall((chunk) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be wrapped in common.mustCall()
, I don't think. It is theoretically possible for the data
event to be emitted more than once (although perhaps not without the underlying implementation changing).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps common.mustCallAtLeast()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't use common.mustCallAtLeast()
here because it will never result in a failure if the callback never runs, because the process.on('exit')
will fail first. So adding common.mustCallAtLeast()
will only add cognitive overhead when trying to understand the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the on('exit')
callback be common.mustCall()
then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so should I removecommon.mustCall
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richardlau on('exit')
its giving error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the on('exit') callback be common.mustCall() then?
No, because common.mustCall()
does it's checks via .on('exit')
so you can't use common.mustCall()
on exit
handlers. I think the thing to do is not rely on common.mustCall()
for this. (I do think we tend to overuse it and should only use it where needed, but I may be in the minority on that.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so should I removecommon.mustCall ?
IMO, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes made :)
thank you for feedback
@@ -24,6 +24,8 @@ const common = require('../common'); | |||
if (!common.hasCrypto) | |||
common.skip('missing crypto'); | |||
|
|||
// Checks tls delay |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get a more descriptive comment here? This doesn't add any information that isn't in the file name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more description added
@@ -24,6 +24,8 @@ const common = require('../common'); | |||
if (!common.hasCrypto) | |||
common.skip('missing crypto'); | |||
|
|||
// Checks tls connection delay while sending and recieving data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this?
// This test tries to confirm that a TLS Socket will work as expected even if it
// is created after the original socket has received some data.
//
// Ref: https://github.com/nodejs/node-v0.x-archive/issues/6940
// Ref: https://github.com/nodejs/node-v0.x-archive/pull/6950
Landed in e048b15 🎉 |
test: refactor parallel/test-tls-delayed-attach Added description to the test, replace function with arrow function and implemented common.mustCall PR-URL: nodejs#19421 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
test: refactor parallel/test-tls-delayed-attach Added description to the test, replace function with arrow function and implemented common.mustCall PR-URL: #19421 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
test: refactor parallel/test-tls-delayed-attach
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
test