Skip to content

[WIP] test: use ev-reth pr-106 image to validate reth v1.10.1 upgrade#3066

Draft
chatton wants to merge 1 commit intomainfrom
cian/e2e-tests-with-ev-reth-1-10-1
Draft

[WIP] test: use ev-reth pr-106 image to validate reth v1.10.1 upgrade#3066
chatton wants to merge 1 commit intomainfrom
cian/e2e-tests-with-ev-reth-1-10-1

Conversation

@chatton
Copy link
Contributor

@chatton chatton commented Feb 12, 2026

Overview

This PR is bumping the E2E tests to use the version of reth built from 1.10.1 to verify they all pass. evstack/ev-reth#106

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @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 aims to validate the system's end-to-end tests against a specific version of Reth, namely v1.10.1. This is achieved by modifying the test setup to pull a pre-built Docker image of Reth identified by the 'pr-106' tag, ensuring that existing functionalities remain stable with the updated Reth version.

Highlights

  • Reth Image Update: Updated the end-to-end tests to utilize a specific Reth Docker image tagged 'pr-106'.
  • Upgrade Validation: Configured the test environment to validate compatibility with a Reth v1.10.1 upgrade.

🧠 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.

Changelog
  • execution/evm/test/test_helpers.go
    • Added WithTag("pr-106") to the reth.NewNodeBuilder call within SetupTestRethNode.
  • test/docker-e2e/docker_test.go
    • Inserted WithTag("pr-106") into the reth.NewNodeBuilder configuration in SetupRethNode.
Activity
  • No human activity has been recorded on this pull request yet.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@claude
Copy link
Contributor

claude bot commented Feb 12, 2026

Claude finished @chatton's task in 1m 55s —— View job


Code Review Complete

Tasks

  • Read modified files to understand changes in context
  • Search for other reth usage in tests for consistency
  • Analyze changes against code quality, testing, and architecture standards
  • Provide comprehensive review feedback

Summary

This PR adds .WithTag("pr-106") to both reth node builder calls to validate the upgrade to reth v1.10.1. The changes are minimal, consistent, and correctly placed. However, there are some important considerations about the approach taken for version validation.

Detailed Review

✅ Positive Observations

  1. Consistency: Both test files that create reth nodes have been updated identically - execution/evm/test/test_helpers.go:69 and test/docker-e2e/docker_test.go:274
  2. Complete Coverage: All reth node builder calls in the codebase have been updated (verified via grep)
  3. Minimal Change: The change is surgical and doesn't introduce unnecessary complexity
  4. Proper Positioning: The tag is specified early in the builder chain, following the pattern of other configuration methods

⚠️ Concerns & Recommendations

1. Hardcoded PR Tag (High Priority)

Issue: The tag "pr-106" is hardcoded directly in test helpers, which creates several problems:

  • Not Sustainable: PR tags are ephemeral - what happens after ev-reth#106 is merged? Will this image remain available?
  • Unclear Versioning: The tag doesn't clearly communicate the reth version (1.10.1) being tested
  • WIP State: This PR is marked [WIP], suggesting this is temporary validation code

Recommendations:

a. For Temporary Validation (if this is just to validate before merging):

  • Consider using an environment variable override instead:
    builder := reth.NewNodeBuilder(s.T()).WithName(name)
    if tag := os.Getenv("RETH_IMAGE_TAG"); tag != "" {
        builder = builder.WithTag(tag)
    }
    builder = builder.WithGenesis(...)...
  • This keeps the codebase clean while allowing CI/local testing with specific tags
  • Update your CI workflow to set RETH_IMAGE_TAG=pr-106 for validation runs

b. For Permanent Changes:

  • Use a stable, semantic version tag like "v1.10.1" or "1.10.1" instead of "pr-106"
  • Ensure the ev-reth repository publishes versioned images that align with reth releases
  • Document the reth version requirements in comments or configuration
2. Testing & Reliability Considerations

Questions:

  • Has the pr-106 image been published and is it accessible in your test environments (CI, developer machines)?
  • Are there any breaking changes in reth v1.10.1 that might affect test behavior?
  • Should there be a fallback mechanism if the image isn't available?

Recommendation: Before merging, confirm:

docker pull <image-registry>/ev-reth:pr-106

works in your CI environment.

3. Documentation & Communication

Missing Context:

  • No inline comments explaining why this specific tag is used
  • The commit message mentions validation but doesn't clarify if this is temporary

Recommendation: Add a comment above the tag specification:

