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

[mithunsasidharan]

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]

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

[mithunsasidharan]

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

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

Something like this:

constcountdown=newCountdown(methods.length,common.mustCall(()=>{server.close();}));

[aqrln]

Missing new line at the end of file.

[mithunsasidharan]

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

[aqrln]

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

[mithunsasidharan]

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

[aqrln]

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

[jasnell]

CI is failing on this.

[thefourtheye]

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]

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

[mithunsasidharan]

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

[aqrln]

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

[aqrln]

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){ ... }

[mithunsasidharan]

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

[aqrln]

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

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

[mithunsasidharan]

@addaleax@aqrln : Thanks !

[addaleax]

Landed in f6926d5

Thanks for the contribution! ✨ 🎉