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.

Sign up for GitHub

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

Jump to bottom

test: use countdown in http-agent test #17537

Closed
wants to merge 1 commit into from
Closed

test: use countdown in http-agent test #17537

wants to merge 1 commit into from

Conversation

fedekau
Copy link
Contributor

@fedekau fedekau commented Dec 7, 2017 •
edited
Loading

Hi, this is my first PR to node!

I started by tackling one of the tests listed in #17169, in particular the test/parallel/test-http-agent.js test. Hopefully everything looks good to you, otherwise let me know what can be improved 😄

Checklist

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 7, 2017
@fedekau fedekau changed the title test: use countdown to http-agent test test: use countdown in http-agent test Dec 7, 2017
@fedekau fedekau force-pushed the add-countdown-to-http-agent-test branch from ecb661b to 48c0c96 Compare December 7, 2017 22:41
@fedekau fedekau mentioned this pull request Dec 7, 2017
Closed
sreepurnajasti
sreepurnajasti reviewed Dec 8, 2017
View reviewed changes
test/parallel/test-http-agent.js Outdated
@@ -33,12 +34,13 @@ const server = http.Server(common.mustCall(function(req, res) {
}, (N * M))); // N * M = good requests (the errors will not be counted)

function makeRequests(outCount, inCount, shouldFail) {
const countdown = new Countdown(1, common.mustCall(() => 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.

Limit of countdown can be outCount*inCount instead of 1.

sreepurnajasti
sreepurnajasti reviewed Dec 8, 2017
View reviewed changes
test/parallel/test-http-agent.js Outdated
@@ -33,12 +34,13 @@ const server = http.Server(common.mustCall(function(req, res) {
}, (N * M))); // N * M = good requests (the errors will not be counted)

function makeRequests(outCount, inCount, shouldFail) {
const countdown = new Countdown(1, common.mustCall(() => server.close()));
let responseCount = outCount * inCount;
let onRequest = common.mustNotCall(); // Temporary
const p = new Promise((resolve) => {
onRequest = common.mustCall((res) => {
if (--responseCount === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can replace this whole if block with countdown.dec();

sreepurnajasti
sreepurnajasti reviewed Dec 8, 2017
View reviewed changes
test/parallel/test-http-agent.js Outdated
@@ -33,12 +34,13 @@ const server = http.Server(common.mustCall(function(req, res) {
}, (N * M))); // N * M = good requests (the errors will not be counted)

function makeRequests(outCount, inCount, shouldFail) {
const countdown = new Countdown(1, common.mustCall(() => server.close()));
let responseCount = outCount * inCount;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can remove responseCount variable.

@sreepurnajasti
Copy link
Contributor

sreepurnajasti commented Dec 8, 2017 •
edited
Loading

Thanks for contribution!!! Please, do use make lint/vcbuild lint before pr.

@fedekau
Copy link
Contributor Author

fedekau commented Dec 8, 2017 •
edited
Loading

@sreepurnajasti Thanks for the comments, I am pretty sure that I did run make lint, maybe I missed something 🤔 . I will run it again.

Do you think this would be ok ?

...
  const countdown = new Countdown(
    outCount * inCount,
    common.mustCall(() => server.close())
  );
  let onRequest = common.mustNotCall(); // Temporary
  const p = new Promise((resolve) => {
    onRequest = common.mustCall((res) => {
      if (countdown.dec() === 0) {
        resolve();
      }

      if (!shouldFail)
        res.resume();
    }, outCount * inCount);
  });
...

@apapirovski
Copy link
Member

That should work. You can also run make -j4 test to make sure the test runs as expected. Or to run a single test, run tools/test.py --mode=release parallel/test-http-agent.

@fedekau fedekau force-pushed the add-countdown-to-http-agent-test branch from 48c0c96 to a03d533 Compare December 8, 2017 20:05
@fedekau
Copy link
Contributor Author

fedekau commented Dec 8, 2017

Thanks @apapirovski, I have already run all tests and linter 😄 after amending the last commit

@sreepurnajasti
Copy link
Contributor

@fedekau LGTM. Thanks!!

@apapirovski
Copy link
Member

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

@fedekau
Copy link
Contributor Author

fedekau commented Dec 9, 2017

@apapirovski Do you think that this failure is related to my change? To me seems like it is not, or is it? 🤔

@apapirovski
Copy link
Member

@fedekau Completely unrelated, this PR is all good :)

@apapirovski
Copy link
Member

Landed in f373a1d

Thanks for the contribution @fedekau! Congrats on becoming a Contributor! 🎉

apapirovski pushed a commit that referenced this pull request Dec 10, 2017
@fedekau @apapirovski
test: use Countdown in http-agent test
f373a1d 
PR-URL: #17537
Reviewed-By: Anatoli Papirovski <[email protected]>
@fedekau fedekau deleted the add-countdown-to-http-agent-test branch December 10, 2017 23:55
@fedekau
Copy link
Contributor Author

fedekau commented Dec 10, 2017

Thanks @apapirovski and @sreepurnajasti I hope this is the first of many PRs 💪

MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@fedekau @MylesBorins
test: use Countdown in http-agent test
8f4f836 
PR-URL: #17537
Reviewed-By: Anatoli Papirovski <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@fedekau @MylesBorins
test: use Countdown in http-agent test
bb780d2 
PR-URL: #17537
Reviewed-By: Anatoli Papirovski <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@fedekau @gibfahn
test: use Countdown in http-agent test
fb33231 
PR-URL: #17537
Reviewed-By: Anatoli Papirovski <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@fedekau @gibfahn
test: use Countdown in http-agent test
2f92fad 
PR-URL: #17537
Reviewed-By: Anatoli Papirovski <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@fedekau @gibfahn
test: use Countdown in http-agent test
bbb503f 
PR-URL: #17537
Reviewed-By: Anatoli Papirovski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sreepurnajasti sreepurnajasti sreepurnajasti left review comments

@apapirovski apapirovski apapirovski approved these changes

Assignees
No one assigned
Labels
test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fedekau @sreepurnajasti @apapirovski @gibfahn @nodejs-github-bot