test: refactoring test with common.mustCall#12702
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Fishrock123 left a comment
Fishrock123 left a comment There was a problem hiding this comment.
Taking a quick look, there are a couple other places where common.mustCall() could be added, e.g. server.listen(, https.request(, etc.
test/parallel/test-https-simple.js Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 commented Apr 27, 2017
What's going on with the first three commits? |
weewey left a comment
weewey left a comment There was a problem hiding this comment.
I'm not too sure how those were generated or even committed actually.
test/parallel/test-https-simple.js Outdated
There was a problem hiding this comment.
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 added https 
weewey commented Apr 28, 2017
changed testSucceeded to a function declaration and put it at the bottom of the file |
test/parallel/test-https-simple.js Outdated
| constserver=https.createServer(options,serverCallback); | ||
| server.listen(0,function(){ | ||
| server.listen(0,function(){ |
There was a problem hiding this comment.
Please add the space back. Does this pass make jslint?
test/parallel/test-https-simple.js Outdated
| noCertCheckOptions.Agent=newhttps.Agent(noCertCheckOptions); | ||
| constreq=https.request(noCertCheckOptions,function(res){ | ||
| constreq=https.request(noCertCheckOptions,function(res){ |
| }); | ||
| res.on('end',function(){ | ||
| res.on('end',common.mustCall(()=>{ |
There was a problem hiding this comment.
The enclosing https.request() and server.listen() callbacks need to have common.mustCall() too, or there is no guarantee that this will execute.
test/parallel/test-https-simple.js Outdated
| process.on('exit',function(){ | ||
| assert.strictEqual(successful,tests); | ||
| }); | ||
| functiontestSucceeded(){ |
test/parallel/test-https-simple.js Outdated
| assert.strictEqual(successful,tests); | ||
| }); | ||
| functiontestSucceeded(){ | ||
| server.close(); |
There was a problem hiding this comment.
Since this is only called from one place, you can just inline the server.close() there.
weewey commented Apr 29, 2017
after adding common.mustCall to server.listen, it resulted in the above error. So I had to define const port = server.address().port. |
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 |
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 |
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.
santigimeno commented May 5, 2017
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 commented May 7, 2017
Landed in 152966d, thanks for the PR! :) |
PR-URL: #12702 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: nodejs#12702 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: #12702 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: #12702 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
9 participants
test: refactoring test with common.mustCall
Removed assert.strictEqual using common.mustCall
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)