paritytech / paritytech/web3-storage
Interface-driven coverage measurement (follow-up to #196)
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 12
- Forks
- 3
- Avg merge
- 2d 2h
- Merged PRs (30d)
- 33
Description
Motivation
The coverage gate from #196 counts reach via all tests: a patch line covered only by a private unit test passes even if no exposed interface reaches it. The review discussion on #196 (comment) proposed evolving toward interface-driven measurement: a line counts as covered only when it is reachable by entering through the component's own exposed interface.
The property this buys: "the code is wired into the product, not just into a test" — it flags dead code whose only remaining caller is its test.
#196 already ships the diagnostic half (integration-only view in coverage-integration.md, not gated). This issue tracks whether/how to go further.
Current state
scripts/coverage.sh produces two views from one test run:
| View | Source | Role |
|---|---|---|
| merged | all test targets | gated via diff-cover (MIN_PATCH_COV) |
| integration-only | --test '*' targets |
diagnostic, not gated |
The integration-only view approximates "entered via the exposed interface" for provider-node (its tests/ suites drive the axum router and the trait-mocked coordinators), but zeroes out the pallets: pallet tests live in src/ behind mock.rs per polkadot-sdk convention, so they land in the unit bucket even though they enter through extrinsic dispatch — which is the pallet's public surface.
"Exposed interface" is wider than extrinsics + HTTP
Concrete in-repo entry points that a naive definition would mislabel as unreachable:
| Entry point | Where | Entered by |
|---|---|---|
Hooks (on_initialize/on_finalize) |
pallet/src/lib.rs (challenge slashing runs here) |
block execution |
migrations (OnRuntimeUpgrade) |
pallet/src/migrations.rs, storage-interfaces/file-system/pallet-registry/src/migrations.rs |
runtime upgrade |
runtime APIs (provider_info, providers, bucket_info, …) |
declared in pallet/src/runtime_api.rs, implemented in the runtime crates |
node RPC, across the runtime boundary — never an extrinsic |
| library crates (Rust API, no wire protocol) | provider-negotiation (consumed by both node and client) |
pub API |
The runtime-API row deserves emphasis: today the backing pallet query functions count as covered only because pallet/src/tests/runtime_api.rs calls them directly, while their real callers (the impl_runtime_apis! blocks) sit in crates the gate skips entirely. That's exactly the "reached by test, not by interface" pattern — except here it is the correct outcome, which a naive interface gate would flag as dead.
dead_code already covers part of this
The dead-code failure mode motivating the proposal is already partially enforced: a private helper whose only non-test caller is removed trips rustc's dead_code lint in the non-cfg(test) compilation, and CI clippy runs with RUSTFLAGS: -D warnings (.github/workflows/check.yml).
What that does not catch is unused pub items — the residual class interface-gating would add. But per the runtime-API row above, for cross-crate pub surface "unused" cannot even be decided inside the measured crate.
Problems any gating design must answer
-
No tool measures call-graph origin. Neither cargo-llvm-cov nor lcov can express "count this line only if the call stack entered via X". The only mechanization Rust offers is visibility: code in
tests/can only callpubitems. Interface-gating in practice therefore means "gate ontests/-target coverage", which for pallets means movingmock.rs+ tests out ofsrc/— against SDK-wide convention, and it forbids testing internal invariants directly. -
Per-component interface definition. Pallet = Call dispatch + Hooks + inherents + migrations + runtime APIs (via mock runtime); provider-node = axum router + coordinator entry points;
provider-negotiation= itspubAPI. This list must live somewhere reviewed, likeCOV_PACKAGESdoes today. -
Defensive code is unreachable by design.
defensive_*branches and can't-happen error arms are required by our own guidelines and reachable through no interface. A strict interface gate creates pressure to delete exactly the code we mandate. Needs an explicit carve-out (or stays the reason gating is a bad idea). -
Name the check after the property. If the gate's purpose becomes dead-code detection rather than a quality metric, the CI job and summary headings should say so — "Rust coverage" reads as "how well tested is this", which invites percentage-chasing. Something like "Dead code: not reachable from an exposed interface" states what a failure means and what the fix is (wire it in or delete it — not "add a unit test"). Cheap: branch protection pins the aggregate
basic-checksjob, so renaming the coverage job/headings breaks nothing.
Potential directions
A — status quo+
Keep interface-reach as diagnostic column + review convention; rely on dead_code + -D warnings for the private-item case. Cheapest, loses nothing we currently have. Even here, renaming the diagnostic heading (e.g. "Reachable from exposed interfaces" instead of "Integration-only coverage") would make the column's intent self-explanatory.
B — lint the residue
Attack unused-pub directly instead of via coverage — e.g. unreachable_pub where applicable, periodic dead-pub sweeps. Solves the motivating failure mode without touching test layout, but must whitelist cross-crate surface like runtime-API backing functions whose callers live in skipped crates.
C — full interface gating
Restructure pallet tests into tests/, define the per-component interface list, add defensive-code carve-outs, gate diff-cover on lcov-integration.info, and rename the job per problem 4. Highest cost; blocked on problems 1–4 above.
TODO
- Agree which direction (A/B/C) is worth the cost
- Rename job/headings to state the property being checked (applies to any direction)
- If B: pick the lint/tooling and wire it into
check.yml - If C: define the per-component interface list and the defensive-code carve-out first
Related
- #196 — coverage gate + integration-only diagnostic view (this issue's origin)
- #176 — motivating coverage issue
- #265 — Quint spec for the challenge protocol (correctness assurance; coverage proves reach, not correctness)
- #178 — provider split (changes the component list any interface definition hangs off)
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with scripts/coverage.sh, coverage-integration.md, and .github/workflows/check.yml to understand the current coverage views and CI gate. Then review the listed pallet, runtime API, migration, and provider entry points before evaluating directions A, B, and C. Done means one direction is agreed, its exclusions and component interfaces are defined, and the related TODOs are resolved.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- github-actions, rust
- Domain
- ci-cd, testing-qa
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Needs clarification
- Newbie friendliness
- 30/100