test: adding compatibility test to perform syncing #3023
Conversation
Add EVMCompatTestSuite to test syncing between different ev-node versions. The test: - Starts sequencer with version A (env: SEQUENCER_EVM_IMAGE_*) - Submits transactions - Starts full node with version B (env: FULLNODE_EVM_IMAGE_*) - Verifies full node syncs and can query transactions - Submits post-sync transactions to verify ongoing sync Add make test-docker-compat target for running the test.
|
Claude finished @chatton's task in 2m 35s —— View job Code Review CompleteI've performed a comprehensive review of this PR, which adds an excellent cross-version compatibility test suite. Overall, the implementation is well-structured and addresses an important testing gap. Below are my findings organized by category. SummaryThis PR successfully introduces a compatibility test that validates cross-version syncing between different ev-node-evm versions. The new Strengths:
Key Issues to Address:
1. Code Quality & ArchitecturePositive Aspects
IssuesCritical: Fixed sleep introduces test flakiness ( // wait for blocks to be posted to DA before starting full node
time.Sleep(5 * time.Second)This is a reliability concern. The 5-second sleep is arbitrary and doesn't guarantee DA propagation completed. Gemini's suggestion to wait for additional blocks is good, but I'd recommend also verifying DA availability. Recommendation: Replace with a conditional wait that polls for DA height or sequencer block production: s.T().Log("Waiting for DA propagation...")
lastTxHeight, err := s.sequencerClient.BlockNumber(ctx)
s.Require().NoError(err)
targetHeight := lastTxHeight + 3 // Allow a few blocks for DA propagation
s.Require().Eventually(func() bool {
h, err := s.sequencerClient.BlockNumber(ctx)
return err == nil && h >= targetHeight
}, 30*time.Second, time.Second, "sequencer did not produce blocks for DA propagation")2. Error Handling & ResilienceGood Practices
IssuesMissing cleanup logic ( Recommendation: Add a cleanup function or defer statements: func (s *EVMCompatTestSuite) TearDownTest() {
ctx := context.Background()
if s.sequencerNode != nil {
_ = s.sequencerNode.StopAndRemove(ctx)
}
if s.fullNode != nil {
_ = s.fullNode.StopAndRemove(ctx)
}
// Clean up celestia and DA resources
}Timeout values could be configurable ( 3. Security ConsiderationsIssuesJWT Secret exposed in logs ( setup.JWTSecret = rethNode.JWTSecretHex()While this is test code, it sets a pattern. Ensure JWT secrets are never logged at higher log levels. Private key as constant ( evmTestPrivateKey = "cece4f25ac74deb1468965160c7185e07dff413f23fcadb611b05ca37ab0a52e"Good that it's clearly marked for testing, but add a comment warning against production use: // evmTestPrivateKey is a TEST-ONLY private key. NEVER use in production.
evmTestPrivateKey = "cece4f25ac74deb1468965160c7185e07dff413f23fcadb611b05ca37ab0a52e"Passphrase hardcoded ( WithEVMSignerPassphrase("secret")Consider using a test constant or environment variable for consistency. 4. Performance & Resource EfficiencyIssuesConcurrent container operations not optimized ( Transaction submission is sequential ( // Submit all txs first
for i := range count {
tx := evm.GetRandomTransaction(...)
s.Require().NoError(s.sequencerClient.SendTransaction(ctx, tx))
txHashes = append(txHashes, tx.Hash())
}
// Then wait for all
for _, txHash := range txHashes {
s.WaitForTxIncluded(ctx, s.sequencerClient, txHash)
}30-minute timeout may be excessive ( -timeout=30mConsider if 20 minutes would suffice, or make it configurable. 5. Testing Strategy & ReliabilityPositive Aspects
IssuesTODO comment unresolved ( SEQUENCER_EVM_IMAGE_TAG: main # TODO: set this to the last released tagThis should be resolved before merging. Using Recommendation: Set this to the last stable release tag (e.g., Another TODO in upgrade test ( // TODO: upgrade from previous released version instead of main
baseEVMSingleVersion = "main"Same issue - should use a released version tag. Test could verify more sync properties (
6. Documentation & Developer ExperienceGood Practices
IssuesMissing package-level documentation ( // Package docker_e2e contains end-to-end tests that verify ev-node behavior
// in Docker environments, including cross-version compatibility and upgrades.Function documentation incomplete ( Makefile target needs documentation ( Recommendation: Update CLAUDE.md to document: ### Testing
- `make test-docker-compat` - Runs cross-version compatibility tests7. Go Idioms & Best PracticesIssuesContext without cancellation ( ctx := context.Background()For long-running tests, consider using Magic number for transaction count ( preSyncTxHashes = s.submitTransactions(ctx, 50)
postSyncTxHashes := s.submitTransactions(ctx, 5)Consider using named constants: const (
preSyncTxCount = 50
postSyncTxCount = 5
)Error messages could be more specific ( s.Require().NoError(err, "failed to query tx %d on sequencer: %s", i, txHash.Hex())Good, but could include the actual error for debugging: s.Require().NoError(err, "failed to query tx %d (%s) on sequencer: %v", i, txHash.Hex(), err)8. Dependency UpdatesThe PR updates several dependencies:
These updates appear routine and low-risk. The version bumps are minor/patch updates. Recommendation: Ensure these dependency updates were tested and don't introduce breaking changes. The passing CI suggests they're compatible. 9. Refactoring QualityThe refactoring in Positive:
Minor suggestion: 10. Workflow ConfigurationThe new workflow job
Issue: The TODO on line 68 (mentioned earlier) needs resolution. Summary of Priority IssuesMust Fix Before Merge:
Should Fix:
Nice to Have:
ConclusionThis is a valuable addition to the test suite that will help catch compatibility regressions. The code is generally well-written and follows good testing practices. Addressing the test flakiness issue and resolving the TODO comments would make this PR ready to merge. Great work on the implementation! The test architecture is solid and the refactoring improves maintainability. |
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
Summary of ChangesHello @chatton, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the testing infrastructure by introducing a robust Docker E2E compatibility test. This new test ensures that different versions of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable compatibility test suite to ensure syncing works across different node versions. The new EVMCompatTestSuite effectively simulates a real-world scenario where a new full node syncs from a sequencer running a different version. The changes also include beneficial refactoring of the existing test suite, such as centralizing test constants and creating reusable helper functions for setting up test nodes. My review includes one suggestion to improve the robustness of the new test by replacing a fixed sleep with a conditional wait.
| time.Sleep(5 * time.Second) | ||
|
|
||
| sequencerHeight, err := s.sequencerClient.BlockNumber(ctx) | ||
| s.Require().NoError(err) |
There was a problem hiding this comment.
Using a fixed time.Sleep can introduce flakiness into tests, as the time required for an operation can vary between test runs. A more robust approach is to wait for a specific condition. Here, you're waiting for blocks to be posted to the DA. While directly querying the DA might be complex, you can achieve a more reliable wait by ensuring a few more blocks are produced by the sequencer. This provides a time window for DA propagation that is tied to block production rather than a fixed duration.
s.T().Log("Waiting for a few blocks to allow for DA propagation...")
lastTxHeight, err := s.sequencerClient.BlockNumber(ctx)
s.Require().NoError(err)
const daPropagationBlocks = 3
targetHeight := lastTxHeight + daPropagationBlocks
s.Require().Eventually(func() bool {
h, err := s.sequencerClient.BlockNumber(ctx)
return err == nil && h >= targetHeight
}, 15*time.Second, time.Second, "sequencer did not produce new blocks for DA propagation")
sequencerHeight, err := s.sequencerClient.BlockNumber(ctx)
s.Require().NoError(err)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3023 +/- ##
==========================================
+ Coverage 55.55% 55.61% +0.06%
==========================================
Files 116 116
Lines 11477 11477
==========================================
+ Hits 6376 6383 +7
+ Misses 4401 4394 -7
Partials 700 700
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:
|
Overview
This PR adds a compatibility test that will deploy a specified image, execute some transactions, then deploy the PR image and verify that syncing still works correctly.
closes #3019