build(platform): replace per-package Rust tests with sharded workspace runner#3237
build(platform): replace per-package Rust tests with sharded workspace runner#3237QuantumExplorer wants to merge 24 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>
|
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:
📝 WalkthroughWalkthroughConsolidates Rust testing: removes an in-repo package test job, adds a sharded workspace test workflow and a workspace doctests workflow, and updates the main CI to conditionally call the new workflows and surface workflow-change detection outputs. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as GitHub Runner
participant WorkspaceWF as tests-rs-workspace.yml
participant Tooling as Rust + llvm + cargo tools
participant Cache as sccache (S3)
participant TestExec as cargo-llvm-cov + nextest
participant Codecov as Codecov
participant Artifacts as GitHub Artifacts
Runner->>WorkspaceWF: trigger matrix partition (1..4)
WorkspaceWF->>Tooling: setup Rust, llvm-tools, install cargo-llvm-cov & nextest
WorkspaceWF->>Cache: configure sccache (S3)
WorkspaceWF->>TestExec: run nextest partition N/4 with coverage
TestExec->>Codecov: upload lcov (shard N)
alt tests fail
WorkspaceWF->>Artifacts: archive cores + binaries -> upload core-dumps-shard-N
end
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>
…rom source The librocksdb action is broken on cold cache (GCC warning-as-error on Ubuntu 24.04) and redundant when rust-cache is warm. Let librocksdb-sys compile rocksdb via its own build script; rust-cache handles caching. This also removes architecture-specific SNAPPY_* paths, making the workflows portable across x86 and ARM runners. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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-doctests.yml (1)
20-26: Add an explicit post-rewrite assertion forcdylibremoval.This block is exact-match
sed; if manifest formatting drifts, rewrites may no-op silently. Add a validation check immediately after mutation so failures are explicit and early.Proposed hardening
- name: Strip cdylib crate-types (incompatible with static rocksdb) run: | sed -i 's/\["cdylib", "rlib"\]/["rlib"]/' packages/rs-sdk/Cargo.toml packages/wasm-drive-verify/Cargo.toml sed -i 's/\["cdylib", "lib"\]/["lib"]/' packages/wasm-dpp2/Cargo.toml sed -i 's/\["cdylib"\]/["rlib"]/' packages/wasm-sdk/Cargo.toml sed -i 's/\["staticlib", "cdylib", "rlib"\]/["staticlib", "rlib"]/g' packages/rs-sdk-ffi/Cargo.toml packages/rs-platform-wallet-ffi/Cargo.toml + if grep -nE 'crate-type\s*=\s*\[[^]]*"cdylib"' \ + packages/rs-sdk/Cargo.toml \ + packages/wasm-drive-verify/Cargo.toml \ + packages/wasm-dpp2/Cargo.toml \ + packages/wasm-sdk/Cargo.toml \ + packages/rs-sdk-ffi/Cargo.toml \ + packages/rs-platform-wallet-ffi/Cargo.toml; then + echo "::error::cdylib still present after rewrite" + exit 1 + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests-rs-doctests.yml around lines 20 - 26, After the sed rewrites in the "Strip cdylib crate-types (incompatible with static rocksdb)" step, add an explicit validation that no target Cargo.toml still contains the token "cdylib"; run a check (e.g., grep or similar) across the same package files referenced in the sed commands (packages/rs-sdk/Cargo.toml, packages/wasm-drive-verify/Cargo.toml, packages/wasm-dpp2/Cargo.toml, packages/wasm-sdk/Cargo.toml, packages/rs-sdk-ffi/Cargo.toml, packages/rs-platform-wallet-ffi/Cargo.toml) and if any match is found, fail the job with a clear error message so rewrites that silently no-op are detected immediately.
🤖 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.yml:
- Around line 49-50: The rs-workflows-changed filter only watches workflow YAMLs
so changes under the local Rust action directory (./.github/actions/rust) don't
set the flag; update the filter that defines rs-workflows-changed (the step
producing steps.filter-rs-workflows.outputs.rs-workflows) to include the
.github/actions/rust path (and similarly update the other occurrence around the
block mentioned for lines 85-90) so modifications to the local action flip the
rs-workflows-changed flag and cause Rust jobs (workspace/doctest) to run.
---
Nitpick comments:
In @.github/workflows/tests-rs-doctests.yml:
- Around line 20-26: After the sed rewrites in the "Strip cdylib crate-types
(incompatible with static rocksdb)" step, add an explicit validation that no
target Cargo.toml still contains the token "cdylib"; run a check (e.g., grep or
similar) across the same package files referenced in the sed commands
(packages/rs-sdk/Cargo.toml, packages/wasm-drive-verify/Cargo.toml,
packages/wasm-dpp2/Cargo.toml, packages/wasm-sdk/Cargo.toml,
packages/rs-sdk-ffi/Cargo.toml, packages/rs-platform-wallet-ffi/Cargo.toml) and
if any match is found, fail the job with a clear error message so rewrites that
silently no-op are detected immediately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 570f4d4c-5cad-4e00-8a85-8972d51a54c9
📒 Files selected for processing (3)
.github/workflows/tests-rs-doctests.yml.github/workflows/tests-rs-workspace.yml.github/workflows/tests.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/tests-rs-workspace.yml
| rs-workflows-changed: ${{ steps.filter-rs-workflows.outputs.rs-workflows }} | ||
| test-suite: ${{ steps.override.outputs.run || steps.filter-test-suite.outputs.run }} |
There was a problem hiding this comment.
Rust workflow-change filter misses local Rust action changes.
rs-workflows-changed currently watches only workflow YAMLs. A change to ./.github/actions/rust (used by Rust test workflows) won’t flip this flag, so Rust workspace/doctest jobs may be skipped when rs-packages is unchanged.
Proposed fix
filters: |
rs-workflows:
- .github/workflows/tests-rs-workspace.yml
- .github/workflows/tests-rs-doctests.yml
- .github/workflows/tests-rs-package.yml
- .github/workflows/tests.yml
+ - .github/actions/rust/**Also applies to: 85-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/tests.yml around lines 49 - 50, The rs-workflows-changed
filter only watches workflow YAMLs so changes under the local Rust action
directory (./.github/actions/rust) don't set the flag; update the filter that
defines rs-workflows-changed (the step producing
steps.filter-rs-workflows.outputs.rs-workflows) to include the
.github/actions/rust path (and similarly update the other occurrence around the
block mentioned for lines 85-90) so modifications to the local action flip the
rs-workflows-changed flag and cause Rust jobs (workspace/doctest) to run.
…idation Moving the sed step before Swatinem/rust-cache so that Cargo.toml files are already modified when the cache key is computed and when cargo checks fingerprints. Previously, modifying Cargo.toml after cache restore caused cargo to recompile everything. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/tests-rs-workspace.yml (1)
100-104:⚠️ Potential issue | 🟡 MinorInclude llvm-cov target binaries in crash artifacts.
Line 100 currently only scans
target/debug/deps/*. Since tests are launched viacargo llvm-cov nextest(Line 46), symbolization can miss binaries emitted undertarget/llvm-cov-target/debug/deps/*.Suggested patch
- for path in target/debug/deps/*; do - if [[ -f "$path" && -x "$path" && ! "$path" == *.d ]]; then - cp -a "$path" "${ARTIFACT_DIR}/binaries/" - fi - done + for dir in target/llvm-cov-target/debug/deps target/debug/deps; do + for path in "$dir"/*; do + if [[ -f "$path" && -x "$path" && ! "$path" == *.d ]]; then + cp -a "$path" "${ARTIFACT_DIR}/binaries/" + fi + done + done#!/bin/bash set -euo pipefail # Verify command + artifact-path mismatch in workflow nl -ba .github/workflows/tests-rs-workspace.yml | sed -n '43,110p' rg -n 'cargo llvm-cov nextest|target/llvm-cov-target/debug/deps|target/debug/deps' .github/workflows/tests-rs-workspace.yml🤖 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 100 - 104, The artifact-gathering loop currently only copies binaries from "target/debug/deps/*" but tests run via "cargo llvm-cov nextest" can produce binaries under "target/llvm-cov-target/debug/deps/*"; update the loop that references "target/debug/deps/*" (the for path in ...; if [[ -f \"$path\" && -x \"$path\" && ! \"$path\" == *.d ]]; then cp -a \"$path\" ... fi done block) to also scan and copy executables from "target/llvm-cov-target/debug/deps/*" (either by adding a second loop for that glob or iterating both globs) so llvm-cov target binaries are included in the ${ARTIFACT_DIR}/binaries/ crash artifacts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/tests-rs-workspace.yml:
- Around line 100-104: The artifact-gathering loop currently only copies
binaries from "target/debug/deps/*" but tests run via "cargo llvm-cov nextest"
can produce binaries under "target/llvm-cov-target/debug/deps/*"; update the
loop that references "target/debug/deps/*" (the for path in ...; if [[ -f
\"$path\" && -x \"$path\" && ! \"$path\" == *.d ]]; then cp -a \"$path\" ... fi
done block) to also scan and copy executables from
"target/llvm-cov-target/debug/deps/*" (either by adding a second loop for that
glob or iterating both globs) so llvm-cov target binaries are included in the
${ARTIFACT_DIR}/binaries/ crash artifacts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08d50731-b516-46d4-9a9f-74b1dcfbb3c6
📒 Files selected for processing (2)
.github/workflows/tests-rs-doctests.yml.github/workflows/tests-rs-workspace.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/tests-rs-doctests.yml
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Halve the tests per shard to reduce wall clock time. With warm cache, shards should complete in ~8-10 min instead of ~11-15 min. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| - name: Run tests with coverage (shard ${{ matrix.partition }}/8) | ||
| run: | | ||
| ulimit -c unlimited | ||
| cargo llvm-cov nextest \ |
There was a problem hiding this comment.
How do we maintain this list? How we have two packages with paths and this list.
Remove cargo-llvm-cov coverage instrumentation from sharded workspace tests and use sccache + pre-built librocksdb instead, to benchmark speed improvement against the coverage-enabled baseline (PR #3237). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Issue being fixed or feature implemented
Per-package Rust CI test jobs compile each package independently with sccache + pre-built librocksdb, leading to redundant compilation and long wall-clock times. This replaces them with a single sharded workspace runner using
cargo-nextestfor test partitioning andcargo-llvm-covfor coverage, backed bySwatinem/rust-cachefor incremental build caching.What was done?
New workflows
tests-rs-workspace.yml— 8-shard workspace test runner usingcargo llvm-cov nextest --partition count:N/8across 20 packages (drive, dpp, drive-abci, dash-sdk, platform-value, rs-dapi, platform-wallet, rs-sdk-ffi, platform-wallet-ffi, rs-dapi-client, platform-serialization, dapi-grpc, json-schema-compatibility-validator, and 7 contract crates)tests-rs-doctests.yml— separate single-runner workflow forcargo test --workspace --doc(nextest doesn't support doctests)Modified workflows
tests-rs-package.yml— removed thetestjob (replaced by workspace runner); keptlint,formatting,unused_deps,detect_structure_changes,check_each_featuretests.yml— addedrs-workspace-testsandrs-doctestsjobs; added workflow change detection for the new workflow files; addedpushtrigger formasterandv*-devbranchesKey design decisions
cdylibcrate-types viasedbefore cache restore to avoid cargo fingerprint invalidation (cdylib + static rocksdb causesR_X86_64_TPOFF32linker errors)librocksdb-sysbuilds from source, cached bySwatinem/rust-cache--packageflags instead of--workspace --excludeto avoid compiling excluded packages as transitive dependenciescache-on-failure: trueensures build cache is saved even when tests failHow Has This Been Tested?
Breaking Changes
None. CI-only changes — no library or application code modified.
Checklist:
For repository code-owners and collaborators only