test: import test-http-server-keep-alive-timeout by realwakka · Pull Request #13448 · nodejs/node

realwakka · 2017-06-04T05:14:22Z

Making same relibility changes that were applied to the https test.
Https's relibility changes is in ce5745b.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

Making same relibility changes that were applied to the https test. Https's relibility changes is in ce5745b.

Trott

LGTM if CI is green and stress tests have expected results

Trott · 2017-06-04T05:19:21Z

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

gibfahn · 2017-06-04T10:44:02Z

cc/ @refack @jasnell @lpinca @aqrln (who reviewed #13312)

realwakka · 2017-06-04T13:51:31Z

I get this message from CI

not ok 1418 inspector/test-inspector-port-zero-cluster # TODO : Fix flaky test

and test is marked with UNSTABLE. How can I fix this problem???

refack · 2017-06-04T14:09:52Z

I get this message from CI

not ok 1418 inspector/test-inspector-port-zero-cluster # TODO : Fix flaky test
and test is marked with UNSTABLE. How can I fix this problem???

You can ignore this. Yellow CI means that something that we know is broken (or flaky) has failed again. If you're interested the tracking issue is #13343

refack · 2017-06-04T14:09:56Z

On a different subject, question to @nodejs/testing if test-http-server-keep-alive-timeout.js and test-https-server-keep-alive-timeout.js are similar enough, should we parametrize and merge?

realwakka · 2017-06-04T17:11:26Z

Sure, I can fix it that simplify http and https. And I'll work it soon.

Trott · 2017-06-05T00:45:33Z

Nit on the commit message, but it can be handled by whoever lands this: I'd say refactor instead of import. The test isn't being imported, really.

realwakka · 2017-06-07T00:33:51Z

I have some problem with parameterizing and merging. I made some js file for sharing function for http and https. there is dilemma about linter. new js file ( test-server-keep-alive-timeout.js ) requires common and http/https requires new js source. linter says every js file needs to require mandatory common module. because of this, I inserted some source in http/https but it's gonna be unused variable!
is there other way to use new js file for function?

refack · 2017-06-07T01:23:29Z

I have some problem with parameterizing and merging. I made some js file for sharing function for http and https. there is dilemma about linter. new js file ( test-server-keep-alive-timeout.js ) requires common and http/https requires new js source. linter says every js file needs to require mandatory common module. because of this, I inserted some source in http/https but it's gonna be unused variable!
is there other way to use new js file for function?

You could put a comment suppressing the linter, but if it's not a trivial change, IMHO you can skip the merge.

Parameterize and merge two similar test sources. Http's test has function and http's test require it.

lpinca · 2017-06-08T16:10:37Z

Despite @refack suggestion I think keeping the tests separate is better.

Advantages:

easier to read and understand.
easier to modify them independently if necessary.
no need to use workarounds for our test harness.

Disadvantages:

code duplication.

lpinca · 2017-06-08T16:11:50Z

Anyway this needs to be re-reviewed.

refack · 2017-06-08T17:15:27Z

+  let createServer;

-const tests = [];
+  if (isHttps) {


Since this is the only if id do this separately for each file, and pass all the objects as args.

refack

Still 👍
Requesting some changes for readability

refack · 2017-06-08T17:15:50Z

-    const options = {
-      port: server.address().port,
-      allowHalfOpen: true
+    const fs = require('fs');


then move all the require to the top

refack · 2017-06-08T17:20:12Z

-const assert = require('assert');
-const http = require('http');
-const net = require('net');
+module.exports.testServerKeepAliveTimeout = function(common, isHttps) {


As per https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md
put const common = require('../common'); as L2 then a blank line and then a comment

// Testing http[s] server keepalive semantics. // Reusing tests with test-https-server-keep-alive-timeout.js // Ref: https://github.com/nodejs/node/pull/13448

This is a test file, not a library file. It shouldn't be exporting. -100 to exporting something to another test file from it.

refack · 2017-06-08T17:23:01Z

Despite @refack suggestion I think keeping the tests separate is better.

Advantages:

easier to read and understand.

easier to modify them independently if necessary.

no need to use workarounds for our test harness.

Disadvantages:

code duplication.

One more for the pro-merge:

Assert that keepalive semantics are the same for http and https

refack · 2017-06-08T17:28:54Z

-    });
-  }));
-});
+const timeoutTest = require('./test-http-server-keep-alive-timeout');


IMHO this could be moved to other file as well, and delete this one.
So other file will be:

const setup http, net, createOptions, serverOptions, createServer; testServerKeepAliveTimeout(http, net, createOptions, serverOptions, createServer) const https, tls, createOptions, serverOptions, createServer; testServerKeepAliveTimeout(https, tls, createOptions, serverOptions, createServer)

This has the benefit of eliminating node startup time (that could be ~2s)

