test,http: fix http dump test

[mcollina]

Fixes: #19139

Checklist

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

[mcollina]

this was noted in the original PR. req._consuming is never set.

[mcollina]

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

[lpinca]

I think on Windows it's ECONNABORTED, and it might also be EPIPE as per #19139 (comment). Should we just ignore the error?

[lpinca]

Can we still check that _dump() removes all data listeners?

node/lib/_http_incoming.js

Line 299 in 354849e

this.removeAllListeners('data');

maybe adding an assertion for listenerCount('data') in the 'resume' handler?

[mcollina]

Added

[mcollina]

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

@lpinca PTAL

[lpinca]

Added a few more comments. All nits that can be ignored.

[lpinca]

Nit: This can be safely removed as the listener is wrapped in mustCall().

[lpinca]

Nit: I'd wrap the listener with mustCall().

[lpinca]

Nit: this could also be removed.

[BridgeAR]

Nit: would you be so kind and punctuate each sentence and also use a capital letter for the first character of a sentence? :-) That applies to all comments in here. The reason is that it is hard to see the beginning and the end of each sentence right now.

[mcollina]

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

[Trott]

This fixes the common.mustNotCall problem identified by @lpinca but the test is still sensitive to resource availability so we still need to do #19819 as well. I'll land that now and then resolve any resulting merge conflict that ends up here.

[Trott]

(I imagine this also fixes the EPIPE issue that was one of the two ways the test fails in #19139.)

[Trott]

CI after rebase: https://ci.nodejs.org/job/node-test-pull-request/14112/

[lpinca]

so we still need to do #19819 as well

Do we? I don't think this is still flaky after this rewrite.

[mcollina]

I don’t think #19819 was needed.

[mcollina]

Landed as 1f29963

[Trott]

I don’t think #19819 was needed.

It was needed. I landed this PR on top of master locally on my machine and ran the test under system load and it was still failing.

[Trott]

Here's the demo that this alone wasn't enough to fix the test for parallel runs:

$ make -j4
[...SNIP...]
$ git rev-parse HEAD
1f29963eacf587817a6d40da527d05d151668198
$ cp test/sequential/test-http-dump-req-when-res-ends.js test/parallel/test-http-dump-req-when-res-ends.js                                  
$ tools/test.py -j 8 --repeat 192 test/parallel/test-http-dump-req-when-res-ends.js 
=== release test-http-dump-req-when-res-ends ===                    
Path: parallel/test-http-dump-req-when-res-ends
Mismatched <anonymous> function calls. Expected at least 1, actual 0.
    at Object.exports.mustCallAtLeast (/Users/trott/io.js/test/common/index.js:431:10)
    at IncomingMessage.<anonymous> (/Users/trott/io.js/test/parallel/test-http-dump-req-when-res-ends.js:18:27)
    at IncomingMessage.<anonymous> (/Users/trott/io.js/test/common/index.js:467:15)
    at IncomingMessage.emit (events.js:182:13)
    at resume_ (_stream_readable.js:898:10)
    at process._tickCallback (internal/process/next_tick.js:174:19)
Command: out/Release/node /Users/trott/io.js/test/parallel/test-http-dump-req-when-res-ends.js
=== release test-http-dump-req-when-res-ends ===                    
Path: parallel/test-http-dump-req-when-res-ends
Mismatched <anonymous> function calls. Expected at least 1, actual 0.
    at Object.exports.mustCallAtLeast (/Users/trott/io.js/test/common/index.js:431:10)
    at IncomingMessage.<anonymous> (/Users/trott/io.js/test/parallel/test-http-dump-req-when-res-ends.js:18:27)
    at IncomingMessage.<anonymous> (/Users/trott/io.js/test/common/index.js:467:15)
    at IncomingMessage.emit (events.js:182:13)
    at resume_ (_stream_readable.js:898:10)
    at process._tickCallback (internal/process/next_tick.js:174:19)
Command: out/Release/node /Users/trott/io.js/test/parallel/test-http-dump-req-when-res-ends.js
=== release test-http-dump-req-when-res-ends ===                    
Path: parallel/test-http-dump-req-when-res-ends
Mismatched <anonymous> function calls. Expected at least 1, actual 0.
    at Object.exports.mustCallAtLeast (/Users/trott/io.js/test/common/index.js:431:10)
    at IncomingMessage.<anonymous> (/Users/trott/io.js/test/parallel/test-http-dump-req-when-res-ends.js:18:27)
    at IncomingMessage.<anonymous> (/Users/trott/io.js/test/common/index.js:467:15)
    at IncomingMessage.emit (events.js:182:13)
    at resume_ (_stream_readable.js:898:10)
    at process._tickCallback (internal/process/next_tick.js:174:19)
Command: out/Release/node /Users/trott/io.js/test/parallel/test-http-dump-req-when-res-ends.js
[00:11|% 100|+ 189|-   3]: Done 
$

[mcollina]

@Trott on my Macbook:

~/Repositories/node (master)
$ git rev-parse HEAD
1f29963eacf587817a6d40da527d05d151668198
~/Repositories/node (master)
$ tools/test.py -j 8 --repeat 192 test/sequential/test-http-dump-req-when-res-ends.js
[00:35|% 100|+ 192|-   0]: Done

So, something weirder was at play here, but I think @lpinca is on to something in #19866.

[Trott]

@mcollina Try a bigger -j value. I usually don't trust a test until I can do -j 96 --repeat 192. I showed -j 8 because it was the lowest value I triggered it at. (You probably have a newer/more powerful laptop than I do. In fact, I pretty much guarantee it.)