Node: [Tracking issue] Converting tests to use Countdown

[jasnell]

One possible set of tasks that I can suggest for new contributors who are bit more confident in their Node.js skills, would be converting tests to use the new ../common/countdown utility module.

For instance, if you take a look at: https://github.com/nodejs/node/blob/master/test/parallel/test-http2-client-destroy.js, you'll see that there is a remaining counter (https://github.com/nodejs/node/blob/master/test/parallel/test-http2-client-destroy.js#L19). The test then counts down remaining before closing the server. There are quite a large number of tests in our suite that perform similar actions in ways that are rather inconsistent. The ../common/countdown utility was designed to bring some consistency there.

The way the Countdown utility works is straightforward:

const common = require('../common');
const Countdown = require('../common/countdown');

// ...

const countdown = new Countdown(n, common.mustCall(() => {
  // do something here
}));

// Decrement the counter, the callback is called synchronously when
// countdown.dec is called n times.
countdown.dec();

I know that there are quite a few of the http2 tests that can benefit from this, along with a bunch of other http and https tests.

These tasks would be for folks who are a bit more comfortable with their Node.js skills.

There are many tests in the suite that could be modified to use this new utility. However, rather than modifying them all at once, if a new contributor wishes to take this on, please one change one or two at a time, leaving other tests for other contributors to pick up on. Just reference this tracking issue in the commit/PR so that we can keep track.

[Bamieh]

@jasnell I am willing to contribute to this issue, but first i recommend creating a list with all the places where this refactor is needed so we an keep a tracked list of whats being worked on and what is yet to be refactored.

[jasnell]

@Bamieh ... absolutely... I'll start digging through later on today and building a list out here in this issue :-)

[joyeecheung]

Would be nice if there is a guide or a link to https://github.com/nodejs/node/tree/master/test/common#countdown-module in https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md

(Aside: we should add a note on the usage of common/fixtures in that testing guide as well..)

[Bamieh]

### Here is a full list of the http test cases to use Countdown:
Feel free to update it please!

| Http Test Name | Complete | Pull request ready |
| --------------- | ---------------- | ------------------ |
| test/parallel/test-http-1.0-keep-alive.js | no | no |
| test/parallel/test-http-after-connect.js | yes | no |
| test/parallel/test-http-agent-maxsockets-regress-4050.js | yes | no |
| test/parallel/test-http-agent-maxsockets.js | yes | no |
| test/parallel/test-http-agent.js | no | no |
| test/parallel/test-http-allow-req-after-204-res.js | no | yes |
| test/parallel/test-http-client-abort.js | yes | no |
| test/parallel/test-http-client-agent.js | no | no |
| test/parallel/test-http-client-check-http-token.js | yes | no |
| test/parallel/test-http-client-default-headers-exist.js | yes | no |
| test/parallel/test-http-client-parse-error.js | yes | no |
| test/parallel/test-http-client-timeout-agent.js _(not sure)_ | no | no |
| test/parallel/test-http-content-length.js | no | yes |
| test/parallel/test-http-end-throw-socket-handling.js | no | yes |
| test/parallel/test-http-exceptions.js | no | yes |
| test/parallel/test-http-expect-continue.js | no | no |
| test/parallel/test-http-expect-handling.js | no | no |
| test/parallel/test-http-get-pipeline-problem.js | no | no |
| test/parallel/test-http-host-header-ipv6-fail.js | no | no |
| test/parallel/test-http-host-headers.js | no | no |
| test/parallel/test-http-incoming-pipelined-socket-destroy.js | no | no |
| test/parallel/test-http-invalidheaderfield.js | no | no |
| test/parallel/test-http-keep-alive-close-on-header.js | no | no |
| test/parallel/test-http-keepalive-client.js | no | no |
| test/parallel/test-http-keepalive-override.js | no | no |
| test/parallel/test-http-keepalive-request.js | no | no |
| test/parallel/test-http-malformed-request.js | no | no |
| test/parallel/test-http-max-headers-count.js | no | no |
| test/parallel/test-http-parser-bad-ref.js | no | no |
| test/parallel/test-http-parser-free.js | no | no |
| test/parallel/test-http-pipe-fs.js | no | yes |
| test/parallel/test-http-pipeline-flood.js | no | no |
| test/parallel/test-http-pipeline-regr-2639.js | no | no |
| test/parallel/test-http-pipeline-regr-3332.js | no | no |
| test/parallel/test-http-proxy.js | no | no |
| test/parallel/test-http-request-dont-override-options.js | no | no |
| test/parallel/test-http-res-write-end-dont-take-array.js | no | no |
| test/parallel/test-http-response-multi-content-length.js | no | no |
| test/parallel/test-http-response-multiheaders.js | no | no |
| test/parallel/test-http-response-splitting.js | no | yes |
| test/parallel/test-http-response-status-message.js | no | no |
| test/parallel/test-http-response-statuscode.js | no | yes |
| test/parallel/test-http-same-map.js | no | no |
| test/parallel/test-http-server.js | no | no |
| test/parallel/test-http-set-cookies.js | no | no |
| test/parallel/test-http-set-trailers.js | no | no |
| test/parallel/test-http-should-keep-alive.js | no | no |
| test/parallel/test-http-status-code.js | no | no |
| test/parallel/test-http-status-reason-invalid-chars.js | no | yes |
| test/parallel/test-http-timeout-overflow.js | no | no |
| test/parallel/test-http-timeout.js | no | yes |
| test/parallel/test-http-upgrade-advertise.js _(not sure)_ | no | no |
| test/parallel/test-http-upgrade-client.js | no | yes |
| test/parallel/test-http-upgrade-server.js | no | no |
| test/parallel/test-http.js _(not sure)_ | no | no |

The ones marked with _(not sure)_ have arrays of data that they use both as a counter and as the data for each counter, so i am not sure if they should be used in combination with a countdown or just kept as is.

[Bamieh]

@AyoAlfonso you have to fork the node package on github, then apply your changes on the fork. Finally you submit a pull request against the main node github master branch. good luck!

[mithunsasidharan]

@Bamieh @daxlab : Can you help review my PRs... thanks !

[AyoAlfonso]

Thanks, @Bamieh ! Just did exactly that after tripping about a bit.

[mithunsasidharan]

@apapirovski @daxlab : Can you please review the PRs associated with this issues. Several of them are still unreviewed. Thanks a lot !

[onneri]

@Bamieh @daxlab : I just created a PR for this issue, is my first issue, so any help to review the PR and comments would be great. Thank you!!!

[shilomagen]

Created PR for test-http-set-cookies.js
https://github.com/nodejs/node/pull/17504

It's my first PR for NodeJS so be gentle ;)

[idandagan1]

I've created my first PR.
I added Countdown to test-http-incoming-pipelined-socket-destroy.js test. Ref

After it got approved, 1 test failed (test/arm-fanned) and 12 passed.
Does anyone have encountered the same problem?

[mithunsasidharan]

@idandagan1 : Congrats on your first PR 👍 That failure hasn't got anything to do with your PR !

[fedekau]

I have created a PR (#17537) for test/parallel/test-http-agent.js, it is my first PR 😄

[maclover7]

I believe this was closed by accident. Please close again if not.