Refactored `test-http-allow-req-after-204-res` file to use countdown #17211

mithunsasidharan · 2017-11-22T06:59:46Z

Refactored the test case test-http-allow-req-after-204-res to use countdown, as per issue #17169

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test

Bamieh · 2017-11-22T09:50:52Z

test/parallel/test-http-allow-req-after-204-res.js

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

 // first 204 or 304 works, subsequent anything fails
 const codes = [204, 200];

 // Methods don't really matter, but we put in something realistic.
 const methods = ['DELETE', 'DELETE'];


this was basically used as the counter, so i do not think it's necessary anymore (hardcoding the method into the request is enough).

@Bamieh : Thanks. I've updated the PR. Kindly review!

aqrln

Hi and thanks a lot for your contribution! I left a couple of comments.

aqrln · 2017-11-22T09:53:36Z

test/parallel/test-http-allow-req-after-204-res.js


 // first 204 or 304 works, subsequent anything fails
 const codes = [204, 200];

 // Methods don't really matter, but we put in something realistic.
 const methods = ['DELETE', 'DELETE'];
+const countdown = new Countdown(methods.length, () => server.close());


Can you please wrap the callback into common.mustCall()?

Something like this:

const countdown = new Countdown(methods.length, common.mustCall(() => { server.close(); }));

aqrln · 2017-11-22T09:58:02Z

test/parallel/test-http-allow-req-after-204-res.js

    }));
    response.resume();
  }));
  request.end();
 }

-server.listen(0, nextRequest);
+server.listen(0, nextRequest);


Missing new line at the end of file.

mithunsasidharan · 2017-11-22T13:19:39Z

@aqrln : Thanks for the feedback. I've updated the PR with changes. I accidentally closed the PR...apologies for that. Kindly review!

aqrln · 2017-11-22T13:57:46Z

test/parallel/test-http-allow-req-after-204-res.js


 // first 204 or 304 works, subsequent anything fails
 const codes = [204, 200];

