test: improve the code in test-http-chunked-304 #10462

edsadr · 2016-12-26T13:47:50Z

Checklist

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

Affected core subsystem(s)

test

Description of change

use const and let instead of var
use common.mustCall to control functions executions
use assert.strictEqual instead of assert.equal
use arrow functions

thefourtheye · 2016-12-26T18:47:01Z

test/parallel/test-http-chunked-304.js

-        if (next) process.nextTick(next);
+        conn.on('end', common.mustCall(function() {
+          assert.strictEqual(/^Connection: close\r\n$/m.test(resp), true);
+          assert.strictEqual(/^0\r\n$/m.test(resp), false);


I am not sure what exactly this tests.

@thefourtheye as I see it, this is testing this code: https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L255-L279 , so Connection: close should be in the resp and as the comment there says : // Make sure we don't end the 0\r\n\r\n at the end of the message.

Perhaps add that information as a comment to the test?

thefourtheye · 2016-12-27T05:47:21Z

test/parallel/test-http-chunked-304.js

@@ -12,32 +12,33 @@ var net = require('net');
 // Likewise for 304 responses. Verify that no empty chunk is sent when
 // the user explicitly sets a Transfer-Encoding header.

-test(204, function() {
+test(204, () => {
  test(304);


I am not a big fan of nesting functions unless it is unavoidable. Now that we have common.mustCall to ensure that assertions will happen for sure, why don't we refactor this to sequential calls?

edsadr · 2016-12-27T17:45:36Z

@Trott Added the comments clarifying the assertions, @thefourtheye changed the nested functions to run in parallel

italoacasas · 2016-12-27T18:30:22Z

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

Trott · 2016-12-27T19:09:52Z

test/parallel/test-http-chunked-304.js

+        conn.on('end', common.mustCall(function() {
+          // Connection: close should be in the response
+          assert.strictEqual(/^Connection: close\r\n$/m.test(resp), true);
+          // Make sure this don't end with 0\r\n\r\n


Totally unimportant nit that should not stop this from landing but if you want to fix it up, go for it:

don't -> doesn't

edsadr · 2016-12-28T02:19:58Z

@Trott , sorry about that... just fixed it 🙂

thefourtheye · 2016-12-28T14:10:29Z

I think there is a git commit problem. I see two changed files.

edsadr · 2016-12-28T15:43:49Z

@thefourtheye you are right, sorry about that handling too many branches and I made a mistake, just fixed it...

thefourtheye

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

Just to be sure :-)

lpinca · 2017-01-07T11:33:07Z

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

lpinca · 2017-01-07T12:29:54Z

test/parallel/test-http-chunked-304.js

-      conn.write('GET / HTTP/1.1\r\n\r\n');
+  server.listen(0, common.mustCall(function() {
+    const conn = net.createConnection(this.address().port,
+      common.mustCall(() => {


Linter is not happy with this indentation.

* change the nested functions call to run tests in parallel * use const and let instead of var * use common.mustCall to control functions execution * use assert.strictEqual instead of assert.equal * use arrow functions

edsadr · 2017-01-07T16:01:41Z

test/parallel/test-http-chunked-304.js

-    var conn = net.createConnection(this.address().port, function() {
-      conn.write('GET / HTTP/1.1\r\n\r\n');
+  server.listen(0, common.mustCall(() => {
+    const conn = net.createConnection(


@lpinca didn't get an error before with make -j8 lint, but changed it this way to make it look better, same.. no error with make -j8 lint, let me know if this way works or if not please provide an example and will make it fit

It works now, thanks.

lpinca · 2017-01-07T17:08:53Z

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

lpinca · 2017-01-07T17:43:52Z

Landed in f895715.

* change the nested functions call to run tests in parallel * use const and let instead of var * use common.mustCall to control functions execution * use assert.strictEqual instead of assert.equal * use arrow functions PR-URL: #10462 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

* change the nested functions call to run tests in parallel * use const and let instead of var * use common.mustCall to control functions execution * use assert.strictEqual instead of assert.equal * use arrow functions PR-URL: nodejs#10462 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

* change the nested functions call to run tests in parallel * use const and let instead of var * use common.mustCall to control functions execution * use assert.strictEqual instead of assert.equal * use arrow functions PR-URL: #10462 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Dec 26, 2016

lpinca added the http Issues or PRs related to the http subsystem. label Dec 26, 2016

cjihrig approved these changes Dec 26, 2016

View reviewed changes

thefourtheye reviewed Dec 26, 2016

View reviewed changes

jasnell approved these changes Dec 26, 2016

View reviewed changes

edsadr force-pushed the http-chunked branch from 07802fc to 0c9c1da Compare December 27, 2016 02:31

nodejs-github-bot added lts-watch-v6.x labels Dec 27, 2016

thefourtheye reviewed Dec 27, 2016

View reviewed changes

edsadr force-pushed the http-chunked branch from 0c9c1da to 168b83a Compare December 27, 2016 17:42

nodejs-github-bot added the lts-watch-v6.x label Dec 27, 2016

italoacasas approved these changes Dec 27, 2016

View reviewed changes

Trott reviewed Dec 27, 2016

View reviewed changes

edsadr force-pushed the http-chunked branch from 168b83a to c40c17c Compare December 28, 2016 02:19

nodejs-github-bot added the lts-watch-v6.x label Dec 28, 2016

edsadr force-pushed the http-chunked branch from c40c17c to 5f30793 Compare December 28, 2016 15:42

nodejs-github-bot added lts-watch-v6.x labels Dec 28, 2016

thefourtheye approved these changes Dec 28, 2016

View reviewed changes

princejwesley approved these changes Dec 28, 2016

View reviewed changes

lpinca approved these changes Jan 7, 2017

View reviewed changes

lpinca reviewed Jan 7, 2017

View reviewed changes

test: improve the code in test-http-chunked-304

842f514

* change the nested functions call to run tests in parallel * use const and let instead of var * use common.mustCall to control functions execution * use assert.strictEqual instead of assert.equal * use arrow functions

edsadr force-pushed the http-chunked branch from 5f30793 to 842f514 Compare January 7, 2017 15:57

nodejs-github-bot added lts-watch-v6.x labels Jan 7, 2017

edsadr commented Jan 7, 2017

View reviewed changes

lpinca closed this Jan 7, 2017

italoacasas mentioned this pull request Jan 25, 2017

Proposal to update v7.x-staging #10989

Closed

italoacasas mentioned this pull request Jan 29, 2017

v7.5.0 proposal #11062

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Mar 8, 2017

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

MylesBorins mentioned this pull request Mar 9, 2017

v4.8.1 proposal #11760

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: improve the code in test-http-chunked-304 #10462

test: improve the code in test-http-chunked-304 #10462

edsadr commented Dec 26, 2016

thefourtheye Dec 26, 2016

edsadr Dec 27, 2016 •

edited

Loading

Trott Dec 27, 2016

thefourtheye Dec 27, 2016

edsadr commented Dec 27, 2016

italoacasas commented Dec 27, 2016

Trott Dec 27, 2016

edsadr commented Dec 28, 2016

thefourtheye commented Dec 28, 2016

edsadr commented Dec 28, 2016

thefourtheye left a comment

lpinca commented Jan 7, 2017

lpinca Jan 7, 2017

edsadr Jan 7, 2017

lpinca Jan 7, 2017

lpinca commented Jan 7, 2017

lpinca commented Jan 7, 2017

test: improve the code in test-http-chunked-304 #10462

test: improve the code in test-http-chunked-304 #10462

Conversation

edsadr commented Dec 26, 2016

Checklist

Affected core subsystem(s)

Description of change

thefourtheye Dec 26, 2016

Choose a reason for hiding this comment

edsadr Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

Trott Dec 27, 2016

Choose a reason for hiding this comment

thefourtheye Dec 27, 2016

Choose a reason for hiding this comment

edsadr commented Dec 27, 2016

italoacasas commented Dec 27, 2016

Trott Dec 27, 2016

Choose a reason for hiding this comment

edsadr commented Dec 28, 2016

thefourtheye commented Dec 28, 2016

edsadr commented Dec 28, 2016

thefourtheye left a comment

Choose a reason for hiding this comment

lpinca commented Jan 7, 2017

lpinca Jan 7, 2017

Choose a reason for hiding this comment

edsadr Jan 7, 2017

Choose a reason for hiding this comment

lpinca Jan 7, 2017

Choose a reason for hiding this comment

lpinca commented Jan 7, 2017

lpinca commented Jan 7, 2017

edsadr Dec 27, 2016 •

edited

Loading