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

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

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

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

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Italo A. Casas <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

* 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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Italo A. Casas <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

* 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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Italo A. Casas <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

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/parallel/test-http-chunked-304.js Outdated

Copy link

Member

lpinca Jan 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linter is not happy with this indentation.

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

Uh oh!

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

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

Uh oh!

Conversation

edsadr commented Dec 26, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

thefourtheye Dec 26, 2016

Choose a reason for hiding this comment

Uh oh!

edsadr Dec 27, 2016 • 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 Dec 27, 2016

Choose a reason for hiding this comment

Uh oh!

thefourtheye Dec 27, 2016

Choose a reason for hiding this comment

Uh oh!

edsadr commented Dec 27, 2016

Uh oh!

italoacasas commented Dec 27, 2016

Uh oh!

Trott Dec 27, 2016

Choose a reason for hiding this comment

Uh oh!

edsadr commented Dec 28, 2016

Uh oh!

thefourtheye commented Dec 28, 2016

Uh oh!

edsadr commented Dec 28, 2016

Uh oh!

thefourtheye left a comment

Choose a reason for hiding this comment

Uh oh!

lpinca commented Jan 7, 2017

Uh oh!

lpinca Jan 7, 2017

Choose a reason for hiding this comment

Uh oh!

edsadr Jan 7, 2017

Choose a reason for hiding this comment

Uh oh!

lpinca Jan 7, 2017

Choose a reason for hiding this comment

Uh oh!

lpinca commented Jan 7, 2017

Uh oh!

lpinca commented Jan 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

edsadr Dec 27, 2016 •

edited

Loading