Skip to content
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.

Sign up for GitHub

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

Jump to bottom

test: refactor test-tls-timeout-server-2 #9876

Closed
wants to merge 2 commits into from
Closed

test: refactor test-tls-timeout-server-2 #9876

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9a01cf3
test: refactor test-tls-timeout-server-2
drifkin Dec 1, 2016
8b77629
fixup for github comments
drifkin Dec 4, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions test/parallel/test-tls-timeout-server-2.js
View file
Open in desktop
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() {
Copy link
Contributor

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().

Copy link
Contributor Author

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.

Copy link
Contributor

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 :-)

Copy link
Contributor Author

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 use common.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 using assert.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 the tlsClientError 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?

Copy link
Contributor

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, then server.close() won't be called either, keeping the event loop open, and timing out the test.

Copy link
Contributor Author

@drifkin drifkin Dec 4, 2016 •
edited
Loading

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 from test-tls-timeout-server?

Copy link
Contributor

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 the server.listen() callback. I don't see much point in changing that aspect of the test, but there are some vars that could be changed to const in that test.

cleartext.destroy();
server.close();
});
}));
assert.ok(s instanceof tls.TLSSocket);
});
}));

server.listen(0, function() {
server.listen(0, common.mustCall(function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure what the convention of using common.mustCall is. This one could be removed and the test would still work as expected, since line 18 also has a common.mustCall.

My instinct is to use common.mustCall as much as possible to avoid any problems in the future as this code is further refactored, but happy to change this based on what other people tend to do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

common.mustCall() should be used to ensure that all assertions are executed.

tls.connect({
host: '127.0.0.1',
port: this.address().port,
rejectUnauthorized: false
});
});
}));