// Using pr-106 tag to validate reth v1.10.1 upgrade (see https://github.com/evstack/ev-reth/pull/106)
// TODO: Update to stable version tag after ev-reth#106 merges
n, err := reth.NewNodeBuilderWithTestName(t, fmt.Sprintf("%s-%s", t.Name(), randomString(6))).
    WithTag("pr-106").
    ...
4. Version Management Strategy

Observation: Currently, reth version selection appears to be implicit (default from tastora framework). This PR introduces explicit version pinning in tests.

Consider:

  • Should the reth version be centralized in a configuration file or constant?
  • Will different test suites need different reth versions in the future?
  • Is there a version compatibility matrix between ev-node and reth?

Recommendation: For better maintainability, consider defining a test configuration constant:

// In a shared test config file
const (
    TestRethImageTag = "pr-106" // reth v1.10.1 validation
)

Then reference it in both locations. This makes future updates easier and more visible.

5. Integration with Dependency Management

Note: The go.mod files show tastora v0.12.0 (docker-e2e, e2e) and v0.8.0 (execution/evm/test). The reth image version is orthogonal to the tastora framework version, but worth considering:

  • Does the tastora framework version impose any constraints on reth versions?
  • Should reth version upgrades be coordinated with tastora upgrades?

Code Quality Assessment

Criteria Rating Notes
Correctness ✅ Good Code is syntactically correct and follows the builder pattern properly
Architecture ⚠️ Fair Hardcoded values in helpers reduce flexibility; consider configuration layer
Testing ⚠️ Fair Changes enable validation but lack fallback/error handling for missing images
Documentation ❌ Poor No comments explaining the change or temporary nature
Maintainability ⚠️ Fair Duplication of hardcoded tag across files; no centralized version management
Security ✅ Good No security concerns with image tag specification
Resilience ⚠️ Fair No handling for missing/unavailable image tags

Action Items

Before merging, please address:

  1. Clarify Intent: Is this a temporary validation change or permanent upgrade?

  2. Choose Strategy:

    • If Temporary: Use environment variable override approach [Fix this →]
    • If Permanent: Update to stable version tag and add documentation [Fix this →]
  3. Add Documentation: Include inline comments explaining the reth version and rationale

  4. Verify Image Availability: Confirm the image is accessible in all test environments

  5. Consider Centralization: Extract the tag to a shared constant if this pattern will be used long-term

Verdict

Conditional Approval: The code changes are technically correct and achieve the stated goal of validation. However, the approach needs refinement before merging to ensure maintainability and clarity.

The main decision point is whether this is temporary validation code (use env var) or a permanent upgrade (use stable tag + docs).


@github-actions
Copy link
Contributor

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedFeb 12, 2026, 11:01 AM

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the E2E tests to use a specific Reth image tag (pr-106) for validation purposes. My review focuses on the maintainability implications of this change. While using a specific tag is fine for a work-in-progress change, I've added comments to highlight that these hardcoded tags should be removed or made configurable before merging. This will prevent the test suite from depending on temporary build artifacts and ensure long-term stability.

dockerCli, dockerNetID := getTestScopedDockerSetup(t)

n, err := reth.NewNodeBuilderWithTestName(t, fmt.Sprintf("%s-%s", t.Name(), randomString(6))).
WithTag("pr-106").
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Hardcoding the image tag pr-106 makes the test suite dependent on a temporary build artifact. Since this PR is a work-in-progress for testing, this is understandable, but this line should be removed before merging. For future flexibility, consider making the tag configurable via an environment variable, a pattern seen elsewhere in the test suite.

func (s *DockerTestSuite) SetupRethNode(ctx context.Context, name string) RethSetupConfig {
rethNode, err := reth.NewNodeBuilder(s.T()).
WithName(name).
WithTag("pr-106").
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This hardcoded tag pr-106 appears to be for temporary testing. Please consider making this configurable via an environment variable, similar to how getEvNodeImage handles image tags. This will improve maintainability and prevent pinning tests to a temporary build. This line should be removed before merging if the custom tag is not needed for the default test run.

@codecov
Copy link

codecov bot commented Feb 12, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.89%. Comparing base (b92e97e) to head (760b747).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3066      +/-   ##
==========================================
- Coverage   61.92%   61.89%   -0.04%     
==========================================
  Files         111      111              
  Lines       11113    11113              
==========================================
- Hits         6882     6878       -4     
- Misses       3492     3496       +4     
  Partials      739      739              
Flag Coverage Δ
combined 61.89% <ø> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant