Fix integration tests: anvil reliability & celo-alfajores migration#34
Merged
Fix integration tests: anvil reliability & celo-alfajores migration#34
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReworked CLI flag comments and viper binding for network/namespace, replaced a fixed sleep with RPC-health polling and switched RPC host to 127.0.0.1 in anvil, tightened spinner/solc output normalization, updated foundry test config, and refreshed many golden logs and deployment hashes (artifact/compiler updates and added addressbook load logs). Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/cli/list.go`:
- Line 85: The network flag is registered with cmd.Flags().StringP but never
bound or read; declare a local variable (e.g., network string), replace or add a
binding using cmd.Flags().StringVarP(&network, "network", "n", "", "Network to
run on...") (or use StringVar if consistent), and in the RunE handler check if
network != "" then set app.Config.Network = network before proceeding; locate
the flag registration and the RunE function in list.go and mirror the pattern
used in show.go for overriding app.Config.Network.
| // Add flags (removed namespace and chain - these come from runtime config) | ||
| // Add flags (namespace/network can override runtime config when provided) | ||
| cmd.Flags().StringVarP(&namespace, "namespace", "s", "", "Namespace to use (defaults to current context namespace) [also sets foundry profile]") | ||
| cmd.Flags().StringP("network", "n", "", "Network to run on (e.g., mainnet, sepolia, local)") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, explore the list.go file structure and the network flag
head -150 internal/cli/list.go | cat -n
echo "---"
# Check the RunE function in list.go to see if network is used
rg -n "RunE|network" internal/cli/list.goRepository: trebuchet-org/treb-cli
Length of output: 3816
🏁 Script executed:
#!/bin/bash
# Check how namespace is used in list.go (mentioned at line 15)
rg -n -B5 -A10 "namespace" internal/cli/list.goRepository: trebuchet-org/treb-cli
Length of output: 2161
🏁 Script executed:
#!/bin/bash
# Check other CLI commands to understand the pattern
find internal/cli -name "*.go" ! -name "*_test.go" -type f | head -10Repository: trebuchet-org/treb-cli
Length of output: 311
🏁 Script executed:
#!/bin/bash
# Search for network flag usage patterns across CLI commands
rg -n --type=go 'network.*StringP|GetString.*network' internal/cli/ --glob '!*_test.go'Repository: trebuchet-org/treb-cli
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check how show, tag, run commands handle the network flag
for cmd in show tag run; do
echo "=== $cmd.go ==="
rg -n -B2 -A10 "StringP.*network|StringVar.*network|GetString.*network" internal/cli/$cmd.go 2>/dev/null || echo "No network flag found"
doneRepository: trebuchet-org/treb-cli
Length of output: 1905
🏁 Script executed:
#!/bin/bash
# Check if there's a network variable declared in show, tag, or run
rg -n "var \(" internal/cli/show.go -A20 | head -40Repository: trebuchet-org/treb-cli
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Look for how app.Config.Network is set in other commands
rg -n "app.Config.Network" internal/cli/ --glob '!*_test.go'Repository: trebuchet-org/treb-cli
Length of output: 370
The network flag is registered but never read or used.
The network flag on Line 85 is declared with cmd.Flags().StringP(...) but is not bound to any variable and is never referenced in RunE. Unlike namespace (which uses StringVarP to bind to a variable and is referenced on line 45), the network flag value is inaccessible. The comment on line 43 incorrectly suggests network override is implemented, but only namespace is actually handled.
Wire up the network flag similar to how show.go implements it: declare a local network variable, use StringVar to bind the flag, and override app.Config.Network in RunE if the flag is provided.
🤖 Prompt for AI Agents
In `@internal/cli/list.go` at line 85, The network flag is registered with
cmd.Flags().StringP but never bound or read; declare a local variable (e.g.,
network string), replace or add a binding using cmd.Flags().StringVarP(&network,
"network", "n", "", "Network to run on...") (or use StringVar if consistent),
and in the RunE handler check if network != "" then set app.Config.Network =
network before proceeding; locate the flag registration and the RunE function in
list.go and mirror the pattern used in show.go for overriding
app.Config.Network.
…olden file updates - Fix OZGovernorSender test: rename buildProposalDescription to buildProposalMetadata - Fix anvil startup race condition: replace fixed 200ms sleep with health check retry loop - Fix anvil IPv6 issue: use 127.0.0.1 instead of localhost for RPC calls - Disable forge lint on build for test project (Proxy.sol uses legacy Solidity 0.5.17) - Migrate celo-alfajores to celo-sepolia (alfajores RPC endpoint no longer exists) - Add celo-sepolia explorer URL to network resolver - Update golden files for bytecode/gas changes from compiler updates Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace polygon-rpc.com (API key disabled) with polygon-bor-rpc.publicnode.com. Update golden files for solc 0.8.33, new addressbook log line, and library naming changes from treb-sol update. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
solofberlin
force-pushed
the
fix/integrationTestsSepolia
branch
from
e34043c to
20432a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/testdata/golden/integration/TestRunCommand/run_with_constructor_args/deployments.json.golden (1)
14-23:⚠️ Potential issue | 🟡 MinorInconsistent compiler version metadata.
The
initCodeHash(line 14) andbytecodeHash(line 23) have been updated to new values, but thecompilerVersionon line 22 still shows0.8.30+commit.73712a01. In other golden files (e.g.,run_multiple_contracts/deployments.json.golden), the compiler version was updated to0.8.33+commit.64118f21alongside the hash changes.If the hashes changed due to a compiler update, the
compilerVersionshould also be updated for consistency.Suggested fix
"artifact": { "path": "src/SampleToken.sol", - "compilerVersion": "0.8.30+commit.73712a01", + "compilerVersion": "0.8.33+commit.64118f21", "bytecodeHash": "0x02f2ffe05483bbfed0ba587658e23030b9e12b1b442c0ebfcea464d45fbb746d",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/testdata/golden/integration/TestRunCommand/run_with_constructor_args/deployments.json.golden` around lines 14 - 23, The artifact's compilerVersion is inconsistent with the updated hashes: update the "compilerVersion" field inside the "artifact" object (referenced as "artifact": { "path": "src/SampleToken.sol", "compilerVersion": ... }) to match the compiler that produced the new initCodeHash and bytecodeHash (e.g., change "0.8.30+commit.73712a01" to "0.8.33+commit.64118f21") so the metadata aligns with the changed initCodeHash and bytecodeHash entries.
♻️ Duplicate comments (1)
internal/cli/list.go (1)
83-85:⚠️ Potential issue | 🟠 MajorThe
networkflag is registered but never used.The comment on line 83 states "namespace/network can override runtime config when provided", and
namespaceis correctly wired up (lines 15, 44-46, 84). However, thenetworkflag on line 85 is still registered withStringPbut:
- Not bound to any variable
- Never read in
RunE- Never used to override
app.Config.NetworkEither wire up the network flag similar to namespace, or remove it to avoid confusing users who might expect it to work.
Suggested fix to wire up network flag
func NewListCmd() *cobra.Command { var ( namespace string + network string contractName string label string deployType string )// Override config if namespace/network flags are provided (same as show/tag/run) if namespace != "" { app.Config.Namespace = namespace } + if network != "" { + app.Config.Network = network + }// Add flags (namespace/network can override runtime config when provided) cmd.Flags().StringVarP(&namespace, "namespace", "s", "", "Namespace to use (defaults to current context namespace) [also sets foundry profile]") - cmd.Flags().StringP("network", "n", "", "Network to run on (e.g., mainnet, sepolia, local)") + cmd.Flags().StringVarP(&network, "network", "n", "", "Network to run on (e.g., mainnet, sepolia, local)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/list.go` around lines 83 - 85, The "network" flag is registered with cmd.Flags().StringP but not wired or used; declare a network string variable (like the existing namespace), change the flag registration to cmd.Flags().StringVarP(&network, "network", "n", "", "Network to run on...") and in the command's RunE handler (where namespace is processed) check if network != "" then set app.Config.Network = network (or otherwise apply it to the runtime config) before running the rest of RunE; alternatively remove the StringP registration if you intend not to support the flag.
🧹 Nitpick comments (1)
test/testdata/golden/integration/TestRunCommand/run_with_debug/commands.golden (1)
12-13: Stabilize volatile progress output in the golden snapshot.Line 12 and Line 13 include spinner frame churn and exact elapsed time (
3.53s), which can make this golden test flaky across CI/runtime environments. Prefer asserting a normalized final compile status line only.Proposed snapshot simplification
-[⠢] Compiling 104 files with Solc 0.8.33 ... (many spinner frames) - -[⠘] Solc 0.8.33 finished in 3.53s +[⠃] Compiling 104 files with Solc 0.8.33 +Solc 0.8.33 finished🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/testdata/golden/integration/TestRunCommand/run_with_debug/commands.golden` around lines 12 - 13, The golden snapshot contains volatile spinner frames and a precise elapsed time ("[⠘] Solc 0.8.33 finished in 3.53s") causing flakes; update the snapshot (commands.golden) to remove intermediate spinner lines and the timing detail and assert a single normalized final compile status line such as "[*] Solc 0.8.33 finished" or "[✓] Solc 0.8.33 finished" (i.e., keep the final completion text for "Solc 0.8.33 finished" but drop spinner churn and the "in X.XXs" duration).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/anvil/anvil.go`:
- Around line 134-144: The readiness loop may return an error leaving the
spawned anvil process and its PID file behind; update the timeout path after the
loop (where it currently returns fmt.Errorf("anvil not ready after 5s: %w",
healthErr)) to cleanly terminate the spawned process and remove its PID file
before returning: call the instance shutdown/kill logic (e.g., use the process
handle/method on the anvil instance that started the process) and delete the PID
file(s) the code creates (referencing the same PID filename used when starting
the process), then return the formatted error; ensure you use
inst.checkRPCHealth(), the same process handle stored on inst, and PID file
identifier to locate and remove the artifacts.
- Around line 468-470: The HTTP POST in makeRPCCallWithResponse currently uses
http.Post (no timeout), which can hang and violate the startup 5s guarantee used
by checkRPCHealth; replace the call with an http.Client (or context.WithTimeout)
that enforces a short timeout (e.g., matching the startup window) and use
client.Do to perform the request, ensuring errors/timeouts are properly handled
before returning from makeRPCCallWithResponse.
---
Outside diff comments:
In
`@test/testdata/golden/integration/TestRunCommand/run_with_constructor_args/deployments.json.golden`:
- Around line 14-23: The artifact's compilerVersion is inconsistent with the
updated hashes: update the "compilerVersion" field inside the "artifact" object
(referenced as "artifact": { "path": "src/SampleToken.sol", "compilerVersion":
... }) to match the compiler that produced the new initCodeHash and bytecodeHash
(e.g., change "0.8.30+commit.73712a01" to "0.8.33+commit.64118f21") so the
metadata aligns with the changed initCodeHash and bytecodeHash entries.
---
Duplicate comments:
In `@internal/cli/list.go`:
- Around line 83-85: The "network" flag is registered with cmd.Flags().StringP
but not wired or used; declare a network string variable (like the existing
namespace), change the flag registration to cmd.Flags().StringVarP(&network,
"network", "n", "", "Network to run on...") and in the command's RunE handler
(where namespace is processed) check if network != "" then set
app.Config.Network = network (or otherwise apply it to the runtime config)
before running the rest of RunE; alternatively remove the StringP registration
if you intend not to support the flag.
---
Nitpick comments:
In
`@test/testdata/golden/integration/TestRunCommand/run_with_debug/commands.golden`:
- Around line 12-13: The golden snapshot contains volatile spinner frames and a
precise elapsed time ("[⠘] Solc 0.8.33 finished in 3.53s") causing flakes;
update the snapshot (commands.golden) to remove intermediate spinner lines and
the timing detail and assert a single normalized final compile status line such
as "[*] Solc 0.8.33 finished" or "[✓] Solc 0.8.33 finished" (i.e., keep the
final completion text for "Solc 0.8.33 finished" but drop spinner churn and the
"in X.XXs" duration).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
internal/cli/list.gopkg/anvil/anvil.gotest/testdata/golden/integration/TestComposeCommand/with_config/commands.goldentest/testdata/golden/integration/TestComposeResume/compose_with_failure_and_resume/commands.goldentest/testdata/golden/integration/TestGovernorCommand/deploy_governance_infrastructure/commands.goldentest/testdata/golden/integration/TestGovernorCommand/deploy_governance_infrastructure/deployments.json.goldentest/testdata/golden/integration/TestInitCommand/init_and_deploy/commands.goldentest/testdata/golden/integration/TestListCommand/list_multiple_contract_types/deployments.json.goldentest/testdata/golden/integration/TestListCommand/list_with_tags/deployments.json.goldentest/testdata/golden/integration/TestRunCommand/run_deploy_with_library/commands.goldentest/testdata/golden/integration/TestRunCommand/run_dry_run/commands.goldentest/testdata/golden/integration/TestRunCommand/run_library_deployment/commands.goldentest/testdata/golden/integration/TestRunCommand/run_multiple_contracts/commands.goldentest/testdata/golden/integration/TestRunCommand/run_multiple_contracts/deployments.json.goldentest/testdata/golden/integration/TestRunCommand/run_redeployment_same_label/commands.goldentest/testdata/golden/integration/TestRunCommand/run_subdirectory_contract/commands.goldentest/testdata/golden/integration/TestRunCommand/run_upgradeable_contract/commands.goldentest/testdata/golden/integration/TestRunCommand/run_with_constructor_args/commands.goldentest/testdata/golden/integration/TestRunCommand/run_with_constructor_args/deployments.json.goldentest/testdata/golden/integration/TestRunCommand/run_with_custom_proxy/commands.goldentest/testdata/golden/integration/TestRunCommand/run_with_debug/commands.goldentest/testdata/golden/integration/TestRunCommand/run_with_env_vars/commands.goldentest/testdata/golden/integration/TestRunCommand/run_with_json_output/commands.goldentest/testdata/golden/integration/TestRunCommand/run_with_multiple_env_vars/commands.goldentest/testdata/golden/integration/TestRunCommand/run_with_namespace/commands.goldentest/testdata/golden/integration/TestRunCommand/run_with_network/commands.goldentest/testdata/golden/integration/TestRunCommand/simple/commands.goldentest/testdata/golden/integration/TestShowCommand/show_token_with_constructor_args/deployments.json.goldentest/testdata/golden/integration/TestSyncCommand/sync_with_deployments/deployments.json.goldentest/testdata/golden/integration/TestTagCommand/tag_multiple_contracts_different_tags/deployments.json.goldentest/testdata/golden/integration/TestWorkflow/complex_with_libraries/commands.goldentest/testdata/project/foundry.toml
✅ Files skipped from review due to trivial changes (7)
- test/testdata/golden/integration/TestRunCommand/run_with_network/commands.golden
- test/testdata/golden/integration/TestRunCommand/run_with_env_vars/commands.golden
- test/testdata/golden/integration/TestRunCommand/run_upgradeable_contract/commands.golden
- test/testdata/golden/integration/TestRunCommand/run_deploy_with_library/commands.golden
- test/testdata/golden/integration/TestRunCommand/run_with_multiple_env_vars/commands.golden
- test/testdata/golden/integration/TestListCommand/list_with_tags/deployments.json.golden
- test/testdata/golden/integration/TestRunCommand/run_with_custom_proxy/commands.golden
Comment on lines
+134
to
+144
| // Wait for anvil to be ready (up to 5 seconds) | ||
| var healthErr error | ||
| for i := 0; i < 25; i++ { | ||
| time.Sleep(200 * time.Millisecond) | ||
| if healthErr = inst.checkRPCHealth(); healthErr == nil { | ||
| break | ||
| } | ||
| } | ||
| if healthErr != nil { | ||
| return fmt.Errorf("anvil not ready after 5s: %w", healthErr) | ||
| } |
There was a problem hiding this comment.
Clean up anvil process/PID when readiness check times out.
If Line 142 triggers, the function returns but leaves the spawned process (and PID file) behind, which can poison subsequent test runs.
🔧 Proposed fix
if healthErr != nil {
+ _ = cmd.Process.Kill()
+ _ = os.Remove(inst.PidFile)
return fmt.Errorf("anvil not ready after 5s: %w", healthErr)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Wait for anvil to be ready (up to 5 seconds) | |
| var healthErr error | |
| for i := 0; i < 25; i++ { | |
| time.Sleep(200 * time.Millisecond) | |
| if healthErr = inst.checkRPCHealth(); healthErr == nil { | |
| break | |
| } | |
| } | |
| if healthErr != nil { | |
| return fmt.Errorf("anvil not ready after 5s: %w", healthErr) | |
| } | |
| // Wait for anvil to be ready (up to 5 seconds) | |
| var healthErr error | |
| for i := 0; i < 25; i++ { | |
| time.Sleep(200 * time.Millisecond) | |
| if healthErr = inst.checkRPCHealth(); healthErr == nil { | |
| break | |
| } | |
| } | |
| if healthErr != nil { | |
| _ = cmd.Process.Kill() | |
| _ = os.Remove(inst.PidFile) | |
| return fmt.Errorf("anvil not ready after 5s: %w", healthErr) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/anvil/anvil.go` around lines 134 - 144, The readiness loop may return an
error leaving the spawned anvil process and its PID file behind; update the
timeout path after the loop (where it currently returns fmt.Errorf("anvil not
ready after 5s: %w", healthErr)) to cleanly terminate the spawned process and
remove its PID file before returning: call the instance shutdown/kill logic
(e.g., use the process handle/method on the anvil instance that started the
process) and delete the PID file(s) the code creates (referencing the same PID
filename used when starting the process), then return the formatted error;
ensure you use inst.checkRPCHealth(), the same process handle stored on inst,
and PID file identifier to locate and remove the artifacts.
Comment on lines
+468
to
470
| httpResp, err := http.Post(fmt.Sprintf("http://127.0.0.1:%s", a.Port), "application/json", bytes.NewBuffer(jsonData)) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to make HTTP request: %w", err) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the actual code around lines 468-470
sed -n '465,475p' pkg/anvil/anvil.goRepository: trebuchet-org/treb-cli
Length of output: 458
🏁 Script executed:
# Check for timeout mechanisms in the entire function or nearby
sed -n '450,500p' pkg/anvil/anvil.go | cat -nRepository: trebuchet-org/treb-cli
Length of output: 1526
🏁 Script executed:
# Search for timeout usage patterns in the file
rg -n 'Timeout|time\.Duration|context\.' pkg/anvil/anvil.go | head -20Repository: trebuchet-org/treb-cli
Length of output: 48
🏁 Script executed:
# Check for context usage with timeout in the function signature or caller
rg -B 5 'makeRPCCallWithResponse|makeRPCCall' pkg/anvil/anvil.go | head -40Repository: trebuchet-org/treb-cli
Length of output: 818
🏁 Script executed:
# Search for any timeout or deadline context in the codebase
rg -n 'context\.(Background|TODO|WithTimeout|WithDeadline)' pkg/anvil/ --type goRepository: trebuchet-org/treb-cli
Length of output: 48
🏁 Script executed:
# Check if there's a startup timeout guarantee mentioned
rg -n '5.*second|timeout|deadline' pkg/anvil/anvil.goRepository: trebuchet-org/treb-cli
Length of output: 304
🏁 Script executed:
# Look at the full AnvilInstance struct and its methods
rg -A 30 'type AnvilInstance struct' pkg/anvil/anvil.goRepository: trebuchet-org/treb-cli
Length of output: 926
🏁 Script executed:
# Check where makeRPCCallWithResponse is called and if there's timeout context
rg -B 10 'makeRPCCallWithResponse' pkg/anvil/anvil.go | grep -A 10 -B 10 'makeRPCCall'Repository: trebuchet-org/treb-cli
Length of output: 856
🏁 Script executed:
# Check the startup code around line 134 to understand the 5 second guarantee
sed -n '130,150p' pkg/anvil/anvil.goRepository: trebuchet-org/treb-cli
Length of output: 737
🏁 Script executed:
# Check if there are any other HTTP client usages with timeout in the codebase
rg -n 'http\.Client.*Timeout|&http\.Client' pkg/anvil/Repository: trebuchet-org/treb-cli
Length of output: 48
🏁 Script executed:
# Verify the exact line numbers mentioned in the review
wc -l pkg/anvil/anvil.go && sed -n '468,470p' pkg/anvil/anvil.goRepository: trebuchet-org/treb-cli
Length of output: 284
Add timeout to HTTP RPC requests to honor startup guarantee.
Line 468 uses http.Post without a timeout in makeRPCCallWithResponse(). During startup, this function is called within the 5-second initialization window (via checkRPCHealth() at line 139). An indefinite HTTP hang will break the startup guarantee.
🔧 Proposed fix
+ client := &http.Client{Timeout: 1 * time.Second}
+ httpResp, err := client.Post(fmt.Sprintf("http://127.0.0.1:%s", a.Port), "application/json", bytes.NewBuffer(jsonData))
- httpResp, err := http.Post(fmt.Sprintf("http://127.0.0.1:%s", a.Port), "application/json", bytes.NewBuffer(jsonData))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| httpResp, err := http.Post(fmt.Sprintf("http://127.0.0.1:%s", a.Port), "application/json", bytes.NewBuffer(jsonData)) | |
| if err != nil { | |
| return fmt.Errorf("failed to make HTTP request: %w", err) | |
| client := &http.Client{Timeout: 1 * time.Second} | |
| httpResp, err := client.Post(fmt.Sprintf("http://127.0.0.1:%s", a.Port), "application/json", bytes.NewBuffer(jsonData)) | |
| if err != nil { | |
| return fmt.Errorf("failed to make HTTP request: %w", err) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/anvil/anvil.go` around lines 468 - 470, The HTTP POST in
makeRPCCallWithResponse currently uses http.Post (no timeout), which can hang
and violate the startup 5s guarantee used by checkRPCHealth; replace the call
with an http.Client (or context.WithTimeout) that enforces a short timeout
(e.g., matching the startup window) and use client.Do to perform the request,
ensuring errors/timeouts are properly handled before returning from
makeRPCCallWithResponse.
…normalizer - Remove manual namespace override in list.go — viper already binds the flag via SetupViper - Fix SpinnerNormalizer regex to match "Compiling N files with Solc X.Y.Z" not just "Compiling...", making debug test stable across compiler versions - Update run_with_debug golden file Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@test/testdata/golden/integration/TestRunCommand/run_with_debug/commands.golden`:
- Line 12: Add a normalizer that collapses compile/execution duration strings
like "finished in 1.68s" into a stable token (e.g., "<duration>") so tests don't
depend on fluctuating timing; implement a DurationNormalizer (or extend
SpinnerNormalizer) to match the regex pattern /finished in \d+\.\d+s/ (and
variants like \d+s) and replace the numeric portion, and register this new
normalizer alongside SpinnerNormalizer and TimestampNormalizer in the test
output normalizers list so the golden file line "[⠃] Solc 0.8.33 finished in
1.68s" becomes deterministic.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/cli/list.gotest/helpers/normalizers.gotest/testdata/golden/integration/TestRunCommand/run_with_debug/commands.golden
test/testdata/golden/integration/TestRunCommand/run_with_debug/commands.golden
Outdated
Show resolved
Hide resolved
The "Solc X.Y.Z finished in N.NNs" line also has a non-deterministic spinner char and timing value. Normalize it alongside the compiling spinner frames. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use gotestsum --rerun-fails to automatically retry failed tests once, capped at 5 failures max. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
buildProposalDescription→buildProposalMetadata)127.0.0.1instead oflocalhostfor RPC callsProxy.soluses legacy Solidity 0.5.17 syntax)celo-alfajorestocelo-sepolia(alfajores RPC endpoint no longer exists)Test plan
make integration-test)make unit-test)go vet ./...clean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores