Refactor calibnet wallet test#6983
Open
sudo-shashank wants to merge 20 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughBash calibnet wallet integration checks were replaced by a Rust test suite. New helpers and tests under ChangesWallet Integration Test Migration
Sequence DiagramsequenceDiagram
participant CI as CI (GitHub Actions)
participant Harness as Harness (harness.sh)
participant Test as Rust Test Runner
participant Helpers as Helpers (calibnet_wallet_helpers.rs)
participant Wallet as forest-wallet CLI
participant RPC as Forest RPC Node
CI->>Harness: forest_wallet_init(preloaded_key)
Harness->>Wallet: import preloaded_wallet.key
Wallet-->>Harness: PRELOADED_ADDRESS
Harness-->>CI: export PRELOADED_ADDRESS
CI->>Test: cargo test --ignored (calibnet_wallet)
Test->>Helpers: export_to_temp_file(address, Backend)
Helpers->>Wallet: forest-wallet export <address>
Wallet-->>Helpers: wallet.json
Test->>Helpers: send_from(preloaded, target, amount, Backend)
Helpers->>Wallet: forest-wallet send --from ... --to ...
Wallet->>RPC: broadcast message
RPC-->>Wallet: message CID
Wallet-->>Helpers: CID
loop poll for funding / change
Helpers->>Wallet: forest-wallet balance <address>
Wallet->>RPC: StateGetActor / StateReadState
RPC-->>Wallet: balance
Wallet-->>Helpers: balance response
end
Helpers->>RPC: Filecoin.StateSearchMsg (poll)
RPC-->>Helpers: message mined
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/common/calibnet_wallet_helpers.rs (1)
127-130: ⚡ Quick winCheck first, then sleep in polling loops.
Both loops sleep before the first attempt, which adds guaranteed delay to every successful call path. Run
checkimmediately, then sleep only between retries.Suggested refactor
pub async fn poll<F>(what: &str, mut check: F) -> anyhow::Result<String> where F: FnMut() -> anyhow::Result<Option<String>>, { for i in 1..=POLL_RETRIES { eprintln!("Polling {what} {i}/{POLL_RETRIES}"); - tokio::time::sleep(POLL_DELAY).await; if let Some(value) = check()? { return Ok(value); } + if i < POLL_RETRIES { + tokio::time::sleep(POLL_DELAY).await; + } } bail!("Timed out waiting for {what} after {POLL_RETRIES} retries") } pub async fn poll_until_state_search_msg(msg_cid: &str) -> anyhow::Result<()> { for i in 1..=SEARCH_MSG_RETRIES { - tokio::time::sleep(SEARCH_MSG_DELAY).await; eprintln!("StateSearchMsg polling {msg_cid} attempt {i}/{SEARCH_MSG_RETRIES}"); let params = json!([[], { "/": msg_cid }, 800_i64, true]); if rpc_call_opt("Filecoin.StateSearchMsg", params) .await? .is_some() { return Ok(()); } + if i < SEARCH_MSG_RETRIES { + tokio::time::sleep(SEARCH_MSG_DELAY).await; + } }Also applies to: 262-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/common/calibnet_wallet_helpers.rs` around lines 127 - 130, The polling loops currently sleep before the first check (using POLL_DELAY in the for i in 1..=POLL_RETRIES loop), which adds unnecessary delay; update the loop(s) in tests/common/calibnet_wallet_helpers.rs (the loop around the check() call and the similar occurrence at the region noted around lines 262-264) to call check() immediately and return on success, and move tokio::time::sleep(POLL_DELAY).await to run only between retries (e.g., after a failed check and not on the final iteration) so the first attempt is immediate and subsequent attempts are delayed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/calibnet_wallet.rs`:
- Line 52: The tests are always invoking send_from with the remote flag
hardcoded true, so the local-wallet code path is never exercised; update the
test invocations of send_from (e.g., calls currently like
send_from(PRELOADED_ADDRESS, &target, FIL_AMT, true)) to pass remote = false for
tests named send_to_local_* and delegated_send_local_* (and the other affected
invocations), ensuring the local-send path in send_from is executed while
keeping PRELOADED_ADDRESS and other arguments unchanged.
---
Nitpick comments:
In `@tests/common/calibnet_wallet_helpers.rs`:
- Around line 127-130: The polling loops currently sleep before the first check
(using POLL_DELAY in the for i in 1..=POLL_RETRIES loop), which adds unnecessary
delay; update the loop(s) in tests/common/calibnet_wallet_helpers.rs (the loop
around the check() call and the similar occurrence at the region noted around
lines 262-264) to call check() immediately and return on success, and move
tokio::time::sleep(POLL_DELAY).await to run only between retries (e.g., after a
failed check and not on the final iteration) so the first attempt is immediate
and subsequent attempts are delayed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f56951c0-4974-48c8-9313-4b1dd6cd5e74
📒 Files selected for processing (5)
.github/workflows/forest.ymlscripts/tests/calibnet_delegated_wallet_check.shscripts/tests/calibnet_wallet_check.shtests/calibnet_wallet.rstests/common/calibnet_wallet_helpers.rs
💤 Files with no reviewable changes (2)
- scripts/tests/calibnet_delegated_wallet_check.sh
- .github/workflows/forest.yml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/common/calibnet_wallet_helpers.rs (2)
120-148: ⚡ Quick win
send_fromusesstd::thread::sleep— blocks the tokio worker thread if called fromasynctest code.
std::thread::sleep(SEND_RETRY_DELAY)with a 15-second delay will park the OS thread. When called inside a#[tokio::test]body, this starves the runtime of a worker thread for up to(SEND_RETRIES - 1) * 15s = 30s. Replace withtokio::time::sleepand make the functionasync.♻️ Convert `send_from` to async
-pub fn send_from(from: &str, to: &str, amount: &str, backend: Backend) -> anyhow::Result<String> { +pub async fn send_from(from: &str, to: &str, amount: &str, backend: Backend) -> anyhow::Result<String> { let args = ["send", "--from", from, to, amount]; let mut attempt = 1; loop { - match wallet(backend, &args) { + match wallet(backend, &args).await { Ok(out) => return Ok(out), Err(e) if attempt < SEND_RETRIES && is_min_gas_price_error(&e) => { eprintln!( "send {from} -> {to} hit min-gas-price floor on attempt {attempt}/{SEND_RETRIES}, retrying" ); - std::thread::sleep(SEND_RETRY_DELAY); + tokio::time::sleep(SEND_RETRY_DELAY).await; attempt += 1; } Err(e) => return Err(e), } } }(Requires
walletto also beasyncper the previous comment's suggestion.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/common/calibnet_wallet_helpers.rs` around lines 120 - 148, The send_from function currently blocks the tokio runtime by calling std::thread::sleep; change send_from to an async function (pub async fn send_from(...)) and replace std::thread::sleep(SEND_RETRY_DELAY) with tokio::time::sleep(SEND_RETRY_DELAY).await; also propagate async changes to wallet by calling an async wallet(backend, &args).await (update its signature to async if not already) and update all callers/tests to await send_from; keep the retry logic and is_min_gas_price_error unchanged.
74-94: ⚡ Quick winBlocking subprocess call inside async
pollloop blocks a tokio worker thread.
run_wallet_rawusesstd::process::Command::output()— a blocking I/O call. It is invoked synchronously viabalance()→wallet()→run_wallet_raw()inside the synchronous closure passed to the asyncpollfunction. Eachcheck()call inpollblocks the tokio worker thread for the subprocess duration without yielding.The idiomatic fix is to use
tokio::process::Commandinrun_wallet_raw(making it async), or wrap the synchronous call withtokio::task::spawn_blockingat the call sites. Based on coding guidelines: usetokio::task::spawn_blockingfor blocking operations called from async contexts.♻️ Minimal fix: wrap blocking call in `run_wallet_raw`
-pub fn run_wallet_raw(backend: Backend, args: &[&str]) -> anyhow::Result<Vec<u8>> { - let _guard = (backend == Backend::Local).then(|| LOCAL_KEYSTORE_LOCK.lock()); - - let mut full = Vec::with_capacity(backend.extra_args().len() + args.len()); - full.extend_from_slice(backend.extra_args()); - full.extend_from_slice(args); - - let output = Command::new("forest-wallet") - .args(&full) - .output() - .context("failed to spawn `forest-wallet`")?; +pub async fn run_wallet_raw(backend: Backend, args: &[&str]) -> anyhow::Result<Vec<u8>> { + let extra: &[&str] = backend.extra_args(); + let mut full: Vec<String> = Vec::with_capacity(extra.len() + args.len()); + full.extend(extra.iter().map(|s| s.to_string())); + full.extend(args.iter().map(|s| s.to_string())); + + let _guard = (backend == Backend::Local).then(|| LOCAL_KEYSTORE_LOCK.lock()); + let output = tokio::process::Command::new("forest-wallet") + .args(&full) + .output() + .await + .context("failed to spawn `forest-wallet`")?;This also requires making
wallet,balance, and derived helpersasync fn, and updatingpoll's closure bound toasync.Alternatively, keep the synchronous API and wrap calls in
tokio::task::spawn_blockingat thepolllevel.Also applies to: 153-165
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/common/calibnet_wallet_helpers.rs` around lines 74 - 94, The blocking std::process::Command::output() in run_wallet_raw is being called inside async poll code and must not block a tokio worker — change run_wallet_raw to perform the blocking subprocess on a blocking thread: make run_wallet_raw an async fn and wrap the existing Command::new(...).output() call inside tokio::task::spawn_blocking (await the JoinHandle and propagate errors), then update the callers wallet() and balance() to be async and adjust the closure passed into poll/check to be async so it can await balance()/wallet(); alternatively if you prefer to keep run_wallet_raw sync, wrap the current run_wallet_raw(...) calls at the poll/check call sites with tokio::task::spawn_blocking(), but do not leave std::process::Command::output() running on the tokio worker thread.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/common/calibnet_wallet_helpers.rs`:
- Line 192: The global HTTP client HTTP is created with reqwest::Client::new()
which has no timeout and can hang tests; change the initialization of the static
LazyLock<reqwest::Client> named HTTP to use reqwest::Client::builder() with a
per-request timeout (e.g. Duration::from_secs(30) or configurable from an env
var) so rpc_call and rpc_call_opt cannot block indefinitely; add the necessary
std::time::Duration import and ensure the builder finishes with
.build().expect(...) so the static still constructs reliably.
---
Nitpick comments:
In `@tests/common/calibnet_wallet_helpers.rs`:
- Around line 120-148: The send_from function currently blocks the tokio runtime
by calling std::thread::sleep; change send_from to an async function (pub async
fn send_from(...)) and replace std::thread::sleep(SEND_RETRY_DELAY) with
tokio::time::sleep(SEND_RETRY_DELAY).await; also propagate async changes to
wallet by calling an async wallet(backend, &args).await (update its signature to
async if not already) and update all callers/tests to await send_from; keep the
retry logic and is_min_gas_price_error unchanged.
- Around line 74-94: The blocking std::process::Command::output() in
run_wallet_raw is being called inside async poll code and must not block a tokio
worker — change run_wallet_raw to perform the blocking subprocess on a blocking
thread: make run_wallet_raw an async fn and wrap the existing
Command::new(...).output() call inside tokio::task::spawn_blocking (await the
JoinHandle and propagate errors), then update the callers wallet() and balance()
to be async and adjust the closure passed into poll/check to be async so it can
await balance()/wallet(); alternatively if you prefer to keep run_wallet_raw
sync, wrap the current run_wallet_raw(...) calls at the poll/check call sites
with tokio::task::spawn_blocking(), but do not leave
std::process::Command::output() running on the tokio worker thread.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: acde64a4-1e8e-4efc-90b8-a95a5264725d
📒 Files selected for processing (6)
.github/workflows/forest.ymlmise.tomlscripts/tests/calibnet_wallet_check.shscripts/tests/harness.shtests/calibnet_wallet.rstests/common/calibnet_wallet_helpers.rs
💤 Files with no reviewable changes (1)
- scripts/tests/calibnet_wallet_check.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/calibnet_wallet.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/forest.yml (1)
264-271:cargo testcompiles from scratch in this job — may be tight against the 30-minute timeoutThe job downloads only the pre-built
forest*binaries frombuild-ubuntu; thecalibnet_wallettest binary is compiled fresh here. With a cold sccache (first run, or cache miss), compilation plus test execution could approach theSCRIPT_TIMEOUT_MINUTESlimit. This is purely an operational concern — if it becomes a problem, consider either: (a) uploading Cargo build cache artifacts frombuild-ubuntu, or (b) increasing the timeout for this specific step.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/forest.yml around lines 264 - 271, The "Wallet commands check" job step may recompile the calibnet_wallet test from scratch and risk hitting the 30-minute SCRIPT_TIMEOUT_MINUTES limit; update this step to avoid cold Cargo builds by either (A) restoring/uploading Cargo build cache/artifacts from the earlier build job (e.g., export/import target and Cargo registry/cache or the compiled calibnet_wallet binary from the "build-ubuntu" job) so the test binary isn't rebuilt, or (B) increase the timeout specifically for this step (the step named "Wallet commands check") by raising timeout-minutes to a larger value to accommodate a full cargo build and test run.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/tests/harness.sh`:
- Line 159: The line exporting PRELOADED_ADDRESS masks the exit status of the
command substitution; change it to run the import command and assign its output
to PRELOADED_ADDRESS first (e.g., run "$FOREST_WALLET_PATH import
preloaded_wallet.key" and capture the output), check the command's exit status
and fail early if it failed, and only then export PRELOADED_ADDRESS so failures
in FOREST_WALLET_PATH or the import are not silenced; update references to
PRELOADED_ADDRESS accordingly in the script.
---
Nitpick comments:
In @.github/workflows/forest.yml:
- Around line 264-271: The "Wallet commands check" job step may recompile the
calibnet_wallet test from scratch and risk hitting the 30-minute
SCRIPT_TIMEOUT_MINUTES limit; update this step to avoid cold Cargo builds by
either (A) restoring/uploading Cargo build cache/artifacts from the earlier
build job (e.g., export/import target and Cargo registry/cache or the compiled
calibnet_wallet binary from the "build-ubuntu" job) so the test binary isn't
rebuilt, or (B) increase the timeout specifically for this step (the step named
"Wallet commands check") by raising timeout-minutes to a larger value to
accommodate a full cargo build and test run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7f209665-b51c-4532-bfe2-f85bbb8cc7bc
📒 Files selected for processing (6)
.github/workflows/forest.ymlmise.tomlscripts/tests/calibnet_wallet_check.shscripts/tests/harness.shtests/calibnet_wallet.rstests/common/calibnet_wallet_helpers.rs
💤 Files with no reviewable changes (1)
- scripts/tests/calibnet_wallet_check.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/calibnet_wallet.rs
- tests/common/calibnet_wallet_helpers.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mise.toml`:
- Line 219: The test invocation now runs with default parallel threads causing
nonce-safety races for tests in calibnet_wallet.rs that reuse PRELOADED_ADDRESS;
update the cargo test command invocation (the line containing "cargo test
--profile quick-test --test calibnet_wallet -- --ignored --nocapture") to
include "--test-threads=1" so tests run single-threaded and avoid concurrent
sends from PRELOADED_ADDRESS.
In `@tests/calibnet_wallet.rs`:
- Around line 65-77: Tests that call send_from(&PRELOADED_ADDRESS, ...) can race
on nonce when Backend::Local uses the non-atomic MpoolGetNonce → sign →
MpoolPush flow (MpoolGetNonce / MpoolPush), causing duplicate nonces; fix by
either serializing test execution (add --test-threads=1 to the test invocation
in mise.toml) or change the local wallet send path to use the same atomic push
used by remote (MpoolPushMessage) — if you choose the latter, update the
forest-wallet send implementation to call MpoolPushMessage for Backend::Local
and adapt its API/signing flow so the keystore signing is preserved (modify the
send function to accept pre-signed or signing-callback semantics compatible with
MpoolPushMessage).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 47e4ba02-8a0f-4481-9825-cb5d10292527
📒 Files selected for processing (6)
.github/workflows/forest.ymlmise.tomlscripts/tests/calibnet_wallet_check.shscripts/tests/harness.shtests/calibnet_wallet.rstests/common/calibnet_wallet_helpers.rs
💤 Files with no reviewable changes (1)
- scripts/tests/calibnet_wallet_check.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/common/calibnet_wallet_helpers.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mise.toml`:
- Around line 216-218: The script enables xtrace which can leak the preloaded
private key when calling forest_wallet_init with the usage_preloaded_key
variable; remove the xtrace flag or disable tracing around that call by changing
the initial shell flags so they do not include -x (e.g., use set -euo pipefail)
or temporarily turn off tracing before invoking forest_wallet_init and re-enable
it afterwards, ensuring forest_wallet_init and the usage_preloaded_key value are
never printed to the logs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7d88333a-c329-48c7-9f19-981047054a71
📒 Files selected for processing (6)
.github/workflows/forest.ymlmise.tomlscripts/tests/calibnet_wallet_check.shscripts/tests/harness.shtests/calibnet_wallet.rstests/common/calibnet_wallet_helpers.rs
💤 Files with no reviewable changes (1)
- scripts/tests/calibnet_wallet_check.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/forest.yml
- tests/common/calibnet_wallet_helpers.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mise.toml (1)
219-219:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSerialize this test target to avoid nonce races between cases.
calibnet_walletstill runs with the defaultcargo testthread count, but several ignored cases reuse the same funded sender. That makes the Rust replacement less stable than the old sequential shell flow and can trigger nondeterministic local-wallet send failures. Please add--test-threads=1here, or otherwise serialize this target.🛡️ Minimal fix
-cargo test --profile quick-test --test calibnet_wallet -- --ignored --nocapture +cargo test --profile quick-test --test calibnet_wallet -- --ignored --nocapture --test-threads=1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mise.toml` at line 219, The calibnet_wallet test invocation in mise.toml runs with parallel threads causing nonce races; update the cargo test command that targets "calibnet_wallet" to serialize execution by adding --test-threads=1 (i.e., change the existing line "cargo test --profile quick-test --test calibnet_wallet -- --ignored --nocapture" to include --test-threads=1) so the ignored cases reuse the funded sender sequentially and avoid nondeterministic wallet send failures.
🧹 Nitpick comments (1)
tests/common/calibnet_wallet_helpers.rs (1)
81-84: ⚡ Quick winHonor
FOREST_WALLET_PATHhere for parity with the harness.This hardcodes
forest-wallet, so overridingFOREST_WALLET_PATHinscripts/tests/harness.shno longer affects the Rust tests. Reading the env var here keeps the new test path aligned with the existing harness behavior.♻️ Minimal adjustment
- let output = Command::new("forest-wallet") + let wallet_bin = std::env::var("FOREST_WALLET_PATH") + .ok() + .filter(|value| !value.is_empty()) + .unwrap_or_else(|| "forest-wallet".to_owned()); + let output = Command::new(&wallet_bin) .args(&full) .output() .context("failed to spawn `forest-wallet`")?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/common/calibnet_wallet_helpers.rs` around lines 81 - 84, The test hardcodes "forest-wallet" in the Command::new call so FOREST_WALLET_PATH is ignored; change the Command::new invocation in tests/common/calibnet_wallet_helpers.rs to read the executable path from the FOREST_WALLET_PATH environment variable (e.g., via std::env::var or var_os) and fall back to "forest-wallet" if the env var is not set, then pass that path to Command::new instead of the literal "forest-wallet" so the harness override is honored.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/common/calibnet_wallet_helpers.rs`:
- Around line 153-165: The poll function (and the similar retry block at
275-289) currently uses the `?` operator inside the retry loop which aborts on
the first transient CLI/RPC error; change the loop to capture errors from
`check()` instead of propagating them: call `check()` inside the loop, on
Ok(Some(v)) return Ok(v), on Ok(None) continue retrying, on Err(e) store the
error into a `last_err` variable and continue (do not return); after the retries
are exhausted, if `last_err` is Some(err) return that error (or wrap it with
context), otherwise bail with the existing timeout message; update both `poll`
and the other retry helper at lines 275-289 to use this same pattern so
transient failures are retried.
---
Duplicate comments:
In `@mise.toml`:
- Line 219: The calibnet_wallet test invocation in mise.toml runs with parallel
threads causing nonce races; update the cargo test command that targets
"calibnet_wallet" to serialize execution by adding --test-threads=1 (i.e.,
change the existing line "cargo test --profile quick-test --test calibnet_wallet
-- --ignored --nocapture" to include --test-threads=1) so the ignored cases
reuse the funded sender sequentially and avoid nondeterministic wallet send
failures.
---
Nitpick comments:
In `@tests/common/calibnet_wallet_helpers.rs`:
- Around line 81-84: The test hardcodes "forest-wallet" in the Command::new call
so FOREST_WALLET_PATH is ignored; change the Command::new invocation in
tests/common/calibnet_wallet_helpers.rs to read the executable path from the
FOREST_WALLET_PATH environment variable (e.g., via std::env::var or var_os) and
fall back to "forest-wallet" if the env var is not set, then pass that path to
Command::new instead of the literal "forest-wallet" so the harness override is
honored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5627ba94-4804-4ce5-ab6b-46821ab60e41
📒 Files selected for processing (6)
.github/workflows/forest.ymlmise.tomlscripts/tests/calibnet_wallet_check.shscripts/tests/harness.shtests/calibnet_wallet.rstests/common/calibnet_wallet_helpers.rs
💤 Files with no reviewable changes (1)
- scripts/tests/calibnet_wallet_check.sh
sudo-shashank
requested review from
LesnyRumcajs and
hanabi1224
and removed request for
a team
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/common/calibnet_wallet_helpers.rs (1)
153-165:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTransient
check()errors still abort the poll loop immediately (unresolved from prior review).
check()?at line 160 propagates the first error returned by the closure, so a single transientforest-walletprocess failure (daemon briefly unavailable, spawn error, etc.) fails the test immediately instead of retrying. The original bash scripts continued the sleep loop on non-zero exits. The same issue applies topoll_until_state_search_msgat line 281 (await?).🛡️ Suggested fix
pub async fn poll<F>(what: &str, mut check: F) -> anyhow::Result<String> where F: FnMut() -> anyhow::Result<Option<String>>, { + let mut last_err: Option<anyhow::Error> = None; for i in 1..=POLL_RETRIES { eprintln!("Polling {what} {i}/{POLL_RETRIES}"); tokio::time::sleep(POLL_DELAY).await; - if let Some(value) = check()? { - return Ok(value); + match check() { + Ok(Some(value)) => return Ok(value), + Ok(None) => {} + Err(e) => { + eprintln!("Polling {what}: transient error on attempt {i}: {e}"); + last_err = Some(e); + } } } - bail!("Timed out waiting for {what} after {POLL_RETRIES} retries") + if let Some(e) = last_err { + return Err(e).with_context(|| format!("last error while polling {what}")); + } + bail!("Timed out waiting for {what} after {POLL_RETRIES} retries") }Apply the same pattern to
poll_until_state_search_msg:for i in 1..=SEARCH_MSG_RETRIES { tokio::time::sleep(SEARCH_MSG_DELAY).await; eprintln!("StateSearchMsg polling {msg_cid} attempt {i}/{SEARCH_MSG_RETRIES}"); let params = json!([[], { "/": msg_cid }, 800_i64, true]); - if rpc_call_opt("Filecoin.StateSearchMsg", params) - .await? - .is_some() - { - return Ok(()); + match rpc_call_opt("Filecoin.StateSearchMsg", params).await { + Ok(Some(_)) => return Ok(()), + Ok(None) => {} + Err(e) => eprintln!("StateSearchMsg transient error on attempt {i}: {e}"), } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/common/calibnet_wallet_helpers.rs` around lines 153 - 165, The poll function (pub async fn poll) currently uses check()? which propagates any error from the closure and aborts retries; change it to call check() and match on its Result so transient errors are treated like a missing value (log or ignore and continue sleeping/retrying) instead of returning Err immediately, i.e., map Err(_) to continue the loop and only return Ok(value) when check() yields Ok(Some(_)), and apply the same pattern to poll_until_state_search_msg so its await? does not propagate transient errors but instead retries until POLL_RETRIES are exhausted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@tests/common/calibnet_wallet_helpers.rs`:
- Around line 153-165: The poll function (pub async fn poll) currently uses
check()? which propagates any error from the closure and aborts retries; change
it to call check() and match on its Result so transient errors are treated like
a missing value (log or ignore and continue sleeping/retrying) instead of
returning Err immediately, i.e., map Err(_) to continue the loop and only return
Ok(value) when check() yields Ok(Some(_)), and apply the same pattern to
poll_until_state_search_msg so its await? does not propagate transient errors
but instead retries until POLL_RETRIES are exhausted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 35263da2-460c-4f7e-ae71-950528fcd68d
📒 Files selected for processing (6)
.github/workflows/forest.ymlmise.tomlscripts/tests/calibnet_wallet_check.shscripts/tests/harness.shtests/calibnet_wallet.rstests/common/calibnet_wallet_helpers.rs
💤 Files with no reviewable changes (1)
- scripts/tests/calibnet_wallet_check.sh
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted filessee 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
LesnyRumcajs
requested changes
May 8, 2026
Member
LesnyRumcajs
left a comment
LesnyRumcajs
left a comment
There was a problem hiding this comment.
The test themselves shouldn't use the ? shorthand; in case of a failure the exact line which failed won't propagate and will make things more difficult to debug. unwrap should be used there and the tests shouldn't return Result.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/calibnet_wallet.rs (1)
27-33: ⚡ Quick winUse
anyhow::Result<()>with.context()throughout this test file instead of.unwrap()and.expect()panics.The file has 33 instances of panic-style error handling (
.unwrap()and.expect()). Converting test functions to returnResultand using?with.context()makes failures more debuggable and aligns with the repo's Rust guideline for error handling.Suggested pattern
+use anyhow::Context; @@ -async fn export_import_roundtrip(#[case] backend: Backend) { - let addr = wallet(backend, &["new"]).unwrap(); +async fn export_import_roundtrip(#[case] backend: Backend) -> anyhow::Result<()> { + let addr = wallet(backend, &["new"]).context("wallet new failed")?; @@ - let imported = wallet(backend, &["import", path]).unwrap(); + let imported = wallet(backend, &["import", path]).context("wallet import failed")?; @@ -} + Ok(()) +}Apply to all test functions and the
funded_delegated_addrhelper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/calibnet_wallet.rs` around lines 27 - 33, Change these test functions (e.g., export_import_roundtrip) and the funded_delegated_addr helper to return anyhow::Result<()> instead of panicking; replace all occurrences of .unwrap()/.expect() in tests with the ? operator and add .context("descriptive message") where helpful to attach context to failures (for example when calling wallet(...), export_to_temp_file(...), path().to_str(), etc.), and update imports to use anyhow::Context so the file compiles and errors propagate instead of panicking.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/calibnet_wallet.rs`:
- Around line 27-33: Change these test functions (e.g., export_import_roundtrip)
and the funded_delegated_addr helper to return anyhow::Result<()> instead of
panicking; replace all occurrences of .unwrap()/.expect() in tests with the ?
operator and add .context("descriptive message") where helpful to attach context
to failures (for example when calling wallet(...), export_to_temp_file(...),
path().to_str(), etc.), and update imports to use anyhow::Context so the file
compiles and errors propagate instead of panicking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 69d6f2c3-e049-436e-9091-9555b3aefcff
📒 Files selected for processing (1)
tests/calibnet_wallet.rs
| set -euo pipefail | ||
| source ./scripts/tests/harness.sh | ||
| forest_wallet_init "${usage_preloaded_key?}" | ||
| cargo test --profile quick-test --test calibnet_wallet -- --ignored --nocapture |
Member
There was a problem hiding this comment.
I completely miss your usage of ignored in these tests. Please elaborate.
Contributor
Author
There was a problem hiding this comment.
since these test require a running calibnet node, ignored is used so that these test doesn't get executed as a part of the default test suite
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
2 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 of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #6850
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Tests
Chores