Fix calibnet wallet test flakiness#7197
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
WalkthroughThe PR replaces ChangesCalibnet integration test helper and test refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/calibnet_mpool_tools.rs (1)
63-65: ⚡ Quick winPreserve
wait_for_msgerror details in test failures.Using
is_ok()discards the root-cause error chain fromwait_for_msg; emitting{err:#}will make failed CI runs much easier to diagnose.Suggested patch
- assert!( - wait_for_msg(&cid).await.is_ok(), - "mpool replace --auto should replace message {cid} from {addr} at nonce {nonce}." - ); + wait_for_msg(&cid).await.unwrap_or_else(|err| { + panic!( + "mpool replace --auto should replace message {cid} from {addr} at nonce {nonce}: {err:#}" + ) + });🤖 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_mpool_tools.rs` around lines 63 - 65, The assertion on wait_for_msg result uses is_ok() which discards the actual error information from the Result, making it difficult to diagnose test failures in CI. Instead of checking is_ok() on the await result of wait_for_msg, capture the Result and use pattern matching or conditional logic to extract and include the actual error details (using format strings like {err:#}) in the assertion failure message. This way, when the test fails, the root cause error information will be visible to help with debugging.
🤖 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 311-314: The wait_for_msg function currently relies on the HTTP
client's implicit 120-second timeout without explicit error context. Wrap the
rpc_call invocation with tokio::time::timeout(POLL_TIMEOUT, ...) and add
.context() with a descriptive message to provide clearer diagnostics when the
function times out or fails. This approach should follow similar patterns
already established in the codebase, such as those found in stateful_tests.rs,
to ensure consistency and improve test observability.
---
Nitpick comments:
In `@tests/calibnet_mpool_tools.rs`:
- Around line 63-65: The assertion on wait_for_msg result uses is_ok() which
discards the actual error information from the Result, making it difficult to
diagnose test failures in CI. Instead of checking is_ok() on the await result of
wait_for_msg, capture the Result and use pattern matching or conditional logic
to extract and include the actual error details (using format strings like
{err:#}) in the assertion failure message. This way, when the test fails, the
root cause error information will be visible to help with debugging.
🪄 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: e263a229-62ee-4189-b215-c5d5c0db9504
📒 Files selected for processing (3)
tests/calibnet_mpool_tools.rstests/calibnet_wallet.rstests/common/calibnet_wallet_helpers.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
akaladarshi
left a comment
There was a problem hiding this comment.
Can you place all utils function in a common file so they can be used across tests instead of keeping them separate.
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 Harness.
🚀 New features to boost your workflow:
|
f44eda1 to
4e11fd7
Compare
Summary of changes
Changes introduced in this pull request:
wait_for_msghelper that blocks onFilecoin.StateWaitMsguntil a message lands on-chain and assertsExitCode == 0, replacing the hand-rolledpoll_until_state_search_msgso there's a single "wait for inclusion" helper.Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Release Notes
Tests
Chores