test: improve tx injector and common test suite #61
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes simplify and clarify various test files. In the gRPC client–server test, an intermediate state variable is removed in favor of using the expected state root directly. The dummy executor now injects transactions using a new random generation method, replacing the static injection approach. The test suite has been refactored to adopt the new transaction injection method and includes an added helper for chain initialization. These modifications standardize the transaction creation process and improve test clarity. Changes
Possibly related PRs
Suggested labels
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: 1
🧹 Nitpick comments (1)
proxy/grpc/util.go (1)
5-9: Add input validation and documentation to improve robustness.The function could benefit from input validation and documentation to improve its robustness and maintainability.
Apply this diff to add input validation and documentation:
+// copyHash creates a new Hash by copying the contents of the input byte slice. +// It panics if the input slice is not of the expected length for a Hash. func copyHash(src []byte) types.Hash { + if len(src) != 32 { + panic("invalid hash length") + } - dst := make([]byte, len(src)) + dst := make([]byte, 32) copy(dst, src) return dst }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
proxy/grpc/client.go(2 hunks)proxy/grpc/client_server_test.go(2 hunks)proxy/grpc/server.go(1 hunks)proxy/grpc/util.go(1 hunks)test/dummy.go(2 hunks)test/dummy_test.go(1 hunks)test/suite.go(5 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
test/suite.go
105-105: ineffectual assignment to maxBytes
(ineffassign)
🔇 Additional comments (10)
test/dummy_test.go (1)
31-32: LGTM! The changes align with PR objectives.The switch from static to random transactions improves the test suite by making it more realistic while maintaining the same test logic.
proxy/grpc/client_server_test.go (2)
71-73: LGTM! Good addition of timeout context.The addition of a timeout context improves test reliability by preventing potential hangs.
59-60: LGTM! Clean state root handling.The changes to use
types.Hashdirectly and compare it in assertions make the code more type-safe and cleaner.Also applies to: 76-76
proxy/grpc/server.go (1)
86-86: LGTM! Consistent use of copyHash.The change to use
copyHashaligns with the new utility function and maintains consistency across the codebase.test/dummy.go (2)
48-53: LGTM! Good practice to return the generated transaction.The implementation allows test cases to verify the exact transaction that was injected, improving test reliability.
55-62: LGTM! Secure random number generation with proper error handling.Using
crypto/randinstead ofmath/randis a good security practice, and panicking in test code is acceptable.proxy/grpc/client.go (1)
64-64: LGTM! Consistent use of copyHash helper improves code maintainability.The refactoring eliminates duplicate code and reduces the chance of copy-related bugs.
Also applies to: 101-101
test/suite.go (3)
20-20: LGTM! Good practice to define timeout constant.The 3-second timeout is reasonable for tests and helps prevent hanging tests.
24-24: LGTM! Interface change improves test verification capabilities.Returning the generated transaction allows test cases to verify the exact transaction that was injected.
121-129: LGTM! Helper function reduces code duplication.The
initChainhelper improves code maintainability by centralizing chain initialization logic.
# Conflicts: # test/dummy.go
Previously, hashes was not copied properly because of nil destination for copy function. Replaced manual slice copying with a dedicated `copyHash` function for cleaner and reusable code. Updated related assertions and method calls to align with the new approach, improving code readability and maintainability.
Introduced a context with a timeout to ensure all tests in `ExecutorSuite` and the gRPC client-server tests properly respect execution time limits. Standardized the use of a `maxTestDuration` constant to improve maintainability and readability across the test suite.
tzdybal
force-pushed
the
tzdybal/tx_injector
branch
from
07faf8e to
4c257e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/suite.go (1)
105-105:⚠️ Potential issueFix ineffectual assignment to maxBytes.
The
maxBytesvariable is assigned but never used in theTestMultipleBlocksfunction.Apply this diff to fix the issue:
- genesisTime, stateRoot, maxBytes := s.initChain(ctx, initialHeight) + genesisTime, stateRoot, _ := s.initChain(ctx, initialHeight)🧰 Tools
🪛 GitHub Check: lint / golangci-lint
[failure] 105-105:
ineffectual assignment to maxBytes (ineffassign)🪛 golangci-lint (1.62.2)
105-105: ineffectual assignment to maxBytes
(ineffassign)
🪛 GitHub Actions: CI and Release
[error] 105-105: ineffectual assignment to maxBytes (ineffassign)
🧹 Nitpick comments (1)
test/dummy.go (1)
67-74: Consider returning error instead of panicking.While panicking in test code is generally acceptable, returning an error would provide better error handling and flexibility.
-func mustGetRandomBytes(n int) []byte { +func getRandomBytes(n int) ([]byte, error) { b := make([]byte, n) _, err := rand.Read(b) if err != nil { - panic(fmt.Errorf("failed to generate random bytes: %w", err)) + return nil, fmt.Errorf("failed to generate random bytes: %w", err) } - return b + return b, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
proxy/grpc/client.go(2 hunks)proxy/grpc/client_server_test.go(2 hunks)proxy/grpc/server.go(1 hunks)proxy/grpc/util.go(1 hunks)test/dummy.go(2 hunks)test/dummy_test.go(1 hunks)test/suite.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- proxy/grpc/util.go
- test/dummy_test.go
- proxy/grpc/client.go
- proxy/grpc/server.go
🧰 Additional context used
🪛 GitHub Check: lint / golangci-lint
test/suite.go
[failure] 105-105:
ineffectual assignment to maxBytes (ineffassign)
🪛 golangci-lint (1.62.2)
test/suite.go
105-105: ineffectual assignment to maxBytes
(ineffassign)
🪛 GitHub Actions: CI and Release
test/suite.go
[error] 105-105: ineffectual assignment to maxBytes (ineffassign)
🔇 Additional comments (5)
proxy/grpc/client_server_test.go (1)
59-77: Great improvements to type safety and context handling!The changes enhance the code quality by:
- Using
types.Hashinstead of raw byte slices for state root- Adding proper timeout context with cancellation
- Simplifying state root comparison
test/dummy.go (1)
57-65: Implementation contradicts PR objectives.The PR objectives state the goal is to "allow test suites to provide actual transactions instead of relying on random transactions". However, this change removes
InjectTxin favor ofInjectRandomTx, which seems to do the opposite by enforcing random transactions instead of allowing actual transactions to be provided.Please clarify if this is intentional or if we should maintain both capabilities:
InjectTx(tx types.Tx)- for providing actual transactionsInjectRandomTx() types.Tx- for generating random transactionsLikely an incorrect or invalid review comment.
test/suite.go (3)
20-20: Good addition of timeout constant.Adding
maxTestDurationimproves consistency in timeout handling across tests.
22-25: TxInjector interface change contradicts PR objectives.Similar to the changes in dummy.go, removing
InjectTxin favor ofInjectRandomTxcontradicts the PR objective of allowing test suites to provide actual transactions.Likely an incorrect or invalid review comment.
33-35: Great improvement in context handling!The consistent use of timeout contexts with proper cancellation across all test methods is a significant improvement.
Also applies to: 46-48, 71-73, 85-87, 102-103
Replaced the reuse of "stateRoot" with "prevStateRoot" for clarity and updated state transition logic. Added assertion to ensure state roots differ between iterations, improving test robustness.
tzdybal
changed the title
tzdybal
changed the title
Pull request was converted to draft
Add a `stateRootChanged` flag to test cases to explicitly verify state root changes. Update logic in `Exec.ExecuteTxs` to handle state root equality checks properly and return previous state root for empty transactions in `dummy.go`.
Refactored `TxInjector` to replace `InjectRandomTx` with `GetRandomTxs` and `InjectTxs` for better flexibility and clarity. Updated related test cases to reflect the new interface, ensuring consistency and enhanced test coverage.
Overview
This PR enables test suites to provide actual transactions, because we cannot just use random ones (they would most probably be invalid in actual execution environments).
Resolves #33.
Summary by CodeRabbit
New Features
Refactor
Tests