-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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 test-tls-timeout-server-2 #9876
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,32 +1,32 @@ | ||
'use strict'; | ||
var common = require('../common'); | ||
var assert = require('assert'); | ||
const common = require('../common'); | ||
const assert = require('assert'); | ||
|
||
if (!common.hasCrypto) { | ||
common.skip('missing crypto'); | ||
return; | ||
} | ||
var tls = require('tls'); | ||
const tls = require('tls'); | ||
|
||
var fs = require('fs'); | ||
const fs = require('fs'); | ||
|
||
var options = { | ||
const options = { | ||
key: fs.readFileSync(common.fixturesDir + '/keys/agent1-key.pem'), | ||
cert: fs.readFileSync(common.fixturesDir + '/keys/agent1-cert.pem') | ||
}; | ||
|
||
var server = tls.createServer(options, function(cleartext) { | ||
var s = cleartext.setTimeout(50, function() { | ||
const server = tls.createServer(options, common.mustCall(function(cleartext) { | ||
const s = cleartext.setTimeout(50, common.mustCall(function() { | ||
cleartext.destroy(); | ||
server.close(); | ||
}); | ||
})); | ||
assert.ok(s instanceof tls.TLSSocket); | ||
}); | ||
})); | ||
|
||
server.listen(0, function() { | ||
server.listen(0, common.mustCall(function() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unsure what the convention of using My instinct is to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
tls.connect({ | ||
host: '127.0.0.1', | ||
port: this.address().port, | ||
rejectUnauthorized: false | ||
}); | ||
}); | ||
})); |
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 change isn't needed so much, as there are no assertions inside. If this isn't called, the test should fail without
common.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.
In that case, would you prefer we add an assertion inside as well? It seems to me that if this test exits prior to cleanup, that indicates that something is wrong.
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.
No, I don't think we need to add extra assertions unless there is something that should be tested that currently is not. We use
common.mustCall()
to make sure that the paths containing the assertions are hit. If there are no assertions in that path, then the test should fail naturally if the code paths are not run. If the test can pass without the code being run, then the code can probably just be removed :-)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.
Don't we want to ensure that
cleartext.setTimeout
actually calls its callback? I get that we don't want to usecommon.mustCall
when there are no assertions, so I'm thinking we want to actually add an assertion that the function is called (e.g., by usingassert.ok(true)
within the function body or whatever is most idiomatic).I'm not entirely sure what the intention behind this test file is; is it really just to check the type of
tls.createServer
's argument? Given the name of the test file, I figured it was that the timeout actually gets called.I took a look at the other test file for
test-tls-timeout-server
and it seems to just be testing thetlsClientError
event.Incidentally, on line 21 it does something similar to what I was doing in this file. Depending on the outcome of our conversation here, it seems as though that file should be changed as well?
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.
Sorry for the delay responding. I lost track of this in my email.
If the
cleartext.setTimeout()
callback is not called, thenserver.close()
won't be called either, keeping the event loop open, and timing out 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.
No worries, I'm sure there's been a lot of email with all of the Code & Learn commits going around.
Your response makes sense, so I just pushed a commit that removes this inner
common.mustCall
. For consistency, would you like me to submit a separate PR that also removes something very similar fromtest-tls-timeout-server
?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.
Looking at that test, it looks like
common.mustCall()
is required either where it currently is, or on theserver.listen()
callback. I don't see much point in changing that aspect of the test, but there are somevar
s that could be changed toconst
in that test.