test(integration): fix flaky integration tests with proper health checks #16
Conversation
WalkthroughThe changes in this pull request focus on 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 (
|
RollkitBot
requested review from
a team,
tzdybal,
gupadhyaya and
MSevey
and removed request for
a team
![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 (3)
integration_test.go (3)
134-136: Consider making timeout configurableThe HTTP client timeout is hardcoded to 1 second. Consider making this configurable or using a constant to improve maintainability.
+const httpClientTimeout = 1 * time.Second client := &http.Client{ - Timeout: 1 * time.Second, + Timeout: httpClientTimeout, }
181-181: Consider making polling interval configurableThe sleep duration of 500ms is hardcoded. Consider making this configurable or using a constant for better maintainability.
+const pollInterval = 500 * time.Millisecond -time.Sleep(500 * time.Millisecond) +time.Sleep(pollInterval)
127-184: Consider adding unit tests for waitForRethContainerWhile the function is used in integration tests, consider adding unit tests to verify its behavior with mocked HTTP responses, timeout scenarios, and error conditions.
Would you like me to help generate unit tests for this function?
🧰 Tools
🪛 golangci-lint (1.62.2)
147-147: Error return value of
resp.Body.Closeis not checked(errcheck)
174-174: Error return value of
resp.Body.Closeis not checked(errcheck)
🪛 GitHub Check: lint / golangci-lint
[failure] 147-147:
Error return value ofresp.Body.Closeis not checked (errcheck)
[failure] 174-174:
Error return value ofresp.Body.Closeis not checked (errcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
integration_test.go(3 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
integration_test.go
147-147: Error return value of resp.Body.Close is not checked
(errcheck)
174-174: Error return value of resp.Body.Close is not checked
(errcheck)
🪛 GitHub Check: lint / golangci-lint
integration_test.go
[failure] 147-147:
Error return value of resp.Body.Close is not checked (errcheck)
[failure] 174-174:
Error return value of resp.Body.Close is not checked (errcheck)
🔇 Additional comments (1)
integration_test.go (1)
114-115: LGTM: Good replacement of fixed sleep with health checks
The change properly replaces the fixed sleep duration with active health checks, improving test reliability.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
integration_test.go (1)
174-177: Simplify JWT secret decoding by removing unnecessary prefix trimmingIn the line where the JWT secret is decoded, the
strings.TrimPrefix(jwtSecret, "0x")is unnecessary becausejwtSecretdoes not include a"0x"prefix. You can simplify the code by removing theTrimPrefixcall.Apply this diff to simplify the code:
- jwtSecretBytes, err := hex.DecodeString(strings.TrimPrefix(jwtSecret, "0x")) + jwtSecretBytes, err := hex.DecodeString(jwtSecret)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
integration_test.go(7 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
integration_test.go
[medium] 65-65: G301: Expect directory permissions to be 0750 or less
(gosec)
165-165: Error return value of resp.Body.Close is not checked
(errcheck)
192-192: Error return value of resp.Body.Close is not checked
(errcheck)
🪛 GitHub Check: lint / golangci-lint
integration_test.go
[failure] 165-165:
Error return value of resp.Body.Close is not checked (errcheck)
[failure] 192-192:
Error return value of resp.Body.Close is not checked (errcheck)
🔇 Additional comments (2)
integration_test.go (2)
165-165: Ensure proper handling of response body closure
The error return value of resp.Body.Close() is not checked, which could lead to resource leaks, as indicated by the static analysis tool.
Please refer to the previous review comment suggesting to use defer to properly close the response body and check for errors. This ensures resources are released appropriately and aligns with best practices.
Also applies to: 192-192
🧰 Tools
🪛 GitHub Check: lint / golangci-lint
[failure] 165-165:
Error return value of resp.Body.Close is not checked (errcheck)
🪛 golangci-lint (1.62.2)
165-165: Error return value of resp.Body.Close is not checked
(errcheck)
159-159: Enhance timeout error message with duration information
The timeout error message can be more informative by including the duration that was waited. This can help with debugging and understanding the context of the timeout.
Please refer to the previous review comment suggesting the inclusion of the timeout duration in the error message. Here's how you can adjust it:
- return fmt.Errorf("timeout waiting for reth container to be ready")
+ return fmt.Errorf("timeout after %v waiting for reth container to be ready", 30*time.Second)Alternatively, use the actual duration from the context to avoid hardcoding the timeout value.
|
if you are moving to dockertest, ignore.
|
- Replace fixed sleep with active health checks using timer - Use stateless engine_getClientVersionV1 for health checks - Move JWT token generation to shared utility function - Register cleanup handlers immediately after resource creation - Check subtest exit status to fail fast - Move invalid timestamp test into lifecycle test - Optimize timeouts for faster test execution
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
execution.go(3 hunks)integration_test.go(7 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
integration_test.go
[medium] 64-64: G301: Expect directory permissions to be 0750 or less
(gosec)
172-172: Error return value of resp.Body.Close is not checked
(errcheck)
190-190: Error return value of resp.Body.Close is not checked
(errcheck)
🪛 GitHub Check: lint / golangci-lint
integration_test.go
[failure] 172-172:
Error return value of resp.Body.Close is not checked (errcheck)
[failure] 190-190:
Error return value of resp.Body.Close is not checked (errcheck)
🔇 Additional comments (6)
integration_test.go (4)
45-53: LGTM! Secure implementation of JWT secret generation.
The function correctly uses crypto/rand for secure random number generation and properly handles errors.
156-158: LGTM! Appropriate timeout value.
The 100ms timeout is suitable for local network calls and aligns with the observed container readiness time.
160-161: LGTM! Proper timer implementation.
The code correctly implements the suggested timer-based approach instead of using context for timeout handling.
64-64: 🛠️ Refactor suggestion
Adjust directory permissions to enhance security
The directory permissions (0755) are more permissive than necessary. Consider using 0750 for better security.
-err = os.MkdirAll(jwtPath, 0755)
+err = os.MkdirAll(jwtPath, 0750)Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
[medium] 64-64: G301: Expect directory permissions to be 0750 or less
(gosec)
execution.go (2)
287-307: LGTM! Well-implemented JWT token generation.
The function follows JWT best practices with proper:
- Empty secret handling
- Hex decoding with "0x" prefix support
- Token expiration
- Comprehensive error handling
62-65: LGTM! Proper error handling and resource cleanup.
The code correctly handles token generation errors and ensures proper cleanup of resources by closing the ethClient on error.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
integration_test.go (2)
41-42: Consider grouping related constants togetherConsider grouping the path-related constants (
DOCKER_PATH,JWT_FILENAME) with other related constants for better organization.const ( TEST_ETH_URL = "http://localhost:8545" TEST_ENGINE_URL = "http://localhost:8551" + DOCKER_PATH = "./docker" + JWT_FILENAME = "testsecret.hex" + CHAIN_ID = "1234" GENESIS_HASH = "0x8bf225d50da44f60dee1c4ee6f810fe5b44723c76ac765654b6692d50459f216" GENESIS_STATEROOT = "0x362b7d8a31e7671b0f357756221ac385790c25a27ab222dc8cbdd08944f5aea4" TEST_PRIVATE_KEY = "cece4f25ac74deb1468965160c7185e07dff413f23fcadb611b05ca37ab0a52e" TEST_TO_ADDRESS = "0x944fDcD1c868E3cC566C78023CcB38A32cDA836E" )
152-203: Consider improving error handling in the health check loopWhile the implementation is good, the error handling in the health check loop could be more informative. Consider capturing and including specific error messages in the timeout error.
func waitForRethContainer(t *testing.T, jwtSecret string) error { t.Helper() + var lastErr error client := &http.Client{ Timeout: 100 * time.Millisecond, } timer := time.NewTimer(500 * time.Millisecond) defer timer.Stop() for { select { case <-timer.C: - return fmt.Errorf("timeout waiting for reth container to be ready") + return fmt.Errorf("timeout waiting for reth container to be ready: %v", lastErr) default: // check :8545 is ready rpcReq := strings.NewReader(`{"jsonrpc":"2.0","method":"net_version","params":[],"id":1}`) resp, err := client.Post(TEST_ETH_URL, "application/json", rpcReq) + if err != nil { + lastErr = fmt.Errorf("HTTP endpoint not ready: %v", err) + time.Sleep(100 * time.Millisecond) + continue + } - if err == nil {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
integration_test.go(7 hunks)
🔇 Additional comments (3)
integration_test.go (3)
45-53: LGTM! Secure implementation of JWT secret generation
The implementation correctly uses crypto/rand for secure random number generation and includes proper error handling.
Line range hint 55-150: LGTM! Well-structured setup with proper resource management
The implementation follows best practices:
- Immediate cleanup registration after resource creation
- Secure file permissions (0750 for directory, 0600 for file)
- Proper error handling and resource cleanup
🧰 Tools
🪛 Gitleaks (8.21.2)
38-38: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
228-242: LGTM! Well-structured test cases with proper error validation
The test cases are well-organized and include proper validation of error conditions. The use of require.True(t.Run()) ensures proper test termination on failure.
|
Created follow-up issue: #18 |
Fix Flaky Integration Tests with Proper Container Health Checks
Problem
Changes
Test Reliability
engine_getClientVersionV1for health checks instead of configuration negotiationCode Organization
Resource Management
.gitkeepto maintain jwttoken directory structureFile Structure
Testing
The changes have been tested with multiple runs of the integration tests. The tests now:
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
New Features