test: test-http(s)-set-timeout-server.js tests should be more similar #13588

mscdex · 2017-06-09T20:21:24Z

Version: all
Platform: n/a
Subsystem: test, http, https

Both of these tests contain unique changes that should copied to the other.

Examples:

test-http-set-timeout-server.js has a secReceived check in serverResponseTimeoutWithPipeline() that was added to prevent flakiness, but this does not exist in the https counterpart.
test-https-set-timeout-server.js utilizes common.mustCall(), common.mustNotCall(), etc., while the http counterpart still utilizes the old method of checking flags on exit of the process.

The text was updated successfully, but these errors were encountered:

jklepatch · 2017-06-10T10:53:30Z

I would like to work on this :)

gibfahn · 2017-06-10T14:03:21Z

I would like to work on this :)

Let us know if you need any help.

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 <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

aqrln · 2017-06-19T09:37:26Z

Copying @mscdex's comment from #13625 over here:

There are some other minor differences that could be made consistent, for example:

server.listen() vs server.listen(0, )

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.

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 <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

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 <luigipinca@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

* 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 <luigipinca@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

* 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 <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <luigipinca@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

* 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 <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: James M Snell <jasnell@gmail.com>