paritytech / paritytech/web3-storage

Interface-driven coverage measurement (follow-up to #196)

Open
#286 0 comments 0 reactions 0 assignees View on GitHub

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

  1. 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 call pub items. Interface-gating in practice therefore means "gate on tests/-target coverage", which for pallets means moving mock.rs + tests out of src/ — against SDK-wide convention, and it forbids testing internal invariants directly.

  2. Per-component interface definition. Pallet = Call dispatch + Hooks + inherents + migrations + runtime APIs (via mock runtime); provider-node = axum router + coordinator entry points; provider-negotiation = its pub API. This list must live somewhere reviewed, like COV_PACKAGES does today.

  3. 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).

  4. 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-checks job, 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.