test: fix failing tests after protocompat changes#22889
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces modifications across multiple files in the Cosmos SDK, focusing on enhancing error handling, testing utilities, and mock implementations. The changes primarily affect the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
julienrbrt
requested review from
akhilkumarpilli,
auricom,
kocubinski and
tac0turtle
as code owners
julienrbrt
requested review from
aaronc,
facundomedica,
raynaudoe and
testinginprod
as code owners
| // ref: https://github.com/cosmos/cosmos-sdk/issues/22779 | ||
| func setPointer(dst, src any) { | ||
| reflect.ValueOf(dst).Elem().Set(reflect.ValueOf(src).Elem()) | ||
| func setPointer(dst, src any) error { |
There was a problem hiding this comment.
took this from the original PR coderabbit suggestion, as it best if we don't panic.
ghost
assigned
aaronc,
akhilkumarpilli,
auricom,
facundomedica,
kocubinski,
raynaudoe,
tac0turtle and
testinginprod
![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
🧹 Nitpick comments (1)
x/authz/testutil/expected_keepers_mocks.go (1)
16-17: Use descriptive aliases for imported packagesThe aliases
typesandtypes0may cause confusion. Consider using more specific aliases likebanktypesandsdktypesfor clarity.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
baseapp/internal/protocompat/protocompat.go(3 hunks)scripts/mockgen.sh(1 hunks)x/authz/keeper/keeper_test.go(1 hunks)x/authz/testutil/bank_helpers.go(0 hunks)x/authz/testutil/expected_keepers.go(1 hunks)x/authz/testutil/expected_keepers_mocks.go(4 hunks)x/group/keeper/msg_server_test.go(14 hunks)x/group/testutil/expected_keepers.go(1 hunks)
💤 Files with no reviewable changes (1)
- x/authz/testutil/bank_helpers.go
🧰 Additional context used
📓 Path-based instructions (6)
x/group/testutil/expected_keepers.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
baseapp/internal/protocompat/protocompat.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/authz/testutil/expected_keepers.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/authz/keeper/keeper_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"
x/authz/testutil/expected_keepers_mocks.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/group/keeper/msg_server_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 (7)
x/authz/testutil/expected_keepers.go (1)
1-19: Code conforms to Go conventions
The BankKeeper interface is well-defined and follows Go best practices.
x/group/testutil/expected_keepers.go (1)
35-35: Placement of SpendableCoins method is appropriate
The SpendableCoins method is correctly positioned within the BankKeeper interface.
scripts/mockgen.sh (1)
18-18: LGTM: Mock generation path updated correctly
The path update for authz mock generation aligns with the new location of expected_keepers.go in the testutil directory.
baseapp/internal/protocompat/protocompat.go (3)
236-251: Improved error handling in setPointer function
The function now properly validates pointers and returns errors instead of potentially panicking. The validation includes:
- Checking for valid values
- Ensuring non-nil pointers
- Verifying type compatibility
163-164: Consistent error handling pattern applied
The error handling pattern is consistently applied across both code paths in the hybrid handler.
Also applies to: 168-169
126-126: Verify error handling in makeGogoHybridHandler
The handler now properly propagates errors from setPointer, but we should verify that all callers handle these errors appropriately.
Also applies to: 131-132
✅ Verification successful
Error handling for setPointer is properly implemented
The verification shows that all calls to setPointer are properly error-checked:
- In the first handler path, the error from
setPointeris returned via the closure and checked withif err != nil - In the second handler path (gogo handler), errors are similarly propagated and checked
- The final
setPointercalls in both paths directly return the error to the caller
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all callers of setPointer to verify error handling
ast-grep --pattern 'err := setPointer($_, $_)
if err != nil {
$$$
}'
Length of output: 78
Script:
#!/bin/bash
# Let's first check all usages of setPointer function
rg "setPointer\(" -A 3
# Also check the implementation of setPointer to understand its error handling
ast-grep --pattern 'func setPointer($_, $_) error {
$$$
}'
Length of output: 1475
Script:
#!/bin/bash
# Let's check the surrounding context of setPointer usages to verify error handling
rg "setPointer\(" -B 3 -A 5 baseapp/internal/protocompat/protocompat.go
# Also check the full implementation of makeGogoHybridHandler to understand the error flow
ast-grep --pattern 'func makeGogoHybridHandler($$$) {
$$$
}'
Length of output: 1369
x/authz/keeper/keeper_test.go (1)
75-75: LGTM: Improved mock expectations
The mock now returns a proper MsgSendResponse instead of nil, which better reflects the actual behavior of the bank keeper's Send method.
|
|
||
| msgs := []sdk.Msg{msgSend1, msgSend2} | ||
| s.bankKeeper.EXPECT().Send(gomock.Any(), msgSend1).Return(nil, errors.New("error")) | ||
| s.bankKeeper.EXPECT().Send(gomock.Any(), msgSend1).Return(&banktypes.MsgSendResponse{}, errors.New("error")) |
There was a problem hiding this comment.
Correct the return values when mocking Send with an error
When mocking s.bankKeeper.EXPECT().Send to return an error, the response should be nil. Currently, it returns a non-nil MsgSendResponse alongside an error, which is inconsistent with Go conventions.
Apply this diff to fix the test:
-s.bankKeeper.EXPECT().Send(gomock.Any(), msgSend1).Return(&banktypes.MsgSendResponse{}, errors.New("error"))
+s.bankKeeper.EXPECT().Send(gomock.Any(), msgSend1).Return(nil, errors.New("error"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| s.bankKeeper.EXPECT().Send(gomock.Any(), msgSend1).Return(&banktypes.MsgSendResponse{}, errors.New("error")) | |
| s.bankKeeper.EXPECT().Send(gomock.Any(), msgSend1).Return(nil, errors.New("error")) |
There was a problem hiding this comment.
@julienrbrt, understood. Let me know if there's anything else I can assist with.
(_/)
( •_•)
(/ )
Co-authored-by: Marko <marko@baricevic.me>
| // ref: https://github.com/cosmos/cosmos-sdk/issues/22779 | ||
| func setPointer(dst, src any) { | ||
| reflect.ValueOf(dst).Elem().Set(reflect.ValueOf(src).Elem()) | ||
| func setPointer(dst, src any) error { |
There was a problem hiding this comment.
It might be worthwhile to test this function to ensure no regressions. Is there a function where it is already indirectly exercised?
There was a problem hiding this comment.
yes, the failing tests :D
Co-authored-by: Marko <marko@baricevic.me> (cherry picked from commit cfe1940) # Conflicts: # x/authz/testutil/expected_keepers_mocks.go
11 participants
Description
Follow-up of #22866
Fixes:
https://github.com/cosmos/cosmos-sdk/actions/runs/12337602773/job/34431523563?pr=22864 https://github.com/cosmos/cosmos-sdk/actions/runs/12344400459/job/34447459757?pr=22873
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
BankKeeperinterface for enhanced testing capabilities.Bug Fixes
Documentation
Chores