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(evmstaking/types): add test cases for withdraw #55

Merged

Conversation

zsystm
Copy link
Contributor

@zsystm zsystm commented Aug 26, 2024

This PR increased test coverage of client/x/evmstaking/types/withdraw.go from 0% to 100%.

issue: #64

@zsystm zsystm marked this pull request as draft August 26, 2024 02:22
@zsystm zsystm marked this pull request as ready for review August 26, 2024 02:26
@zsystm zsystm requested a review from limengformal August 31, 2024 17:05

// Test with spaces, it should trim the spaces
stringWithSpaces := " " + ws.String() + " " // add leading and trailing spaces
trimmedString := strings.TrimSpace(stringWithSpaces)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to test that String() will correctly trim spaces not TrimSpace can trim spaces. You should add a new test case where it has leading or trailing spaces in the resulted string from a NewWithdrawal. If it is impossible to construct such a case, you may skip the test case.

Copy link
Contributor Author

@zsystm zsystm Sep 2, 2024

Choose a reason for hiding this comment

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

@limengformal
I think it's good to skip the test case because testing that case is quite tricky.
To test that part, we would need to mock the String() of Withdrawal that intentionally returns a string with leading or trailing spaces. However, this seems quite complex and cumbersome. (Currently we'are using default String defined in evmstaking.pb.go for Withdrwal struct)

Copy link
Contributor

Choose a reason for hiding this comment

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

@zsystm Then let's remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@limengformal
You mean let's remove the code we are currently discussing in the comments?
I'll remove it soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

@zsystm zsystm self-assigned this Sep 2, 2024
@zsystm zsystm force-pushed the zsystm/test/evmstaking/types/withdraw branch from a116f10 to c042fa4 Compare September 2, 2024 05:47
@limengformal limengformal merged commit b5bde35 into piplabs:main Sep 2, 2024
6 checks passed
@zsystm zsystm deleted the zsystm/test/evmstaking/types/withdraw branch September 25, 2024 04:13
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