-// Methods don't really matter, but we put in something realistic.
-const methods = ['DELETE', 'DELETE'];
+const countdown = new Countdown(methods.length, common.mustCall(() => {


Now that you removed the methods array, it should be codes.length, shouldn't it?

@aqrln : Thanks. I've fixed it..kindly review.

aqrln · 2017-11-22T14:42:20Z

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

jasnell · 2017-11-22T18:53:44Z

CI is failing on this.

thefourtheye · 2017-11-26T19:53:12Z

This patch works

diff --git a/test/common/countdown.js b/test/common/countdown.js
index 6a22be0a07..93bdbbfb16 100644
--- a/test/common/countdown.js
+++ b/test/common/countdown.js
@@ -17,6 +17,7 @@ class Countdown {
     assert(this[kLimit] > 0, 'Countdown expired');
     if (--this[kLimit] === 0)
       this[kCallback]();
+    return this[kLimit];
   }

   get remaining() {
diff --git a/test/parallel/test-http-allow-req-after-204-res.js b/test/parallel/test-http-allow-req-after-204-res.js
index cd55a4f17c..53237f6677 100644
--- a/test/parallel/test-http-allow-req-after-204-res.js
+++ b/test/parallel/test-http-allow-req-after-204-res.js
@@ -23,12 +23,12 @@
 const common = require('../common');
 const http = require('http');
 const assert = require('assert');
+const Countdown = require('../common/countdown');

 // first 204 or 304 works, subsequent anything fails
 const codes = [204, 200];

-// Methods don't really matter, but we put in something realistic.
-const methods = ['DELETE', 'DELETE'];
+const countdown = new Countdown(codes.length, () => server.close());

 const server = http.createServer(common.mustCall((req, res) => {
   const code = codes.shift();
@@ -39,17 +39,13 @@ const server = http.createServer(common.mustCall((req, res) => {
 }, codes.length));

 function nextRequest() {
-  const method = methods.shift();
-
-  const request = http.request({
+  const request = http.get({
     port: server.address().port,
-    method: method,
     path: '/'
   }, common.mustCall((response) => {
     response.on('end', common.mustCall(() => {
-      if (methods.length === 0) {
-        server.close();
-      } else {
+      if (countdown.dec()) {
         // throws error:
         nextRequest();
         // works just fine:

mithunsasidharan · 2017-11-27T05:04:30Z

@thefourtheye : Thanks for suggesting the correction. I've update the PR. Kindly review!

mithunsasidharan · 2017-11-27T15:45:43Z

@maclover7 : Kindly run the CI for this PR too. Thanks !

aqrln · 2017-11-28T03:02:40Z

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

aqrln · 2017-11-28T03:05:57Z

test/parallel/test-http-allow-req-after-204-res.js

-      if (methods.length === 0) {
-        server.close();
-      } else {
+      if (countdown.dec()) {


No strong opinion, but instead of modifying common/countdown.js, you can just use the existing API and do something like

countdown.dec(); if (countdown.remaining > 0) { ... }

@aqrln : I feel making the change in countdown.jswill be better as it makes the test case condition more readable and save an extra line! Kindly suggest. Also, could you please run the CI. I've fixed the lint issues. Thanks.

@mithunsasidharan sure, no problem. In the future, please run make -j4 test locally before pushing to the PR branch to save your time and the number of review iterations. Thanks!

aqrln · 2017-11-28T23:11:32Z

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

mithunsasidharan · 2017-11-29T05:24:20Z

@addaleax @aqrln : Thanks !

addaleax · 2017-12-01T20:00:05Z

Landed in f6926d5

Thanks for the contribution! ✨ 🎉

PR-URL: #17211 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

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

Bamieh reviewed Nov 22, 2017

View reviewed changes

aqrln reviewed Nov 22, 2017

View reviewed changes

mscdex added the http Issues or PRs related to the http subsystem. label Nov 22, 2017

mithunsasidharan closed this Nov 22, 2017

mithunsasidharan reopened this Nov 22, 2017

aqrln reviewed Nov 22, 2017

View reviewed changes

jasnell approved these changes Nov 22, 2017

View reviewed changes

aqrln approved these changes Nov 22, 2017

View reviewed changes

thefourtheye approved these changes Nov 23, 2017

View reviewed changes

aqrln reviewed Nov 28, 2017

View reviewed changes

Refactored test-http-allow-req-after-204-res file to use countdown

3833470

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

addaleax closed this Dec 1, 2017

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

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

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

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17772

Closed

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17774

Merged

MylesBorins mentioned this pull request Dec 20, 2017

v6.12.3 proposal #17776

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored `test-http-allow-req-after-204-res` file to use countdown #17211

Refactored `test-http-allow-req-after-204-res` file to use countdown #17211

mithunsasidharan commented Nov 22, 2017 •

edited

Loading

Bamieh Nov 22, 2017

mithunsasidharan Nov 22, 2017

aqrln left a comment

aqrln Nov 22, 2017

aqrln Nov 22, 2017

mithunsasidharan commented Nov 22, 2017 •

edited

Loading

aqrln Nov 22, 2017 •

edited

Loading

mithunsasidharan Nov 22, 2017

aqrln commented Nov 22, 2017

jasnell commented Nov 22, 2017

thefourtheye commented Nov 26, 2017

mithunsasidharan commented Nov 27, 2017

mithunsasidharan commented Nov 27, 2017

aqrln commented Nov 28, 2017

aqrln Nov 28, 2017

mithunsasidharan Nov 28, 2017

aqrln Nov 28, 2017

aqrln commented Nov 28, 2017

mithunsasidharan commented Nov 29, 2017

addaleax commented Dec 1, 2017

Refactored test-http-allow-req-after-204-res file to use countdown #17211

Refactored test-http-allow-req-after-204-res file to use countdown #17211

Conversation

mithunsasidharan commented Nov 22, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqrln left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mithunsasidharan commented Nov 22, 2017 • edited Loading

aqrln Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqrln commented Nov 22, 2017

jasnell commented Nov 22, 2017

thefourtheye commented Nov 26, 2017

mithunsasidharan commented Nov 27, 2017

mithunsasidharan commented Nov 27, 2017

aqrln commented Nov 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqrln commented Nov 28, 2017

mithunsasidharan commented Nov 29, 2017

addaleax commented Dec 1, 2017

Refactored `test-http-allow-req-after-204-res` file to use countdown #17211

Refactored `test-http-allow-req-after-204-res` file to use countdown #17211

mithunsasidharan commented Nov 22, 2017 •

edited

Loading

mithunsasidharan commented Nov 22, 2017 •

edited

Loading

aqrln Nov 22, 2017 •

edited

Loading