ci: run the shielded wallet suite and stop the shield filter sweeping up unrelated tests - #4579
ci: run the shielded wallet suite and stop the shield filter sweeping up unrelated tests#4579Claudius-Maginificent wants to merge 3 commits into
Conversation
…rom the reserve `regression_reports_max_from_usable_suffix_not_total_account_balance` failed on `--all-features`, on this branch and on its base. The failing line was the test's own shape-precondition guard, not a production assertion, and its message said what to do: re-seed the balances when the versioned reserve drops below the hardcoded leading balance. That is exactly what happened. `reserve()` is `2 x compute_minimum_shielded_fee(2 actions)`. Under the v9 event constants (proof verification 100_000_000, storage 344 bytes/action) it was 325_702_400; under v10 (40_000_000 and 550 bytes/action) it is 228_280_000. The fixture's 297_264_780 sits between the two, so the leading address stopped being sub-reserve dust and the "usable suffix" shape the test claims to build no longer existed. The planner is right either way: with a lower reserve that address genuinely is a viable input 0, so the whole balance genuinely is usable. Maximum spendable balance was never miscomputed. Derive the leading balance as `reserve() - 1` instead — the largest balance that must still be rejected by the strict `> reserve` viability test, so a tighter boundary than the magic number was, and one no future fee re-balance can invalidate. The guard goes with it: the precondition is now true by construction. CI never caught this. The wallet job filtered nextest with `not test(~shield)`, a substring match on the full test path, so the `shield_input_selection_tests` module was excluded as collateral by a filter aimed at the shielded-wallet suite. It took 47 pure-logic tests across the three wallet crates with it — input selection, FFI error codes and memo encoding, SQLite viewing-key rows — for a measured 0.089 s of runtime. Exclude by module path (`wallet::shielded::`) so the step skips what it means to skip, and pin `--no-tests fail` so a filter that stops selecting anything fails the step instead of passing green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ages `-E 'not test(~shield)'` is a substring match on the full test path, so it selects "anything with the word shield in its name", not "the shielded suites". For dpp, drive, drive-abci and dash-sdk that is survivable: the "Run shielded tests" phase compensates, and those packages genuinely need it — they are Orchard proving tests, and that phase runs them under `cargo test` in one shared process so a single verifying key is built instead of one per test. The three wallet packages have neither the compensator nor the need. They are absent from the shielded phase's package list, so everything the substring dropped fell into a gap no job covered: 145 `wallet::shielded::` tests, plus ~47 more whose only sin is the word — input selection, FFI error codes, memo encoding, SQLite viewing-key rows. Measured, they cost ~6 s wall under nextest's process-per-test model, so the VK-reuse argument that justifies the exclusion elsewhere does not apply to them. Carve the wallet packages out of the exclusion by package rather than retarget the pattern by module path: a module-path filter would also stop excluding dpp/drive/drive-abci/dash-sdk, pulling their proving tests into the nextest phase where each would rebuild its own verifying key. The strategy-simulation phase keeps its `~shield` exclusion for the same reason — drive-abci IS in the compensating phase. Both nextest invocations pin `--no-tests fail` so an expression that stops selecting anything fails the step instead of passing green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 145 `wallet::shielded::` tests ran in no CI job at all. Both workflows excluded them, and tests-rs-workspace.yml's compensating "Run shielded tests" phase covers dpp / drive / drive-abci / dash-sdk only — the three wallet packages are absent from its package list. The sibling commit fixes the workspace path; this fixes the fast path a wallet-scoped PR actually takes, and both are needed because the two workflows are mutually exclusive for any given PR. Drop the test filter entirely rather than narrow it again. Nothing in these three packages is worth excluding: the shielded suite is 145 tests in ~6 s wall (~97 s CPU) under nextest's process-per-test model, and `--all-features` compiles those binaries whether or not they are selected — so the exclusion was saving execution time on artifacts already built and discarded. Removing the expression also removes the whole class of substring-filter bug from this workflow rather than moving it. Measured on this base: 1804 tests, 1804 passed, 25.6 s for the step. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChangesWallet test coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change restores wallet test coverage, prevents empty filtered runs from passing, and updates a stale boundary fixture without changing product or runtime behavior; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
🕓 Ready for review — 19 ahead in queue (commit 5377ad6) |
lklimek
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4579 +/- ##
=========================================
Coverage 86.91% 86.91%
=========================================
Files 2756 2756
Lines 360329 360329
=========================================
+ Hits 313162 313163 +1
+ Misses 47167 47166 -1
🚀 New features to boost your workflow:
|
|
Reviewed this against two findings from the #3968 review that were deferred here. One is fully closed by this PR; the other is not, and this PR slightly widens it. Closed. The two wallet CI paths no longer disagree on the shielded filter. Still open: no
On a runner carrying a pre-0.9.85 nextest, the step dies with a clap usage error naming an argument — a red build whose diagnosis looks nothing like a test failure, on a self-hosted runner whose toolchain state nobody in this repo controls. The failure mode is worse than the bug it guards against, because a maintainer reading the log sees a broken workflow rather than a broken filter. Cheap fixes, either is fine:
Not a blocker for the filter work here — it just needs to not be forgotten, since this PR is what makes 🤖 Reported by Claudius the Magnificent AI Agent, from the #3968 review triage |
3 participants
TL;DR
Two bugs in our test workflows meant 185 wallet tests were being skipped on every CI run, and 145 of them were running in no job at all. One of the skipped tests had been failing for some time — a test that guards the maximum spendable balance reported to a user. Nothing surfaced it, because the filter meant to skip one suite was matching a word instead.
This turns those tests back on, fixes the one that was red, and adds a guard so a filter that stops matching anything fails the build instead of quietly reporting success.
User story
As a developer working on wallet code, I want the tests I write to actually run in CI, so that a broken change is caught before it merges rather than months later by someone who did not write it.
As a reviewer, I want a green check to mean the suite ran, so that an empty test run cannot pass as success.
Scenario
Actual behavior
A test filter intended to skip the shielded wallet suite instead skipped every test whose name contained the word "shield" anywhere. That swept in 40 unrelated tests — input-selection arithmetic, error-code mappings, memo encoding — one of which was failing and reported by nothing. Separately, the 145 tests it was actually aimed at were excluded by both workflows and covered by neither, so they ran nowhere.
Expected behavior
Each workflow skips only what it means to skip and what another job genuinely covers. The 145 shielded wallet tests run. The 40 unrelated tests run. A filter that matches nothing fails the step.
Issue being fixed or feature implemented
Two independent CI defects, plus the one failing test they were hiding.
Bug 1 — the filter matched a word, not a suite
Both Rust test workflows excluded shielded tests with
-E 'not test(~shield)'. nextest'stest(~…)is a substring match against the full test path, module segments included, so this does not select "the shielded suites" — it selects anything with the wordshieldanywhere in its name.Across the three wallet packages it dropped 185 tests. Only 145 were the shielded suite. The other 40 were ordinary logic tests that happen to contain the word:
wallet::platform_wallet::shield_input_selection_tests— pure arithmetic over input selection and reported maximum spendable balancechangeset::shielded_changeset::activity_changeset_testsplatform-wallet-ffi— error-code mappings, memo encoding, preflight null-pointer checksThey cost about a tenth of a second and were being thrown away.
One of them was failing.
regression_reports_max_from_usable_suffix_not_total_account_balanceguards the maximum spendable balance a shield operation reports — funds-adjacent. It had been red for some time and nothing surfaced it, because the filter aimed at the shielded suite swept up the module by name.The failure was a stale fixture, not a code bug.
reserve()is2 × compute_minimum_shielded_fee(2 actions); the v10 event constants (proof verification100_000_000 → 40_000_000, storage344 → 550bytes/action) moved it from325_702_400to228_280_000. The test's hardcoded leading balance of297_264_780sits between the two, so the "leading address is below the reserve" shape it asserts stopped existing. The planner is correct either way — with a lower reserve that address genuinely is a viable fee-paying input. The fixture now derives that balance asreserve() - 1, the largest value that must still be rejected by the strict> reservetest: a tighter boundary than the constant was, and one no future fee change can invalidate. The shape-precondition guard is deleted because it became true by construction.Bug 2 — 145 tests ran in no CI job at all
tests-rs-workspace.ymlexcluded shielded tests and compensated with a dedicated "Run shielded tests" phase. That phase runs--package dpp --package drive --package drive-abci --package dash-sdk. The three wallet packages are not in it.So the entire
wallet::shielded::suite — Orchard proving, viewing-key binds, note scans, sync — was excluded by both workflows and executed by neither. 145 tests, no coverage, no signal.Measured on this branch: 145 run, 145 passed, 6.1 s wall clock (roughly 40–100 s of CPU depending on cache warmth). Nothing was red; the suite had simply gone unobserved.
The cost argument for excluding them does not hold either.
--all-featurescompiles those test binaries whether or not the filter selects them, so the exclusion saved only execution time on artifacts that were already built and then discarded. Six seconds.Fixed in both workflows, because they are mutually exclusive paths —
tests-rs-wallet.ymlis a fast path that runs instead of the workspace job for wallet-scoped PRs, so fixing one alone leaves the suite unrun on the other. Dropping either commit leaves the suite unrun on that path.What was done?
Three files: the two workflows and one test fixture.
Why the workspace fix scopes by package, not module path
The workspace job's
~shieldexclusion is legitimate for dpp / drive / drive-abci / dash-sdk: those are Orchard proving tests, and the compensating phase deliberately runs them undercargo testin one shared process so a single verifying key is built rather than one per test. Retargeting that filter by module path would pull them into thenextestphase, where process-per-test means each rebuilds its own VK — and they would run twice.The wallet packages need none of that: 145 tests in 6 s under nextest. So the exclusion is scoped by package instead — keep a test if it is not shield-named or it belongs to a wallet package:
The strategy-simulation step keeps its
~shieldexclusion unchanged, because drive-abci is in the compensating phase, so that exclusion is compensated and is not a hole.Zero-match guard
Every nextest invocation touched here pins
--no-tests fail. A filter that stops selecting anything now fails the step instead of reporting green over an empty run — verified by pointing a deliberately non-matching expression at the suite and watching it exit 4. Pinned rather than left to nextest's default: a default that can change across a version bump is not a guarantee.Scope note
The fixture fix travels with the workflow change on purpose — narrowing the filter is precisely what makes that test start running, so shipping the workflow alone would land this PR red on its own change.
How Has This Been Tested?
On this branch, through the repository's own commands:
diff).package()scoping confirmed to discriminate: excluding two wallet packages from atest(~shield)selection leaves exactly the 24platform-wallet-ffitests and nothing else.cargo clippy -p platform-wallet --all-targets --all-features --locked -- -D warnings: clean.Not verified, stated deliberately. The workspace job itself was not executed — proving the expression's effect on dpp/drive/drive-abci would mean building their test binaries. Its behaviour there rests on the
package()discrimination proof plus boolean reasoning, not on execution.Flakiness caveat. The 145-test result is one run on one machine with generous parallelism. It establishes that the suite passes and is not slow; it does not establish that it is non-flaky on a loaded CI runner, and Orchard proving tests are exactly the kind that can be. The honest expectation is "turn it on and watch the first few runs", not "turn it on and forget it". If it proves flaky, the right response is to fix or quarantine the specific tests, not to reinstate a filter that hides 40 unrelated ones with them.
Breaking Changes
None. CI configuration and one test fixture; no library, API, or runtime behaviour changes.
Note for reviewers also following #3968
That branch adds further
platform-wallet-storagetests withshieldedin their names, which the old filter would also have swept up. They do not exist on this base, so they are not counted in any figure above.Checklist:
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit