test: fix integration & unit tests #14
Conversation
WalkthroughThe changes involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
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: 0
🧹 Outside diff range and nitpick comments (7)
execution_test.go (1)
Line range hint
80-117: Consider adding test cases for edge scenariosWhile the current changes improve the test, consider adding test cases for:
- Invalid state root formats
- Error scenarios when state root validation fails
- Edge cases with empty or malformed state roots
This would enhance the test coverage and ensure robust handling of state roots.
integration_test.go (3)
203-204: Consider improving test maintainabilityThe hardcoded state root value makes the test brittle and harder to maintain. Consider:
- Adding a comment explaining why this specific state root is expected
- Deriving the expected state root dynamically if possible
Example improvement:
+ // This state root is expected after executing a single transfer transaction + // with value 1 ETH from the test account to TEST_TO_ADDRESS newStateroot := common.HexToHash("0x362b7d8a31e7671b0f357756221ac385790c25a27ab222dc8cbdd08944f5aea4") assert.Equal(t, rollkit_types.Hash(newStateroot[:]), stateroot)
Line range hint
89-91: Improve container startup reliabilityThe current implementation uses a fixed sleep duration which could lead to flaky tests. Consider implementing a more robust wait mechanism.
Example improvement:
- // a reasonable time to wait for the container to start! - // do we want a more predictable elaborate code to wait for the container to be running? - time.Sleep(50 * time.Millisecond) + // Wait for the container to be ready by polling the HTTP endpoint + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + for { + select { + case <-ctx.Done(): + t.Fatal("timeout waiting for container to start") + default: + if _, err := ethclient.Dial(TEST_ETH_URL); err == nil { + return + } + time.Sleep(100 * time.Millisecond) + } + }
Line range hint
156-204: Consider adding error case testsThe test suite thoroughly covers the happy path but could benefit from additional test cases for error scenarios:
- Invalid transactions
- Network failures
- Timeout scenarios
Would you like me to help create additional test cases for these scenarios?
execution.go (3)
148-151: Consider adding nil check for ExecutionPayloadWhile the state root handling has been simplified, consider adding a defensive nil check for
payloadResult.ExecutionPayloadbefore accessing the StateRoot to prevent potential panic.+ if payloadResult.ExecutionPayload == nil { + return execution_types.Hash{}, 0, fmt.Errorf("nil execution payload") + } stateRoot := payloadResult.ExecutionPayload.StateRoot rollkitStateRoot := execution_types.Hash(stateRoot[:])
148-151: Inconsistent state root handling between InitChain and ExecuteTxsThere's an inconsistency in how the state root is handled:
InitChainusesexecution_types.Hash(stateRoot[:])ExecuteTxsusespayloadResult.ExecutionPayload.StateRoot.Bytes()Both approaches work but using different methods could lead to confusion. Consider standardizing the approach across both methods.
For ExecuteTxs, consider using:
- return payloadResult.ExecutionPayload.StateRoot.Bytes(), payloadResult.ExecutionPayload.GasUsed, nil + stateRoot := payloadResult.ExecutionPayload.StateRoot + return execution_types.Hash(stateRoot[:]), payloadResult.ExecutionPayload.GasUsed, nilAlso applies to: 266-266
148-151: Consider adding documentation for state root handlingGiven that this code is part of a test fix PR and involves changes to state root handling, consider adding documentation to explain:
- Why the state root is handled differently in InitChain vs ExecuteTxs
- The relationship between common.Hash and execution_types.Hash
- Any assumptions about the Engine API payload structure
This will help future maintainers understand the design decisions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
execution.go(1 hunks)execution_test.go(2 hunks)integration_test.go(1 hunks)
🔇 Additional comments (3)
execution_test.go (2)
80-87: LGTM: Improved test initialization with explicit state root
The change improves test clarity by using a known state root value instead of an empty hash, making the test setup more explicit and realistic.
117-117: LGTM: Simplified state root assertion
The assertion has been simplified to directly compare execution_types.Hash values without unnecessary intermediate conversions, making the test more maintainable.
integration_test.go (1)
204-204: LGTM! Type consistency improvement
The change correctly ensures type consistency by comparing state roots using rollkit_types.Hash type, which aligns with the changes in execution.go.
|
Tests are working correctly, except the waiting for docker containers 👍 |
|
Merging as is, will followup. |
Overview
Summary by CodeRabbit
New Features
StateRootin transaction execution and chain initialization processes.Bug Fixes
Tests
prevStateRootand simplify assertions for state root comparisons.