Trott

Well, it was OK. This refactoring to remove code duplication is not something I'm excited about for the reasons @lpinca outlines. This PR should have landed when it was getting approvals and it worked and it was a clear improvement over what was there. The additional refactoring to remove code duplication could have been a separate PR. @realwakka if you want to revert it to what you had, that would be great by me. You can open a separate PR for the refactoring to remove code duplication if interested.

Trott · 2017-06-08T20:54:07Z

-    });
-  }));
-});
+const timeoutTest = require('./test-http-server-keep-alive-timeout');


Please do not use require() to load an adjacent test file as a module

refack · 2017-06-08T21:05:09Z

You can open a separate PR for the refactoring to remove code duplication if interested.

👍

aqrln · 2017-06-08T23:43:42Z

+1 to what @lpinca and @Trott said. I don't think that code duplication is a problem for tests. To the contrary, the testing code should be maximally dumb and straightforward, or otherwise you'd need to test your tests ;)

aqrln · 2017-06-08T23:44:42Z

+
+    createOptions = function(server) {
+      return {
+ port: server.address().port,


FWIW, the indentation is off here.

This reverts commit f0ef3e6.

aqrln · 2017-06-09T00:26:09Z

@realwakka I've noticed you reverted the last commit, thanks.
Fresh CI: https://ci.nodejs.org/job/node-test-pull-request/8575/

refack

Still LGTM.

code reverted, dismissing my request for changes

aqrln · 2017-06-09T18:40:42Z

Landed in 1f9b1aa. Congratulations on your first contribution 🎉

Make the same reliability changes that were applied to the https test in ce5745b. Refs: #13312 PR-URL: #13448 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

test: import test-http-server-keep-alive-timeout

489454d

Making same relibility changes that were applied to the https test. Https's relibility changes is in ce5745b.

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

mscdex added the http Issues or PRs related to the http subsystem. label Jun 4, 2017

Trott approved these changes Jun 4, 2017

View reviewed changes

lpinca approved these changes Jun 4, 2017

View reviewed changes

refack approved these changes Jun 4, 2017

View reviewed changes

jasnell approved these changes Jun 4, 2017

View reviewed changes

aqrln approved these changes Jun 5, 2017

View reviewed changes

refack self-assigned this Jun 7, 2017

test: refactor test-http-server-keep-alive-timeout

f0ef3e6

Parameterize and merge two similar test sources. Http's test has function and http's test require it.

refack reviewed Jun 8, 2017

View reviewed changes

refack suggested changes Jun 8, 2017

View reviewed changes

refack reviewed Jun 8, 2017

View reviewed changes

Trott previously requested changes Jun 8, 2017

View reviewed changes

Trott reviewed Jun 8, 2017

View reviewed changes

aqrln reviewed Jun 8, 2017

View reviewed changes

Revert "test: refactor test-http-server-keep-alive-timeout"

c2f3f7e

This reverts commit f0ef3e6.

refack approved these changes Jun 9, 2017

View reviewed changes

aqrln closed this Jun 9, 2017

refack removed their assignment Jun 9, 2017

addaleax mentioned this pull request Jun 10, 2017

v8.1.1 proposal #13598

Merged

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the dont-land-on-v6.x label Jul 17, 2017

Uh oh!

Conversation

realwakka commented Jun 4, 2017 • edited by gibfahn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Jun 4, 2017

Uh oh!

gibfahn commented Jun 4, 2017

Uh oh!

realwakka commented Jun 4, 2017

Uh oh!

refack commented Jun 4, 2017

Uh oh!

refack commented Jun 4, 2017

Uh oh!

realwakka commented Jun 4, 2017

Uh oh!

Trott commented Jun 5, 2017

Uh oh!

realwakka commented Jun 7, 2017

Uh oh!

refack commented Jun 7, 2017

Uh oh!

lpinca commented Jun 8, 2017

Uh oh!

lpinca commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack Jun 8, 2017

Choose a reason for hiding this comment

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refack Jun 8, 2017

Choose a reason for hiding this comment

Uh oh!

refack Jun 8, 2017

Choose a reason for hiding this comment

Uh oh!

Trott Jun 8, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack Jun 8, 2017

Choose a reason for hiding this comment

Uh oh!

Trott left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott Jun 8, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented Jun 8, 2017

Uh oh!

aqrln commented Jun 8, 2017

Uh oh!

aqrln Jun 8, 2017

Choose a reason for hiding this comment

Uh oh!

aqrln commented Jun 9, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

aqrln commented Jun 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

realwakka commented Jun 4, 2017 •

edited by gibfahn

Loading

lpinca commented Jun 8, 2017 •

edited

Loading

refack commented Jun 8, 2017 •

edited

Loading

Trott left a comment •

edited

Loading