Converting test to use Countdown by TomerOmri · Pull Request #17505 · nodejs/node · GitHub

Hi,
this is my first PR :)
I worked on test-http-should-keep-alive.js
Ref: #17169

#Goodness squad

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)

Thanks for the contribution @TomerOmri. Since this is using Countdown now, it should be possible to refactor such that the requests and responses counters are no longer necessary.

Since there's always only one makeRequest in progress, we could for example use something like this in makeRequest:

response = SERVER_RESPONSES.shift();
shouldKeepAlive = SHOULD_KEEP_ALIVE.shift();

where response and shouldKeepAlive are declared in the top scope (and can then be used within both the client handler and the server one).

This appears to be an unrelated change. The newline should remain.

Same here. Unrelated change, newline should remain.

Unrelated change, please remove.

This whole process.on('exit') block shouldn't be necessary if we're trying to use Countdown.

Hi, Thanks for the quick response,
I will be happy if you can clarify some things:

i kept the "requests" and the "responses" variables because they are used on SERVER_RESPONSES[requests] under the makeRequest function.
since those variables are used inside of an Array if we use countdown we will get another value from the array SERVER_RESPONSES.
can you be more clear about your solution with the shift() ?
should I make another countdown instance for requests as well?

There are two directions this can go in.

Instead of using requests and responses, just use SERVER_RESPONSES.length - countdown.remaining — that will get you the same index.
shift removes and returns the element within an array that's at index 0. You can use this to store the current value from SERVER_RESPONSES and SHOULD_KEEP_ALIVE.

Hi @apapirovski after an update,
waiting for your review.
Thanks!

This needs to be the first include within the file. Could you update?

You won't be able to store this in a variable since it won't update. I suppose you could make it a function if you want to avoid having to repeat it everywhere... const getCountdownIndex = () => SERVER_RESPONSES.length - countdown.remaining and then call it as getCountdownIndex().

This block can be removed altogether. The process won't exit unless the countdown finishes.

A few small things left.

Nit: missing semi-colon.

Just noticed but I don't think this const common is actually used? In general, I recommend running make test -j4 — it will run eslint to make sure there are no small issues like this.

I'm sorry, I should've been clearer. This is still required but it should not be assigned to a variable. Also, please try to run make test -j4 before committing as it will flag these types of issues in advance and also run the test being modified.

Hi @apapirovski
Thanks for the help im still learning. the make test -j4` runs with no issues.

Let me know if you find anything else that need to be changed.
Thanks!

Almost all good but it looks like there's a - sign ahead of the require statement. Could you remove? Thanks!

It seems like there's still no semi-colon at the end of this line btw.

Hi, I`m probably not running the eslint as should. the command
just pass without no issues.
how can I make sure its ok?

You could try running make lint-js from your command line, that should lint the files. The -require might not be caught but the missing semi-colon definitely should.

One quick comment

Can you please remove the space between length and the comma?

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

Landing...

Landed in e9d1e12, congrats on your first PR to Node.js!
❤️ 💚 💙 💛 💜

PR-URL: #17505 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>

tomeromrix added 2 commits December 6, 2017 21:13

Edit countdown replace response counter

16e1c01

Revert files

7c58d6d

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

apapirovski reviewed Dec 6, 2017

View reviewed changes

tomeromrix added 2 commits December 6, 2017 23:09

Fix countdown

b711964

Fix countdown

c8c14ab

apapirovski reviewed Dec 6, 2017

View reviewed changes

Fix- add function and remove assertequals

216cced

apapirovski reviewed Dec 6, 2017

View reviewed changes

Fix- semi-colon and removes commo countdown

47d82f5

apapirovski reviewed Dec 6, 2017

View reviewed changes

fix require common

1f9a90d

apapirovski reviewed Dec 7, 2017

View reviewed changes

fix require and semicolon

a94c818

apapirovski approved these changes Dec 7, 2017

View reviewed changes

maclover7 reviewed Dec 8, 2017

View reviewed changes

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

delete space on countdown line

b97270a

daxlab approved these changes Dec 9, 2017

View reviewed changes

maclover7 approved these changes Dec 9, 2017

View reviewed changes

maclover7 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 9, 2017

maclover7 self-assigned this Dec 9, 2017

maclover7 closed this Dec 9, 2017

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

test: update test-http-should-keep-alive to use countdown

9cad427

PR-URL: #17505 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>

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

test: update test-http-should-keep-alive to use countdown

6576382

PR-URL: #17505 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>

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 outstanding review comments, and a CI started. label Dec 13, 2017

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

test: update test-http-should-keep-alive to use countdown

bd4b97f

PR-URL: #17505 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>

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

test: update test-http-should-keep-alive to use countdown

c8220b9

PR-URL: #17505 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>

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: update test-http-should-keep-alive to use countdown

19aa3b1

PR-URL: #17505 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>

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

               process.on('exit', function() {
                 assert.strictEqual(2, testsComplete);
-              });
+              });

                
                    No newline at end of file

                 assert.strictEqual(actualRequests, expectRequests);
                 console.log('ok');
-              });
+              });

                
                    No newline at end of file

               const assert = require('assert');
               const http = require('http');
               const net = require('net');
+              const common = require('../common');

-                assert.strictEqual(responses, SHOULD_KEEP_ALIVE.length);
+                countdownIndexInc = SERVER_RESPONSES.length - countdown.remaining;
+                assert.strictEqual(countdownIndexInc, SERVER_RESPONSES.length);
+                assert.strictEqual(countdownIndexInc, SHOULD_KEEP_ALIVE.length);

               http.globalAgent.maxSockets = 5;
+              const countdown = new Countdown(SHOULD_KEEP_ALIVE.length , () => server.close());
+              let countdownIndexInc = SERVER_RESPONSES.length - countdown.remaining;

               // USE OR OTHER DEALINGS IN THE SOFTWARE.
               'use strict';
-              require('../common');


		const countdown = new Countdown(SHOULD_KEEP_ALIVE.length , () => server.close());

		const getCountdownIndex = () => SERVER_RESPONSES.length - countdown.remaining

		makeRequest();
		});

Converting test to use Countdown by TomerOmri · Pull Request #17505 · nodejs/node · GitHub

Uh oh!

Uh oh!

Converting test to use Countdown - #17505

Converting test to use Countdown#17505TomerOmri wants to merge 9 commits intonodejs:masternodejs/node:masterfrom TomerOmri:countdownTomerOmri/node:countdownCopy head branch name to clipboard

Conversation

TomerOmri commented Dec 6, 2017 • edited by apapirovski 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomerOmri commented Dec 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomerOmri commented Dec 7, 2017

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maclover7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Dec 9, 2017

Uh oh!

maclover7 commented Dec 9, 2017

Uh oh!

maclover7 commented Dec 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Converting test to use Countdown#17505
TomerOmri wants to merge 9 commits into
nodejs:masterfrom
TomerOmri:countdown

TomerOmri commented Dec 6, 2017 •
edited by apapirovski
Loading

apapirovski Dec 7, 2017 •
edited
Loading