Add test for HTTP client "aborted" event

[kemitchell]

Checklist

• make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
• a test and/or benchmark is included
• the commit message follows commit guidelines

Affected core subsystem(s)

None. A test for current http.

Description of change

Following on from #7270 to add a test for aborted events on http.IncomingMessage emitters from http.request. New test was derived from old test/parallel/test-http-abort-client.js.

cc @addaleax

[addaleax]

We usually use const instead of var wherever it makes sense.

[kemitchell]

Update the other tests, as well?

[addaleax]

It should be perfectly fine to change these just here – formatting changes tend to mess up the history/git blame, so it’s always easier to do them in new code/code that’s being touched anyway.

[addaleax]

Thanks, this is generally looking good… I know the other tests don’t always hold up to what is usually expected from new ones. :)

again, /cc @nodejs/http

[kemitchell]

If the fixes are obvious, please feel free to hack up what I've tried to do. I'm by no means territorial about it...

[addaleax]

@kemitchell That’s completely up to you, if you prefer, I guess I could PR against your branch (definitely later, though).

There’s also resources like https://github.com/nodejs/node/blob/master/doc/guides/writing_tests.md#test-structure if you’re interested. (And, sorry if I’m misunderstanding you language-barrier-wise…?)

[Trott]

Hey, @kemitchell! Thanks for putting this together. There are a number of conventions in tests and elsewhere in the code that aren't checked by the linter unfortunately. This is because (for example) converting all those var instances that can use const instead would be a huge amount of churn. So there tends to be a "fix it when you're already in the code doing something else" approach. That has its own problems but, you know, trade-offs.

Anyway, since you're so close on this one, I went ahead and put together what I think is what the various nits above are after. Feel free to cut-and-paste and commit to your branch, or otherwise use in whatever way you find useful. Use it as a starting point, or just look at it and ask questions if stuff doesn't make sense, or ignore me completely. Whatever works for you.

'use strict';constcommon=require('../common');consthttp=require('http');constserver=http.Server(function(req,res){console.log('Server accepted request.');res.write('Part of my res.');res.destroy();});constrequiredCallback=common.mustCall(function(){console.log('Response aborted.');});server.listen(0,function(){http.get({port: this.address().port,headers: {connection: 'keep-alive'}},function(res){server.close();console.log('Got res: '+res.statusCode);res.on('aborted',requiredCallback);});});

[kemitchell]

@Trott@addaleax thank you both for patient explanations. I just did my own surgery on the test, but will paste in @Trott's code and force-push shortly.

[kemitchell]

@Trott: I don't mean to take credit for your code. And I'm a bit embarrassed that you ended up rewriting my attempt entirely. Please feel free to drop my commit entirely, or otherwise tweak it to reflect where credit is truly due---to @addaleax and yourself. Best, K

[Trott]

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

