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

Implement part of #1557: Add tests for replayRate controller. #1579

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rahat2134
Copy link

@rahat2134 rahat2134 commented May 17, 2024

Proof

image

Checklist

  • A link to the issue/user story that the pull request relates to
  • How to recreate the problem without the fix
  • Design of the fix
  • How to prove that the fix works
  • Automated tests that prove the fix keeps on working
  • Documentation - any JSDoc, website, or Stackoverflow answers?

Issue/User story

Add tests for replayRate controller.

Signed-off-by: rahat2134 <[email protected]>
@rahat2134
Copy link
Author

rahat2134 commented May 17, 2024

@davidkel @aklenik PTAL now, I have corrected the sign-off issue. Thanks.

@rahat2134 rahat2134 marked this pull request as ready for review May 17, 2024 19:10
@rahat2134 rahat2134 requested a review from a team May 17, 2024 19:10
@rahat2134
Copy link
Author

@davidkel PTAL.

Copy link
Contributor

@davidkel davidkel 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 minor comments about how to describe the tests

afterEach(() => {
sandbox.restore();
});

Copy link
Contributor

Choose a reason for hiding this comment

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

we need to describe what was being done here as well, so we need a describe wrapping the first 3 it statements with a description such as
When creating the replay rate controller

fs.unlinkSync(tempFilePath);
});

it('should correctly apply rate control and log warning when running out of transaction timings', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are 2 different tests here, can we split them into 2 please. One where there are no log warnings and one where there are log warnings

fs.unlinkSync(tempFilePath);
});

it('should call end() method without any errors', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We try to avoid method names in descriptions of tests and focus on behaviour descriptions. It's a way to understand what this test is trying to achieve. Then it also helps to understand what behaviours may also be missing in the tests. So for this one it is testing that the rate controller is never actually used, but what about when it is used ? We don't want be influenced by the implementation as it then keeps the tests viable for when changes are made to the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants