test: improve numerous http tests #12930
Conversation
jasnell
force-pushed
the
improve-http-tests
branch
from
95e23cd
to
2489dfe
Compare
* Use common.mustCall where appropriate * Remove extraneous console output * Misc cleanups
jasnell
force-pushed
the
improve-http-tests
branch
from
2489dfe
to
b23ec7c
Compare
| c.write(request_generator()); | ||
| }); | ||
| c.on('connect', common.mustCall(() => c.write(request_generator()))); | ||
| c.on('data', common.mustCall((chunk) => server_response += chunk)); |
There was a problem hiding this comment.
I don't think we should add common.mustCall() around 'data' event handlers because they can be called a number of times.
There was a problem hiding this comment.
The idea for me is that tests should be predictable, and that for each of these tests, the number of times an event like data would be called should be deterministic. If a change is made that changes that, then the test should pick up on it and update the test. Doing so would increase the visibility of such changes.
There was a problem hiding this comment.
common.mustCall() takes an expected:int as second argument.
I'll write a PR to have it expect '+' as "more than zero".
There was a problem hiding this comment.
I agree with that point. But aren't we at the mercy of different platforms and whatnot for the flow of data? If it can be done reliably, and without platform specific checks, then 👍
There was a problem hiding this comment.
Given that there's no way for the test to pass if the callback is not called (because server_response will not have the expected data), I would be inclined to drop common.mustCall(). The more layers of stuff added to the test, the harder it is to figure out what the test is actually testing.
There was a problem hiding this comment.
FWIW fccc0bf landed now you have common.mustCallAtLeast
| }); | ||
|
|
||
| // it would be nice if this worked: | ||
| res.on('data', common.mustCall()); |
There was a problem hiding this comment.
Maybe replace with the longer but more explicit:
let data = '';
res.on('data', (chunk) => { data += chunk; });
res.on('end', common.mustCall(() => {
assert.strictEqual(data, 'Part of my res.');
}));This way it verifies all of these:
- the
datacallback is called at least once - the
datacallback is sent the expected value(s) - the
'endcallback is only fired after thedatacallback(s)
| @@ -107,7 +107,8 @@ function check(tests) { | |||
| const test = tests[0]; | |||
| let server; | |||
| if (test) { | |||
| server = http.createServer(serverHandler).listen(0, '127.0.0.1', client); | |||
| server = http.createServer(serverHandler) | |||
| .listen(0, '127.0.0.1', common.mustCall(client)); | |||
There was a problem hiding this comment.
/s/127.0.0.1/common.localhostIPv4/
| c.write(request_generator()); | ||
| }); | ||
| c.on('connect', common.mustCall(() => c.write(request_generator()))); | ||
| c.on('data', common.mustCall((chunk) => server_response += chunk)); |
There was a problem hiding this comment.
common.mustCall() takes an expected:int as second argument.
I'll write a PR to have it expect '+' as "more than zero".
|
Closing in favor of #14315 |
Minor refactoring in a number of http tests. I've been going through an auditing the existing http tests to identify which we will need equivalents for on the http2 implementation and spotted a number of cleanups that could be made.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
tests (http)