test: adding Countdown to test-http-set-cookies test by shilomagen · Pull Request #17504 · nodejs/node

shilomagen · 2017-12-06T18:48:46Z

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)

Hi, first PR to NodeJS.
Worked on improving test by using Countdown:
#17169
Worked on file test-http-set-cookies.js

#goodnesssquad

apapirovski

LGTM but some small changes required

apapirovski · 2017-12-06T19:24:38Z

test/parallel/test-http-set-cookies.js

  if (req.url === '/one') {
    res.writeHead(200, [['set-cookie', 'A'],
-                        ['content-type', 'text/plain']]);
+      ['content-type', 'text/plain']]);


Could you undo this change? It should stay lined up.

apapirovski · 2017-12-06T19:24:48Z

test/parallel/test-http-set-cookies.js

-                        ['set-cookie', 'B'],
-                        ['content-type', 'text/plain']]);
+      ['set-cookie', 'B'],
+      ['content-type', 'text/plain']]);


Same for these two lines. They should be lined up as before.

apapirovski · 2017-12-06T19:25:10Z

test/parallel/test-http-set-cookies.js

  // one set-cookie header
  //
-  http.get({ port: this.address().port, path: '/one' }, function(res) {
+  http.get({port: this.address().port, path: '/one'}, function(res) {


Please keep the space to the left and right within the braces.

apapirovski · 2017-12-06T19:25:38Z

test/parallel/test-http-set-cookies.js


 process.on('exit', function() {
-  assert.strictEqual(2, nresponses);
+  assert.strictEqual(countdown.remaining, 0);


The process.on('exit') can be removed as server.close() needs to run before the process will exit.

apapirovski · 2017-12-06T19:25:56Z

test/parallel/test-http-set-cookies.js

  // two set-cookie headers

-  http.get({ port: this.address().port, path: '/two' }, function(res) {
+  http.get({port: this.address().port, path: '/two'}, function(res) {


Same as above, please add the spaces back in.

apapirovski

LGTM

apapirovski · 2017-12-06T19:42:03Z

test/parallel/test-http-set-cookies.js


-let nresponses = 0;
-
+const countdown = new Countdown(2, common.mustCall(() => server.close()));


This doesn't strictly need the common.mustCall but it's not a deal breaker. (If you do remove it, you'll need to also remove the const common = part above (but keep the actual require).

shilomagen · 2017-12-06T19:45:33Z

@apapirovski all of your comments were fixed, thanks!

maclover7 · 2017-12-07T14:44:03Z

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

maclover7 · 2017-12-08T14:40:56Z

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

apapirovski · 2017-12-08T18:48:55Z

Landed in 6a089f9

PR-URL: #17504 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]>

adding Countdown to test-http-set-cookies test

3ef1c59

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

shilomagen mentioned this pull request Dec 6, 2017

[Tracking issue] Converting tests to use Countdown #17169

Closed

apapirovski reviewed Dec 6, 2017

View reviewed changes

adding Countdown to test-http-set-cookies test

f855134

apapirovski approved these changes Dec 6, 2017

View reviewed changes

adding Countdown to test-http-set-cookies test

2676d7c

shilomagen changed the title ~~adding Countdown to test-http-set-cookies test #goodnessSquad~~ adding Countdown to test-http-set-cookies test Dec 6, 2017

shilomagen changed the title ~~adding Countdown to test-http-set-cookies test~~ test: adding Countdown to test-http-set-cookies test Dec 7, 2017

lpinca approved these changes Dec 7, 2017

View reviewed changes

maclover7 approved these changes Dec 7, 2017

View reviewed changes

adding Countdown to test-http-set-cookies test

39f8099

Bamieh approved these changes Dec 8, 2017

View reviewed changes

maclover7 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 8, 2017

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:38

apapirovski mentioned this pull request Dec 8, 2017

Update test-http-set-cookies to use Countdown #17550

Closed

3 tasks

apapirovski closed this Dec 8, 2017

apapirovski pushed a commit that referenced this pull request Dec 8, 2017

test: use Countdown in test-http-set-cookies

6a089f9

PR-URL: #17504 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]>

MylesBorins pushed a commit that referenced this pull request Dec 12, 2017

test: use Countdown in test-http-set-cookies

933e210

PR-URL: #17504 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]>

MylesBorins pushed a commit that referenced this pull request Dec 12, 2017

test: use Countdown in test-http-set-cookies

919625b

PR-URL: #17504 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]>

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2017

gibfahn pushed a commit that referenced this pull request Dec 20, 2017

test: use Countdown in test-http-set-cookies

68dabce

PR-URL: #17504 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]>

gibfahn pushed a commit that referenced this pull request Dec 20, 2017

test: use Countdown in test-http-set-cookies

210d860

PR-URL: #17504 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]>

gibfahn added land-on-v6.x labels Dec 20, 2017

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17772

Closed

gibfahn pushed a commit that referenced this pull request Dec 20, 2017

test: use Countdown in test-http-set-cookies

925db27

PR-URL: #17504 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]>

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17774

Merged

MylesBorins mentioned this pull request Dec 20, 2017

v6.12.3 proposal #17776

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: adding Countdown to test-http-set-cookies test#17504

test: adding Countdown to test-http-set-cookies test#17504
shilomagen wants to merge 4 commits intonodejs:masterfrom
shilomagen:master

shilomagen commented Dec 6, 2017 •

edited

Loading

Uh oh!

apapirovski left a comment

Uh oh!

apapirovski Dec 6, 2017

Uh oh!

apapirovski Dec 6, 2017

Uh oh!

apapirovski Dec 6, 2017

Uh oh!

apapirovski Dec 6, 2017

Uh oh!

apapirovski Dec 6, 2017

Uh oh!

apapirovski left a comment

Uh oh!

apapirovski Dec 6, 2017 •

edited

Loading

Uh oh!

shilomagen commented Dec 6, 2017 •

edited

Loading

Uh oh!

maclover7 commented Dec 7, 2017

Uh oh!

maclover7 commented Dec 8, 2017

Uh oh!

apapirovski commented Dec 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants


		let nresponses = 0;

		const countdown = new Countdown(2, common.mustCall(() => server.close()));

Uh oh!

Conversation

shilomagen commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

apapirovski Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

apapirovski Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

apapirovski Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

apapirovski Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

apapirovski Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

apapirovski Dec 6, 2017 • 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!

shilomagen commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maclover7 commented Dec 7, 2017

Uh oh!

maclover7 commented Dec 8, 2017

Uh oh!

apapirovski commented Dec 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

shilomagen commented Dec 6, 2017 •

edited

Loading

apapirovski Dec 6, 2017 •

edited

Loading

shilomagen commented Dec 6, 2017 •

edited

Loading