test: refactoring test with common.mustCall by weewey · Pull Request #12702 · nodejs/node

weewey · 2017-04-27T18:25:23Z

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)

Fishrock123

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

Fishrock123 · 2017-04-27T18:32:48Z

test/parallel/test-https-simple.js

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.

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

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 · 2017-04-27T18:40:55Z

What's going on with the first three commits?

weewey

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

weewey · 2017-04-27T18:43:27Z

test/parallel/test-https-simple.js

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

weewey · 2017-04-28T03:35:04Z

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

cjihrig · 2017-04-28T14:22:28Z

test/parallel/test-https-simple.js

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

-server.listen(0, function() {
+server.listen(0, function(){


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

cjihrig · 2017-04-28T14:22:40Z

test/parallel/test-https-simple.js

  noCertCheckOptions.Agent = new https.Agent(noCertCheckOptions);

-  const req = https.request(noCertCheckOptions, function(res) {
+  const req = https.request(noCertCheckOptions, function(res){


Same comment here.

cjihrig · 2017-04-28T14:24:11Z

test/parallel/test-https-simple.js

    });

-    res.on('end', function() {
+    res.on('end', common.mustCall(() => {


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

cjihrig · 2017-04-28T14:24:30Z

test/parallel/test-https-simple.js

-process.on('exit', function() {
-  assert.strictEqual(successful, tests);
-});
+function testSucceeded (){


function testSucceeded() {

cjihrig · 2017-04-28T14:25:30Z

test/parallel/test-https-simple.js

-  assert.strictEqual(successful, tests);
-});
+function testSucceeded (){
+  server.close();


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

…).port with port

weewey · 2017-04-29T07:31:46Z

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 · 2017-04-29T14:19:42Z

@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 · 2017-04-29T14:25:30Z

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 · 2017-05-02T16:08:22Z

@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.

cjihrig

LGTM if the CI passes.

santigimeno · 2017-05-05T09:42:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/7885/

lpinca · 2017-05-07T09:55:59Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/7925/

lpinca · 2017-05-07T10:02:19Z

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

addressed

addaleax · 2017-05-07T21:17:15Z

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

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>

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>

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>

Fishrock123 previously requested changes Apr 27, 2017

View reviewed changes

weewey commented Apr 27, 2017

View reviewed changes

mscdex added https Issues or PRs related to the https subsystem. test Issues and PRs related to the tests. labels Apr 27, 2017

test: refactoring test with common.mustCall

4ca46bb

weewey force-pushed the wee2 branch from de7a74a to 4ca46bb Compare April 28, 2017 03:33

cjihrig requested changes Apr 28, 2017

View reviewed changes

improved formatting, added common.mustCall and refactor this.address(…

78753ee

…).port with port

adding back assert.strictEqual to track both requests are completed b…

047045c

…efore server is closed. Adding common.mustCall to checkCertReq.

cjihrig approved these changes May 2, 2017

View reviewed changes

jasnell approved these changes May 2, 2017

View reviewed changes

lpinca approved these changes May 7, 2017

View reviewed changes

addaleax approved these changes May 7, 2017

View reviewed changes

addaleax closed this May 7, 2017

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

gibfahn added the land-on-v6.x label Jun 20, 2017

MylesBorins mentioned this pull request Jul 18, 2017

v6.11.2 proposal #14356

Merged

leonardoadame mentioned this pull request Dec 2, 2023

[Snyk] Fix for 1 vulnerabilities leonardoadame/node#427

Open

aliscco mentioned this pull request Dec 25, 2023

[Snyk] Security upgrade jest from 26.4.2 to 28.0.0 aliscco/alisco-node#1181

Open

leonardoadame mentioned this pull request Jan 3, 2024

[Snyk] Security upgrade jest from 26.6.3 to 28.0.0 leonardoadame/node#590

Open

leonardoadame mentioned this pull request May 13, 2024

[Snyk] Fix for 2 vulnerabilities leonardoadame/node#831

Open

This was referenced May 14, 2024

[Snyk] Security upgrade jest from 24.9.0 to 28.0.0 aliscco/alisco-node#1452

Open

[Snyk] Fix for 2 vulnerabilities aliscco/alisco-node#1499

Open

leonardoadame mentioned this pull request May 16, 2024

[Snyk] Security upgrade jest from 25.5.4 to 28.0.0 leonardoadame/node#969

Open

Uh oh!

Conversation

weewey commented Apr 27, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Apr 27, 2017

Uh oh!

weewey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weewey commented Apr 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weewey commented Apr 29, 2017

Uh oh!

cjihrig commented Apr 29, 2017

Uh oh!

cjihrig commented Apr 29, 2017

Uh oh!

weewey commented May 2, 2017

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

santigimeno commented May 5, 2017

Uh oh!

lpinca commented May 7, 2017

Uh oh!

lpinca commented May 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented May 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Fishrock123 Apr 27, 2017 •

edited

Loading

lpinca commented May 7, 2017 •

edited

Loading