test: update test-http-status-code to use countdown #17477

onneri · 2017-12-06T04:03:31Z

The test was changed to use countdown instead of validating
if the index of the array of codes exceeded the size of the
array.

Also the validation that was before didn't allow to execute
the last test, so the assert equal was to 5, and the last
status in the array wasn't tested. The change was to
assert 6 tests complete.

Refs: #17169

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

The test was changed to use countdown instead of validating if the index of the array of codes exceeded the size of the array. Also the validation that was before didn't allow to execute the last test, so the assert equal was to 5, and the last status in the array wasn't tested. The change was to assert 6 tests complete. Fixes: nodejs#17169

mithunsasidharan · 2017-12-06T09:12:47Z

test/parallel/test-http-status-code.js

    });
    response.resume();
  });
 }


 process.on('exit', function() {
-  assert.strictEqual(5, testsComplete);
+  assert.strictEqual(6, testsComplete);


How about replacing 6 with tests.length ?

This code can actually be removed, as well as the testsComplete variable. Since there's a Countdown now, there's no need to verify that all 6 test cases executed.

apapirovski

LGTM but some small changes needed

Thank you for taking this on!

apapirovski · 2017-12-06T14:37:05Z

test/parallel/test-http-status-code.js

    });
    response.resume();
  });
 }


 process.on('exit', function() {
-  assert.strictEqual(5, testsComplete);
+  assert.strictEqual(6, testsComplete);


This code can actually be removed, as well as the testsComplete variable. Since there's a Countdown now, there's no need to verify that all 6 test cases executed.

apapirovski · 2017-12-06T14:38:00Z

test/parallel/test-http-status-code.js

@@ -43,9 +45,6 @@ s.listen(0, nextTest);


 function nextTest() {
-  if (testIdx + 1 === tests.length) {
-    return s.close();
-  }
  const test = tests[testIdx];


We can just do test = tests.shift() here (and not declare it as a const).

apapirovski · 2017-12-06T14:38:14Z

test/parallel/test-http-status-code.js

@@ -23,13 +23,15 @@
 require('../common');
 const assert = require('assert');
 const http = require('http');
+const Countdown = require('../common/countdown');

 // Simple test of Node's HTTP ServerResponse.statusCode
 // ServerResponse.prototype.statusCode

 let testsComplete = 0;
 const tests = [200, 202, 300, 404, 451, 500];
 let testIdx = 0;


testsComplete and testIdx can be removed. Instead add let test;.

apapirovski · 2017-12-06T14:39:53Z

test/parallel/test-http-status-code.js


 // Simple test of Node's HTTP ServerResponse.statusCode
 // ServerResponse.prototype.statusCode

 let testsComplete = 0;
 const tests = [200, 202, 300, 404, 451, 500];
 let testIdx = 0;
+const countdown = new Countdown(tests.length, () => s.close());

 const s = http.createServer(function(req, res) {
  const t = tests[testIdx];


This can be removed and then the assert.strictEqual() can compare to the top scope test variable.

apapirovski · 2017-12-06T14:41:34Z

test/parallel/test-http-status-code.js

@@ -55,13 +54,14 @@ function nextTest() {
    response.on('end', function() {
      testsComplete++;
      testIdx += 1;


These variable additions would both be removed.

The test was changed to use countdown instead of validating if the index of the array of codes exceeded the size of the array. Also the validation that was before didn't allow to execute the last test, so the assert equal was to 5, and the last status in the array wasn't tested. The change was to assert 6 tests complete. Variable testIdx to manage index in array removed, using array shift instead to manage the actual test. Variable testsComplete removed, not necessary, Countdown takes care of running right amount of tests Added a test variable that has the actual code for testing Fixes: nodejs#17169

onneri · 2017-12-06T17:35:26Z

You are right, I did't notice that assertEqual for tests completed wasn't necessary due to the Countdown variable.
I made some changes to the suggestions, if you have more comments please let me know, thank you!

apapirovski

Changes LGTM. Thank you!

apapirovski · 2017-12-06T17:37:22Z

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

onneri · 2017-12-06T20:16:51Z

One test failed, could I do something about that?, or did I do something that produced that?

apapirovski · 2017-12-06T20:50:15Z

Not your fault at all, the CI is all green as far as this PR is concerned. The failure is completely unrelated. :)

PR-URL: #17477 Refs: #17169 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>

apapirovski · 2017-12-08T15:45:21Z

Landed in 07549c6

Thank you for your contribution @onneri! 👍

onneri · 2017-12-08T17:43:49Z

awesome, thank you, I'll try to contribute more times, @apapirovski what does "closed with unmerged commits" mean?

apapirovski · 2017-12-08T17:52:22Z

@onneri We don't use GitHub's buttons for merging, we have our own process for landing PRs so it just means that we didn't merge directly from your branch. As you'll note above, there's a reference to your commit landing on the master branch ("test: update http test to use Countdown"), listed right above my comment re: landing it.

Let me know if I can provide any more info! Thanks again for helping out. Looking forward to hopefully reviewing more PRs from you in the future :)

onneri · 2017-12-08T17:54:45Z

thank you again!

PR-URL: #17477 Refs: #17169 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>

nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 6, 2017

mithunsasidharan reviewed Dec 6, 2017

View reviewed changes

apapirovski approved these changes Dec 6, 2017

View reviewed changes

maclover7 approved these changes Dec 8, 2017

View reviewed changes

maclover7 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 8, 2017

apapirovski pushed a commit that referenced this pull request Dec 8, 2017

test: update http test to use Countdown

07549c6

PR-URL: #17477 Refs: #17169 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>

apapirovski closed this Dec 8, 2017

MylesBorins pushed a commit that referenced this pull request Dec 12, 2017

test: update http test to use Countdown

62fc68f

PR-URL: #17477 Refs: #17169 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>

MylesBorins pushed a commit that referenced this pull request Dec 12, 2017

test: update http test to use Countdown

7b8622f

PR-URL: #17477 Refs: #17169 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2017

gibfahn pushed a commit that referenced this pull request Dec 20, 2017

test: update http test to use Countdown

b54bc9c

PR-URL: #17477 Refs: #17169 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>

gibfahn added dont-land-on-v6.x labels Dec 20, 2017

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17772

Closed

gibfahn pushed a commit that referenced this pull request Dec 20, 2017

test: update http test to use Countdown

65b0db4

PR-URL: #17477 Refs: #17169 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17774

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: update test-http-status-code to use countdown #17477

test: update test-http-status-code to use countdown #17477

onneri commented Dec 6, 2017 •

edited by apapirovski

Loading

mithunsasidharan Dec 6, 2017

apapirovski Dec 6, 2017

apapirovski left a comment •

edited

Loading

apapirovski Dec 6, 2017

apapirovski Dec 6, 2017

apapirovski Dec 6, 2017

apapirovski Dec 6, 2017

apapirovski Dec 6, 2017

onneri commented Dec 6, 2017

apapirovski left a comment

apapirovski commented Dec 6, 2017

onneri commented Dec 6, 2017 •

edited

Loading

apapirovski commented Dec 6, 2017

apapirovski commented Dec 8, 2017

onneri commented Dec 8, 2017

apapirovski commented Dec 8, 2017 •

edited

Loading

onneri commented Dec 8, 2017

test: update test-http-status-code to use countdown #17477

test: update test-http-status-code to use countdown #17477

Conversation

onneri commented Dec 6, 2017 • edited by apapirovski Loading

Checklist

Affected core subsystem(s)

mithunsasidharan Dec 6, 2017

Choose a reason for hiding this comment

apapirovski Dec 6, 2017

Choose a reason for hiding this comment

apapirovski left a comment • edited Loading

Choose a reason for hiding this comment

apapirovski Dec 6, 2017

Choose a reason for hiding this comment

apapirovski Dec 6, 2017

Choose a reason for hiding this comment

apapirovski Dec 6, 2017

Choose a reason for hiding this comment

apapirovski Dec 6, 2017

Choose a reason for hiding this comment

apapirovski Dec 6, 2017

Choose a reason for hiding this comment

onneri commented Dec 6, 2017

apapirovski left a comment

Choose a reason for hiding this comment

apapirovski commented Dec 6, 2017

onneri commented Dec 6, 2017 • edited Loading

apapirovski commented Dec 6, 2017

apapirovski commented Dec 8, 2017

onneri commented Dec 8, 2017

apapirovski commented Dec 8, 2017 • edited Loading

onneri commented Dec 8, 2017

onneri commented Dec 6, 2017 •

edited by apapirovski

Loading

apapirovski left a comment •

edited

Loading

onneri commented Dec 6, 2017 •

edited

Loading

apapirovski commented Dec 8, 2017 •

edited

Loading