test: move 07-tendermint client state testing to use light client module entry points #6074
Conversation
…to-light-client-module-entry-points
WalkthroughThe changes involve enhancing error handling, refining test structures, and introducing a new error for invalid trust levels across several files in the Tendermint light client module. The modifications focus on improving validation methods, error messaging, and test coverage for client state and light client functionalities. This includes restructuring test cases, updating error handling mechanisms, and adding new test functions to ensure comprehensive validation and verification processes. Changes
Assessment against linked issues
Possibly 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 (
|
charleenfei
changed the title
| @@ -123,7 +123,7 @@ func (cs ClientState) Validate() error { | |||
| } | |||
|
|
|||
| if err := light.ValidateTrustLevel(cs.TrustLevel.ToTendermint()); err != nil { | |||
| return err | |||
| return errorsmod.Wrapf(ErrInvalidTrustLevel, "%s", err) | |||
There was a problem hiding this comment.
added this error wrapping for debugging ux
There was a problem hiding this comment.
Just a nit, but I think in other places we do something just like...
| return errorsmod.Wrapf(ErrInvalidTrustLevel, "%s", err) | |
| return errorsmod.Wrap(ErrInvalidTrustLevel, err.Error()) |
| @@ -50,7 +52,8 @@ func (l LightClientModule) Initialize(ctx sdk.Context, clientID string, clientSt | |||
|
|
|||
| var consensusState ConsensusState | |||
| if err := l.keeper.Codec().Unmarshal(consensusStateBz, &consensusState); err != nil { | |||
| return err | |||
| return fmt.Errorf("failed to unmarshal consensus state bytes into consensus state: %w", err) | |||
…to-light-client-module-entry-points
…ient-module-entry-points' of github.com:cosmos/ibc-go into charly/issue#6001-move-client-state-testing-to-light-client-module-entry-points
charleenfei
requested review from
colin-axner,
AdityaSripal,
damiannolan 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.
Actionable comments posted: 1
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (6)
- modules/light-clients/07-tendermint/client_state.go (2 hunks)
- modules/light-clients/07-tendermint/client_state_test.go (2 hunks)
- modules/light-clients/07-tendermint/errors.go (1 hunks)
- modules/light-clients/07-tendermint/light_client_module.go (3 hunks)
- modules/light-clients/07-tendermint/light_client_module_test.go (2 hunks)
- modules/light-clients/07-tendermint/tendermint_test.go (1 hunks)
Additional Context Used
Path-based Instructions (6)
modules/light-clients/07-tendermint/errors.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/light-clients/07-tendermint/tendermint_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/light-clients/07-tendermint/client_state_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/light-clients/07-tendermint/light_client_module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/light-clients/07-tendermint/client_state.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/light-clients/07-tendermint/light_client_module_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (11)
modules/light-clients/07-tendermint/errors.go (1)
22-22: The addition ofErrInvalidTrustLevelwith code 15 is clear and follows the pattern established in the file for defining errors. This should enhance error handling by providing a more specific error for invalid trust levels.modules/light-clients/07-tendermint/tendermint_test.go (1)
35-35: The addition ofinvalidUpgradePathwith the specific value[]string{"upgrade", ""}is a good enhancement to the test suite, allowing for testing scenarios with invalid upgrade paths.modules/light-clients/07-tendermint/client_state_test.go (1)
23-120: The refactoring of test cases inTestValidateto use theexpErr errorpattern instead ofexpPass boolis a positive change, enhancing the clarity and robustness of error handling in tests. Each test case is well-structured, covering a wide range of validation scenarios for theClientState.modules/light-clients/07-tendermint/light_client_module.go (2)
4-5: The import of thefmtpackage is correctly placed and is necessary for the subsequent use offmt.Errorffor error wrapping.
46-46: Enhancing error messages in theInitializefunction usingfmt.Errorfto wrap errors with additional context is a good practice. It improves the debugging experience by providing more descriptive error messages.Also applies to: 55-55
modules/light-clients/07-tendermint/client_state.go (1)
126-126: Wrapping the error with additional context in theValidatemethod when trust level validation fails usingerrorsmod.Wrapfis a good enhancement. It improves error messages, making them more informative and easier to debug.modules/light-clients/07-tendermint/light_client_module_test.go (5)
30-99: The test cases inTestStatusare well-structured and cover various scenarios. However, consider adding a brief comment above each test case to explain the scenario being tested for better readability and maintainability.
101-173: InTestGetTimestampAtHeight, the test case "failure: consensus state not found for height" modifies theheightvariable directly, which could lead to unintended side effects in other test cases due to shared state. Consider isolating test case state or resetting it after each test case.
175-262: InTestInitialize, the error handling checks usingErrorContainsare good for specific error messages. However, for errors likefmt.Errorf("failed to unmarshal client state bytes into client state"), consider defining these errors in the module's error file for consistency and easier maintenance.
519-784: InTestVerifyMembership, the use ofdelayTimePeriodanddelayBlockPeriodas global variables for the test suite could lead to unintended side effects if tests are run in parallel or out of order. Consider passing these as parameters to themalleatefunction or resetting their values before each test case.
786-1010: Similar toTestVerifyMembership,TestVerifyNonMembershipalso uses global variables fordelayTimePeriodanddelayBlockPeriod. This approach can lead to flaky tests if the execution order changes or if tests are run in parallel. It's advisable to encapsulate test case state within each test case or reset these variables before each test.
modules/light-clients/07-tendermint/light_client_module_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (1)
- modules/light-clients/07-tendermint/light_client_module_test.go (2 hunks)
Additional Context Used
Path-based Instructions (1)
modules/light-clients/07-tendermint/light_client_module_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (5)
modules/light-clients/07-tendermint/light_client_module_test.go (5)
30-99: The test functionTestStatusis well-structured and covers various scenarios for the client status. However, consider adding a brief comment above each test case within thetestCasesslice to explain the scenario being tested for better readability and maintainability.
101-173: The test functionTestGetTimestampAtHeighteffectively covers both success and failure scenarios. Ensure that the error messages and conditions being tested are up-to-date with the current implementation to maintain accuracy in test coverage.
175-262: InTestInitialize, the error handling and various initialization scenarios are tested. It's crucial to verify that all new error conditions introduced in the PR are covered by these tests. Additionally, consider adding comments to explain complex test setups for future maintainability.
519-784: TheTestVerifyMembershipfunction tests various scenarios for verifying membership. It's important to ensure that the test cases cover all new paths and error conditions introduced in the PR. Also, consider simplifying complex test setups or extracting repeated setup code into helper functions for clarity.
786-1010: TheTestVerifyNonMembershipfunction provides comprehensive coverage for non-membership verification scenarios. Similar toTestVerifyMembership, ensure all new error conditions and paths are covered. Additionally, review the use of hardcoded values and consider if they can be replaced with constants or variables for easier maintenance.
…to-light-client-module-entry-points
…to-light-client-module-entry-points
…to-light-client-module-entry-points
…to-light-client-module-entry-points
modules/light-clients/07-tendermint/light_client_module_test.go
Outdated
Show resolved
Hide resolved
hmm good call on the code cov. I took a look and it looks like the methods: |
…to-light-client-module-entry-points
…ient-module-entry-points' of github.com:cosmos/ibc-go into charly/issue#6001-move-client-state-testing-to-light-client-module-entry-points
…to-light-client-module-entry-points
…to-light-client-module-entry-points
…to-light-client-module-entry-points
There was a problem hiding this comment.
Looks good to me. Thanks for taking this task, @charleenfei.
| @@ -123,7 +123,7 @@ func (cs ClientState) Validate() error { | |||
| } | |||
|
|
|||
| if err := light.ValidateTrustLevel(cs.TrustLevel.ToTendermint()); err != nil { | |||
| return err | |||
| return errorsmod.Wrapf(ErrInvalidTrustLevel, "%s", err) | |||
There was a problem hiding this comment.
Just a nit, but I think in other places we do something just like...
| return errorsmod.Wrapf(ErrInvalidTrustLevel, "%s", err) | |
| return errorsmod.Wrap(ErrInvalidTrustLevel, err.Error()) |
| @@ -41,7 +43,7 @@ func (l *LightClientModule) RegisterStoreProvider(storeProvider exported.ClientS | |||
| func (l LightClientModule) Initialize(ctx sdk.Context, clientID string, clientStateBz, consensusStateBz []byte) error { | |||
| var clientState ClientState | |||
| if err := l.keeper.Codec().Unmarshal(clientStateBz, &clientState); err != nil { | |||
| return err | |||
| return fmt.Errorf("failed to unmarshal client state bytes into client state: %w", err) | |||
There was a problem hiding this comment.
I think I might change this in a follow-up PR, because at the moment we are not consistent with the error we return in these situations for other light clients (in some places we do return err but in other places we do errorsmod.Wrap(clienttypes.ErrInvalidClient, err.Error())...
There was a problem hiding this comment.
we mostly return this kind of nonspecific error for unmarshalling/proof verification right now. i guess consistency makes sense but i think for testing/debugging purposes this error also works!
…ient-module-entry-points' of github.com:cosmos/ibc-go into charly/issue#6001-move-client-state-testing-to-light-client-module-entry-points
…to-light-client-module-entry-points
|
There was a problem hiding this comment.
LGTM thank you @charleenfei !
Happy to have additional tests in a follow up.
…to-light-client-module-entry-points
charleenfei
deleted the
charly/issue#6001-move-client-state-testing-to-light-client-module-entry-points
branch
…ule entry points (cosmos#6074)
Description
partially closes: #6001
addresses the 07-tendermint client state testing functions, uses expErr instead of expPass.
Before 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
Bug Fixes
ClientStatevalidation and delay period verification.New Features
Refactor
Tests
Documentation