challenger unit test #62
Conversation
WalkthroughThis pull request introduces comprehensive testing and refactoring across multiple packages in the child and provider modules. The changes focus on enhancing test coverage for deposit, withdrawal, and oracle-related functionalities. New mock implementations for hosts and challengers are added, and several test utilities are created to simulate different scenarios. The modifications simplify event parsing, update function signatures, and improve the overall testing infrastructure for the child and provider components. Changes
Sequence DiagramsequenceDiagram
participant Child
participant Host
participant Challenger
participant Provider
Child->>Provider: Parse Event Attributes
Provider-->>Child: Return Parsed Event Data
Child->>Host: Set Synced Outputs
Child->>Challenger: Send Pending Challenges
Challenger-->>Host: Process Challenges
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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: 4
🧹 Nitpick comments (10)
executor/child/oracle_test.go (1)
Line range hint
35-52: Consider adding error test cases.The test suite currently only covers the success case. Consider adding test cases for:
- Invalid l1BlockHeight format
- Missing required attributes
- Empty sender address
challenger/child/oracle_test.go (1)
35-59: Expand test coverage with additional scenarios.The test cases could be enhanced by adding:
- Oracle disabled scenario
- Invalid external commit bytes
- Zero block height case
- Future block time validation
Example test case:
{ name: "oracle disabled", oracleEnabled: false, blockHeight: 3, blockTime: time.Unix(0, 100).UTC(), extCommitBz: []byte("oracle_tx"), expected: []challengertypes.ChallengeEvent{}, // Empty queue when oracle disabled },challenger/child/common_test.go (1)
47-61: Consider using custom error types for better error handling.Instead of using a generic "collections: not found" error message, consider defining custom error types for better error handling and testing.
+var ( + ErrBridgeNotFound = errors.New("bridge not found") + ErrOutputNotFound = errors.New("output not found") +) func (m *mockHost) QuerySyncedOutput(ctx context.Context, bridgeId uint64, outputIndex uint64) (*ophosttypes.QueryOutputProposalResponse, error) { if _, ok := m.syncedOutputs[bridgeId]; !ok { - return nil, errors.New("collections: not found") + return nil, ErrBridgeNotFound } if _, ok := m.syncedOutputs[bridgeId][outputIndex]; !ok { - return nil, errors.New("collections: not found") + return nil, ErrOutputNotFound }executor/child/deposit_test.go (1)
43-43: Consider adding more test cases.The test looks good but only covers the success case. Consider adding test cases for:
- Invalid event attributes
- Missing required attributes
- Edge cases with different amounts
provider/child/parse.go (1)
109-109: Consider adding validation for denom format.While the denom parsing is implemented correctly, consider adding validation to ensure the denom follows the correct format (e.g., starts with 'u' for micro units).
case opchildtypes.AttributeKeyDenom: - denom = attr.Value + if !strings.HasPrefix(attr.Value, "u") { + err = fmt.Errorf("invalid denom format %s, must start with 'u'", attr.Value) + return + } + denom = attr.ValueAlso applies to: 131-132
provider/child/parse_test.go (3)
12-89: Add test case for invalid amount format in TestParseDepositEvents.Consider adding a test case to verify error handling when the amount attribute contains an invalid format.
{ + name: "invalid amount format", + eventAttrs: FinalizeDepositEvents(1, "sender", "recipient", "uinit", "baseDenom", sdk.NewInt64Coin("uinit", 10000), 2), + err: true, + },
91-131: Add test case for invalid height format in TestParseUpdateOracle.Consider adding a test case to verify error handling when the height attribute contains an invalid format.
{ + name: "invalid height format", + eventAttrs: []abcitypes.EventAttribute{ + {Key: opchildtypes.AttributeKeyHeight, Value: "invalid"}, + {Key: opchildtypes.AttributeKeyFrom, Value: "sender"}, + }, + err: true, + },
133-204: Add test case for invalid amount format in TestParseInitiateWithdrawal.Consider adding a test case to verify error handling when the amount attribute contains an invalid format.
{ + name: "invalid amount format", + eventAttrs: []abcitypes.EventAttribute{ + {Key: opchildtypes.AttributeKeyFrom, Value: "from"}, + {Key: opchildtypes.AttributeKeyTo, Value: "to"}, + {Key: opchildtypes.AttributeKeyDenom, Value: "denom"}, + {Key: opchildtypes.AttributeKeyBaseDenom, Value: "uinit"}, + {Key: opchildtypes.AttributeKeyAmount, Value: "invalid"}, + {Key: opchildtypes.AttributeKeyL2Sequence, Value: "1"}, + }, + err: true, + },challenger/child/withdraw_test.go (1)
1-738: Comprehensive test coverage with room for improvement.The test implementation thoroughly covers withdrawal scenarios, tree handling, and output preparation. Consider adding the following improvements:
- Add assertions for event emission in
TestInitiateWithdrawalHandler- Add boundary test cases for tree height in
TestPrepareTree- Add test cases for concurrent withdrawals in
TestHandleOutputexecutor/child/withdraw_test.go (1)
Line range hint
1-738: Add test cases for error scenarios.Consider adding the following test cases to improve coverage:
- Error handling in
InitiateWithdrawalEventsfor invalid inputs- Invalid denomination scenarios
- Edge cases for withdrawal amounts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
challenger/child/common_test.go(1 hunks)challenger/child/deposit.go(2 hunks)challenger/child/deposit_test.go(1 hunks)challenger/child/handler.go(1 hunks)challenger/child/handler_test.go(1 hunks)challenger/child/oracle_test.go(1 hunks)challenger/child/withdraw.go(1 hunks)challenger/child/withdraw_test.go(1 hunks)challenger/host/common_test.go(1 hunks)executor/child/deposit_test.go(1 hunks)executor/child/oracle_test.go(1 hunks)executor/child/withdraw.go(1 hunks)executor/child/withdraw_test.go(3 hunks)provider/child/parse.go(4 hunks)provider/child/parse_test.go(1 hunks)provider/child/testutil.go(2 hunks)
🔇 Additional comments (14)
challenger/child/deposit.go (1)
Line range hint
18-29: LGTM! Event queue handling and logging look good.The event queue handling is properly implemented with clear logging of all relevant fields.
challenger/host/common_test.go (1)
67-68: LGTM! Good practice with compile-time interface check.The addition of
var _ challenger = (*mockChallenger)(nil)ensures thatmockChallengerproperly implements thechallengerinterface at compile time.executor/child/oracle_test.go (1)
41-41: LGTM! Good use of provider package.Using
childprovider.UpdateOracleEventscentralizes the event attribute creation logic in the provider package.challenger/child/oracle_test.go (2)
33-33: LGTM! Good use of cryptographic hash.Using SHA3-256 for the oracle transaction data checksum is a good choice for integrity verification.
61-68: Consider adding error assertions.The test should verify that the handler properly handles error cases and doesn't panic.
challenger/child/common_test.go (2)
12-17: LGTM! Clean mock implementation with proper initialization.The mockHost struct and its constructor are well-designed with proper field initialization.
Also applies to: 19-26
65-85: LGTM! Clean mock implementation.The mockChallenger struct and its methods are well-implemented with proper initialization.
provider/child/testutil.go (3)
47-86: LGTM! Well-structured event creation utility.The FinalizeDepositEvents function is well-implemented with proper type conversions and consistent attribute key usage.
88-102: LGTM! Clean implementation of oracle event creation.The UpdateOracleEvents function follows the established pattern and properly handles type conversions.
104-138: LGTM! Well-implemented withdrawal event creation.The InitiateWithdrawalEvents function maintains consistency with other event creation utilities and properly handles all required attributes.
challenger/child/handler.go (1)
95-95: LGTM!The variable rename from
challengestopendingChallengesmaintains consistency with the variable used throughout the function.provider/child/parse.go (1)
27-27: LGTM!The rename from
l1BlockHeighttofinalizeHeightimproves clarity and better represents the variable's purpose.challenger/child/withdraw.go (1)
24-24: LGTM!The change correctly handles the new denom parameter by ignoring it with
_as it's not needed in this context. Error handling remains robust.challenger/child/handler_test.go (1)
1-424: Well-structured test implementation!The test cases are comprehensive and cover important scenarios including:
- Begin block handling with event queue and stage management
- End block handling with various conditions
- Deposit timeout scenarios
- Error cases and panic conditions
Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
Release Notes
Testing Improvements
Code Refactoring
Minor Enhancements
No user-facing features or breaking changes are introduced in this release.