test: improve tx injector and common test suite #61
Conversation
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
✨ Finishing Touches
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.
Added a new test `TestSyncScenario` to validate block sync without mempool transactions and refined `TestMultipleBlocks` for clarity. Updated dummy logic to handle empty transaction cases and improved transaction injection error handling in tests.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/dummy.go(3 hunks)test/dummy_test.go(2 hunks)test/suite.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/dummy_test.go
🧰 Additional context used
🪛 GitHub Check: lint / golangci-lint
test/suite.go
[failure] 117-117:
Error return value of s.Exec.SetFinal is not checked (errcheck)
[failure] 111-111:
G115: integer overflow conversion int -> uint64 (gosec)
🪛 golangci-lint (1.62.2)
test/suite.go
117-117: Error return value of s.Exec.SetFinal is not checked
(errcheck)
[high] 111-111: G115: integer overflow conversion int -> uint64
(gosec)
🪛 GitHub Actions: CI and Release
test/suite.go
[error] 117-117: Error return value of s.Exec.SetFinal is not checked (errcheck)
🔇 Additional comments (11)
test/dummy.go (5)
5-6: LGTM: Added necessary imports for random byte generationThe additions of
crypto/randandfmtimports are appropriate for the new random bytes generation functionality.
76-83: Implementation of random transaction generation looks goodThe
GetRandomTxsmethod provides a clean way to generate a specified number of random transactions for testing purposes.
85-92: Improved transaction injection with batch capabilityReplacing the single transaction injection with a batch approach improves efficiency when multiple transactions need to be injected.
93-100: Well-implemented random bytes generation with proper error handlingThe
mustGetRandomBytesfunction follows Go conventions with themustprefix indicating it will panic on errors. The error message is clear and includes the wrapped original error.
129-132: Proper handling of empty transaction setsThis change adds explicit handling for empty transaction slices, ensuring the previous state root is returned and stored, which is necessary for valid empty blocks.
test/suite.go (6)
4-6: Added necessary imports for the enhanced test suiteThe addition of
bytesandfmtimports supports the new functionality in the test suite.
22-26: Interface update aligns with implementation changesThe
TxInjectorinterface has been appropriately updated to match the changes in theDummyExecutorimplementation, providing a cleaner approach to transaction generation and injection.
50-66: Improved test structure with clear separation of concernsThe test now properly verifies the empty state before injecting transactions, and uses the new methods for random transaction generation and batch injection. The assertions are more precise with explicit checks for transaction containment.
129-137: Test case update for SetFinal with a more realistic scenarioThe test for
SetFinalnow uses a more realistic height (7) for the initial error case and properly uses the state root from chain initialization, which improves test validity.
166-187: Good addition of a separate sync scenario testThe new test method
TestSyncScenariois a valuable addition that verifies the API can handle syncing blocks without injecting transactions to the mempool, which is a different workflow than the existingTestMultipleBlockstest.
189-197: Added helper method improves code reusabilityThe new
initChainmethod encapsulates chain initialization logic, reducing code duplication across test methods.
| s.Assert().Equal(c.stateRootChanged, !bytes.Equal(lastStateRoot, stateRoot)) | ||
| s.Require().Greater(maxBytes, uint64(0)) | ||
| lastStateRoot = stateRoot | ||
| s.Exec.SetFinal(ctx, uint64(i+1)) |
There was a problem hiding this comment.
Error return value from SetFinal is not checked
The error returned by s.Exec.SetFinal is not being checked, which could mask failures in the test.
- s.Exec.SetFinal(ctx, uint64(i+1))
+ err = s.Exec.SetFinal(ctx, uint64(i+1))
+ s.Require().NoError(err)📝 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.Exec.SetFinal(ctx, uint64(i+1)) | |
| err = s.Exec.SetFinal(ctx, uint64(i+1)) | |
| s.Require().NoError(err) |
🧰 Tools
🪛 GitHub Check: lint / golangci-lint
[failure] 117-117:
Error return value of s.Exec.SetFinal is not checked (errcheck)
🪛 golangci-lint (1.62.2)
117-117: Error return value of s.Exec.SetFinal is not checked
(errcheck)
🪛 GitHub Actions: CI and Release
[error] 117-117: Error return value of s.Exec.SetFinal is not checked (errcheck)
| s.Require().NoError(err) | ||
| txs, _ := s.Exec.GetTxs(ctx) | ||
| fmt.Println(len(txs)) | ||
| stateRoot, maxBytes, err := s.Exec.ExecuteTxs(ctx, c.txs, uint64(i+1), genesisTime.Add(time.Duration(i)*time.Second), lastStateRoot) |
There was a problem hiding this comment.
Fix potential integer overflow in type conversion
There's a potential integer overflow when converting i+1 from int to uint64.
- stateRoot, maxBytes, err := s.Exec.ExecuteTxs(ctx, c.txs, uint64(i+1), genesisTime.Add(time.Duration(i)*time.Second), lastStateRoot)
+ stateRoot, maxBytes, err := s.Exec.ExecuteTxs(ctx, c.txs, uint64(i)+1, genesisTime.Add(time.Duration(i)*time.Second), lastStateRoot)📝 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.
| stateRoot, maxBytes, err := s.Exec.ExecuteTxs(ctx, c.txs, uint64(i+1), genesisTime.Add(time.Duration(i)*time.Second), lastStateRoot) | |
| stateRoot, maxBytes, err := s.Exec.ExecuteTxs(ctx, c.txs, uint64(i)+1, genesisTime.Add(time.Duration(i)*time.Second), lastStateRoot) |
🧰 Tools
🪛 GitHub Check: lint / golangci-lint
[failure] 111-111:
G115: integer overflow conversion int -> uint64 (gosec)
🪛 golangci-lint (1.62.2)
[high] 111-111: G115: integer overflow conversion int -> uint64
(gosec)
| cases := []struct { | ||
| name string | ||
| txs []types.Tx | ||
| stateRootChanged bool | ||
| }{ | ||
| { | ||
| name: "empty txs", | ||
| txs: []types.Tx{}, | ||
| stateRootChanged: false, | ||
| }, | ||
| { | ||
| name: "nil txs", | ||
| txs: nil, | ||
| stateRootChanged: false, | ||
| }, | ||
| { | ||
| name: "two txs", | ||
| txs: s.TxInjector.GetRandomTxs(2), | ||
| stateRootChanged: true, | ||
| }, | ||
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| defer cancel() | ||
|
|
||
| stateRoot, maxBytes, err := s.Exec.ExecuteTxs(ctx, txs, blockHeight, timestamp, prevStateRoot) | ||
| s.Require().NoError(err) | ||
| s.NotEqual(types.Hash{}, stateRoot) | ||
| s.Greater(maxBytes, uint64(0)) | ||
| genesisTime, lastStateRoot, _ := s.initChain(ctx, uint64(1)) | ||
|
|
||
| for i, c := range cases { | ||
| s.Run(c.name, func() { | ||
| err := s.TxInjector.InjectTxs(c.txs) | ||
| s.Require().NoError(err) | ||
| txs, _ := s.Exec.GetTxs(ctx) | ||
| fmt.Println(len(txs)) | ||
| stateRoot, maxBytes, err := s.Exec.ExecuteTxs(ctx, c.txs, uint64(i+1), genesisTime.Add(time.Duration(i)*time.Second), lastStateRoot) | ||
| s.Require().NoError(err) | ||
| s.Require().NotEmpty(stateRoot) | ||
| s.Assert().Equal(c.stateRootChanged, !bytes.Equal(lastStateRoot, stateRoot)) | ||
| s.Require().Greater(maxBytes, uint64(0)) | ||
| lastStateRoot = stateRoot | ||
| s.Exec.SetFinal(ctx, uint64(i+1)) | ||
| time.Sleep(3 * time.Second) | ||
| }) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Table-driven testing approach enhances test coverage and readability
The refactoring to a table-driven test structure is a significant improvement that makes the test cases more organized and maintainable. However, there's an unchecked error return from SetFinal on line 117.
- s.Exec.SetFinal(ctx, uint64(i+1))
+ err = s.Exec.SetFinal(ctx, uint64(i+1))
+ s.Require().NoError(err)Additionally, there's a potential integer overflow warning on line 111 when converting i+1 to uint64:
- stateRoot, maxBytes, err := s.Exec.ExecuteTxs(ctx, c.txs, uint64(i+1), genesisTime.Add(time.Duration(i)*time.Second), lastStateRoot)
+ stateRoot, maxBytes, err := s.Exec.ExecuteTxs(ctx, c.txs, uint64(i)+1, genesisTime.Add(time.Duration(i)*time.Second), lastStateRoot)Action Required: Fix error handling and integer conversion issues in the table-driven test
-
Update the call to
SetFinalto capture and check the error return:- s.Exec.SetFinal(ctx, uint64(i+1)) + err = s.Exec.SetFinal(ctx, uint64(i+1)) + s.Require().NoError(err)
-
Revise the conversion of the loop index when calling
ExecuteTxsto prevent any potential integer overflow warning. For instance, change:- stateRoot, maxBytes, err := s.Exec.ExecuteTxs(ctx, c.txs, uint64(i+1), genesisTime.Add(time.Duration(i)*time.Second), lastStateRoot) + stateRoot, maxBytes, err := s.Exec.ExecuteTxs(ctx, c.txs, uint64(i)+1, genesisTime.Add(time.Duration(i)*time.Second), lastStateRoot)
📝 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.
| cases := []struct { | |
| name string | |
| txs []types.Tx | |
| stateRootChanged bool | |
| }{ | |
| { | |
| name: "empty txs", | |
| txs: []types.Tx{}, | |
| stateRootChanged: false, | |
| }, | |
| { | |
| name: "nil txs", | |
| txs: nil, | |
| stateRootChanged: false, | |
| }, | |
| { | |
| name: "two txs", | |
| txs: s.TxInjector.GetRandomTxs(2), | |
| stateRootChanged: true, | |
| }, | |
| } | |
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | |
| defer cancel() | |
| stateRoot, maxBytes, err := s.Exec.ExecuteTxs(ctx, txs, blockHeight, timestamp, prevStateRoot) | |
| s.Require().NoError(err) | |
| s.NotEqual(types.Hash{}, stateRoot) | |
| s.Greater(maxBytes, uint64(0)) | |
| genesisTime, lastStateRoot, _ := s.initChain(ctx, uint64(1)) | |
| for i, c := range cases { | |
| s.Run(c.name, func() { | |
| err := s.TxInjector.InjectTxs(c.txs) | |
| s.Require().NoError(err) | |
| txs, _ := s.Exec.GetTxs(ctx) | |
| fmt.Println(len(txs)) | |
| stateRoot, maxBytes, err := s.Exec.ExecuteTxs(ctx, c.txs, uint64(i+1), genesisTime.Add(time.Duration(i)*time.Second), lastStateRoot) | |
| s.Require().NoError(err) | |
| s.Require().NotEmpty(stateRoot) | |
| s.Assert().Equal(c.stateRootChanged, !bytes.Equal(lastStateRoot, stateRoot)) | |
| s.Require().Greater(maxBytes, uint64(0)) | |
| lastStateRoot = stateRoot | |
| s.Exec.SetFinal(ctx, uint64(i+1)) | |
| time.Sleep(3 * time.Second) | |
| }) | |
| } | |
| cases := []struct { | |
| name string | |
| txs []types.Tx | |
| stateRootChanged bool | |
| }{ | |
| { | |
| name: "empty txs", | |
| txs: []types.Tx{}, | |
| stateRootChanged: false, | |
| }, | |
| { | |
| name: "nil txs", | |
| txs: nil, | |
| stateRootChanged: false, | |
| }, | |
| { | |
| name: "two txs", | |
| txs: s.TxInjector.GetRandomTxs(2), | |
| stateRootChanged: true, | |
| }, | |
| } | |
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | |
| defer cancel() | |
| genesisTime, lastStateRoot, _ := s.initChain(ctx, uint64(1)) | |
| for i, c := range cases { | |
| s.Run(c.name, func() { | |
| err := s.TxInjector.InjectTxs(c.txs) | |
| s.Require().NoError(err) | |
| txs, _ := s.Exec.GetTxs(ctx) | |
| fmt.Println(len(txs)) | |
| stateRoot, maxBytes, err := s.Exec.ExecuteTxs(ctx, c.txs, uint64(i)+1, genesisTime.Add(time.Duration(i)*time.Second), lastStateRoot) | |
| s.Require().NoError(err) | |
| s.Require().NotEmpty(stateRoot) | |
| s.Assert().Equal(c.stateRootChanged, !bytes.Equal(lastStateRoot, stateRoot)) | |
| s.Require().Greater(maxBytes, uint64(0)) | |
| lastStateRoot = stateRoot | |
| err = s.Exec.SetFinal(ctx, uint64(i+1)) | |
| s.Require().NoError(err) | |
| time.Sleep(3 * time.Second) | |
| }) | |
| } |
🧰 Tools
🪛 GitHub Check: lint / golangci-lint
[failure] 117-117:
Error return value of s.Exec.SetFinal is not checked (errcheck)
[failure] 111-111:
G115: integer overflow conversion int -> uint64 (gosec)
🪛 golangci-lint (1.62.2)
117-117: Error return value of s.Exec.SetFinal is not checked
(errcheck)
[high] 111-111: G115: integer overflow conversion int -> uint64
(gosec)
🪛 GitHub Actions: CI and Release
[error] 117-117: Error return value of s.Exec.SetFinal is not checked (errcheck)
|
No needed anymore. |
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