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

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

@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…?)

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';
const common = require('../common');
const http = require('http');

const server = http.Server(function(req, res) {
  console.log('Server accepted request.');
  res.write('Part of my res.');
  res.destroy();
});

const requiredCallback = 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);
  });
});

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

@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

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

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

LGTM with a nit

LGTM with two nits.

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

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

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

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

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

@jasnell: Sorry. Did I break it?

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

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

@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

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

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

@jasnell @indutny still LGTY?

With @cjihrig's nits addressed, yes.

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 ;-)

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

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

Landed in 6d6f9e5.

Thanks to all!