fix: run health check and try multiple RPCs in fork tests#708
fix: run health check and try multiple RPCs in fork tests#708gustavogama-cll wants to merge 1 commit intomainfrom
Conversation
🦋 Changeset detectedLatest commit: d7130f1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
gustavogama-cll
force-pushed
the
ggama/fix/fork-tests
branch
from
c09b4bd to
d7130f1
Compare
|
There was a problem hiding this comment.
Pull request overview
This PR enhances the fork test infrastructure to improve reliability when working with public RPCs. It implements health checking of RPCs before selection and enables retry logic with multiple RPC URLs when Anvil container startup fails.
Changes:
- Added health check functionality to validate RPC availability before use
- Implemented round-robin RPC selection when container startup fails
- Removed validation requirement for pre-configured archive HTTP URLs
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Added httpmock dependency for testing HTTP interactions |
| engine/cld/environment/anvil_test.go | Updated tests to verify health checks and multiple RPC handling with mock HTTP responses |
| engine/cld/environment/anvil.go | Added health check function and modified selectPublicRPC to return multiple healthy URLs |
| engine/cld/config/network/metadata_test.go | Removed test case for archive HTTP URL validation |
| engine/cld/config/network/metadata.go | Removed validation requiring archive HTTP URL |
| chain/evm/provider/ctf_anvil_provider.go | Added ForkURLs field and retry logic to cycle through URLs on container startup failure |
| .changeset/true-lizards-search.md | Added changeset entry for the fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| func Test_selectPublicRPC(t *testing.T) { | ||
| t.Parallel() | ||
| httpmock.Activate(t) |
There was a problem hiding this comment.
The httpmock.Activate(t) call activates mocking globally for all tests, but lacks a corresponding Deactivate call. This will cause mock responses to leak into other parallel tests. Add t.Cleanup(httpmock.DeactivateAndReset) after activation to ensure proper cleanup.
| httpmock.Activate(t) | |
| httpmock.Activate(t) | |
| t.Cleanup(httpmock.DeactivateAndReset) |
| client, err := ethclient.DialContext(ctx, rpcURL) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to connect to rpc %v: %w", rpcURL, err) | ||
| } |
There was a problem hiding this comment.
The ethclient connection is never closed, creating a resource leak. Add defer client.Close() after successful dial to ensure proper cleanup of the connection.
| } | |
| } | |
| defer client.Close() |
| if len(p.config.ForkURLs) > 0 { | ||
| url := p.config.ForkURLs[attempt%uint(len(p.config.ForkURLs))] | ||
| p.config.DockerCmdParamsOverrides = append(p.config.DockerCmdParamsOverrides, "--fork-url", url) | ||
| } |
There was a problem hiding this comment.
The DockerCmdParamsOverrides slice is being mutated on every retry attempt, causing --fork-url parameters to accumulate. Each retry will append new fork-url arguments without clearing previous ones. Store the base overrides separately and create a fresh slice for each attempt, or clear the fork-url entries before appending.
| // gas limits, or other Anvil-specific options. | ||
| DockerCmdParamsOverrides []string | ||
|
|
||
| ForkURLs []string |
There was a problem hiding this comment.
should we add some godoc for this?
There was a problem hiding this comment.
yeah, let me add it
| lggr.Infow("selected rpc for fork environment", "url", rpc.HTTPURL, "chainSelector", chainSelector) | ||
|
|
||
| return nil | ||
| err := runHealthCheck(ctx, rpc.HTTPURL) |
There was a problem hiding this comment.
i wonder if it is worth parallelizing here instead of doing sequential health check?
There was a problem hiding this comment.
the health checks I ran failed fast, so I didn't bother. We can reassess once we get a bit more real world usage.
|
|
||
| if len(p.config.ForkURLs) > 0 { | ||
| url := p.config.ForkURLs[attempt%uint(len(p.config.ForkURLs))] | ||
| p.config.DockerCmdParamsOverrides = append(p.config.DockerCmdParamsOverrides, "--fork-url", url) |
There was a problem hiding this comment.
so everytime this runs, we will append another --fork-url into p.config.DockerCmdParamsOverrides, so then DockerCmdParamsOverrides will have more than one --fork-url ?
There was a problem hiding this comment.
ooops; good catch. The earlier tests didn't have the health check, so this code path wasn't re-tested
2 participants
Modify the anvil provider and the fork test setup so that (1) we do a health check on public rpcs before selecting them; (2) we try different rpcs if the anvil container fails to start (using a simple round-robin algorithm).