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

test: use countdown utility #32224

Closed
wants to merge 2 commits into from

Conversation

HarshithaKP
Copy link
Member

Refs: #32082 (comment)

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 12, 2020
@addaleax
Copy link
Member

I’m not sure that this actually makes the code easier to understand…

@Trott
Copy link
Member

Trott commented Mar 12, 2020

I know @jasnell wrote it and likes it, so I'm always hesitant to express the opinion, but I dislike using the Countdown() abstraction in tests in most cases. Tests are the one place where readability is almost always more important than avoiding redundant code. As @addaleax notes, this doesn't make the test more readable.

@@ -57,7 +61,8 @@ server.listen(0, function() {
}, function(res) {
res.resume();
assert.strictEqual(res.statusCode, 200);
if (++responses === N * M) server.close();
++responses;
Copy link
Member

Choose a reason for hiding this comment

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

By adding the countdown you can eliminate the responses counter and the process.on('exit') check

@jasnell
Copy link
Member

jasnell commented Mar 12, 2020

It's not always about making the code more understandable... this doesn't make it less understandable either. There is Do-thing-when-X-events-happen logic all over our tests and it has been done inconsistently and in some cases incorrectly. Countdown has allowed us to unify that logic. I'd be a strong -1 on going back to having every test implementing their own countdown logic.

@HarshithaKP
Copy link
Member Author

  1. I removed the need to use response variable, as per the suggestion
  2. Is the difference in opinion on the usage of countdown latch in this particular test case, or its usage anywhere in general? Is there anything I can do to look at?

@addaleax
Copy link
Member

Is the difference in opinion on the usage of countdown latch in this particular test case, or its usage anywhere in general?

@HarshithaKP A bit of both. Usually, the Countdown utility doesn’t add much value to a test. In this particular case, using it hides the fact that there is an explicit assertion about the number of responses.

My preference would be to not go ahead with this.

@HarshithaKP
Copy link
Member Author

Closing based on the feedback.

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.

5 participants