From 80930af448ebabc84174146b56bc106d37fab3b4 Mon Sep 17 00:00:00 2001 From: Ayaan Zaidi Date: Wed, 23 Sep 2026 17:39:44 +0530 Subject: [PATCH] docs(skills): prevent junk tests and add subsystem test-pruning campaigns (#156316) ## Problem Agents keep adding tests that restate implementation or repeat the same contract across layers, and `test-audit` had no repeatable workflow for pruning a whole subsystem's tests. ## Fix The `test-audit` authoring gate now checks new tests against a shared junk-pattern list (tests the retention bar protects are exempt). It requires one primary test per contract at its strongest boundary and one regression per bug. A new campaign mode (`CAMPAIGN.md`) records the step order from the Telegram cleanup in #155040. ## User impact No user-visible change. Agents writing tests reject known junk patterns, and maintainers can run the same subsystem-wide test cleanup on other areas. ## Proof A fresh read-only agent ran the revised skill on `extensions/policy`: 7 lanes, a 329-declaration ledger (296 retain, 26 consolidate, 7 delete), one test-only seam flagged for a maintainer decision, and both deliberately junk test proposals correctly rejected by the authoring gate. Ambiguities it found were fixed in the final commit. Co-authored-by: Ayaan Zaidi --- .agents/skills/test-audit/CAMPAIGN.md | 116 ++++++++++++++++++++++++++ .agents/skills/test-audit/SKILL.md | 77 +++++++++++------ 2 files changed, 168 insertions(+), 25 deletions(-) create mode 100644 .agents/skills/test-audit/CAMPAIGN.md diff --git a/.agents/skills/test-audit/CAMPAIGN.md b/.agents/skills/test-audit/CAMPAIGN.md new file mode 100644 index 000000000000..f29346bf4302 --- /dev/null +++ b/.agents/skills/test-audit/CAMPAIGN.md @@ -0,0 +1,116 @@ +# Test-pruning campaign + +Campaign mode prunes one subsystem's whole test surface in one PR: a plugin +such as `extensions/telegram`, or one core area. The value bar, retention bar, +candidate evidence, and validation in [SKILL.md](SKILL.md) apply to every +lane. This file adds the order of work and the lessons of a full campaign. +Each step ends on its completion criterion; do not start the next step early. + +## 1. Baseline + +Record the subsystem's test and support line counts and every test file's +pass/fail state at a pinned `main` SHA. Keep baseline failures in their own +list: in the Telegram campaign, all three were real delivery bugs, not stale +tests. + +Done when every in-scope test file has a recorded baseline result. + +## 2. Lanes and inventory + +Split the surface into **lanes** along production owner boundaries, not file +prefixes. For Telegram these were accounts, commands, context, dispatch, +inbound, outbound, persistence, transport, shared, harness, and live/QA +scenarios. Include the subsystem's cases at shared core boundaries and its QA +and live-proof harness tests. + +Done when every test file and QA scenario the subsystem owns belongs to exactly +one lane. + +## 3. Read-only ledger per lane + +Give each lane to its own read-only agent. The agent reads every assigned test +in full, including parameter tables. It also reads the production owners and +their entry points, callers, history, and CI routing. Each test declaration +goes into a written **ledger** with one mark. An `it.each` is one declaration +unless its rows need different marks; then mark each row. + +- `R`: retain, naming the contract and the bug it catches; a retained test that + only moves to a better-named file stays `R` with the move noted; +- `F`: retain the contract but repair the assertion, such as a vacuous negative + that passes when only one of several items is missing; +- `C`: consolidate, naming the owner that absorbs the assertion first: a sibling + table case, a stronger boundary suite, or the shared owner in another package; +- `D`: delete, naming the proof that remains, or why no contract exists. + +Judge a test by its assertions, not its name. One Telegram test named for +retiring a progress window asserted the window was _not_ cleared. + +Done when every declaration in the lane has a mark and an evidence line. + +## 4. Layer plan per lane + +Treat the per-test ledger as input, not as the edit list. A second read-only +pass, starting from the ledger, looks for the redundant **layer**. In Telegram, +several dispatch suites replayed the same shared compositor through one mocked +preview, around stronger real-stream and HTTP-fixture suites. Name the +**keeper** suite for each contract. Prefer the real transport boundary with a +fake network over a mocked collaborator. Correct any ledger errors this pass +finds. + +Done when each lane plan names its retired files, its keeper per contract, the +assertions to carry into keepers, and the test-only production seams unlocked. + +## 5. Cutover + +Edit lane by lane. Serialize changes to shared harnesses and support files +through one owner. With each lane, remove the test-only production seams it +unlocks: injection parameters, getters, reset exports, and indirection layers. +Register moved suites in CI routing and test inventories. Update shrink-only +line-cap baselines. Put durable test-ownership rules in the subsystem's +`AGENTS.md`, drawn from mistakes this campaign actually found. + +Done when every lane plan is applied and each lane's keepers pass. + +## 6. Preservation review + +Before claiming completion, have independent reviewers compare deleted +coverage against the keepers, one reviewer per boundary group. They look for +contracts that lost their only proof. They also look for new assertions that +cannot fail, such as a rejection row the production code never reaches. The +Telegram review found nine real gaps and one unreachable assertion. + +For each restored contract, make one deliberate **mutation** of the production +owner and confirm the keeper goes red. Then restore the source byte for byte. + +Done when every reported gap is restored or rejected with source evidence, and +every restored contract has a caught mutation. + +## 7. Product defects + +A baseline failure that survives into a keeper is a bug report. Fix it at its +owner as a separate commit, and prove it through the real user flow, with a +**control** run that reverts the fix and shows the old behavior. Record +unrelated product discrepancies you find as follow-ups instead of fixing them +in the campaign. + +Done when each repaired defect has a failing control and a passing candidate +on the same harness. + +## 8. Reconcile and hand off + +Campaigns outlive many `main` commits. Merge `main` rather than rebasing a +long, many-commit campaign. When `main` modified a file the campaign +deleted, keep the deletion. Port the new contract into the keeper instead, and +confirm every new regression `main` added still has a home. Rerun the whole +subsystem suite and repeat live proof on the merged head. + +Expect review tooling to see a truncated file list on a diff this large. +Record maintainer decisions for generic compatibility flags in the PR evidence +rather than editing gates. + +Hand off with the [SKILL.md](SKILL.md) report, plus: + +- baseline and final test/support line counts, with production counted separately; +- lanes, retired layers, and keepers; +- preservation gaps found and their mutations; +- product defects with control and candidate proof. diff --git a/.agents/skills/test-audit/SKILL.md b/.agents/skills/test-audit/SKILL.md index df8db38db50c..10418bd560c7 100644 --- a/.agents/skills/test-audit/SKILL.md +++ b/.agents/skills/test-audit/SKILL.md @@ -5,11 +5,13 @@ description: "Invoke whenever writing, changing, reviewing, or sweeping tests. A # Test Audit -Two modes, one value bar. Authoring mode gates every new or changed test at +Three modes, one value bar. Authoring mode gates every new or changed test at write time. Audit mode runs focused sweeps of tests that re-assert source, duplicate stronger proof, couple behavior to implementation, or keep test-only production seams alive. Continue broad audits as separate coherent follow-up -PRs; optimize for confidence, not deletion count. +PRs; optimize for confidence, not deletion count. Campaign mode prunes one +whole subsystem's test surface (every test file a plugin or core area owns); +before starting one, read [CAMPAIGN.md](CAMPAIGN.md). ## Authoring gate @@ -18,26 +20,57 @@ add it yet: 1. What observable behavior, invariant, or independent contract does it protect? 2. What credible regression makes it fail? -3. Why does existing coverage not already catch that failure? Prefer extending a - table-driven case or shared fixture over a near-duplicate test; consolidate - duplicated setup in the same change. +3. Why does existing coverage not already catch that failure? Each contract has + one primary test owner at the strongest boundary; another layer needs its + own distinct risk, such as a transport or lifecycle failure the owner cannot + reach. Prefer extending a table-driven case or shared fixture over a + near-duplicate test; consolidate duplicated setup in the same change. 4. Does it need a production seam (export, flag, wrapper, injection hook) that no production caller needs? If yes, move the test to the real boundary instead. -A test that would break under behavior-preserving refactoring is asserting -implementation, not behavior; rewrite it at the owning boundary before landing -it. +Then check the test against every [junk pattern](#junk-patterns); a match fails +the gate unless the [retention bar](#retention-bar) names the contract it +independently guards. A test that would break under behavior-preserving +refactoring is asserting implementation, not behavior; rewrite it at the +owning boundary before landing it. Bug regression tests must fail on the pre-fix code for the intended reason and pass after the owner-boundary repair. A regression test that never demonstrably -failed proves the mock, not the fix. +failed proves the mock, not the fix. One regression at the owner boundary +covers the bug; do not replay the same scenario at every layer it crosses. + +## Junk patterns + +The shared checklist for both modes: the authoring gate rejects a new test that +matches one, and audits hunt for existing tests that do. + +- assertion-free coverage probes; +- self-comparisons and identity copiers; +- copied fixtures, inventories, manifests, or export lists; +- exact source, import, or string greps; +- private predicate or call-shape tests duplicated at real boundaries; +- duplicate invocations of the same contract; +- provider-local replays of shared helpers; +- tests whose only purpose is preserving test-only exports, globals, or wrappers; +- dead production code whose only callers are tests; +- expected values produced by the helper or renderer under test; +- mocks that implement the asserted behavior, or one identical mock standing in + for different APIs; +- fixtures that supply the receipt, admission, or callback ordering the owner + should produce, or persistence asserted against a store the path never writes; +- capability tests that restate declared flags instead of exercising the + delivery or acknowledgement the flag promises; +- negative controls that pass for an unrelated reason, such as a denial from a + different guard or a rejection the production path never reaches; +- names or fixtures that promise more than the input exercises, such as a + "retires the window" test asserting the window was not cleared. ## Value bar Tests justify their maintenance cost by protecting behavior, a credible -regression, or an independently meaningful contract. A test that must change -for behavior-preserving source reorganization is suspect, not automatically -deletable. +regression, or an independently meaningful contract. In an audit, an existing +test that must change for behavior-preserving source reorganization is suspect, +not automatically deletable; the authoring gate still rejects new ones. Before judging a candidate, read the complete test and production owner, its entry point, callers, callees, sibling implementations, overlapping tests, CI @@ -55,18 +88,8 @@ run parallel discovery lanes when available: - UI, apps, scripts, and tooling; - a cross-cutting pattern sweep. -Prefer a few high-confidence candidates over a large speculative inventory. -Look for: - -- assertion-free coverage probes; -- self-comparisons and identity copiers; -- copied fixtures, inventories, manifests, or export lists; -- exact source, import, or string greps; -- private predicate or call-shape tests duplicated at real boundaries; -- duplicate invocations of the same contract; -- provider-local replays of shared helpers; -- tests whose only purpose is preserving test-only exports, globals, or wrappers; -- dead production code whose only callers are tests. +Outside campaign mode, prefer a few high-confidence candidates over a large +speculative inventory. Hunt for the [junk patterns](#junk-patterns). ## Retention bar @@ -76,7 +99,11 @@ cross-language, package, release, or architecture contract. Also keep: - call ordering when order is observable behavior; - regressions with a credible failure mode; -- source inspection when it is the cheapest independent guard. +- source inspection when it is the cheapest independent guard: it fails when + the contract changes (the user-facing key, byte, or path) and survives an + identifier-only refactor; +- a retained test that fails on the baseline: treat it as a possible product + bug, reproduce it, and repair the owner rather than deleting it. Static or slow is not a deletion reason. A test that resembles implementation may still be the independent contract; prove otherwise before removing it.