fix(ci): speed up wallet tests on CI#7218
Open
hanabi1224 wants to merge 1 commit into
Open
Conversation
hanabi1224
requested review from
LesnyRumcajs and
sudo-shashank
and removed request for
a team
Contributor
WalkthroughRemoves the ChangesCalibnet Test Migration to CLI Subcommand
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
6 tasks
![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: 2
🧹 Nitpick comments (2)
src/dev/subcommands/tests_cmd.rs (1)
14-17: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd command-level error context in the dispatcher.
Line 16 currently forwards errors directly; adding
.context(...)will make CLI failures easier to diagnose upstream.Proposed change
+use anyhow::Context as _; + impl TestsCommand { pub async fn run(self) -> anyhow::Result<()> { match self { - Self::Calibnet(cmd) => cmd.run().await, + Self::Calibnet(cmd) => cmd.run().await.context("running tests calibnet subcommand"), } } }As per coding guidelines,
**/*.rs: "Useanyhow::Result<T>for most operations and add context with.context()when errors occur".🤖 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 `@src/dev/subcommands/tests_cmd.rs` around lines 14 - 17, The run method in the dispatcher is not adding error context when forwarding the result from cmd.run().await for the Calibnet variant. Add an anyhow context message using .context() after the cmd.run().await call to provide diagnostic information about what command failed, making the error easier to diagnose upstream when it propagates to the CLI level.Source: Coding guidelines
src/dev/subcommands/tests_cmd/calibnet.rs (1)
16-20: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winWrap nested command failures with calibnet-specific context.
Lines 18-19 should attach context so wallet vs mpool failures are immediately distinguishable in the error chain.
Proposed change
+use anyhow::Context as _; + impl CalibnetTestsCommand { pub async fn run(self) -> anyhow::Result<()> { match self { - Self::Wallet(cmd) => cmd.run().await, - Self::Mpool(cmd) => cmd.run().await, + Self::Wallet(cmd) => cmd.run().await.context("running calibnet wallet tests"), + Self::Mpool(cmd) => cmd.run().await.context("running calibnet mpool tests"), } } }As per coding guidelines,
**/*.rs: "Useanyhow::Result<T>for most operations and add context with.context()when errors occur".🤖 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 `@src/dev/subcommands/tests_cmd/calibnet.rs` around lines 16 - 20, The run method in the calibnet command handler does not attach context to errors from nested wallet and mpool commands, making it difficult to distinguish which subcommand failed. Wrap both cmd.run().await calls (for Self::Wallet and Self::Mpool branches) with .context() calls that provide calibnet-specific context identifying which command failed, such as "wallet command failed" and "mpool command failed" respectively, so that errors in the chain are immediately distinguishable.Source: Coding guidelines
🤖 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 @.github/workflows/forest.yml:
- Line 229: The GitHub workflow is missing the `ready_for_review` event type in
its pull request trigger configuration, preventing the workflow from re-running
when a draft PR transitions to ready status. Locate the workflow trigger
definition (around line 18) where the pull request event types are specified,
and add `ready_for_review` to the `types:` array alongside the existing event
types. This ensures that the `calibnet-wallet-check` job, which properly checks
`github.event.pull_request.draft == false` at line 229, will execute when drafts
are marked ready without requiring an additional commit.
In `@src/dev/subcommands/tests_cmd/calibnet/mpool.rs`:
- Around line 10-35: The test closures in the tests() function are using
futures::executor::block_on to execute async test functions, but these functions
internally call helpers that depend on Tokio runtime operations like
tokio::time::sleep and tokio::time::Instant::now, which fail without a Tokio
runtime context. Replace the futures::executor::block_on call with
tokio::runtime::Runtime::new()?.block_on() to provide the necessary Tokio
runtime context for all test trials (both in the Trial::test closures for
mpool_nonce_fix_auto_unblocks_pending and mpool_replace_auto_unblocks_pending,
and similarly for other test functions).
---
Nitpick comments:
In `@src/dev/subcommands/tests_cmd.rs`:
- Around line 14-17: The run method in the dispatcher is not adding error
context when forwarding the result from cmd.run().await for the Calibnet
variant. Add an anyhow context message using .context() after the
cmd.run().await call to provide diagnostic information about what command
failed, making the error easier to diagnose upstream when it propagates to the
CLI level.
In `@src/dev/subcommands/tests_cmd/calibnet.rs`:
- Around line 16-20: The run method in the calibnet command handler does not
attach context to errors from nested wallet and mpool commands, making it
difficult to distinguish which subcommand failed. Wrap both cmd.run().await
calls (for Self::Wallet and Self::Mpool branches) with .context() calls that
provide calibnet-specific context identifying which command failed, such as
"wallet command failed" and "mpool command failed" respectively, so that errors
in the chain are immediately distinguishable.
🪄 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: 51272f9c-a2ee-4b20-8bea-14b273c0843d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.github/workflows/forest.ymlCargo.tomlmise.tomlsrc/dev/subcommands/mod.rssrc/dev/subcommands/tests_cmd.rssrc/dev/subcommands/tests_cmd/calibnet.rssrc/dev/subcommands/tests_cmd/calibnet/helpers.rssrc/dev/subcommands/tests_cmd/calibnet/mpool.rssrc/dev/subcommands/tests_cmd/calibnet/wallet.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
hanabi1224
force-pushed
the
hm/ci-speed-up-wallet-tests
branch
from
8a92440 to
085a8ee
Compare
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 `@src/dev/subcommands/tests_cmd/calibnet/helpers.rs`:
- Around line 382-384: The block_on helper function uses Handle::current() which
can panic if tokio runtime context is not available in the test closure. Replace
the Handle::current() call with try_current() and propagate any error using
anyhow::Result and .context() for clear error messaging. This ensures the
function fails gracefully with a descriptive error message rather than panicking
when runtime context is unavailable.
🪄 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: c6512da0-f78e-4a00-b085-b4c1abedb682
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.config/forest.dic.github/workflows/forest.ymlCargo.tomlmise.tomlsrc/dev/subcommands/mod.rssrc/dev/subcommands/tests_cmd.rssrc/dev/subcommands/tests_cmd/calibnet.rssrc/dev/subcommands/tests_cmd/calibnet/helpers.rssrc/dev/subcommands/tests_cmd/calibnet/mpool.rssrc/dev/subcommands/tests_cmd/calibnet/wallet.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
✅ Files skipped from review due to trivial changes (1)
- .config/forest.dic
🚧 Files skipped from review as they are similar to previous changes (8)
- src/dev/subcommands/tests_cmd/calibnet.rs
- src/dev/subcommands/mod.rs
- .github/workflows/forest.yml
- mise.toml
- src/dev/subcommands/tests_cmd.rs
- src/dev/subcommands/tests_cmd/calibnet/mpool.rs
- Cargo.toml
- src/dev/subcommands/tests_cmd/calibnet/wallet.rs
hanabi1224
force-pushed
the
hm/ci-speed-up-wallet-tests
branch
from
085a8ee to
37ce48d
Compare
hanabi1224
force-pushed
the
hm/ci-speed-up-wallet-tests
branch
from
37ce48d to
ccb8b78
Compare
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
1 participant
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
This PR attempts to speed up wallet CI tests, as they cannot run in parallel for multiple PRs
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
testssubcommand to run Calibnet integration tests from the command line, including wallet and mpool scenarios.Chores
libtest_mimicharness.forest-dev tests calibnetfor wallet and mpool.ready_for_reviewtrigger and to skip the wallet check job for draft pull requests.