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.

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: refactoring test with common.mustCall #12702

Closed
wants to merge 3 commits into from
Closed

Conversation

weewey
Copy link
Contributor

@weewey weewey commented Apr 27, 2017

test: refactoring test with common.mustCall

Removed assert.strictEqual using common.mustCall

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

Taking a quick look, there are a couple other places where common.mustCall() could be added, e.g. server.listen(, https.request(, etc.

if (successful === tests) {
server.close();
}
server.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

When this is run later, it picks up the server variable still? 🤔😖

Maybe we should make this into a function declaration (no assignment) and put it at the bottom of the file for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

server.close() is within the testSucceeded function? and it is only called after this

checkCertReq.on('error', function(e) {
	assert.strictEqual(e.code, 'UNABLE_TO_VERIFY_LEAF_SIGNATURE');
    testSucceeded();
})

@ line 100

Copy link
Contributor

@Fishrock123 Fishrock123 Apr 27, 2017

Choose a reason for hiding this comment

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

I would prefer you change it to a function declaration (function testSucceeded() without the const ... =.) and move it to the bottom of the file. That should be more clear.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 27, 2017

What's going on with the first three commits?

Copy link
Contributor Author

@weewey weewey left a comment

Choose a reason for hiding this comment

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

I'm not too sure how those were generated or even committed actually.

if (successful === tests) {
server.close();
}
server.close();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

server.close() is within the testSucceeded function? and it is only called after this

checkCertReq.on('error', function(e) {
	assert.strictEqual(e.code, 'UNABLE_TO_VERIFY_LEAF_SIGNATURE');
    testSucceeded();
})

@ line 100

@mscdex mscdex added https Issues or PRs related to the https subsystem. test Issues and PRs related to the tests. labels Apr 27, 2017
@weewey
Copy link
Contributor Author

weewey commented Apr 28, 2017

changed testSucceeded to a function declaration and put it at the bottom of the file

@@ -55,7 +45,7 @@ const serverCallback = common.mustCall(function(req, res) {

const server = https.createServer(options, serverCallback);

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

Choose a reason for hiding this comment

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

Please add the space back. Does this pass make jslint?

@@ -66,16 +56,15 @@ server.listen(0, function() {
};
noCertCheckOptions.Agent = new https.Agent(noCertCheckOptions);

const req = https.request(noCertCheckOptions, function(res) {
const req = https.request(noCertCheckOptions, function(res){
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here.

let responseBody = '';
res.on('data', function(d) {
responseBody = responseBody + d;
});

res.on('end', function() {
res.on('end', common.mustCall(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The enclosing https.request() and server.listen() callbacks need to have common.mustCall() too, or there is no guarantee that this will execute.

process.on('exit', function() {
assert.strictEqual(successful, tests);
});
function testSucceeded (){
Copy link
Contributor

Choose a reason for hiding this comment

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

function testSucceeded() {

assert.strictEqual(successful, tests);
});
function testSucceeded (){
server.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is only called from one place, you can just inline the server.close() there.

@weewey
Copy link
Contributor Author

weewey commented Apr 29, 2017

port: this.address().port,
TypeError: this.address is not a function

after adding common.mustCall to server.listen, it resulted in the above error. So I had to define const port = server.address().port.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 29, 2017

@weewey that error was most likely because of the change from a function to an arrow function, not the use of common.mustCall(). Either way, it looks like you got it straightened out.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 29, 2017

Looking at the entire test, it looks like this has a race condition now. There are two parallel requests made, and the server is closed when the checkCertReq errors. If that is the first request to be handled (even though it is the second request created), then the test will fail.

@weewey
Copy link
Contributor Author

weewey commented May 2, 2017

@cjihrig Thanks for helping to review. Yes true. I think it will be better if I bring back the assert but keeping the common.mustCall. So only after both requests are made, then server.close() is ran.

…efore server is closed. Adding common.mustCall to checkCertReq.
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM if the CI passes.

@santigimeno
Copy link
Member

@lpinca
Copy link
Member

lpinca commented May 7, 2017

@lpinca
Copy link
Member

lpinca commented May 7, 2017

@weewey can you please rebase against master and resolve conflicts? Although GitHub doesn't report any issues, the patch doesn't land cleanly.

@addaleax
Copy link
Member

addaleax commented May 7, 2017

Landed in 152966d, thanks for the PR! :)

@addaleax addaleax closed this May 7, 2017
addaleax pushed a commit that referenced this pull request May 7, 2017
PR-URL: #12702
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
PR-URL: nodejs#12702
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
PR-URL: #12702
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
https Issues or PRs related to the https subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants