test: update test-http-status-code to use countdown by onneri · Pull Request #17477 · nodejs/node · GitHub

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

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.

LGTM but some small changes needed

Thank you for taking this on!

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.

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

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

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

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

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!

Changes LGTM. Thank you!

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

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

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>

Landed in 07549c6

Thank you for your contribution @onneri! 👍

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

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

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 outstanding review comments, 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 outstanding review comments, 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

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

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

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

               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];

test: update test-http-status-code to use countdown by onneri · Pull Request #17477 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

test: update test-http-status-code to use countdown#17477onneri wants to merge 2 commits intonodejs:masternodejs/node:masterfrom onneri:converting-tests-to-use-countdownonneri/node:converting-tests-to-use-countdownCopy head branch name to clipboard

Conversation

onneri commented Dec 6, 2017 • edited by apapirovski Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

mithunsasidharan Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

apapirovski Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

apapirovski left a comment • 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!

apapirovski Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

apapirovski Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

apapirovski Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

apapirovski Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

apapirovski Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

onneri commented Dec 6, 2017

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Dec 6, 2017

Uh oh!

onneri commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apapirovski commented Dec 6, 2017

Uh oh!

apapirovski commented Dec 8, 2017

Uh oh!

onneri commented Dec 8, 2017

Uh oh!

apapirovski commented Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

onneri commented Dec 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

test: update test-http-status-code to use countdown#17477
onneri wants to merge 2 commits into
nodejs:masterfrom
onneri:converting-tests-to-use-countdown

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