diff --git a/.github/workflows/quality-resolve-probe.yml b/.github/workflows/quality-resolve-probe.yml index f12b5738..382b22ec 100644 --- a/.github/workflows/quality-resolve-probe.yml +++ b/.github/workflows/quality-resolve-probe.yml @@ -335,6 +335,42 @@ jobs: PY echo "OK — the matrix is derived from info.xml, and it changes when info.xml does." + coverage-guard: + name: "The coverage guard's deletion rule" + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v4 + + # quality-config/coverage-guard.php is copied into all 18 app repos and its + # verdict blocks pull requests, and until now nothing in this repository + # executed it. That is the same shape as every other job here: an + # instrument nobody has watched refuse. + # + # The suite runs the SHIPPED program against clover reports built to the + # shape PHPUnit actually emits, and it FAILS FIRST — the two pull requests + # the file-scoped rule blocked (opencatalogi#895, launchpad#128) are + # asserted to still fail under the old flags, with the exact percentages CI + # printed, before the new flag is exercised at all. A test that only ever + # sees the fixed program cannot tell a fix from a no-op. + - name: "Exercise the shipped coverage guard" + run: python3 quality-config/tests/test-coverage-guard.py + + # POSITIVE CONTROL FOR THE HARNESS ITSELF. The suite above shells out to + # `php`; if php were missing, or the guard path wrong, a harness that + # treated "could not run" as "nothing to report" would print a clean pass + # forever. Point it at a file that is not the guard and require it to fail. + - name: "Positive control — the suite must fail against a program that is not the guard" + run: | + set -eu + printf '#!/usr/bin/env php\n "$RUNNER_TEMP/not-the-guard.php" + if python3 quality-config/tests/test-coverage-guard.py "$RUNNER_TEMP/not-the-guard.php" > /dev/null 2>&1; then + echo "::error::the coverage-guard suite reported PASS against a program that exits 0 and \ + measures nothing. It is not executing the subject, so its verdict above means nothing." + exit 1 + fi + echo "OK — the suite fails when handed the wrong program. Its clean pass above is a verdict." + probe: name: "quality.yml resolves (job count > 0)" runs-on: ubuntu-latest @@ -412,7 +448,7 @@ jobs: guard: name: "Shared-workflow guard" runs-on: ubuntu-latest - needs: [static-limits, seed-semantics, gate-completeness, matrix-derivation, probe] + needs: [static-limits, seed-semantics, gate-completeness, matrix-derivation, coverage-guard, probe] if: ${{ !cancelled() }} steps: - name: Assert both guards reached a verdict @@ -421,13 +457,14 @@ jobs: SEED: ${{ needs.seed-semantics.result }} LEGS: ${{ needs.gate-completeness.result }} MATRIX: ${{ needs.matrix-derivation.result }} + COVERAGE: ${{ needs.coverage-guard.result }} PROBE: ${{ needs.probe.result }} run: | set -eu - echo "static-limits=${STATIC} seed-semantics=${SEED} gate-completeness=${LEGS} matrix-derivation=${MATRIX} probe=${PROBE}" + echo "static-limits=${STATIC} seed-semantics=${SEED} gate-completeness=${LEGS} matrix-derivation=${MATRIX} coverage-guard=${COVERAGE} probe=${PROBE}" # `skipped` is failed here on purpose. A guard that did not run is # not a guard that passed. - for pair in "static-limits:${STATIC}" "seed-semantics:${SEED}" "gate-completeness:${LEGS}" "matrix-derivation:${MATRIX}" "probe:${PROBE}"; do + for pair in "static-limits:${STATIC}" "seed-semantics:${SEED}" "gate-completeness:${LEGS}" "matrix-derivation:${MATRIX}" "coverage-guard:${COVERAGE}" "probe:${PROBE}"; do name="${pair%%:*}"; result="${pair##*:}" [ "${result}" = "success" ] || { echo "::error::${name} did not succeed (result=${result}). A guard that \ diff --git a/.github/workflows/quality.yml b/.github/workflows/quality.yml index cf822e82..5bbc82d9 100644 --- a/.github/workflows/quality.yml +++ b/.github/workflows/quality.yml @@ -2596,9 +2596,51 @@ jobs: # changed, nothing to compare" rather than as zero coverage. git diff --name-only "$MERGE_BASE" HEAD -- '*.php' > "$RUNNER_TEMP/changed-php.txt" || true echo "Changed PHP files in this PR: $(wc -l < "$RUNNER_TEMP/changed-php.txt")" + + # ── DELETIONS ARE NOT COVERAGE REGRESSIONS ──────────────────────── + # + # The scoping above removed the noise; it did not remove the + # ARITHMETIC. `cgRatioDropped()` is one integer cross-product with no + # tolerance, and read plainly it demands that the code you touched be + # at least as well covered as the code you did not. For additions that + # is exactly right. For deletions it inverts: removing `d` statements + # of which `c` were covered lowers the ratio whenever the deleted code + # was better tested than what remains — and DEAD CODE IS DEAD BECAUSE + # NOTHING CALLS IT, NOT BECAUSE NOTHING TESTED IT. gate-57 + # (orphaned-write-capability) exists to find precisely that code, so + # the two are in arithmetic opposition, not tension. Such a pull + # request cannot satisfy the ratchet from inside its own subject: the + # only moves are delete less, add filler, or delete additional + # *uncovered* statements until it balances. Measured on + # opencatalogi#895 (-0.62% scoped) and launchpad#128 (-0.32% scoped), + # both of which deleted dead code and neither of which could comply. + # + # `--deletion-neutral` compares METHOD BUCKETS asymmetrically: + # base-only methods leave the base side, head-only methods stay on the + # head side. A pure deletion becomes exactly neutral; a regression in + # surviving code and new untested code both still fail. The symmetric + # version of that rule is the obvious one and it is broken — it also + # drops head-only statements, so forty new statements with none + # covered pass. quality-config/tests/test-coverage-guard.py asserts all + # four cases and kills a mutant that reintroduces the symmetric rule. + # + # PROBED, not assumed, for the same reason as the flag above: a copy + # predating this flag would accept it and ignore it, silently + # restoring the deletion penalty while reporting success. Older copies + # keep today's behaviour rather than erroring — this narrows an + # existing check, it is not the difference between a check running and + # not running. + CG_MODE=() + if php scripts/coverage-guard.php --capabilities 2>/dev/null | grep -qx deletion-neutral; then + CG_MODE+=(--deletion-neutral) + else + echo "::notice::scripts/coverage-guard.php predates --deletion-neutral, so deleting well-tested dead code will still read as a coverage drop. Copy the canonical version from ConductionNL/.github at quality-config/coverage-guard.php to pick it up." + fi + php scripts/coverage-guard.php coverage/clover.xml \ --against="$RUNNER_TEMP/base-clover.xml" \ - --changed-files="$RUNNER_TEMP/changed-php.txt" + --changed-files="$RUNNER_TEMP/changed-php.txt" \ + "${CG_MODE[@]}" else php scripts/coverage-guard.php coverage/clover.xml --against="$RUNNER_TEMP/base-clover.xml" fi diff --git a/quality-config/coverage-guard.php b/quality-config/coverage-guard.php index 281de08f..1b27e667 100644 --- a/quality-config/coverage-guard.php +++ b/quality-config/coverage-guard.php @@ -5,6 +5,8 @@ * * Usage: * php scripts/coverage-guard.php [--against=] + * php scripts/coverage-guard.php --against= --changed-files= + * php scripts/coverage-guard.php --against= --changed-files= --deletion-neutral * php scripts/coverage-guard.php [--update-baseline] * php scripts/coverage-guard.php --capabilities * @@ -53,7 +55,16 @@ * check that did not run looking exactly like one that passed. The probe turns * that into a loud failure. */ -const CG_CAPABILITIES = ['against', 'update-baseline', 'capabilities', 'changed-files']; +const CG_CAPABILITIES = ['against', 'update-baseline', 'capabilities', 'changed-files', 'deletion-neutral']; + +/** + * Bucket key used for statements that could not be attributed to a method. + * + * It is a real key, present on both sides, so a file that falls back is compared + * exactly as it is today — conservatively, deletions included. It is never a way + * for a file to disappear from the comparison. + */ +const CG_WHOLE_FILE = ''; /** * Sum clover metrics for a named subset of files. @@ -138,6 +149,242 @@ function cgMeasureFiles(string $file, string $label, array $only): array return [$statements, $covered, $percentage]; } +/** + * Attribute every statement in the named files to the METHOD it sits in. + * + * WHY BY METHOD NAME, AND WHY NOT BY LINE NUMBER. + * + * Clover identifies a statement only by its `num` — the line it sits on — and a + * deletion shifts every line after the cut. On launchpad#128's file, 117 of the + * 254 surviving statements (46%) sit at or after the deletion site, so matching + * base statement `num=310` to head statement `num=310` compares two unrelated + * lines across half the file and returns a confident, meaningless verdict. The + * test suite measures that directly on its fixture: a line-number intersection + * there reports 182/234 head against 189/234 base, i.e. a fabricated regression, + * where method-name attribution reconciles exactly at 183/254 on both sides. + * Method names survive the shift; line numbers do not. + * + * THE SHAPE CLOVER ACTUALLY EMITS, verified against 2 608 file entries in four + * real PHPUnit artifacts from this fleet: + * + * + * + * + * + * ... + * + * + * + * `` elements are FLAT and in document order; a `type="method"` line opens + * a method and every `type="stmt"` line after it belongs to that method until the + * next one. `metrics/@statements` counts the `stmt` lines only — methods are + * counted separately and `elements` is their sum — so summing attributed + * statements reproduces the file metric wherever the line data is complete. + * + * The bucket key is the REPO-RELATIVE path that matched, never the clover + * `name`. The two reports are produced from two different checkouts and their + * absolute paths differ, so keying on `name` would put every head bucket and + * every base bucket in disjoint namespaces and the intersection would be empty — + * which reads as "nothing to compare", i.e. a pass. + * + * TWO DEGENERATE SHAPES, BOTH HANDLED FAIL-CLOSED: + * + * - A file whose metrics declare statements but which carries NO usable line + * data (the `processUncoveredFiles` shape: PHPUnit counts a file it never + * loaded). Attribution would measure it as 0/0 — an invisible pass on exactly + * the file most likely to be untested. Such a file falls back to a single + * CG_WHOLE_FILE bucket carrying its file-level metrics, the fallback is + * printed by name, and cgCollapseFiles() then puts BOTH sides on the same + * footing so the fallback cannot be mistaken for a deletion. + * - Two methods of the same name in one file (two classes in one file, in + * principle). Their statements are SUMMED into one bucket rather than + * disambiguated by ordinal, because ordinals shift when one of them is + * deleted. Summing keeps the bucket in the intersection, so deleting one of a + * same-named pair is still charged against the change. Not seen in any of the + * 2 608 entries surveyed; handled so it cannot become a silent hole. + * + * @param string $file Clover report to read. + * @param string $label Human label for error messages. + * @param array $only Repo-relative paths to include. + * + * @return array{0: array, 1: array, 2: array} + * buckets keyed "::", the repo-relative paths this report + * matched, and the paths that had to fall back to file-level metrics. + */ +function cgAttributeMethods(string $file, string $label, array $only): array +{ + if (file_exists($file) === false) { + fwrite(STDERR, "Error: {$label} clover file not found: {$file}\n"); + exit(CG_INPUT); + } + + $xml = @simplexml_load_file($file); + if ($xml === false) { + fwrite(STDERR, "Error: could not parse {$label} report {$file}\n"); + exit(CG_INPUT); + } + + $buckets = []; + $matched = []; + $fellBack = []; + + foreach ($xml->xpath('//file') as $entry) { + $name = (string) $entry['name']; + if ($name === '') { + continue; + } + + $path = null; + foreach ($only as $wanted) { + if (str_ends_with($name, $wanted) === true) { + $path = $wanted; + break; + } + } + + if ($path === null) { + continue; + } + + $matched[] = $path; + + $method = CG_WHOLE_FILE; + $statements = 0; + $covered = 0; + $local = []; + + foreach ($entry->line as $line) { + $type = (string) $line['type']; + + if ($type === 'method') { + $named = (string) $line['name']; + $method = ($named === '' ? CG_WHOLE_FILE : $named); + continue; + } + + if ($type !== 'stmt') { + continue; + } + + $key = ($path . '::' . $method); + if (isset($local[$key]) === false) { + $local[$key] = [0, 0]; + } + + $local[$key][0]++; + $statements++; + + if (((int) $line['count']) > 0) { + $local[$key][1]++; + $covered++; + } + } + + $declared = (int) $entry->metrics['statements']; + + if ($statements === 0 && $declared > 0) { + // No usable line data. Measuring 0/0 here would drop the file out of + // the comparison entirely, so fall back to the file metric and say so. + $fellBack[] = $path; + $local = [ + ($path . '::' . CG_WHOLE_FILE) => [$declared, (int) $entry->metrics['coveredstatements']], + ]; + echo " note: {$label} report carries no line data for {$path}; " + . "falling back to its file-level metric ({$entry->metrics['coveredstatements']}/{$declared}).\n"; + } else if ($statements !== $declared) { + echo " note: {$label} report declares {$declared} statement(s) for {$path} but emits " + . "{$statements} attributable line(s); the attributable ones are what is compared.\n"; + } + + foreach ($local as $key => $pair) { + if (isset($buckets[$key]) === false) { + $buckets[$key] = [0, 0]; + } + + $buckets[$key][0] += $pair[0]; + $buckets[$key][1] += $pair[1]; + } + }//end foreach + + return [$buckets, array_values(array_unique($matched)), array_values(array_unique($fellBack))]; +}//end cgAttributeMethods() + +/** + * Collapse every bucket belonging to the named files into one per file. + * + * THIS IS THE HOLE THAT WRITING THE TESTS FOUND, and it is worth naming because + * it is the invisible-pass shape in its purest form. If one side of the + * comparison cannot be attributed to methods it falls back to a single + * CG_WHOLE_FILE bucket — but that bucket's key then exists on ONE side only, so + * the asymmetric rule classifies the entire base-side file as "deleted", drops + * it, finds nothing left to compare, and reports: + * + * OK: none of the changed PHP existed at the merge base + * + * A file the guard could not read would have passed every possible drop. So when + * EITHER side falls back for a file, BOTH sides are collapsed to that file's + * single bucket: the key then exists on both sides, the file is compared exactly + * as the file-scoped mode compares it today, and the deletion penalty applies to + * it. Conservative, and visible in the output. + * + * @param array $buckets + * @param array $paths + * + * @return array + */ +function cgCollapseFiles(array $buckets, array $paths): array +{ + if (empty($paths) === true) { + return $buckets; + } + + $collapse = array_fill_keys($paths, true); + $out = []; + + foreach ($buckets as $key => $pair) { + $split = strrpos($key, '::'); + $path = ($split === false ? $key : substr($key, 0, $split)); + + if (isset($collapse[$path]) === true) { + $key = ($path . '::' . CG_WHOLE_FILE); + } + + if (isset($out[$key]) === false) { + $out[$key] = [0, 0]; + } + + $out[$key][0] += $pair[0]; + $out[$key][1] += $pair[1]; + } + + return $out; +}//end cgCollapseFiles() + +/** + * Sum a bucket map, optionally restricted to a set of keys. + * + * @param array $buckets + * @param array|null $keep Keys to include, or null for all. + * + * @return array{0:int,1:int} statements, covered + */ +function cgSumBuckets(array $buckets, ?array $keep = null): array +{ + $statements = 0; + $covered = 0; + + foreach ($buckets as $key => $pair) { + if ($keep !== null && isset($keep[$key]) === false) { + continue; + } + + $statements += $pair[0]; + $covered += $pair[1]; + } + + return [$statements, $covered]; +}//end cgSumBuckets() + /** * Read the changed-file list, keeping only PHP files the guard can measure. * @@ -299,6 +546,15 @@ function cgReport(string $label, int $statements, int $covered, float $percentag $baselineFile = (__DIR__ . '/../.coverage-baseline'); $against = ($options['against'] ?? null); $changedList = ($options['changed-files'] ?? null); +$deletionFree = isset($options['deletion-neutral']); + +// A flag that is accepted and ignored is the silent-downgrade shape this script's +// `--capabilities` probe exists to prevent, so refuse rather than fall through to +// a comparison the caller did not ask for. +if ($deletionFree === true && (is_string($changedList) === false || $changedList === '')) { + fwrite(STDERR, "Error: --deletion-neutral requires --changed-files (and therefore --against). It refines the file-scoped comparison; it has no meaning against the whole project.\n"); + exit(CG_INPUT); +} // ── scoped mode: compare ONLY the PHP the change touched ──────────────────── // @@ -321,6 +577,153 @@ function cgReport(string $label, int $statements, int $covered, float $percentag echo 'Scoped to ' . count($changed) . " changed PHP file(s).\n"; + // ── deletion-neutral mode ─────────────────────────────────────────────── + // + // WHAT THIS FIXES. `cgRatioDropped()` is a single integer cross-product with + // no tolerance, so the file-scoped ratchet demands, exactly: + // + // the code you touched must be at least as well covered as the code you + // did not. + // + // For ADDITIONS that is right, and it earns its keep: openconnector#1265 + // covered 27 of 54 new statements against a 61.86% floor, needed 34, and + // writing the missing seven is what exposed a cascade that deleted nothing + // and reported success. + // + // For DELETIONS it INVERTS. Removing `d` statements of which `c` were covered + // lowers the ratio whenever c/d exceeds the ratio of what remains — i.e. + // whenever the deleted code was better tested than average. And dead code is + // dead because nothing CALLS it, not because nothing TESTED it: gate-57 + // (orphaned-write-capability) exists to find precisely that code, so gate-57 + // and this ratchet are in arithmetic opposition, not tension. Such a pull + // request cannot satisfy the ratchet from inside its own subject at all — + // the only moves are delete less, add filler, or delete additional + // *uncovered* statements until it balances. The gate can be satisfied by + // deleting more code and cannot be satisfied by testing anything. + // + // Measured, both blocked by the file-scoped rule: + // opencatalogi#895 head 112/115 (97.39%) base 148/151 (98.01%) -0.62% + // launchpad#128 head 183/254 (72.05%) base 220/304 (72.37%) -0.32% + // + // THE RULE, AND IT IS ASYMMETRIC ON PURPOSE: + // + // drop BASE-ONLY methods (deletions) from the base side; + // KEEP HEAD-ONLY methods (additions) on the head side. + // + // The symmetric version — "compare over statements present in both reports" — + // is the obvious one and it is broken. It also drops head-only statements, so + // a change adding 40 new statements with 0 of them covered compares an empty + // set to an empty set and PASSES. That is the openconnector#1265 shape, and + // the symmetric rule would have retired the half of this ratchet that works. + // A mutant reintroducing it is in the test suite and must stay there. + // + // Consequences, all four measured in quality-config/tests/test-coverage-guard.py: + // pure deletion (opencatalogi) PASS 112/115 vs 112/115 — exactly neutral + // pure deletion (launchpad) PASS 183/254 vs 183/254 + // regression in survivors FAIL 180/254 vs 183/254 + // new untested code FAIL 183/294 (62.24%) vs 183/254 (72.05%) + // + // KNOWN RESIDUAL, stated rather than discovered: a RENAME reads as a delete + // plus an add. The old name leaves the base side, the new name arrives on the + // head side and must be covered. That is the right incentive — a renamed + // method is new code as far as the test suite is concerned — but it means a + // pure rename of a well-covered method is not free, and an author who did not + // expect it will read the failure as noise. It is documented here and in the + // failure message so it is legible when it happens. + if ($deletionFree === true) { + [$headBuckets, $headFiles, $headFellBack] = cgAttributeMethods($cloverFile, 'current', $changed); + [$baseBuckets, $baseFiles, $baseFellBack] = cgAttributeMethods($against, 'merge-base', $changed); + + // Either side falling back forces BOTH sides to file level for that file — + // see cgCollapseFiles() for why anything else is an invisible pass. + $collapse = array_values(array_unique(array_merge($headFellBack, $baseFellBack))); + if (empty($collapse) === false) { + echo 'Falling back to file-level metrics on both sides for: ' . implode(', ', $collapse) . "\n"; + $headBuckets = cgCollapseFiles($headBuckets, $collapse); + $baseBuckets = cgCollapseFiles($baseBuckets, $collapse); + } + + echo 'Deletion-neutral: attributed by method name (head ' . count($headFiles) . ' file(s), ' + . 'base ' . count($baseFiles) . ' file(s), ' . count($headBuckets) . ' head method(s), ' + . count($baseBuckets) . " base method(s)).\n"; + + // A changed file the base measured and the head did not is normally a + // DELETED file, and treating it as deleted is the point of this mode. But + // it is also what an accidental coverage exclusion looks like, and the two + // are indistinguishable from here — so it is said out loud rather than + // absorbed silently. + $goneFiles = array_values(array_diff($baseFiles, $headFiles)); + if (empty($goneFiles) === false) { + echo 'Measured at the merge base and absent from the head report: ' + . implode(', ', $goneFiles) . "\n"; + echo " Treated as deleted. If one of those files still exists, it has been dropped from\n"; + echo " coverage measurement and that is the thing to fix, not this guard.\n"; + } + + if (empty($headBuckets) === true && empty($baseBuckets) === true) { + echo "OK: none of the changed PHP files appear in either coverage report.\n"; + echo " Nothing was measured, so nothing is claimed about them.\n"; + exit(CG_OK); + } + + $keep = []; + foreach ($headBuckets as $key => $unused) { + $keep[$key] = true; + } + + $dropped = []; + foreach ($baseBuckets as $key => $pair) { + if (isset($keep[$key]) === false) { + $dropped[$key] = $pair; + } + } + + [$statements, $covered] = cgSumBuckets($headBuckets); + [$baseStatements, $baseCovered] = cgSumBuckets($baseBuckets, $keep); + + $current = ($statements > 0 ? round((($covered / $statements) * 100), 2) : 0.0); + $base = ($baseStatements > 0 ? round((($baseCovered / $baseStatements) * 100), 2) : 0.0); + + if (empty($dropped) === false) { + [$droppedStatements, $droppedCovered] = cgSumBuckets($dropped); + echo 'Removed from the base side: ' . count($dropped) + . " method(s) absent from head, {$droppedCovered}/{$droppedStatements} statements.\n"; + echo " A method that no longer exists is not a coverage regression; it is deleted code.\n"; + } + + cgReport('Surviving code, head:', $statements, $covered, $current); + cgReport('Surviving code, base:', $baseStatements, $baseCovered, $base); + + if ($baseStatements === 0) { + echo "OK: none of the changed PHP existed at the merge base, so there is no prior figure to drop below.\n"; + exit(CG_OK); + } + + if (cgRatioDropped($covered, $statements, $baseCovered, $baseStatements) === true) { + $delta = round(($base - $current), 2); + echo($delta > 0 + ? "FAIL: coverage of the code this change KEEPS or ADDS dropped by {$delta}%.\n" + : "FAIL: coverage of the code this change KEEPS or ADDS dropped by less than 0.01% — too " + . "little to show in the percentage, but a real loss in the counts below.\n"); + echo " base {$baseCovered}/{$baseStatements} -> head {$covered}/{$statements} statements, " + . "comparing only methods that exist on BOTH sides plus everything new on this branch.\n"; + + if ($statements > $baseStatements) { + $added = ($statements - $baseStatements); + echo " This change adds {$added} statements to those files. Adding code without tests drops coverage.\n"; + } + + echo " Deleted methods were already excluded, so this is not a deletion penalty. If you\n"; + echo " RENAMED a method, note that a rename reads as a delete plus an add: the new name is\n"; + echo " new code here and has to be covered.\n"; + + exit(CG_DROPPED); + } + + echo "OK: coverage of the surviving and added code did not drop.\n"; + exit(CG_OK); + }//end if + [$statements, $covered, $current] = cgMeasureFiles($cloverFile, 'current', $changed); [$baseStatements, $baseCovered, $base] = cgMeasureFiles($against, 'merge-base', $changed); diff --git a/quality-config/tests/test-coverage-guard.py b/quality-config/tests/test-coverage-guard.py new file mode 100644 index 00000000..de7ab016 --- /dev/null +++ b/quality-config/tests/test-coverage-guard.py @@ -0,0 +1,500 @@ +#!/usr/bin/env python3 +"""Exercise quality-config/coverage-guard.php against synthetic clover reports. + +WHAT IS BEING TESTED, AND WHY IT NEEDED A TEST AT ALL +----------------------------------------------------- +`cgRatioDropped()` is one integer cross-product with no tolerance. Read plainly +it says: *the code you touched must be at least as well covered as the code you +did not*. That is right for additions and inverted for deletions — removing `d` +statements of which `c` were covered lowers the ratio whenever `c/d` exceeds the +ratio of what remains, and dead code is dead because nothing CALLS it, not +because nothing TESTED it. + +`--deletion-neutral` fixes that by comparing METHOD BUCKETS asymmetrically: +base-only methods (deletions) leave the base side, head-only methods (additions) +stay on the head side. + +The asymmetry is the part that needs guarding. The obvious symmetric rule — +"compare over statements present in both reports" — also drops head-only +statements, so a change adding forty new statements with none of them covered +compares an empty set to an empty set and passes. CASE_NEW below is that shape, +and the mutation battery at the bottom reintroduces the symmetric rule and +requires this suite to go red for it. + +RUN + python3 quality-config/tests/test-coverage-guard.py [path/to/coverage-guard.php] + +Exit code is 0 when every assertion holds, 1 otherwise. +""" + +from __future__ import annotations + +import os +import re +import shutil +import subprocess +import sys +import tempfile +import xml.etree.ElementTree as ET +from pathlib import Path +from typing import Tuple + +HERE = Path(__file__).resolve().parent +GUARD = Path(sys.argv[1]).resolve() if len(sys.argv) > 1 else (HERE.parent / "coverage-guard.php") + +failures: list[str] = [] +checks = 0 + + +def flat(text: str) -> str: + """Collapse runs of spaces so assertions do not depend on printf padding.""" + return re.sub(r"[ \t]+", " ", text) + + +def says(stdout: str, *needles: str) -> bool: + haystack = flat(stdout) + return all(flat(n) in haystack for n in needles) + + +def check(ok: bool, what: str, detail: str = "") -> None: + global checks + checks += 1 + if ok: + print(f" ok — {what}") + return + failures.append(what) + print(f" FAIL — {what}") + if detail: + for line in detail.rstrip("\n").split("\n"): + print(f" {line}") + + +# ── clover generation ─────────────────────────────────────────────────────── +# +# The shape below is the one PHPUnit actually emits, verified against 2 608 file +# entries in four real artifacts from this fleet: `` first with metrics +# only, then FLAT `` children in document order, a `type="method"` line +# opening each method and `type="stmt"` lines belonging to whichever method +# preceded them. `metrics/@statements` counts the stmt lines; methods are counted +# separately and `elements` is their sum. +# +# Line numbers are assigned by walking the method list, so a fixture built from a +# shorter method list AUTOMATICALLY shifts every line after the cut — which is +# the whole reason a line-number intersection cannot work and is asserted below. + +# `typing.Tuple`, not `tuple[...]`: this is a real assignment rather than an +# annotation, so PEP 563 does not defer it and the subscript is evaluated on +# import. Runners have 3.12; a developer machine here has 3.8. +Method = Tuple[str, int, int] # (name, statements, covered) + + +def clover(root_dir: str, files: dict[str, list[Method]]) -> str: + project_st = 0 + project_cov = 0 + parts = ['', "", " "] + + for path, methods in files.items(): + num = 10 + lines = [] + st = 0 + cov = 0 + for name, statements, covered in methods: + lines.append(f' ') + num += 1 + for i in range(statements): + hit = 1 if i < covered else 0 + lines.append(f' ') + num += 1 + st += statements + cov += covered + num += 3 # a gap, as real source has between methods + + project_st += st + project_cov += cov + nmethods = len(methods) + covered_methods = sum(1 for _, _, c in methods if c) + + parts.append(f' ') + parts.append(f' ') + parts.append(f' ') + parts.append(" ") + parts.extend(lines) + parts.append(f' ') + parts.append(" ") + + parts.append(f' ') + parts.append(" ") + parts.append("") + return "\n".join(parts) + "\n" + + +# ── the fixtures ──────────────────────────────────────────────────────────── +# +# LAUNCHPAD#128 SHAPE — a pure deletion of a well-covered method. +# Surviving code: 15 methods, 254 statements, 183 covered -> 72.05% +# Deleted method: 50 statements, 37 covered +# Base file total: 304 statements, 220 covered -> 72.37% +# so the file-scoped rule sees a 0.32% drop, which is what CI reported. +SURVIVORS: list[Method] = [ + ("registerWidget", 20, 20), + ("resolveLayout", 18, 18), + ("buildTiles", 16, 16), + ("applyTheme", 22, 22), + ("readPreferences", 14, 14), + ("writePreferences", 19, 19), + ("serialiseBoard", 17, 17), + # the deletion site sits here — everything below shifts + ("restoreBoard", 21, 12), + ("mergeDefaults", 15, 8), + ("pruneStale", 13, 6), + ("auditPlacement", 18, 10), + ("exportBoard", 20, 9), + ("importBoard", 16, 5), + ("validateSlot", 12, 4), + ("describeSlot", 13, 3), +] +DELETED: Method = ("legacyWidgetPayload", 50, 37) +LP_FILE = "lib/Service/WidgetService.php" +LP_BASE = SURVIVORS[:7] + [DELETED] + SURVIVORS[7:] + +# OPENCATALOGI#895 SHAPE — the deleted block was 100% covered. +# head 112/115 (97.39%), base 148/151 (98.01%), file-scoped drop 0.62%. +OC_SURVIVORS: list[Method] = [ + ("loadSettings", 20, 20), + ("saveSettings", 18, 18), + ("normaliseKey", 15, 14), + ("readDefaults", 12, 12), + ("mergeOverrides", 14, 13), + ("validateShape", 16, 16), + ("describeKey", 10, 10), + ("flushCache", 10, 9), +] +OC_DELETED: Method = ("renderLegacyBanner", 36, 36) +OC_FILE = "lib/Service/SettingsService.php" +OC_BASE = OC_SURVIVORS[:4] + [OC_DELETED] + OC_SURVIVORS[4:] + +# CASE 3 — a real regression in surviving code: registerWidget loses 3 covered. +REGRESSED = [("registerWidget", 20, 17)] + SURVIVORS[1:] + +# CASE 4 — the openconnector#1265 shape: 40 new statements, 0 covered. +ADDED_UNTESTED: Method = ("renderWidgetV2", 40, 0) +WITH_NEW = SURVIVORS + [ADDED_UNTESTED] + + +def totals(methods: list[Method]) -> tuple[int, int]: + return sum(m[1] for m in methods), sum(m[2] for m in methods) + + +def run_guard(guard: Path, work: Path, head: str, base: str, changed: list[str], + deletion_neutral: bool) -> subprocess.CompletedProcess: + (work / "head.xml").write_text(head) + (work / "base.xml").write_text(base) + (work / "changed.txt").write_text("\n".join(changed) + "\n") + cmd = [ + "php", str(guard), str(work / "head.xml"), + f"--against={work / 'base.xml'}", + f"--changed-files={work / 'changed.txt'}", + ] + if deletion_neutral: + cmd.append("--deletion-neutral") + return subprocess.run(cmd, capture_output=True, text=True) + + +def case(work: Path, path: str, head_methods: list[Method], base_methods: list[Method]) -> tuple[str, str]: + return ( + clover("/runner/head/app", {path: head_methods}), + clover("/runner/base/app", {path: base_methods}), + ) + + +def suite(guard: Path, label: str) -> int: + """Run every assertion against `guard`. Returns the number of failures.""" + global failures, checks + failures = [] + checks = 0 + print(f"\n== {label} ==") + + with tempfile.TemporaryDirectory() as tmp: + work = Path(tmp) + + # ── FAIL FIRST: the file-scoped rule blocks both pure deletions ────── + print("\n-- the shipped file-scoped rule, on the two PRs it blocked --") + + head, base = case(work, LP_FILE, SURVIVORS, LP_BASE) + r = run_guard(guard, work, head, base, [LP_FILE], deletion_neutral=False) + check(r.returncode == 1, "launchpad#128 shape FAILS under --changed-files alone", + r.stdout + r.stderr) + check(says(r.stdout, "Changed files, head: 72.05% (183/254 statements)", + "Changed files, base: 72.37% (220/304 statements)", + "dropped by 0.32%"), + " ...and reports exactly 72.05% (183/254) vs 72.37% (220/304), -0.32%", r.stdout) + + head_oc, base_oc = case(work, OC_FILE, OC_SURVIVORS, OC_BASE) + r = run_guard(guard, work, head_oc, base_oc, [OC_FILE], deletion_neutral=False) + check(r.returncode == 1, "opencatalogi#895 shape FAILS under --changed-files alone", + r.stdout + r.stderr) + check(says(r.stdout, "Changed files, head: 97.39% (112/115 statements)", + "Changed files, base: 98.01% (148/151 statements)", + "dropped by 0.62%"), + " ...and reports exactly 97.39% (112/115) vs 98.01% (148/151), -0.62% — " + "the numbers CI printed on job 95170517430", r.stdout) + + # ── CASE 1 + 2: pure deletions become exactly neutral ──────────────── + print("\n-- --deletion-neutral: the four measured cases --") + + head, base = case(work, LP_FILE, SURVIVORS, LP_BASE) + r = run_guard(guard, work, head, base, [LP_FILE], deletion_neutral=True) + check(r.returncode == 0, "CASE 1 launchpad#128 (pure deletion) PASSES", r.stdout + r.stderr) + check(says(r.stdout, "Surviving code, head: 72.05% (183/254 statements)", + "Surviving code, base: 72.05% (183/254 statements)"), + " ...at exactly 183/254 vs 183/254 — neutral, not marginally passing", r.stdout) + check("1 method(s) absent from head, 37/50 statements" in r.stdout, + " ...and it names what it removed from the base side (37/50)", r.stdout) + + r = run_guard(guard, work, head_oc, base_oc, [OC_FILE], deletion_neutral=True) + check(r.returncode == 0, "CASE 2 opencatalogi#895 (pure deletion) PASSES", r.stdout + r.stderr) + check(says(r.stdout, "Surviving code, head: 97.39% (112/115 statements)", + "Surviving code, base: 97.39% (112/115 statements)"), + " ...at exactly 112/115 vs 112/115", r.stdout) + + # ── CASE 3: a regression in surviving code still fails ─────────────── + head, base = case(work, LP_FILE, REGRESSED, LP_BASE) + r = run_guard(guard, work, head, base, [LP_FILE], deletion_neutral=True) + check(r.returncode == 1, "CASE 3 regression in SURVIVING code still FAILS", r.stdout + r.stderr) + check(says(r.stdout, "Surviving code, head: 70.87% (180/254 statements)", + "Surviving code, base: 72.05% (183/254 statements)"), + " ...at 180/254 vs 183/254", r.stdout) + + # ── CASE 4: new untested code still fails ──────────────────────────── + head, base = case(work, LP_FILE, WITH_NEW, LP_BASE) + r = run_guard(guard, work, head, base, [LP_FILE], deletion_neutral=True) + check(r.returncode == 1, "CASE 4 new untested code (40 statements, 0 covered) still FAILS", + r.stdout + r.stderr) + check(says(r.stdout, "Surviving code, head: 62.24% (183/294 statements)", + "Surviving code, base: 72.05% (183/254 statements)"), + " ...at 62.24% (183/294) vs 72.05% (183/254)", r.stdout) + check("adds 40 statements" in r.stdout, + " ...and says the change added 40 statements", r.stdout) + check("RENAMED" in r.stdout, + " ...and states the rename residual in the failure message rather than " + "leaving it to be discovered", r.stdout) + + # ── the combination: delete one method AND add an untested one ─────── + head, base = case(work, LP_FILE, WITH_NEW, LP_BASE) + r = run_guard(guard, work, head, base, [LP_FILE], deletion_neutral=True) + check("1 method(s) absent from head" in r.stdout and r.returncode == 1, + "a change that deletes AND adds-untested is charged for the addition only, and fails", + r.stdout) + + # ── degenerate inputs must not read as a pass ──────────────────────── + print("\n-- degenerate inputs --") + + r = subprocess.run(["php", str(guard), str(work / "head.xml"), "--deletion-neutral"], + capture_output=True, text=True) + check(r.returncode == 2 and "requires --changed-files" in r.stderr, + "--deletion-neutral without --changed-files is an ERROR, not a silent whole-project run", + r.stdout + r.stderr) + + # A file present in neither report claims nothing. + head, base = case(work, LP_FILE, SURVIVORS, LP_BASE) + r = run_guard(guard, work, head, base, ["lib/Service/Absent.php"], deletion_neutral=True) + check(r.returncode == 0 and "Nothing was measured" in r.stdout, + "a changed file absent from both reports claims nothing and does not fail", + r.stdout + r.stderr) + + # A file with metrics but no line data must NOT measure 0/0 and pass. + # This is the hole the guard's cgCollapseFiles() exists to close: the + # fallback bucket lives on one side only, so without the collapse the + # asymmetric rule calls the whole base file "deleted" and reports + # "none of the changed PHP existed at the merge base" — a file the guard + # could not read passing every possible drop. + stripped = re.sub(r'\n\s*]*/>', "", base) + r = run_guard(guard, work, head, stripped, [LP_FILE], deletion_neutral=True) + check("no line data" in r.stdout, + "a base file with metrics but no line data falls back to its file metric, out loud", + r.stdout + r.stderr) + check(r.returncode == 1, + " ...and that fallback is conservative — the file-level comparison still fails", + r.stdout + r.stderr) + + return len(failures) + + +# ── the two controls that are NOT about the guard's own output ────────────── + +def control_symmetric_would_pass() -> None: + """A symmetric intersection passes CASE 4. This is why the rule is asymmetric. + + Computed here rather than in the guard, because the symmetric rule is the one + thing the guard must never implement. If this control ever stops passing, the + fixture no longer exercises the hole and CASE 4 stops proving anything. + """ + print("\n-- control: the symmetric rule the guard deliberately does NOT implement --") + head = {name: (s, c) for name, s, c in WITH_NEW} + base = {name: (s, c) for name, s, c in LP_BASE} + both = set(head) & set(base) + hs = sum(head[k][0] for k in both) + hc = sum(head[k][1] for k in both) + bs = sum(base[k][0] for k in both) + bc = sum(base[k][1] for k in both) + dropped = (hc * bs) < (bc * hs) + check(not dropped and (hc, hs) == (183, 254) and (bc, bs) == (183, 254), + "symmetric 'statements present in BOTH reports' PASSES the 40-new-0-covered case " + f"({hc}/{hs} vs {bc}/{bs}) — which is why head-only methods are kept", + "") + + +def control_line_numbers_are_meaningless() -> None: + """Intersecting by clover `num` compares unrelated statements after a deletion.""" + print("\n-- control: why the intersection is by method NAME, never line number --") + head_xml = clover("/runner/head/app", {LP_FILE: SURVIVORS}) + base_xml = clover("/runner/base/app", {LP_FILE: LP_BASE}) + + def stmt_lines(xml: str) -> dict[int, int]: + root = ET.fromstring(xml) + out = {} + for f in root.iter("file"): + for line in f.findall("line"): + if line.get("type") == "stmt": + out[int(line.get("num"))] = int(line.get("count")) + return out + + h = stmt_lines(head_xml) + b = stmt_lines(base_xml) + shared = set(h) & set(b) + hs, hc = len(shared), sum(1 for n in shared if h[n] > 0) + bs, bc = len(shared), sum(1 for n in shared if b[n] > 0) + + # How much of the surviving file sits at or after the cut, i.e. is shifted. + before_cut = sum(m[1] for m in SURVIVORS[:7]) + after_cut = sum(m[1] for m in SURVIVORS) - before_cut + print(f" {after_cut} of {sum(m[1] for m in SURVIVORS)} surviving statements " + f"({after_cut * 100 // sum(m[1] for m in SURVIVORS)}%) sit at or after the deletion site " + f"— launchpad's real file was 117 of 254 (46%).") + + check((hc, hs) != (183, 254) or (bc, bs) != (183, 254), + f"a line-number intersection does NOT reconcile ({hc}/{hs} head vs {bc}/{bs} base) — " + "it compares unrelated statements across the shifted half of the file", + "") + + +def mutation_battery() -> int: + """Reintroduce known defects; the suite above must go red for each. + + A green suite proves nothing on its own — the question is whether it would + have NOTICED. The last mutant is an ANTI-WIDENING control: it edits a log + string nothing asserts on and the suite must STAY GREEN, so a suite that + failed on any edit at all cannot score a perfect kill rate while being + worthless. + """ + print("\n== mutation battery ==") + source = GUARD.read_text() + survivors = 0 + + mutants: list[tuple[str, str, str, bool]] = [ + ( + "the SYMMETRIC rule — head-only methods dropped too (the openconnector#1265 hole)", + " [$statements, $covered] = cgSumBuckets($headBuckets);", + " $mutantKeep = [];\n" + " foreach ($baseBuckets as $mutantKey => $mutantPair) { $mutantKeep[$mutantKey] = true; }\n" + " [$statements, $covered] = cgSumBuckets($headBuckets, $mutantKeep);", + True, + ), + ( + "base-only methods KEPT — i.e. the deletion penalty put back", + " [$baseStatements, $baseCovered] = cgSumBuckets($baseBuckets, $keep);", + " [$baseStatements, $baseCovered] = cgSumBuckets($baseBuckets);", + True, + ), + ( + "attribution by ordinal instead of name — every bucket key made unique", + " $method = ($named === '' ? CG_WHOLE_FILE : $named);", + " $method = ($named === '' ? CG_WHOLE_FILE : ($named . '@' . (string) $line['num']));", + True, + ), + ( + "the no-line-data fallback removed — an unmeasurable file reads as 0/0", + " if ($statements === 0 && $declared > 0) {", + " if (false) {", + True, + ), + ( + "ANTI-WIDENING: a log string nothing asserts on is reworded", + " echo \" A method that no longer exists is not a coverage regression; it is deleted code.\\n\";", + " echo \" Deleted methods are not regressions.\\n\";", + False, + ), + ] + + for name, needle, replacement, must_die in mutants: + if needle not in source: + print(f" FAIL — mutant anchor not found, the battery is not reaching the code: {name}") + survivors += 1 + continue + + with tempfile.TemporaryDirectory() as tmp: + mutant = Path(tmp) / "coverage-guard.php" + mutant.write_text(source.replace(needle, replacement, 1)) + lint = subprocess.run(["php", "-l", str(mutant)], capture_output=True, text=True) + if lint.returncode != 0: + print(f" FAIL — mutant does not parse: {name}\n {lint.stdout}") + survivors += 1 + continue + + broke = suite(mutant, f"mutant: {name}") + + if must_die and broke == 0: + print(f" SURVIVED — {name}: the suite stayed green. The fixture cannot reach that branch.") + survivors += 1 + elif must_die: + print(f" killed — {name} ({broke} assertion(s) went red)") + elif broke != 0: + print(f" OVER-FIRED — {name}: the suite went red on a change that alters no behaviour.") + survivors += 1 + else: + print(f" ok — {name}: suite stayed green, as it must.") + + return survivors + + +def main() -> int: + if shutil.which("php") is None: + print("::error::php is not on PATH; this suite exercises the shipped PHP program and " + "cannot be satisfied by reading it.") + return 1 + if GUARD.exists() is False: + print(f"::error::coverage guard not found at {GUARD}") + return 1 + + broke = suite(GUARD, f"coverage-guard.php ({GUARD})") + + failures.clear() + control_symmetric_would_pass() + control_line_numbers_are_meaningless() + control_failures = len(failures) + + # The battery runs last on purpose: it re-enters suite() against mutated + # copies and resets the module-level counters as it goes. + survivors = mutation_battery() + + print("\n== summary ==") + print(f" subject assertions failed: {broke}") + print(f" control assertions failed: {control_failures}") + print(f" surviving/over-firing mutants: {survivors}") + if broke == 0 and control_failures == 0 and survivors == 0: + print("PASS") + return 0 + print("FAIL") + return 1 + + +if __name__ == "__main__": + sys.exit(main())