Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Converting test to use Countdown #17505

Closed
wants to merge 9 commits into from
Closed

Conversation

TomerOmri
Copy link

@TomerOmri TomerOmri commented Dec 6, 2017

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)

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

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

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).

@@ -52,4 +52,4 @@ function nextTest() {

process.on('exit', function() {
assert.strictEqual(2, testsComplete);
});
});
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -91,4 +91,4 @@ function makeRequest(n) {
process.on('exit', function() {
assert.strictEqual(actualRequests, expectRequests);
console.log('ok');
});
});
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Unrelated change, newline should remain.

makeRequest();
});


Copy link
Member

Choose a reason for hiding this comment

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

Unrelated change, please remove.

makeRequest();
});


process.on('exit', function() {
assert.strictEqual(requests, SERVER_RESPONSES.length);
assert.strictEqual(responses, SHOULD_KEEP_ALIVE.length);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

  1. 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() ?

  2. should I make another countdown instance for requests as well?

Copy link
Member

Choose a reason for hiding this comment

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

There are two directions this can go in.

  1. Instead of using requests and responses, just use SERVER_RESPONSES.length - countdown.remaining — that will get you the same index.
  2. 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.

@TomerOmri
Copy link
Author

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

@@ -24,6 +24,8 @@ require('../common');
const assert = require('assert');
const http = require('http');
const net = require('net');
const common = require('../common');
Copy link
Member

Choose a reason for hiding this comment

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

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

http.globalAgent.maxSockets = 5;

const countdown = new Countdown(SHOULD_KEEP_ALIVE.length , () => server.close());
let countdownIndexInc = SERVER_RESPONSES.length - countdown.remaining;
Copy link
Member

Choose a reason for hiding this comment

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

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().

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);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

A few small things left.

@@ -46,17 +45,18 @@ const SHOULD_KEEP_ALIVE = [
http.globalAgent.maxSockets = 5;

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

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

Choose a reason for hiding this comment

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

Nit: missing semi-colon.

@@ -20,11 +20,10 @@
// USE OR OTHER DEALINGS IN THE SOFTWARE.

'use strict';
require('../common');
const common = require('../common');
Copy link
Member

Choose a reason for hiding this comment

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

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.

@@ -20,10 +20,10 @@
// USE OR OTHER DEALINGS IN THE SOFTWARE.

'use strict';
require('../common');
Copy link
Member

Choose a reason for hiding this comment

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

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.

@TomerOmri
Copy link
Author

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!

@@ -20,10 +20,11 @@
// USE OR OTHER DEALINGS IN THE SOFTWARE.

'use strict';
require('../common');
-require('../common');
Copy link
Member

@apapirovski apapirovski Dec 7, 2017

Choose a reason for hiding this comment

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

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

http.globalAgent.maxSockets = 5;

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

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

One quick comment

http.globalAgent.maxSockets = 5;

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

Choose a reason for hiding this comment

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

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

@apapirovski
Copy link
Member

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

Landing...

@maclover7 maclover7 self-assigned this Dec 9, 2017
@maclover7
Copy link
Contributor

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

@maclover7 maclover7 closed this Dec 9, 2017
maclover7 pushed a commit to maclover7/node that referenced this pull request Dec 9, 2017
PR-URL: nodejs#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
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
PR-URL: #17505
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
@addaleax 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
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
PR-URL: #17505
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #17505
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
@MylesBorins MylesBorins mentioned this pull request Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants