ci: replace per-package Rust tests with sharded workspace nextest#3237
ci: replace per-package Rust tests with sharded workspace nextest#3237QuantumExplorer wants to merge 12 commits intov3.1-devfrom
Conversation
- Add tests-rs-workspace.yml using cargo-nextest with --partition count:N/4 to split workspace tests across 4 shards with cargo-llvm-cov coverage - Remove test job from tests-rs-package.yml (lint/format/check jobs remain) - Add push trigger on master/v*-dev so Codecov updates on merge - Exclude wasm/ffi/tool packages that can't compile natively Previously each Rust package got its own CI runner that independently compiled all shared dependencies. Now the workspace compiles once per shard and tests are distributed evenly via nextest partitioning. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughConsolidates Rust test execution into a new workspace-level GitHub Actions workflow with sharded partitions and removes the prior in-repo package test job; updates main tests workflow to conditionally invoke the workspace workflow before package-level jobs. Changes
Sequence Diagram(s)mermaid GitHub->>WorkspaceWF: trigger matrix partition (1..4) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
📝 Coding Plan
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 |
sccache doesn't help with cargo-llvm-cov instrumented builds since the instrumentation changes compilation output, preventing cache hits. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cache target/llvm-cov-target (where cargo-llvm-cov puts instrumented build artifacts) using GitHub Actions cache, matching GroveDB's approach. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/tests-rs-workspace.yml (1)
39-40: Consider pinning tool versions for reproducibility.The
taiki-e/install-actioninstalls the latest versions ofcargo-llvm-covandcargo-nextest. Pinning specific versions would prevent unexpected CI breakage from upstream changes.Example version pinning
- - uses: taiki-e/install-action@cargo-llvm-cov - - uses: taiki-e/install-action@cargo-nextest + - uses: taiki-e/install-action@v2 + with: + tool: cargo-llvm-cov@0.6.15,cargo-nextest@0.9.85🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests-rs-workspace.yml around lines 39 - 40, The workflow currently uses floating references taiki-e/install-action@cargo-llvm-cov and taiki-e/install-action@cargo-nextest; change these to pinned versions/tags (for example a specific release tag or commit SHA) to ensure reproducible CI runs—update the two uses entries (taiki-e/install-action@cargo-llvm-cov and taiki-e/install-action@cargo-nextest) to explicit pinned identifiers such as taiki-e/install-action@vX.Y.Z or taiki-e/install-action@<commit-sha> and ensure the chosen tags are tested/compatible with the rest of the workflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/tests-rs-workspace.yml:
- Around line 4-7: The workflow currently defines an input named partitions but
still hardcodes strategy.matrix.partition to [1,2,3,4], so it ignores caller
input; fix by either removing the partitions input and its description (if you
want to always shard into 4) or implement dynamic matrix generation: add a setup
job that runs on ubuntu (e.g., job name setup with step id set-matrix) which
emits partition-matrix via GITHUB_OUTPUT using a shell like echo "matrix=$(seq 1
${{ inputs.partitions }} | jq -R . | jq -sc .)" >> $GITHUB_OUTPUT, then change
the test job to depend on setup and set strategy.matrix.partition: ${{
fromJson(needs.setup.outputs.partition-matrix) }} so the matrix respects the
partitions input.
---
Nitpick comments:
In @.github/workflows/tests-rs-workspace.yml:
- Around line 39-40: The workflow currently uses floating references
taiki-e/install-action@cargo-llvm-cov and taiki-e/install-action@cargo-nextest;
change these to pinned versions/tags (for example a specific release tag or
commit SHA) to ensure reproducible CI runs—update the two uses entries
(taiki-e/install-action@cargo-llvm-cov and taiki-e/install-action@cargo-nextest)
to explicit pinned identifiers such as taiki-e/install-action@vX.Y.Z or
taiki-e/install-action@<commit-sha> and ensure the chosen tags are
tested/compatible with the rest of the workflow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 936a548a-81bd-44b3-9570-058c781517fa
📒 Files selected for processing (3)
.github/workflows/tests-rs-package.yml.github/workflows/tests-rs-workspace.yml.github/workflows/tests.yml
The matrix was already hardcoded to [1,2,3,4] so the input was ignored. Simplify by removing it entirely. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ensure sharded workspace tests run even when no Rust source files changed (e.g. workflow-only PRs, push to base branch, nightly schedule). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add rs-workflows-changed path filter so the sharded workspace tests also run on PRs that only modify CI workflow files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dash-sdk has crate-type = ["cdylib", "rlib"] and the static rocksdb library has TLS relocations (R_X86_64_TPOFF32) incompatible with shared objects. Adding --lib --bins --tests tells cargo to only build test targets, skipping the cdylib output that triggers the linker error. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dash-sdk has crate-type = ["cdylib", "rlib"] which causes R_X86_64_TPOFF32 linker errors with static rocksdb when building shared objects under cargo-llvm-cov. Exclude it alongside the other cdylib crates. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/tests-rs-workspace.yml (1)
22-25: Minor: Consider using boolean forcache-on-failure.While the string
"false"works, using the native YAML booleanfalseis more idiomatic.- uses: Swatinem/rust-cache@v2 with: - cache-on-failure: "false" + cache-on-failure: false workspaces: ". -> target/llvm-cov-target"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests-rs-workspace.yml around lines 22 - 25, Change the string value "false" for the rust-cache action to a native YAML boolean false to be more idiomatic: locate the job step that has uses: Swatinem/rust-cache@v2 and update the cache-on-failure setting (cache-on-failure) from the quoted "false" to the unquoted boolean false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/tests-rs-workspace.yml:
- Around line 47-56: The workflow currently excludes the rs-sdk-ffi package so
its 55+ native integration tests never run; update
.github/workflows/tests-rs-workspace.yml to stop excluding rs-sdk-ffi (remove
the "--exclude rs-sdk-ffi" entry) or add a separate native test job (e.g.,
"native-rs-sdk-ffi-tests") that checks out the repo, sets up Rust, and runs
cargo test for the rs-sdk-ffi package (cargo test --manifest-path
packages/rs-sdk-ffi/Cargo.toml or cargo test -p rs-sdk-ffi) on a native runner
to execute its integration tests; ensure the job runs on the same
matrix/conditions as other Rust native tests and document the change in the
workflow comment.
---
Nitpick comments:
In @.github/workflows/tests-rs-workspace.yml:
- Around line 22-25: Change the string value "false" for the rust-cache action
to a native YAML boolean false to be more idiomatic: locate the job step that
has uses: Swatinem/rust-cache@v2 and update the cache-on-failure setting
(cache-on-failure) from the quoted "false" to the unquoted boolean false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c301b89d-ff32-4f44-abf0-7ee42e159d58
📒 Files selected for processing (2)
.github/workflows/tests-rs-workspace.yml.github/workflows/tests.yml
Set cache-on-failure to true so subsequent runs benefit from cached compilation even if a test failure occurs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…lusions Replace --workspace with --package for drive, dpp, and drive-abci. This avoids the cdylib linker issue entirely (no cdylib crates in the build) and is simpler than maintaining a growing exclusion list. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3237 +/- ##
=============================================
+ Coverage 55.87% 68.11% +12.23%
=============================================
Files 3173 3266 +93
Lines 235215 251726 +16511
=============================================
+ Hits 131435 171454 +40019
+ Misses 103780 80272 -23508
🚀 New features to boost your workflow:
|
| - name: Upload coverage | ||
| if: always() | ||
| uses: codecov/codecov-action@v5 | ||
| with: | ||
| files: lcov.info | ||
| flags: rust-shard-${{ matrix.partition }} | ||
| token: ${{ secrets.CODECOV_TOKEN }} | ||
| fail_ci_if_error: false |
There was a problem hiding this comment.
good job setting fail_ci_if_error to avoid forked pre from failing :)
PastaPastaPasta
left a comment
There was a problem hiding this comment.
This looks fine; I haven't noticed anything that will break forked PRs, nor did I notice anything wrong.
Add dash-sdk (234 tests), platform-value (125), rs-dapi (90), platform-wallet (53), rs-dapi-client (11), and platform-serialization (10) to the coverage test runner. Strip cdylib from dash-sdk's Cargo.toml in CI to avoid the static rocksdb TLS relocation linker error. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Strip cdylib from their Cargo.toml in CI (same static rocksdb issue). Adds 328 more tests (260 + 68) to coverage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Issue being fixed or feature implemented
Codecov coverage wasn't updating after PR merges because the Tests workflow only triggered on
pull_requestandschedule(nightly), not onpushto base branches. Additionally, each Rust package got its own CI runner that redundantly compiled all shared dependencies (dpp, grovedb, etc.), wasting CI time.What was done?
.github/workflows/tests-rs-workspace.yml: Sharded workspace test runner usingcargo llvm-cov nextest --workspace --partition count:N/4. Tests are distributed across 4 parallel shards via cargo-nextest partitioning. Each shard uploads coverage to Codecov (auto-merged)..github/workflows/tests.yml: Addedpushtrigger onmaster/v*-devso Codecov updates immediately on merge (matches GroveDB's approach). Addedrs-workspace-testsjob calling the new workflow..github/workflows/tests-rs-package.yml: Removed thetestjob (103 lines). Per-package lint, formatting, unused deps, structure detection, and check-each-feature jobs remain unchanged.wasm-dpp,wasm-dpp2,wasm-sdk,wasm-drive-verify,rs-sdk-ffi,platform-wallet-ffi,strategy-tests,check-features,dash-platform-balance-checkerBefore vs After
How Has This Been Tested?
Breaking Changes
None. This only changes CI workflow configuration.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit