chore: adding upgrade test for evm-single #2780
Conversation
|
The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).
|
|
Claude finished @chatton's task —— View job Code Review CompleteI've completed a comprehensive code review of the upgrade test implementation. Overall, this is a well-structured and valuable addition to the test suite. The upgrade testing functionality is critical for ensuring seamless version transitions in production. Summary AssessmentStrengths:
Priority Issues to Address:
1. Code Quality & ArchitectureStrengths✅ Excellent use of testify suite pattern - The ✅ Clear test phases - Using ✅ Good separation of concerns - Helper methods like Issues🔴 CRITICAL: Hardcoded Private Key (Security) testPrivateKey = "cece4f25ac74deb1468965160c7185e07dff413f23fcadb611b05ca37ab0a52e"
const (
// TEST ONLY - DO NOT USE IN PRODUCTION
// This is a publicly known test key from Ethereum test fixtures
testPrivateKey = "..."
)🟡 Missing Input Validation func (s *EVMSingleUpgradeTestSuite) submitPreUpgradeTxs(ctx context.Context, txCount int) []common.Hash {
var txHashes []common.Hash
for i := range txCount { // No validation if txCount is negative or unreasonably large
if txCount <= 0 || txCount > 1000 {
s.T().Fatalf("Invalid txCount: %d (must be between 1 and 1000)", txCount)
}2. Error Handling & ResilienceIssues🔴 Missing Context Cancellation func (s *EVMSingleUpgradeTestSuite) TestEVMSingleUpgrade() {
ctx := context.Background() // No timeout or cancellation
ctx, cancel := context.WithTimeout(context.Background(), 15*time.Minute)
defer cancel()🟡 Inadequate Error Context in Eventually Blocks s.Require().Eventually(func() bool {
receipt, err := s.ethClient.TransactionReceipt(ctx, txHash)
if err != nil {
return false // Silently returns false without logging error
}
return receipt.Status == 1
}, 30*time.Second, time.Second, "transaction %s was not included", txHash.Hex())
s.Require().Eventually(func() bool {
receipt, err := s.ethClient.TransactionReceipt(ctx, txHash)
if err != nil {
s.T().Logf("Waiting for tx %s: %v", txHash.Hex(), err)
return false
}
if receipt.Status != 1 {
s.T().Logf("Transaction %s failed with status %d", txHash.Hex(), receipt.Status)
return false
}
return true
}, 30*time.Second, time.Second, "transaction %s was not included", txHash.Hex())🟡 Missing Cleanup/Defer Patterns
3. Security🔴 CRITICAL: Exposed JWT Secret Handling s.jwtSecret = rethNode.JWTSecretHex()
🟡 Default Credentials WithEVMSignerPassphrase("secret")
const testSignerPassphrase = "secret" // TEST ONLY - insecure passphrase for local testing4. Performance & Resource EfficiencyIssues🟡 Fixed Sleep Times Instead of Adaptive Waiting time.Sleep(3 * time.Second)
🟡 Sequential Transaction Submission for i := range txCount {
tx := evm.GetRandomTransaction(...)
err := s.ethClient.SendTransaction(ctx, tx)
s.Require().NoError(err)
// ...
s.waitForTxIncluded(ctx, tx.Hash()) // Waits for each tx sequentially
}
🟡 No Connection Pooling or Reuse Verification ethClient, err := ethclient.Dial(s.evmEthURLExternal)
5. Testing & ReliabilityStrengths✅ Good use of Eventually for asynchronous operations ✅ Comprehensive verification - Checks transaction persistence, block production, and account balances Issues🟡 Hardcoded Version String baseEVMSingleVersion = "v1.0.0-beta.8"
func getBaseEVMSingleVersion() string {
if v := os.Getenv("EVM_SINGLE_BASE_VERSION"); v != "" {
return v
}
return "v1.0.0-beta.8" // Default stable version for upgrade tests
}🟡 Insufficient Edge Case Testing
🟡 Timeout Configuration s.Require().Eventually(func() bool {
// ...
}, 60*time.Second, 2*time.Second, "reth did not start in time")
🟡 Nonce Management txNonce uint64
6. Documentation & Developer ExperienceStrengths✅ Excellent inline comments explaining test purpose and flow ✅ Good logging throughout the test for debugging Issues🟡 Missing Function-Level Documentation func (s *EVMSingleUpgradeTestSuite) setupRethNode(ctx context.Context) {
// No docstring explaining what this does
// setupRethNode creates and starts a Reth execution client node.
// It waits for the node to be ready and configures connection URLs
// for both internal (container-to-container) and external (test-to-container) communication.
func (s *EVMSingleUpgradeTestSuite) setupRethNode(ctx context.Context) {🟡 Environment Variable Documentation func getEVMSingleImage() container.Image {
repo := strings.TrimSpace(os.Getenv("EVM_SINGLE_IMAGE_REPO"))
// Environment Variables:
// - EVM_SINGLE_IMAGE_REPO: Docker repository for evm-single image (default: "evm-single")
// - EVM_SINGLE_NODE_IMAGE_TAG: Image tag to upgrade to (default: "local-dev")7. Changes to Existing Test Filestest/docker-e2e/docker_test.go✅ Good: Minor refactoring to use consistent naming ( test/docker-e2e/resiliency_test.go✅ Good: Consistent naming changes 🟡 Minor: These test files have similar issues to upgrade_test.go (hardcoded values, missing context timeouts, etc.) 8. Test Infrastructure (Makefile & Workflow)scripts/test.mk✅ Good: New 🟡 Specific Test Run: Line 67 explicitly runs only -run '^TestEVMSingleUpgradeSuite$$/^TestEVMSingleUpgrade$$'
.github/workflows/test.yml✅ Excellent: New
docker-upgrade-tests:
name: Docker Upgrade E2E Tests
needs: build-ev-node-evm-single-image
runs-on: ubuntu-latest
Recommendation: docker-upgrade-tests:
name: Docker Upgrade E2E Tests
if: "!github.event.pull_request.draft && (contains(github.event.pull_request.labels.*.name, 'test: upgrade') || github.base_ref == 'main')"
needs: build-ev-node-evm-single-image
runs-on: ubuntu-latest9. Dependencies (go.mod/go.sum)✅ Good: Dependency updates appear to be for 🟡 Large Dependency Addition: test/docker-e2e/go.mod added 151 new lines
Recommendation: Periodically audit dependencies with Priority RecommendationsMust Fix (Before Merge)
Should Fix (Next Iteration)
Nice to Have (Future Work)
Overall AssessmentThis is a high-quality, production-ready upgrade test with minor areas for improvement. The code demonstrates good Go practices, clear test structure, and comprehensive verification. The main concerns are around security hygiene (hardcoded credentials), error visibility, and making the test more flexible for different environments. Recommendation: ✅ Approve with minor changes - Address the critical security documentation and add context timeout before merging. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2780 +/- ##
=======================================
Coverage 61.57% 61.57%
=======================================
Files 82 82
Lines 8715 8715
=======================================
Hits 5366 5366
Misses 2845 2845
Partials 504 504
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ## test-docker-upgrade-e2e: Running Docker E2E Upgrade tests | ||
| test-docker-upgrade-e2e: | ||
| @echo "--> Running Docker Upgrade E2E tests" | ||
| @cd test/docker-e2e && go test -mod=readonly -failfast -v -tags='docker_e2e evm' -timeout=30m -run '^TestEVMSingleUpgradeSuite$$/^TestEVMSingleUpgrade$$' ./... |
There was a problem hiding this comment.
explicitly running the one test for now, since these tests are quite heavy, we probably don't want run many in one make target
2 participants
Overview
This PR adds an upgrade test which will run on every PR.
The test deploys a specific version, for now hard coded to
"v1.0.0-beta.8", makes a bunch of transactions, then deploys the evm-single version built from the PR, verifies it can still access those transactions, makes a few more and ensures blocks are still being produced.In a followup we can update the test to be muilti-node, and perform more robust checks at the DA layer.