tests: Adding unit tests #7
Conversation
WalkthroughThe changes in this pull request involve significant updates to the dependency management in the Changes
Sequence Diagram(s)sequenceDiagram
participant A as Client
participant B as Middleware
participant C as MsgServer
A->>B: Send DepositForBurn
B->>C: Call DepositForBurn
C-->>B: Acknowledge
B-->>A: Response
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
middleware_test.go (2)
278-279: Remove unnecessary call toctl.Finish()The
defer ctl.Finish()call is redundant becausegomock.NewControllerautomatically handles this. Removing it can simplify the code.Apply this diff to remove the unnecessary call:
ctl := gomock.NewController(t) -defer ctl.Finish()
348-350: Handle potential errors when unmarshalling acknowledgmentsIn both
TestMiddleware_DepositForBurn_SuccessandTestMiddleware_DepositForBurnWithCaller_Success, error handling after unmarshalling the acknowledgment is minimal. Consider adding assertions to handle potential errors more gracefully.Apply this diff to enhance error handling:
err := channeltypes.SubModuleCdc.UnmarshalJSON(res.Acknowledgement(), &ack) -require.NoError(t, err) +if err != nil { + t.Fatalf("Failed to unmarshal acknowledgment: %v", err) +}Also applies to: 403-405
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
- go.mod (1 hunks)
- middleware.go (4 hunks)
- middleware_test.go (1 hunks)
- test/mock/bank_keeper.go (1 hunks)
- test/mock/cctp_msgserver.go (1 hunks)
- test/mock/ibc_module.go (1 hunks)
- types/keepers.go (1 hunks)
✅ Files skipped from review due to trivial changes (3)
- test/mock/bank_keeper.go
- test/mock/cctp_msgserver.go
- test/mock/ibc_module.go
🔇 Additional comments (10)
types/keepers.go (2)
3-7: LGTM! Clean import management.The imports are correctly organized with the standard library import ("context") separated from third-party imports (cosmos-sdk).
10-10: Verify the impact of context type change across implementations.The change from
sdk.Contexttocontext.Contextis a good move towards standard Go practices and improved testability. However, this is a breaking change that requires updates to all implementations of theBankKeeperinterface.Let's verify that all implementations have been updated:
✅ Verification successful
Let me try a different approach to verify the impact of this context type change.
This should help us find:
- All implementations of SendCoins to verify context type usage
- Mock implementations that need to be updated
- Test files that might be affected by this change
Let me check the middleware_test.go file to see how SendCoins is being used there and verify the context type usage.
Based on the gathered evidence, I can now provide a final response:
The context type change is consistently implemented and properly tested
The verification shows that:
- The mock implementation in
test/mock/bank_keeper.gocorrectly usescontext.Context- The middleware implementation in
middleware.gouses the BankKeeper interface correctly- Test files use
gomock.Any()for context matching, which is compatible with both context types- No instances of
sdk.Contextusage withSendCoinswere foundThe change from
sdk.Contexttocontext.Contexthas been properly propagated across the codebase, and all implementations are consistent with the interface change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for BankKeeper implementations and verify context type usage # Expected: All implementations should use context.Context # Look for BankKeeper implementations ast-grep --pattern 'type $_ struct { $$$ } func ($_ $_) SendCoins(ctx $_, $$$) error { $$$ }' # Search for any remaining uses of sdk.Context with SendCoins rg -l 'SendCoins.*sdk\.Context'Length of output: 134
Script:
#!/bin/bash # Search for all files containing SendCoins method implementations rg -A 5 "func.*SendCoins" # Search for mock implementations fd "mock.*\.go" --exec grep -l "SendCoins" # Look for test files that might use SendCoins fd "_test\.go" --exec grep -l "SendCoins"Length of output: 895
Script:
#!/bin/bash # Check the middleware_test.go content around SendCoins usage rg -B 2 -A 2 "SendCoins" middleware_test.go # Also check the middleware.go implementation to see if it uses BankKeeper's SendCoins rg -B 2 -A 2 "SendCoins" middleware.go # Look at the imports in middleware_test.go to understand context usage rg -A 10 "^import" middleware_test.goLength of output: 2020
middleware.go (3)
28-32: Well-structured dependency injection change!The constructor modification to accept
cctptypes.MsgServerinstead of the concrete keeper type is a good design choice that:
- Improves testability through easier mocking
- Reduces coupling with the keeper implementation
- Follows interface-based design principles
114-114: Good optimization: Removed redundant conversion!The direct usage of
feeAmounteliminates unnecessary type conversion since it's already amath.Int.
139-139: Verify context compatibility with server implementation.The direct usage of
sdk.Contextin server calls (DepositForBurnandDepositForBurnWithCaller) needs verification. Whilesdk.Contextimplementscontext.Context, ensure that:
- The server implementation doesn't require specific SDK context methods
- No unsafe type assertions to
sdk.Contextexist in the server implementationAlso applies to: 194-194
go.mod (3)
18-19: LGTM! Testing dependencies align with PR objectives.The addition of
testifyandmockpackages is appropriate for implementing unit tests and mocks as described in the PR objectives.
Line range hint
3-5: Verify Go toolchain compatibility.The module specifies Go 1.22.1 with toolchain 1.22.7. Ensure all team members and CI/CD pipelines are using compatible Go versions.
Let's check for Go version constraints in the codebase:
#!/bin/bash # Search for Go version constraints in go files rg "^//\s*\+build\s+.*go" # Search for potential Go version specific features ast-grep --pattern 'go:build'
8-13: Verify dependency version compatibility.Several core Cosmos SDK dependencies have been added. However, the AI summary indicates some of these are both added and removed, which might suggest version changes.
Let's verify the dependency compatibility:
✅ Verification successful
Let me try a different approach to verify the dependency compatibility.
After analyzing the complete go.mod file, I can now provide a final response.
Dependencies are compatible and properly aligned
The core Cosmos SDK dependencies are well-aligned and compatible:
cosmossdk.io/log v1.4.1cosmossdk.io/math v1.3.0cosmossdk.io/store v1.1.1github.com/cosmos/cosmos-sdk v0.50.10github.com/cometbft/cometbft v0.38.12These versions are consistent with each other and follow the standard versioning pattern for the Cosmos SDK ecosystem. The file also includes necessary replacements for known compatibility issues:
github.com/gogo/protobufis replaced with the Cosmos-flavored versiongithub.com/syndtr/goleveldbis replaced with a Cosmos-compatible version🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential version conflicts in dependencies rg -A 1 "require \(" go.mod rg "replace.*github\.com.*=>" go.modLength of output: 57
Script:
#!/bin/bash # Get the full content of go.mod to analyze dependencies and their versions cat go.modLength of output: 16014
middleware_test.go (2)
285-286: Ensure acknowledgments are properly handled in mock expectationsThe mocked
OnRecvPacketmethod returns an empty acknowledgment. Consider verifying that this is the intended behavior and adjust the return value if necessary to reflect realistic acknowledgment data.Verify the expected acknowledgment data for
OnRecvPacketand adjust the mock accordingly.
312-313: Ensure mock expectations match the number of callsIn the mocks for
SendCoins, the expectationTimes(2)may not accurately reflect the number of times the method is called. Verify thatSendCoinsis expected to be called exactly twice in these tests. If the number of calls can vary, consider usingAnyTimes()instead.Check the invocation count for
SendCoinsand adjust the mock expectations if necessary.Also applies to: 364-365
| name: "Fail: DepositForBurn - forward amount is greater than transferred amount", | ||
| getPacket: func() channeltypes.Packet { | ||
| overAmount := "200" | ||
| autocctpDepositForBurnMemo.DepositForBurn.Amount = &overAmount |
There was a problem hiding this comment.
Avoid modifying shared global variables within test cases
Modifying shared global variables like autocctpDepositForBurnMemo and autocctpDepositForBurnWithCallerMemo directly in test cases can lead to unexpected side effects and flaky tests due to state leakage between tests. To ensure that each test is independent and does not affect others, create local copies of these variables within each test case.
Apply this diff to create local copies and prevent shared state mutations:
// In each test case's getPacket function:
getPacket: func() channeltypes.Packet {
+ // Create a local copy of the memo
+ memoCopy := autocctpDepositForBurnMemo
overAmount := "200"
- autocctpDepositForBurnMemo.DepositForBurn.Amount = &overAmount
+ memoCopy.DepositForBurn.Amount = &overAmount
memobz, _ := json.Marshal(memoCopy)
// ... rest of the code
}Also applies to: 135-135, 153-153, 171-171, 190-190, 214-214, 238-238, 257-257
middleware_test.go
Outdated
| return channeltypes.Packet{ | ||
| SourcePort: "transfer", | ||
| SourceChannel: "channel-0", | ||
| DestinationPort: "transfer", | ||
| DestinationChannel: "channel-1", | ||
| Data: transferData, | ||
| } | ||
| }, | ||
| expectSuccess: false, | ||
| }, | ||
| { | ||
| name: "Fail: DepositForBurnWithCaller - fee recipient is invalid bech32", | ||
| getPacket: func() channeltypes.Packet { | ||
| feeRecipient := "invalidbech32" | ||
| autocctpDepositForBurnWithCallerMemo.DepositForBurnWithCaller.FeeRecipient = &feeRecipient | ||
| memobz, _ := json.Marshal(autocctpDepositForBurnWithCallerMemo) | ||
| transferPacket := transfertypes.FungibleTokenPacketData{ | ||
| Denom: "transfer/channel-0/uusdc", | ||
| Amount: "100", | ||
| Sender: "cosmos1wnlew8ss0sqclfalvj6jkcyvnwq79fd74qxxue", | ||
| Receiver: authtypes.NewModuleAddress("autocctp").String(), | ||
| Memo: string(memobz), | ||
| } | ||
| transferData, _ := transfertypes.ModuleCdc.MarshalJSON(&transferPacket) | ||
| return channeltypes.Packet{ | ||
| SourcePort: "transfer", | ||
| SourceChannel: "channel-0", | ||
| DestinationPort: "transfer", | ||
| DestinationChannel: "channel-1", | ||
| Data: transferData, | ||
| } | ||
| }, | ||
| expectSuccess: false, | ||
| }, | ||
| { | ||
| name: "Fail: DepositForBurn - forward amount is invalid", | ||
| getPacket: func() channeltypes.Packet { | ||
| cctpAmount := "👻" | ||
| autocctpDepositForBurnMemo.DepositForBurn.Amount = &cctpAmount | ||
| memobz, _ := json.Marshal(autocctpDepositForBurnMemo) | ||
| transferPacket := transfertypes.FungibleTokenPacketData{ | ||
| Denom: "transfer/channel-0/uusdc", | ||
| Amount: "100", | ||
| Sender: "cosmos1wnlew8ss0sqclfalvj6jkcyvnwq79fd74qxxue", | ||
| Receiver: authtypes.NewModuleAddress("autocctp").String(), | ||
| Memo: string(memobz), | ||
| } | ||
| transferData, _ := transfertypes.ModuleCdc.MarshalJSON(&transferPacket) | ||
| packet.Data = transferData | ||
| return packet | ||
| }, | ||
| expectSuccess: false, | ||
| }, | ||
| { | ||
| name: "Fail: DepositForBurnWithCaller - forward amount is invalid", | ||
| getPacket: func() channeltypes.Packet { | ||
| cctpAmount := "👻" | ||
| autocctpDepositForBurnWithCallerMemo.DepositForBurnWithCaller.Amount = &cctpAmount | ||
| memobz, _ := json.Marshal(autocctpDepositForBurnWithCallerMemo) | ||
| transferPacket := transfertypes.FungibleTokenPacketData{ | ||
| Denom: "transfer/channel-0/uusdc", | ||
| Amount: "100", | ||
| Sender: "cosmos1wnlew8ss0sqclfalvj6jkcyvnwq79fd74qxxue", | ||
| Receiver: authtypes.NewModuleAddress("autocctp").String(), | ||
| Memo: string(memobz), | ||
| } | ||
| transferData, _ := transfertypes.ModuleCdc.MarshalJSON(&transferPacket) | ||
| packet.Data = transferData | ||
| return packet | ||
| }, | ||
| expectSuccess: false, | ||
| }, | ||
| } | ||
| for _, tc := range testcases { | ||
| t.Run(fmt.Sprintf("Case: %s", tc.name), func(t *testing.T) { | ||
| logger := log.NewTestLogger(t) | ||
| ctx := sdk.NewContext(store.NewCommitMultiStore(dbm.NewMemDB(), logger, metrics.NewNoOpMetrics()), tmproto.Header{}, false, logger) | ||
| // Setting up mocks | ||
| ctl := gomock.NewController(t) | ||
| defer ctl.Finish() | ||
| packet := tc.getPacket() | ||
| bankKeeper := mock.NewMockBankKeeper(ctl) | ||
| ibcModule := mock.NewMockIBCModule(ctl) | ||
| cctpServer := mock.NewMockMsgServer(ctl) | ||
| ibcModule.EXPECT().OnRecvPacket(gomock.Any(), gomock.Any(), gomock.Any()). | ||
| Return(channeltypes.NewResultAcknowledgement([]byte(""))) | ||
| bankKeeper.EXPECT().SendCoins(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). | ||
| Return(nil).AnyTimes() | ||
| middleware := autocctp.NewMiddleware(ibcModule, bankKeeper, cctpServer) | ||
|
|
||
| ack := middleware.OnRecvPacket(ctx, packet, nil) | ||
|
|
||
| if tc.expectSuccess { | ||
| require.True(t, ack.Success()) | ||
| } else { | ||
| require.False(t, ack.Success()) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repeated code in test setup
The test setup code within TestMiddleware is repeated for each test case. Consider refactoring this into a helper function to improve maintainability and readability.
Apply this diff to refactor the test setup:
func TestMiddleware(t *testing.T) {
// ... existing code
for _, tc := range testcases {
t.Run(fmt.Sprintf("Case: %s", tc.name), func(t *testing.T) {
- logger := log.NewTestLogger(t)
- ctx := sdk.NewContext(store.NewCommitMultiStore(dbm.NewMemDB(), logger, metrics.NewNoOpMetrics()), tmproto.Header{}, false, logger)
- // Setting up mocks
- ctl := gomock.NewController(t)
- defer ctl.Finish()
- packet := tc.getPacket()
- bankKeeper := mock.NewMockBankKeeper(ctl)
- ibcModule := mock.NewMockIBCModule(ctl)
- cctpServer := mock.NewMockMsgServer(ctl)
- ibcModule.EXPECT().OnRecvPacket(gomock.Any(), gomock.Any(), gomock.Any()).
- Return(channeltypes.NewResultAcknowledgement([]byte("")))
- bankKeeper.EXPECT().SendCoins(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
- Return(nil).AnyTimes()
- middleware := autocctp.NewMiddleware(ibcModule, bankKeeper, cctpServer)
+ ctx, middleware := setupTestEnvironment(t, tc)
ack := middleware.OnRecvPacket(ctx, packet, nil)
// ... existing assertionsCreate the helper function setupTestEnvironment:
func setupTestEnvironment(t *testing.T, tc testCaseType) (sdk.Context, autocctp.Middleware) {
logger := log.NewTestLogger(t)
ctx := sdk.NewContext(store.NewCommitMultiStore(dbm.NewMemDB(), logger, metrics.NewNoOpMetrics()), tmproto.Header{}, false, logger)
ctl := gomock.NewController(t)
packet := tc.getPacket()
bankKeeper := mock.NewMockBankKeeper(ctl)
ibcModule := mock.NewMockIBCModule(ctl)
cctpServer := mock.NewMockMsgServer(ctl)
ibcModule.EXPECT().OnRecvPacket(gomock.Any(), gomock.Any(), gomock.Any()).
Return(channeltypes.NewResultAcknowledgement([]byte("")))
bankKeeper.EXPECT().SendCoins(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
Return(nil).AnyTimes()
middleware := autocctp.NewMiddleware(ibcModule, bankKeeper, cctpServer)
return ctx, middleware
}
DepositForBurnas well asDepositForBurnWithCallerSummary by CodeRabbit
New Features
BankKeeper,MsgServer, andIBCModuleinterfaces to facilitate testing.Bug Fixes
SendCoinsmethod to enhance clarity and performance.Refactor