Harmonize test http https set timeout server by jklepatch · Pull Request #13625 · nodejs/node

jklepatch · 2017-06-12T03:43:32Z

Fixes #13588

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows [commit guidelines]

Affected core subsystem(s)

test/sequential and test/parallel

[refack fixed formating]

…NotCall

Trott

LGTM if CI is green

Trott · 2017-06-12T03:47:15Z

@nodejs/testing

mscdex · 2017-06-12T04:31:37Z

If you want, there are some other minor differences that could be made consistent, for example:

server.listen(<callback>) vs server.listen(0, <callback>)
missing assert.ok(s instanceof http.OutgoingMessage);-type asserts in the https test file
missing common.mustCall()s in the https test file (e.g. around server.listen() callback -- compared to http test file) and the http test file (e.g. around request handler callback -- compared to https test file)
etc..

Really the only differences that should exist between the two should be the references to the http module vs the https module, use of net.connect() vs tls.connect(), and https.Server-specific stuff, like the passing of the cert, etc. configuration to https.createServer(). Everything else should be the same.

jklepatch · 2017-06-12T05:16:01Z

ok, Ill do that. May I know which test should be taken as the reference to be copied from? http or https?

mscdex · 2017-06-12T05:25:18Z

@jklepatch That's kind of the problem, it depends. For the server.listen() example I would just omit the port number I think.

cjihrig

LGTM with or without the other requested changes, although they should probably be addressed at some point.

aqrln · 2017-06-16T07:43:40Z

test/parallel/test-http-set-timeout-server.js

-    const s = req.setTimeout(50, function() {
-      caughtTimeout = true;
-      req.socket.destroy();
+    const s = req.setTimeout(50,  common.mustCall(function(socket) {


Nit: one of the spaces after the comma is superfluous.

aqrln · 2017-06-16T08:12:20Z

@jklepatch ping. Are you still up to working on this? This PR is a clear improvement over the existing tests as is, so if you currently don't have time to implement the other suggestions, you or someone else may do that in another PR.

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

jklepatch · 2017-06-17T07:44:41Z

@aqrln I am still up for doing the remaining changes discussed. Should I continue to work on this PR or open another one?

Trott · 2017-06-17T19:39:38Z

@aqrln I am still up for doing the remaining changes discussed. Should I continue to work on this PR or open another one?

I personally prefer multiple small PRs over one omnibus PR. If a bug slips through, we can revert just the problematic part and not everything.

jklepatch · 2017-06-18T08:15:34Z

ok. I'll start another PR for the rest of the work now. Nothing to add to this PR then.

aqrln · 2017-06-18T09:32:22Z

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

aqrln · 2017-06-19T09:32:58Z

Landed in bed3579. Thanks for the contribution!

Make test-http(s)-set-timeout-server tests more similar and resolve the following issues: * `test-http-set-timeout-server.js` had a `secReceived` check in `serverResponseTimeoutWithPipeline()` that was added to prevent flakiness, but this did not exist in the https counterpart. * `test-https-set-timeout-server.js` utilized `common.mustCall()`, `common.mustNotCall()`, etc., while the http counterpart utilized the old method of checking flags on exit of the process. Refs: #13588 PR-URL: #13625 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]>

Make test-http(s)-set-timeout-server tests more similar and resolve the following issues: * `test-https-set-timeout-server.js` was missing some `assert` statements, including with `http` module * Both files were missing some calls to `common.mustCall()` * Both files were calling `createServer()` in different ways Fixes: nodejs#13588 Refs: nodejs#13625

Make test-http(s)-set-timeout-server tests more similar and resolve the following issues: * `test-https-set-timeout-server.js` was missing some `assert` statements, including with `http` module * Both files were missing some calls to `common.mustCall()` * Both files were calling `createServer()` in different ways PR-URL: #13822 Refs: #13588 Refs: #13625 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: James M Snell <[email protected]>

* Make changes to `test-https-set-timeout-server` to resolve inconsistencies with its http counterpart: - Apply the changes analogous to those in nodejsGH-13802 to the https test. - Add a missing `common.mustCall()` wrapper. - Make small stylistic changes (e.g., remove unnecessary line breaks in comments) to make it visually consistent with the http test. * Use arrow functions. Refs: nodejs#13802 Refs: nodejs#13625 Refs: nodejs#13822 Fixes: nodejs#13588

Make test-http(s)-set-timeout-server tests more similar and resolve the following issues: * `test-https-set-timeout-server.js` was missing some `assert` statements, including with `http` module * Both files were missing some calls to `common.mustCall()` * Both files were calling `createServer()` in different ways PR-URL: #13822 Refs: #13588 Refs: #13625 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: James M Snell <[email protected]>

* Make changes to `test-https-set-timeout-server` to resolve inconsistencies with its http counterpart: - Apply the changes analogous to those in nodejsGH-13802 to the https test. - Add a missing `common.mustCall()` wrapper. - Make small stylistic changes (e.g., remove unnecessary line breaks in comments) to make it visually consistent with the http test. * Use arrow functions. PR-URL: nodejs#13935 Fixes: nodejs#13588 Refs: nodejs#13802 Refs: nodejs#13625 Refs: nodejs#13822 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: James M Snell <[email protected]>

Make test-http(s)-set-timeout-server tests more similar and resolve the following issues: * `test-https-set-timeout-server.js` was missing some `assert` statements, including with `http` module * Both files were missing some calls to `common.mustCall()` * Both files were calling `createServer()` in different ways PR-URL: #13822 Refs: #13588 Refs: #13625 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: James M Snell <[email protected]>

* Make changes to `test-https-set-timeout-server` to resolve inconsistencies with its http counterpart: - Apply the changes analogous to those in GH-13802 to the https test. - Add a missing `common.mustCall()` wrapper. - Make small stylistic changes (e.g., remove unnecessary line breaks in comments) to make it visually consistent with the http test. * Use arrow functions. PR-URL: #13935 Fixes: #13588 Refs: #13802 Refs: #13625 Refs: #13822 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: James M Snell <[email protected]>