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

refactor: Refactor negative test cases in refund.test to use Jest assertions #3464

Closed

Conversation

Deepakchowdavarapu
Copy link

Refactor Negative Test Cases in refund.test.ts to Use Jest Assertions

Replaced try-catch blocks with Jest’s .rejects matcher for error scenarios
in refund.test.ts. Updated test cases to use .toMatch and .toThrow for
error assertions, improving test readability and maintainability.

Test results remain consistent, and all assertions now leverage Jest’s
built-in methods.

  • Rebased onto upstream/main
  • Squashed into a single commit
  • Signed-off commit

BREAKING CHANGE: None

Signed-off-by: Deepakchowdavarapu [email protected]

@Deepakchowdavarapu
Copy link
Author

@petermetz @outSH please review this !!

Copy link
Contributor

@outSH outSH 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 updates. In the future don't open a separate PR but contact us on discord so we can assist you with any git problems you may have :)

I've added a single commnet and I'd like you to fix the title as mentioned in previous PR, and read the contributing guide:

Comment on lines +315 to +332
await expect(
htlcCoordinatorBesuApiClient.withdrawCounterpartyV1(withdrawCounterparty)
).rejects.toThrowErrorMatchingSnapshot();

await expect(
htlcCoordinatorBesuApiClient.withdrawCounterpartyV1(withdrawCounterparty)
).rejects.toThrow(expect.objectContaining({
response: {
data: {
cause: {
receipt: {
revertReason: expect.stringMatching(/0e494e56414c49445f5345435245540/)
}
}
}
}
}));

Copy link
Contributor

Choose a reason for hiding this comment

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

Fix the indentation to match the rest of the file

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@Deepakchowdavarapu Plus one to everything @outSH asked for. Please also sync up the PR description with the commit message body.
If there were no breaking changes please do not put the string BREAKING CHANGE in the commit message. The parser will detect that as a breaking change just based on the presence of the string.

Comment on lines +315 to +317
await expect(
htlcCoordinatorBesuApiClient.withdrawCounterpartyV1(withdrawCounterparty)
).rejects.toThrowErrorMatchingSnapshot();
Copy link
Contributor

Choose a reason for hiding this comment

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

@Deepakchowdavarapu What does this additional assertion accomplish? Is this covering some edge case scenario that the other assertion does not?

@petermetz
Copy link
Contributor

Closing this pull request due to inactivity, but please feel free to reopen as needed and then we can continue the review.

@petermetz petermetz closed this Oct 4, 2024
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.

3 participants