test(02-client): adding tests for GetLatestHeight and refactor tests for GetTimestamptAtHeight
#6002
Conversation
…or GetTimestamptAtHeight
WalkthroughThis update focuses on refining error handling in the Changes
Related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
damiannolan
requested review from
colin-axner,
AdityaSripal and
chatton
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (2)
- modules/core/02-client/keeper/keeper.go (1 hunks)
- modules/core/02-client/keeper/keeper_test.go (1 hunks)
Additional comments: 3
modules/core/02-client/keeper/keeper.go (1)
- 504-504: The error wrapping in
GetTimestampAtHeightnow provides more context by including theclientID, which aligns with the PR's objective to enhance error handling. This change improves the clarity and traceability of errors, especially in scenarios where theParseClientIdentifierfunction fails. It's a good practice to include such contextual information in error messages to facilitate easier debugging and issue resolution.modules/core/02-client/keeper/keeper_test.go (2)
- 458-511: The
TestGetLatestHeightfunction tests various scenarios for retrieving the latest height from an IBC client. However, there are a few areas that could be improved for clarity and robustness:
- The test case "invalid client type" might benefit from a more detailed setup or comment explaining why the client ID is considered invalid. This would help future maintainers understand the test's intent more quickly.
- For the test case "client type is not allowed", it's good practice to reset any global or shared state after the test runs to avoid side effects on other tests. Consider adding a teardown step to reset the
ClientKeeper's params to their original values.- It's recommended to add a test case to explicitly check for a scenario where the client exists but has no consensus states, as this is a realistic scenario that should be verified.
Consider adding more detailed comments or setup steps to clarify the intent of test cases, especially for "invalid client type". Also, ensure that modifications to shared state (like params) are reset after each test to prevent unintended side effects.
- 512-581: The
TestGetTimestampAtHeightfunction has been modified to include additional test cases and logic for error handling. Here are some suggestions for improvement:
- Similar to
TestGetLatestHeight, ensure that any changes to global or shared state are properly reset after each test case to prevent side effects.- The test case "consensus state not found" is a good addition. Consider adding a brief comment explaining the setup, especially how the
heightvariable is manipulated to ensure the consensus state is not found.- For better readability, consider using a table-driven approach for defining test cases if not already done. This approach can make it easier to add new test cases in the future and improve the readability of the test code.
Ensure that modifications to shared state are reset after each test. Consider adding more detailed comments for complex test setups and exploring a table-driven approach for defining test cases to improve readability and maintainability.
|
Description
Follow up testing PR from #5806
GetTimestampAtHeightGetLatestHeightGetTimetstampAtHeightBefore we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.Summary by CodeRabbit