Add test for HTTP client "aborted" event #7376

kemitchell · 2016-06-22T23:51:37Z

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 · 2016-06-22T23:53:32Z

test/parallel/test-http-client-aborted-event.js

+'use strict';
+require('../common');
+var http = require('http');
+var assert = require('assert');


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

Update the other tests, as well?

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 · 2016-06-22T23:59:27Z

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 · 2016-06-23T00:08:00Z

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 · 2016-06-23T00:48:33Z

@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 · 2016-06-26T02:13:58Z

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

kemitchell · 2016-06-27T03:01:08Z

@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 · 2016-06-27T03:03:26Z

@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 · 2016-06-27T23:39:54Z

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

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

jasnell · 2016-06-28T15:23:58Z

test/parallel/test-http-client-aborted-event.js

+const http = require('http');
+
+const server = http.Server(function(req, res) {
+  console.log('Server accepted request.');


Are the console.log outputs necessary?

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

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)

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 · 2016-06-28T15:24:44Z

LGTM with a nit

indutny · 2016-06-28T15:32:48Z

test/parallel/test-http-client-aborted-event.js

+  console.log('Response aborted.');
+});
+
+server.listen(0, function() {


Any reason not to use common.PORT here?

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.

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 · 2016-06-28T15:33:14Z

LGTM with two nits.

kemitchell · 2016-06-28T18:03:42Z

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

kemitchell · 2016-06-28T18:04:38Z

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

Trott · 2016-06-28T18:11:42Z

@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 · 2016-06-28T18:25:14Z

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

jasnell · 2016-06-28T21:08:31Z

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

kemitchell · 2016-06-28T22:18:38Z

@jasnell: Sorry. Did I break it?

jasnell · 2016-06-28T22:43:23Z

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

jasnell · 2016-06-28T22:44:42Z

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

jasnell · 2016-06-28T23:02:15Z

@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 · 2016-07-14T00:28:32Z

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

lpinca · 2016-10-09T10:05:40Z

test/parallel/test-http-client-aborted-event.js

+    headers: { connection: 'keep-alive' }
+  }, function(res) {
+    server.close();
+    res.on('aborted', common.mustCall(function() { return; }));


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

lpinca · 2016-10-09T10:06:58Z

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 · 2016-10-10T06:03:51Z

@jasnell @indutny still LGTY?

cjihrig · 2016-10-10T13:49:56Z

test/parallel/test-http-client-aborted-event.js

+  res.destroy();
+});
+
+server.listen(0, function() {


Please add a common.mustCall() here.

cjihrig · 2016-10-10T13:50:20Z

test/parallel/test-http-client-aborted-event.js

+  http.get({
+    port: this.address().port,
+    headers: { connection: 'keep-alive' }
+  }, function(res) {


And a common.mustCall() here.

cjihrig · 2016-10-10T13:51:06Z

test/parallel/test-http-client-aborted-event.js

+    headers: { connection: 'keep-alive' }
+  }, function(res) {
+    server.close();
+    res.on('aborted', common.mustCall(function() { }));


No need for the space inside the empty curly braces.

jasnell · 2016-10-10T15:48:56Z

With @cjihrig's nits addressed, yes.

kemitchell · 2016-10-19T19:09:36Z

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 · 2016-11-01T19:42:57Z

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

lpinca · 2016-11-02T08:13:45Z

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

PR-URL: nodejs#7376 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

lpinca · 2016-11-02T08:55:33Z

Landed in 6d6f9e5.

kemitchell · 2016-11-02T14:54:38Z

Thanks to all!

PR-URL: #7376 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 22, 2016

addaleax reviewed Jun 22, 2016
View reviewed changes

addaleax added the http Issues or PRs related to the http subsystem. label Jun 22, 2016

kemitchell force-pushed the client-side-aborted-test branch from 2682eef to 44534dd Compare June 27, 2016 02:59

kemitchell force-pushed the client-side-aborted-test branch from 44534dd to ae27d11 Compare June 27, 2016 03:01

jasnell reviewed Jun 28, 2016
View reviewed changes

indutny reviewed Jun 28, 2016
View reviewed changes

kemitchell force-pushed the client-side-aborted-test branch from ae27d11 to cd2b014 Compare June 28, 2016 18:03

kemitchell force-pushed the client-side-aborted-test branch from cd2b014 to 09fcb10 Compare June 29, 2016 07:52

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09

lpinca reviewed Oct 9, 2016

View reviewed changes

lpinca approved these changes Oct 9, 2016

View reviewed changes

kemitchell force-pushed the client-side-aborted-test branch from 09fcb10 to ae28dcf Compare October 10, 2016 04:37

cjihrig requested changes Oct 10, 2016

View reviewed changes

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

test: add test for HTTP client "aborted" event

142e3c4

kemitchell force-pushed the client-side-aborted-test branch from ae28dcf to 142e3c4 Compare October 19, 2016 19:02

jasnell approved these changes Oct 20, 2016

View reviewed changes

cjihrig approved these changes Oct 20, 2016

View reviewed changes

lpinca closed this Nov 2, 2016

kemitchell deleted the client-side-aborted-test branch November 2, 2016 14:54

evanlucas mentioned this pull request Nov 3, 2016

v7.1.0 proposal - 2016-11-08 #9438

Merged

MylesBorins added land-on-v4.x labels Nov 22, 2016

This was referenced Nov 22, 2016

v6.9.2 proposal #9735

Merged

v4.7.0 proposal #9736

Merged

Add test for HTTP client "aborted" event #7376

Add test for HTTP client "aborted" event #7376

Conversation

kemitchell commented Jun 22, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Jun 22, 2016

kemitchell commented Jun 23, 2016

addaleax commented Jun 23, 2016

Trott commented Jun 26, 2016

kemitchell commented Jun 27, 2016

kemitchell commented Jun 27, 2016

Trott commented Jun 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Jun 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Jun 28, 2016 • edited Loading

Choose a reason for hiding this comment

indutny commented Jun 28, 2016

kemitchell commented Jun 28, 2016

kemitchell commented Jun 28, 2016

Trott commented Jun 28, 2016

indutny commented Jun 28, 2016

jasnell commented Jun 28, 2016

kemitchell commented Jun 28, 2016

jasnell commented Jun 28, 2016

jasnell commented Jun 28, 2016

jasnell commented Jun 28, 2016

kemitchell commented Jul 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca commented Oct 9, 2016

lpinca left a comment

Choose a reason for hiding this comment

lpinca commented Oct 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Oct 10, 2016

kemitchell commented Oct 19, 2016

lpinca commented Nov 1, 2016 • edited Loading

lpinca commented Nov 2, 2016

lpinca commented Nov 2, 2016

kemitchell commented Nov 2, 2016

kemitchell commented Jun 22, 2016 •

edited

Loading

mscdex Jun 28, 2016 •

edited

Loading

lpinca commented Nov 1, 2016 •

edited

Loading