Skip to content

test: use countdown in http-agent test#17537

Closed
fedekau wants to merge 1 commit into
nodejs:masterfrom
fedekau:add-countdown-to-http-agent-test
Closed

test: use countdown in http-agent test#17537
fedekau wants to merge 1 commit into
nodejs:masterfrom
fedekau:add-countdown-to-http-agent-test

Conversation

@fedekau
Copy link
Copy Markdown
Contributor

@fedekau fedekau commented Dec 7, 2017

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
Comment thread test/parallel/test-http-agent.js Outdated
Copy link
Copy Markdown
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.

Comment thread test/parallel/test-http-agent.js Outdated
Copy link
Copy Markdown
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();

Comment thread test/parallel/test-http-agent.js Outdated
Copy link
Copy Markdown
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
Copy Markdown
Contributor

sreepurnajasti commented Dec 8, 2017

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

@fedekau
Copy link
Copy Markdown
Contributor Author

fedekau commented Dec 8, 2017

@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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

@fedekau LGTM. Thanks!!

@apapirovski
Copy link
Copy Markdown
Contributor

@fedekau
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

@apapirovski
Copy link
Copy Markdown
Contributor

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
PR-URL: #17537
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@fedekau fedekau deleted the add-countdown-to-http-agent-test branch December 10, 2017 23:55
@fedekau
Copy link
Copy Markdown
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
PR-URL: #17537
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #17537
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #17537
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #17537
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #17537
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
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.

5 participants