test: refactor rpc_regression_tests into individual tests
#5930
Conversation
WalkthroughGenerates per-snapshot Tokio tests at build time via build.rs, emits a generated tests file included by the harness, refactors the RPC regression harness to run one snapshot per generated test (single download), serializes proof-parameter downloads, removes the CI-specific nextest override, and updates docs for per-file test runs. Changes
Sequence Diagram(s)sequenceDiagram
participant Cargo Build
participant build.rs
participant FS as OUT_DIR
participant Compiler
Cargo Build->>build.rs: run build script
build.rs->>build.rs: include_str!(".../test_snapshots.txt")
build.rs->>FS: write __rpc_regression_tests_gen.rs
Cargo Build->>Compiler: compile crate (include! generated file)
sequenceDiagram
participant GeneratedTest as #[tokio::test]
participant Harness as rpc_regression_test_run(name)
participant Lock as PROOF_PARAMS_LOCK
participant Net as download_file_with_cache
participant Runner as run_test_from_snapshot
GeneratedTest->>Harness: await rpc_regression_test_run(name)
Harness->>Lock: lock()
Harness->>Harness: maybe_set_proofs_parameter_cache_dir_env()
Harness->>Harness: ensure_proof_params_downloaded()
Harness-->>Lock: unlock()
Harness->>Net: download snapshot URL -> path
Net-->>Harness: local path
Harness->>Runner: run_test_from_snapshot(path)
Runner-->>Harness: result
Harness-->>GeneratedTest: complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
cc851ce to
6d4a881
Compare
6d4a881 to
1ff3b06
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
build.rs (1)
57-60: Consider adding error handling for malformed snapshot names.The current implementation assumes all entries in
test_snapshots.txtend with.rpcsnap.json.zst. If this assumption is violated, theunwrap()on line 73 will panic during the build process.Apply this diff to add validation:
let tests: Vec<_> = include_str!("src/tool/subcommands/api_cmd/test_snapshots.txt") .trim() .split("\n") + .filter(|s| !s.is_empty()) + .map(|s| { + if !s.ends_with(".rpcsnap.json.zst") { + panic!("Invalid snapshot name in test_snapshots.txt: {}", s); + } + s + }) .collect();Then update line 73 to use
expectinstead ofunwrap:- test.strip_suffix(".rpcsnap.json.zst").unwrap(), + test.strip_suffix(".rpcsnap.json.zst").expect("validated above"),src/tool/subcommands/api_cmd/test_snapshot.rs (1)
223-229: Consider adding error context for better debugging.When a test fails, having additional context about which snapshot failed would be helpful for debugging.
Apply this diff to add context to the error:
- run_test_from_snapshot(&path).await.unwrap(); + run_test_from_snapshot(&path) + .await + .unwrap_or_else(|e| panic!("Test failed for snapshot {}: {}", name, e));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
build.rs(3 hunks)src/tool/subcommands/api_cmd/test_snapshot.rs(1 hunks)src/utils/mod.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/tool/subcommands/api_cmd/test_snapshot.rs (3)
src/utils/proofs_api/parameters.rs (1)
maybe_set_proofs_parameter_cache_dir_env(114-118)src/utils/proofs_api/paramfetch.rs (1)
ensure_proof_params_downloaded(59-66)src/utils/net/download_file.rs (1)
download_file_with_cache(30-89)
🔇 Additional comments (4)
src/utils/mod.rs (1)
173-177: LGTM! Appropriate suppression of dead code warnings for test utilities.The addition of
#[allow(dead_code)]attributes to both test helper functions is appropriate since these utilities may not be used in all test configurations but should remain available for conditional use.build.rs (1)
52-78: Test generator implementation looks good.The implementation correctly reads the snapshot list and generates individual tests with proper naming and structure. The use of
strip_suffixensures clean test names, and the generated tests properly call the async test runner.src/tool/subcommands/api_cmd/test_snapshot.rs (2)
193-195: Good approach using generated tests for parallelization.The use of
include!to pull in the generated tests from the build script is a clean approach that enables individual test execution and parallelization via cargo nextest.
196-230: Excellent use of mutex to prevent concurrent proof parameter downloads.The implementation correctly uses a static
LazyLock<Mutex<()>>to serialize access to proof parameter downloads, preventing race conditions and potential corruption when multiple tests run in parallel.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
build.rs (1)
4-4: Scope import locally to the generator (minor nit)
std::io::Writeis only used insiderpc_regression_tests_gen(). Prefer moving it into that function for tighter scope and to mirror the other localized imports.Apply this diff:
-use std::io::Write;And add this alongside the other local uses in
rpc_regression_tests_gen():fn rpc_regression_tests_gen() { - use std::fs::File; - use std::io::BufWriter; + use std::fs::File; + use std::io::{BufWriter, Write}; use std::path::PathBuf;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
build.rs(3 hunks)src/tool/subcommands/api_cmd/test_snapshot.rs(1 hunks)src/utils/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/utils/mod.rs
- src/tool/subcommands/api_cmd/test_snapshot.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Build Ubuntu
- GitHub Check: cargo-publish-dry-run
- GitHub Check: All lint checks
- GitHub Check: tests
- GitHub Check: tests-release
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
build.rs (1)
69-71: Confirm return type of rpc_regression_test_run; avoid silently discarding errorsIf
rpc_regression_test_runreturns a Result, this test will fail to compile without a matching return type, or worse, you might be tempted to add a semicolon and drop the error. Prefer propagating errors from the test function.If
rpc_regression_test_run(name)returnsanyhow::Result<()>, generate:#[tokio::test(flavor = "multi_thread")] async fn rpc_test_...() -> anyhow::Result<()> { rpc_regression_test_run("...").await }If it returns
(), the current form is fine. Please confirm which it is and adjust generator accordingly. I can update the generator template for you.
|
no green checkmark, no review! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/tool/subcommands/api_cmd/test_snapshot.rs (1)
211-221: Consider URL validation and error handling improvements.While the current implementation works, consider these improvements for robustness:
- The URL construction using
format!and.parse().unwrap()could benefit from better error handling- Consider validating the test name to prevent URL injection issues
- let url: Url = - format!("https://forest-snapshots.fra1.cdn.digitaloceanspaces.com/rpc_test/{name}") - .as_str() - .parse() - .unwrap(); + let url = format!("https://forest-snapshots.fra1.cdn.digitaloceanspaces.com/rpc_test/{name}") + .parse::<Url>() + .expect(&format!("Failed to parse URL for test: {name}"));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
build.rs(3 hunks)src/tool/subcommands/api_cmd/test_snapshot.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- build.rs
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: hanabi1224
PR: ChainSafe/forest#5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.282Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.
🧬 Code Graph Analysis (1)
src/tool/subcommands/api_cmd/test_snapshot.rs (3)
src/utils/proofs_api/parameters.rs (1)
maybe_set_proofs_parameter_cache_dir_env(114-118)src/utils/proofs_api/paramfetch.rs (1)
ensure_proof_params_downloaded(59-66)src/utils/net/download_file.rs (1)
download_file_with_cache(30-89)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: All lint checks
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: Build MacOS
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
src/tool/subcommands/api_cmd/test_snapshot.rs (5)
188-190: LGTM! Clean import additions for synchronization.The new imports
LazyLockandMutexare appropriately added to support the proof parameters synchronization mechanism introduced in the refactoring.
193-194: LGTM! Clear documentation and proper code generation integration.The comment provides helpful guidance for running individual tests, and the
include!macro properly integrates the build-time generated tests from the OUT_DIR.
196-235: LGTM! Well-designed per-test execution with proper synchronization.The refactored
rpc_regression_test_runfunction effectively addresses the original design issues:
- Proper synchronization: The
PROOF_PARAMS_LOCKensures thread-safe proof parameter downloads, preventing race conditions that could occur with concurrent test execution.- Single snapshot per test: The function now handles one named snapshot at a time, making tests more isolated and easier to debug.
- Clean URL construction: Direct URL formation from the test name is straightforward and maintainable.
- Preserved functionality: All essential features (CI debug build skipping, RNG seeding, timing measurement) are retained.
The implementation aligns well with the PR objective of generating individual tests via codegen while maintaining the existing test logic.
228-234: LGTM! Excellent timing and logging implementation.The timing measurement and logging provide valuable feedback for test execution, showing both the test name and duration. The use of
humantime::format_durationmakes the output user-friendly.
203-210: PROOF_PARAMS_LOCK is necessary to prevent concurrent races
Our search shows that neither maybe_set_proofs_parameter_cache_dir_env nor ensure_proof_params_downloaded contain any internal locking—both will unconditionally set an env var and invoke get_params_default, which may write to the same cache directory. Without the external Tokio Mutex, parallel test tasks could corrupt or collide on the download. The one-time lock acquisition imposes negligible overhead once the files are already present, so no changes are required.
@LesnyRumcajs Fixed! |
|
|
||
| #[tokio::test(flavor = "multi_thread")] | ||
| async fn rpc_regression_tests() { | ||
| // To run a single test: cargo test --lib -- --nocapture --test filecoin_multisig_statedecodeparams_1754230255631789 |
There was a problem hiding this comment.
we can include this info in rpc_test_snapshot.md
LesnyRumcajs
left a comment
There was a problem hiding this comment.
I guess we can delete this?
Lines 34 to 36 in cc5c4c1
@LesnyRumcajs Good catch! Removed. |
| print!("Testing {name} ..."); | ||
| let start = Instant::now(); | ||
| run_test_from_snapshot(&path).await.unwrap(); | ||
| println!( | ||
| " succeeded, took {}.", | ||
| humantime::format_duration(start.elapsed()) | ||
| ); |
There was a problem hiding this comment.
Do we need the manual timings? Isn't it redundant by what nextest provides already?
There was a problem hiding this comment.
It's still helpful for cargo test running multiple tests. It also eliminates time for downloading the snapshot file. e.g.
cargo test --lib -- --nocapture --test filecoin_ethgetblocktransactioncount
Finished `test` profile [unoptimized] target(s) in 0.37s
Running unittests src/lib.rs (target/debug/deps/forest-7c1bb6fa1af2e8fa)
running 2 tests
Testing filecoin_ethgetblocktransactioncountbynumber_1737446676697272.rpcsnap.json.zst ... succeeded, took 203ms 866us 755ns.
test tool::subcommands::api_cmd::test_snapshot::tests::rpc_test_filecoin_ethgetblocktransactioncountbynumber_1737446676697272 ... ok
Testing filecoin_ethgetblocktransactioncountbyhash_1740132538001911.rpcsnap.json.zst ... succeeded, took 500ms 885us 826ns.
test tool::subcommands::api_cmd::test_snapshot::tests::rpc_test_filecoin_ethgetblocktransactioncountbyhash_1740132538001911 ... ok
test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 738 filtered out; finished in 48.51s
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/docs/developers/guides/rpc_test_snapshot.md (2)
38-39: Clarify upload destination as an S3 URL for consistencyUse the explicit S3-style URL to avoid ambiguity in tooling and docs.
- 2. Upload the `{filename}.rpcsnap.json.zst` files to the DigitalOcean space `forest-snapshots/rpc_test` + 2. Upload the `{filename}.rpcsnap.json.zst` files to the DigitalOcean Space at `s3://forest-snapshots/rpc_test/`
47-49: Add a language to the fenced code block (markdownlint MD040)Minor doc polish; specify shell for better highlighting and linter compliance.
- ``` - s3cmd --configure - ``` + ```sh + s3cmd --configure + ```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.config/nextest.toml(0 hunks)docs/docs/developers/guides/rpc_test_snapshot.md(1 hunks)
💤 Files with no reviewable changes (1)
- .config/nextest.toml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: hanabi1224
PR: ChainSafe/forest#5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.282Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.
🪛 markdownlint-cli2 (0.17.2)
docs/docs/developers/guides/rpc_test_snapshot.md
41-41: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: Build Ubuntu
- GitHub Check: tests
- GitHub Check: Check
- GitHub Check: tests-release
- GitHub Check: Deploy to Cloudflare Pages
- GitHub Check: All lint checks
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
docs/docs/developers/guides/rpc_test_snapshot.md (1)
52-53: Specify shell as code block languageThe script
scripts/tests/upload_rcpsnaps.shis correctly named and located; only the markdown fence needs a language for syntax highlighting.
- File:
docs/docs/developers/guides/rpc_test_snapshot.md- Lines: 52–53
- ``` - ./scripts/tests/upload_rcpsnaps.sh /var/tmp/rpc-snapshots - ``` + ```sh + ./scripts/tests/upload_rcpsnaps.sh /var/tmp/rpc-snapshots + ```
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/docs/developers/guides/rpc_test_snapshot.md (1)
41-43: Add language to fenced code block (and keep the corrected cargo filter position).This satisfies markdownlint MD040 and improves syntax highlighting. The cargo command itself looks correct now with the filter before the
--.- ``` + ```sh cargo test --lib {filename} -- --nocapture ```
🧹 Nitpick comments (3)
docs/docs/developers/guides/rpc_test_snapshot.md (3)
38-39: Clarify thattest_snapshots.txtcontains base names without the archive extension.Avoids ambiguity for users copying names between steps and aligns with generated test names.
- 3. Include the file names in `src/tool/subcommands/api_cmd/test_snapshots.txt` + 3. Include the base file names (without the `.rpcsnap.json.zst` extension) in `src/tool/subcommands/api_cmd/test_snapshots.txt`
47-50: Add language to the s3cmd fenced block for consistency and lint compliance.- ``` + ```sh s3cmd --configure ```
52-54: Add language to the upload script fenced block.- ``` + ```sh ./scripts/tests/upload_rcpsnaps.sh /var/tmp/rpc-snapshots ```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/docs/developers/guides/rpc_test_snapshot.md(1 hunks)src/tool/subcommands/api_cmd/test_snapshot.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tool/subcommands/api_cmd/test_snapshot.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: hanabi1224
PR: ChainSafe/forest#5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.
📚 Learning: 2025-08-13T09:43:20.301Z
Learnt from: hanabi1224
PR: ChainSafe/forest#5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.
Applied to files:
docs/docs/developers/guides/rpc_test_snapshot.md
🪛 markdownlint-cli2 (0.17.2)
docs/docs/developers/guides/rpc_test_snapshot.md
41-41: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Build Ubuntu
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: All lint checks
- GitHub Check: Check
- GitHub Check: Deploy to Cloudflare Pages
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests
- GitHub Check: tests-release
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
docs/docs/developers/guides/rpc_test_snapshot.md (1)
52-53: Script path verified: The documentation’s./scripts/tests/upload_rcpsnaps.shcommand matches the actual file at scripts/tests/upload_rcpsnaps.sh. No updates needed.
Summary of changes
(part of #5894)
Changes introduced in this pull request:
rpc_regression_testsinto individual tests with codegenhttps://github.com/ChainSafe/forest/actions/runs/16933891233/job/47991529895?pr=5930#step:7:2402
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Summary by CodeRabbit
Tests
Refactor
Documentation
Chores