(@kemitchell You're not giving yourself enough credit.)

[jasnell]

Are the console.log outputs necessary?

[indutny]

It should not be, even further it may break the TAP.

[Trott]

I've got no problem at all with seeing the console.log() calls removed. But I'm curious about the TAP comment. I'm pretty sure test.py or whatever captures the test's stdout and stderr and does not interleave it with TAP output. Is there a subtlety I'm missing where a console.log() can cause problems with TAP in that case? Many (perhaps even most) of our tests do console.log() and/or console.error(). So if this is A Thing, I'd definitely like to know more. (/cc @nodejs/testing)

[Trott]

Oh, wait, we definitely directly output TAP sometimes from JS (e.g., when a test is skipped), so yeah, I guess it is A Thing...

[jasnell]

LGTM with a nit

[indutny]

Any reason not to use common.PORT here?

[Trott]

common.PORT has the downside of making subsequent tests fail with EADDRINUSE if one test does not clean up after itself (due to failure, programming error, some kind of OS flakiness, or whatever). There was a bit of an effort for a while to stamp it out except in the few places that absolutely need it. I think @mscdex was at the center of that, but I may be mis-remembering.

Anyway, in general, I prefer to see server.listen(0, ...) over server.listen(common.PORT, ...) if at all possible.

[mscdex]

As far as using random ports goes, most parallel tests are now using random ports since 2bc7841. The exception is cluster tests (at least until #7043 is resolved) and one other subsystem's tests (can't recall offhand).

[indutny]

LGTM with two nits.

[kemitchell]

--force to remove console.log calls, clean up a bit.

[kemitchell]

@indutny I removed the console.log calls. What was your second nit?

[Trott]

@kemitchell The other nit involved suggesting common.PORT but I think @mscdex and I prefer the way it is now and, judging from the phrasing, I'm not sure that @indutny felt particularly strongly about it.

[indutny]

Yeah, I don't feel strongly about it. LGTM

[jasnell]

New CI following updates: https://ci.nodejs.org/job/node-test-pull-request/3112/

[kemitchell]

@jasnell: Sorry. Did I break it?

[jasnell]

Nope! It does look like we have a couple of infrastructure hiccups tho. Going to run it again.

[jasnell]

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

[jasnell]

@kemitchell ... actually, looks like we have a couple of linting issues:

/Users/james/Node/main/node/test/parallel/test-http-client-aborted-event.js 16:47 error Unexpected space before function parentheses space-before-function-paren 16:59 error Missing semicolon semi ✖ 2 problems (2 errors, 0 warnings) [00:05|%100|+ 1591|- 1]: Done make: *** [jslint] Error 1 

[kemitchell]

@jasnell: Last force-push hopefully fixed lint issues.

[lpinca]

Nit: can you get rid of the return statement? It is not really useful.

[kemitchell]

Amended.

[lpinca]

New CI run: https://ci.nodejs.org/job/node-test-pull-request/4439/

[lpinca]

More callbacks could be wrapped with common.mustCall but if the response event is not emitted on the http.ClientRequest the test would fail anyway for timeout.

LGTM

[lpinca]

@jasnell@indutny still LGTY?

[cjihrig]

Please add a common.mustCall() here.

[cjihrig]

And a common.mustCall() here.

[cjihrig]

No need for the space inside the empty curly braces.

[jasnell]

With @cjihrig's nits addressed, yes.

[kemitchell]

I have amended to add common.mustCall and drop a space from between braces.

There are a few more places where the same space could go:

$git grep "function(){}" test/ test/parallel/test-async-wrap-check-providers.js:setTimeout(function(){}); test/parallel/test-buffer-includes.js: b.includes(function(){}); test/parallel/test-buffer-indexof.js: b.indexOf(function(){}); test/parallel/test-child-process-fork-ref2.js: process.on('message', function(){}); test/parallel/test-cluster-rr-domain-listen.js: d.run(function(){}); test/parallel/test-cluster-rr-domain-listen.js: http.Server(function(){}).listen(common.PORT, '127.0.0.1'); test/parallel/test-cluster-worker-exit.js: var server = http.Server(function(){}); test/parallel/test-cluster-worker-kill.js: var server = http.Server(function(){}); test/parallel/test-cluster-worker-kill.js: server.once('listening', function(){}); test/parallel/test-crypto-dh.js: crypto.createDiffieHellman(function(){}); test/parallel/test-dns-regress-6244.js:dns.resolve4('127.0.0.1', common.mustCall(function(){})); test/parallel/test-fs-write-stream-end.js: stream.on('close', common.mustCall(function(){})); test/parallel/test-handle-wrap-close-abort.js:process.on('uncaughtException', function(){}); test/parallel/test-http-connect-req-res.js: req.on('close', common.mustCall(function(){})); test/parallel/test-https-socket-options.js: req.setTimeout(1000, function(){}); test/parallel/test-https-socket-options.js: req.setTimeout(1000, function(){}); test/parallel/test-net-stream.js: socket.write(buf, function(){}); test/parallel/test-process-getactiverequests.js: fs.open(__filename, 'r', function(){}); 

I hope these changes do it. Unfortunately, I haven't time or bandwidth to follow this PR any further. If it needs additional love, please feel free to amend as y'all see fit, either on merge or on my branch, as "edits from maintainers".

Sincerest thanks to all for time and attention on this and so much else that makes Node work. Thanks especially to @Trott. IIRC, this code is mostly his at this point ;-)

[lpinca]

I'll land this tomorrow if there are no objections.

[lpinca]

One last CI run: https://ci.nodejs.org/job/node-test-pull-request/4759/

[lpinca]

Landed in 6d6f9e5.

[kemitchell]

Thanks to all!