tests: move tests from client state to light client module for 08-wasm. #6052
Conversation
|
Important Auto Review SkippedAuto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe updates involve reordering imports, modifying test functions, and improving validations within 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 (
|
DimitrisJim
force-pushed
the
jim/issue6001-08wasm
branch
2 times, most recently
from
9d4d25d to
b3a608d
Compare
| { | ||
| "failure: clientStore prefix does not include clientID", | ||
| func() { | ||
| clientStore = suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), ibctesting.InvalidID) | ||
| }, | ||
| types.ErrVMError, | ||
| }, |
There was a problem hiding this comment.
removed during move, it should now be impossible for the clientStore prefix to not include client id (we look up the store using the client id in the light client method). Correct me if I'm missing something!
DimitrisJim
requested review from
colin-axner,
AdityaSripal,
damiannolan,
charleenfei,
chatton,
srdtrk and
crodriguezvega
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/light-clients/08-wasm/light_client_module_test.go (3 hunks)
- modules/light-clients/08-wasm/types/client_state_test.go (2 hunks)
Additional comments: 9
modules/light-clients/08-wasm/types/client_state_test.go (2)
- 10-11: Consider grouping imports to enhance readability. Standard library imports should be grouped together, followed by third-party packages, and then internal package imports.
- 64-66: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [35-84]
The test cases for
TestStatusare well-structured and cover a variety of scenarios including active, frozen, expired, unknown, and unauthorized client statuses. Ensure that the mock callbacks are correctly simulating the expected behavior for each case.modules/light-clients/08-wasm/light_client_module_test.go (7)
- 35-104: The
TestStatusfunction provides comprehensive test coverage for different client statuses. It's important to ensure that the mock callbacks accurately represent the behavior of the wasm module for each status.- 106-194: The
TestGetTimestampAtHeightfunction tests the timestamp retrieval at a specific height. Ensure that the mock callbacks return the expected timestamp and handle errors as intended. Also, verify that the test cases cover both successful and failure scenarios adequately.- 196-318: The
TestInitializefunction tests the initialization of the client. It's crucial to ensure that the test cases cover various scenarios including success, failure due to invalid consensus state, failure due to unregistered checksum, and VM errors. Pay special attention to the mock callbacks and their return values to ensure they simulate the expected behavior.- 320-467: The
TestVerifyMembershipfunction tests the membership verification logic. Ensure that the test setup, mock callbacks, and assertions correctly simulate and verify the membership proof verification process. It's also important to cover scenarios where the contract returns errors or invalid proofs.- 469-626: The
TestVerifyNonMembershipfunction tests the non-membership verification logic. Similar to the membership verification tests, ensure that the test setup, mock callbacks, and assertions accurately represent the non-membership proof verification process, including handling errors and invalid proofs.- 660-673: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [627-777]
The
TestRecoverClientfunction tests the client recovery process. Ensure that the test cases cover scenarios such as malformed substitute client ID, substitute client ID without the expected prefix, and missing client states. The test setup should accurately simulate the conditions for each scenario.
- 774-780: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [778-888]
The
TestVerifyUpgradeAndUpdateStatefunction tests the upgrade and update state verification logic. Ensure that the test cases comprehensively cover scenarios including success, missing client state, invalid upgraded client or consensus state, and height checks. The mock callbacks should accurately simulate the expected behavior for each scenario.
DimitrisJim
force-pushed
the
jim/issue6001-08wasm
branch
from
b3a608d to
33241c9
Compare
|
After some back and forth and a message in slack, decided to go ahead and move all client state tests to light_client_test. If we go about removing most of the methods on client state, this will be a needed step. Opened a branch sorry for the hassle @charleenfei and @chatton 😞 |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Files selected for processing (2)
- modules/light-clients/08-wasm/light_client_module_test.go (2 hunks)
- modules/light-clients/08-wasm/types/client_state_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/light-clients/08-wasm/light_client_module_test.go
Additional comments: 2
modules/light-clients/08-wasm/types/client_state_test.go (2)
- 303-360: The modifications to the
TestValidatefunction, including the addition of new test cases, enhance the test coverage and robustness. These changes are well-structured and improve the validation of client state data and checksums.- 4-12: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [262-292]
The enhancements to the
TestInitializefunction, including the addition of new test cases and improved error handling, contribute positively to the clarity and comprehensiveness of the testing framework. These changes are well-aligned with the objectives of the PR.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- modules/light-clients/08-wasm/types/client_state_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/light-clients/08-wasm/types/client_state_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: .coderabbit.yml
Files selected for processing (2)
- modules/light-clients/08-wasm/light_client_module_test.go (2 hunks)
- modules/light-clients/08-wasm/types/client_state_test.go (2 hunks)
Files not summarized due to errors (1)
- modules/light-clients/08-wasm/light_client_module_test.go: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (1)
- modules/light-clients/08-wasm/types/client_state_test.go
Additional comments: 8
modules/light-clients/08-wasm/light_client_module_test.go (8)
- 245-367: In
TestInitialize, the test case "success: validate contract address" performs a comprehensive check, including the validation of the contract address and the correct instantiation of client and consensus states. This is a good practice as it ensures the initialization logic is correctly implemented. However, consider adding more negative test cases to cover scenarios where initialization might fail due to invalid inputs or system states.Consider adding test cases for scenarios where initialization fails due to invalid client state, consensus state, or other system constraints.
- 370-515: The
TestVerifyMembershipfunction tests various scenarios for verifying membership using a wasm light client. It's comprehensive and covers a range of success and failure scenarios. However, there's a potential improvement in how test setup is done. Repeated setup code can be refactored into helper functions to improve readability and maintainability.Consider refactoring repeated setup code into helper functions to reduce duplication and improve test readability.
- 518-682: Similar to
TestVerifyMembership,TestVerifyNonMembershipcovers a wide range of scenarios. The comments made forTestVerifyMembershipregarding unique test case names and refactoring setup code apply here as well.Ensure test case names are unique and consider refactoring setup code for better readability.
- 685-791:
TestVerifyClientMessageeffectively tests the verification of client messages. It includes both positive and negative scenarios, which is good for ensuring robustness. However, the test case "failure: invalid client message" might benefit from a more detailed assertion to ensure the correct error type is returned.Add more detailed assertions to ensure the correct error type is returned in failure scenarios.
- 1100-1256:
TestUpdateStatetests the state update functionality. The test cases are comprehensive, covering both success and failure scenarios. However, the test case "failure: invalid ClientMessage type" uses a panic with a formatted error message. It's generally better to use structured errors for more predictable error handling.Consider using structured errors instead of formatted strings for error messages to improve error handling predictability.
- 1258-1403:
TestUpdateStateOnMisbehavioureffectively tests the update state on misbehaviour functionality. The test cases are comprehensive, but similar to previous comments, ensure that test setup code is not unnecessarily repeated and consider using helper functions.Refactor repeated setup code into helper functions for improved test code maintainability.
- 1405-1557:
TestRecoverClienttests the client recovery functionality. It's important to ensure that all edge cases are covered, including error scenarios from the wasm VM and contract. The test cases provided cover a good range of scenarios, but ensure that the test setup and assertions are as clear and concise as possible.Ensure test setup and assertions are clear and concise, covering all edge cases for client recovery functionality.
- 1558-1601:
TestLatestHeighttests the retrieval of the latest height for a wasm light client. The test cases are straightforward and cover the basic scenario and a failure case. Consider adding more test cases to cover additional edge cases or error scenarios that might occur in practice.Consider adding more test cases to cover additional edge cases or error scenarios for the latest height retrieval functionality.
DimitrisJim
force-pushed
the
jim/issue6001-08wasm
branch
from
ee6ec47 to
b665da7
Compare
There was a problem hiding this comment.
Fantastic work moving all these tests, @DimitrisJim. Let me know if you want a hand working on some of the TODOs you have.
| { | ||
| "failure: upgraded consensus state is not wasm consensus state", | ||
| func() { | ||
| // set upgraded consensus state to solomachine consensus state | ||
| upgradedConsState = &solomachine.ConsensusState{} | ||
| }, | ||
| clienttypes.ErrInvalidConsensus, | ||
| }, |
There was a problem hiding this comment.
Will you manage to fix this one? I tried something like upgradedConsState = &solomachine.ConsensusState{Diversifier: "diversifier", Timestamp: 10000000} but looks like the unmarshaling doesn't return an error, it just unmarshals to an empty 08-wasm consensus state...
There was a problem hiding this comment.
No, this is a tricky one yea? 08-wasm's consensus state is actually just a wrapper around a blob of bytes. Whatever random consensus state we pass to it will just dump its contents in there. I feel like there should be a way to cause this bugger to fail but havent found it yet.
More than comfy to tackle this one afterwards though!
| }, | ||
| /* TODO(jim): We don't pass a client state directly now so we need to modify how this is tested. | ||
| { | ||
| "failure: invalid substitute client state", |
There was a problem hiding this comment.
Maybe this could be tested if we pass a substituteClientID for a non-08-wasm client, and then the casting here would fail...
There was a problem hiding this comment.
ah, we check that substitute client identifier contains an 08-wasm prefix here:
We can follow up on this one after more refactoring is done.
|
|
merging, we can address any follow ups afterwards targeting the 08-wasm-refactor branch. |
Description
partially addresses #6001
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