-
Notifications
You must be signed in to change notification settings - Fork 105
Make E2E tests run in parallel #178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Warning Rate limit exceeded@Thaleszh has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request refactors the e2e test suite to enable parallel test execution. The primary changes involve: (1) migrating proposal counter management from local/implicit variables to a per-suite field Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/e2e/e2e_setup_test.go (1)
620-643: Hardcoded port 26657 in RPC client will cause failures in parallel execution.The RPC client connects to a hardcoded
tcp://localhost:26657, but each parallel test uses a different port offset (26657+portOffset). This will cause all parallel tests to connect to the same (or wrong) validator, leading to race conditions or test failures.- rpcClient, err := rpchttp.New("tcp://localhost:26657", "/websocket") + rpcClient, err := rpchttp.New(fmt.Sprintf("tcp://localhost:%d", 26657+portOffset), "/websocket")tests/e2e/e2e_gov_test.go (2)
87-92: Bug: Local counter increment doesn't sync back to suite-level counter.Line 87 increments the local
proposalCounterbut doesn't updates.proposalCounter. This creates a desync: after this function,s.proposalCounterwill be 1 behind the actual proposal count. Subsequent tests in the same suite will use incorrect proposal IDs.- proposalCounter++ + s.proposalCounter++ + proposalCounter = s.proposalCounter
59-59: Adds.proposalCounter = 0at line 59 to match the reset intent.The local variable
proposalCounteris reset at line 59, but the struct fields.proposalCounteris not. SinceSetupSuite()does not resets.proposalCounter, it will retain its incremented value across the suite restart, breaking the proposal ID tracking for subsequent tests. Change line 59 tos.proposalCounter = 0to properly reset the counter after the chain restart.tests/e2e/e2e_rate_limit_test.go (1)
217-220: Bug: Local counter increment doesn't sync back to suite-level counter.Same issue as in
e2e_gov_test.go: line 217 increments the localproposalCounterwithout updatings.proposalCounter, causing a desync for subsequent tests.- proposalCounter++ + s.proposalCounter++ + proposalCounter = s.proposalCounter
🧹 Nitpick comments (1)
tests/e2e/e2e_test.go (1)
252-256: Consider deprecation notice for sequential test entry point.
TestSequentialE2Euses the oldsuite.Runpattern which runs all tests on a single shared suite instance. This may behave differently from the parallel tests (e.g., sharedproposalCounterstate across all tests vs. isolated per-test). Consider adding a deprecation comment or documenting when to use each.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/e2e/e2e_erc20_test.go(1 hunks)tests/e2e/e2e_gov_test.go(4 hunks)tests/e2e/e2e_oracle_test.go(1 hunks)tests/e2e/e2e_rate_limit_test.go(4 hunks)tests/e2e/e2e_rewards_test.go(1 hunks)tests/e2e/e2e_setup_test.go(5 hunks)tests/e2e/e2e_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/e2e_test.go (1)
tests/e2e/e2e_setup_test.go (2)
NewIntegrationTestSuite(133-139)IntegrationTestSuite(106-119)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: golangci-lint
- GitHub Check: Analyze
- GitHub Check: test-e2e
- GitHub Check: tests
- GitHub Check: liveness-test
- GitHub Check: golangci-lint
🔇 Additional comments (7)
tests/e2e/e2e_setup_test.go (1)
132-139: Atomic increment by 2 ensures port isolation, but consider documenting the logic.The
atomic.AddUint64(&testCounter, 2)reserves two consecutive port offsets per suite (one for chainA, one for chainB). This design prevents port collisions across parallel test instances. However, the magic number2and thetestID-1usage inrunValidatorscould benefit from a brief comment explaining the port allocation strategy.tests/e2e/e2e_erc20_test.go (1)
92-93: Proposal counter pattern looks correct for sequential test execution within a suite.The increment-then-alias pattern (
s.proposalCounter++followed byproposalCounter := s.proposalCounter) correctly captures the incremented value for use in governance flags. Since tests within a single suite instance run sequentially, this is thread-safe.tests/e2e/e2e_rewards_test.go (1)
197-198: LGTM!The proposal counter update follows the same pattern as other test files, ensuring consistent proposal ID management across the suite.
tests/e2e/e2e_rate_limit_test.go (1)
175-176: LGTM!The initial proposal counter pattern is correct.
tests/e2e/e2e_oracle_test.go (1)
162-163: LGTM!The proposal counter pattern is consistent with other test files.
tests/e2e/e2e_test.go (2)
36-50: Good parallel test structure with proper isolation.Each test creates its own
IntegrationTestSuiteinstance viaNewIntegrationTestSuite(t), ensuring isolated state (chains, containers, counters) per parallel test. Thedefer s.TearDownSuite()ensures proper cleanup even on test failures.
240-249: Loop variable capture is correct for parallel execution.The
tc := tcon line 241 properly captures the range variable before the parallel goroutine closure, preventing the classic Go loop variable capture bug. Note: This pattern is not needed in Go 1.22+, but maintaining it ensures backward compatibility.
87a866a to
f8e20a7
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Description
Initial version of running E2E tests in parallel.
Current issues:
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration