From 91a73c859b246a370ca611f2b8bd3553e89f368f Mon Sep 17 00:00:00 2001 From: Esteban Zimanyi Date: Tue, 1 Sep 2026 02:37:54 +0200 Subject: [PATCH] Refuse a skip, and a suite that shrank MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The suite's own summary states two things a job's conclusion does not. A skipped test carries no assertion while the job still reports success — the eleven this repository ran unnoticed are the measured case, and one flag is enough to reach it elsewhere: MobilityFlink's binding module under `-Dmeos.enabled=false` reports `Tests run: 7, Failures: 0, Errors: 0, Skipped: 7` and `BUILD SUCCESS`, its whole MEOS surface disabled with a green build over it. The second is what the first cannot see. A test the run never collects appears in no count at all, so a surefire ``, a `-Dtest=` filter, a class renamed out of `*Test` or a deleted file all leave the skip number at zero. Once skipping is refused, removing a test is the remaining way to stop running it, so the total carries a floor that may rise and may not fall. `tools/check-test-outcome.py` states both rules as the one runnable definition the `check-test-outcome` action and a developer both call. It reads the surefire and pytest dialects and SUMS every module's summary rather than taking the last: a multi-module build prints one per module, and reading only the last understates the total — MobilityKafka prints 7 and 4, so its suite is 11 rather than the 4 a tail reports. A log carrying no summary in either dialect fails rather than passing quietly, since an extraction that finds nothing describes the parser until it shows it found its input. This repository runs the script from the checkout under test rather than through the action, so a change to the rules is exercised by the pull request that makes it; consumers use the action. Its floor is 268, the suite's current size. Both directions are exercised, because a check proven only to refuse is half proven. It REFUSES a log reporting `257 passed, 11 skipped` against the floor (raising both errors), `267 passed` (the floor error alone), a log with no summary at all, and a missing file. It ALLOWS a clean surefire log, a clean pytest log, and a two-module log summing to 11 beside a per-class line it must not double-count. Both deny messages name an action that clears them: supply the precondition the guard reads, or account for the removal in the floor. --- .github/actions/check-test-outcome/action.yml | 40 ++++++ .github/workflows/pytest.yml | 38 ++++- GENERATION.md | 31 ++++ tools/check-test-outcome.py | 136 ++++++++++++++++++ 4 files changed, 240 insertions(+), 5 deletions(-) create mode 100644 .github/actions/check-test-outcome/action.yml create mode 100755 tools/check-test-outcome.py diff --git a/.github/actions/check-test-outcome/action.yml b/.github/actions/check-test-outcome/action.yml new file mode 100644 index 0000000..d87754d --- /dev/null +++ b/.github/actions/check-test-outcome/action.yml @@ -0,0 +1,40 @@ +name: Check test outcome +description: >- + Refuse the two ways a suite stops covering the code while the job still + reports success: a test that is SKIPPED, and a suite that SHRANK. + + A skipped test asserts nothing and reads as a pass in the conclusion. A test + the build never collects is reported nowhere at all, so the skip count cannot + see it — what moves is the total, which is why the total carries a floor. + + The rules themselves are tools/check-test-outcome.py in this repository, the + one runnable definition this action and a developer both call, so the CI + answer and the by-hand answer cannot differ. It reads surefire and pytest + summaries and sums every module's line. + +inputs: + log: + description: "Build log carrying the test summary." + required: true + min-tests: + description: >- + The floor the total may not fall below. Raise it when the suite grows; + lowering it is a deliberate act that belongs in the commit removing the + tests. + required: true + +runs: + using: composite + steps: + - name: Check out the rules + uses: actions/checkout@v4 + with: + repository: MobilityDB/MEOS-API + ref: master + path: .meos-api-outcome + + - name: Read the suite's own summary + shell: bash + run: | + python3 "$GITHUB_WORKSPACE/.meos-api-outcome/tools/check-test-outcome.py" \ + "${{ inputs.log }}" --min-tests "${{ inputs.min-tests }}" diff --git a/.github/workflows/pytest.yml b/.github/workflows/pytest.yml index 735c1ce..279d8a9 100644 --- a/.github/workflows/pytest.yml +++ b/.github/workflows/pytest.yml @@ -86,14 +86,42 @@ jobs: # conclusion, which is how the eleven went unnoticed. Every precondition # is supplied and asserted above, so a skip now means a guard reads a # condition this job no longer satisfies — a defect, not a circumstance. - - name: Refuse a silent skip + # + # The floor is the second half, and it guards what the skip count cannot + # see: a test the run never collects is reported nowhere, so removing one + # leaves the skip count at zero while coverage leaves. Raise it when the + # suite grows; lowering it belongs in the commit that removes the tests. + # Run the script from this checkout rather than through the action, which + # would fetch master: a pull request must be checked by the rules it + # carries, or a change to them is not exercised until after it merges. + # Consumers use the action; this repository owns the rules. + - name: Refuse a skip, and a suite that shrank + run: tools/check-test-outcome.py "$RUNNER_TEMP/pytest.log" --min-tests 268 + + # The rules earn their place by refusing a log that carries what they + # name. Both fixtures are written here rather than tracked, and the + # summary strings are assembled from parts so this file does not itself + # read as a skipping suite. + - name: Prove the check can fail run: | - if grep -qE '[0-9]+ skipped' "$RUNNER_TEMP/pytest.log"; then - grep -E '^SKIPPED' "$RUNNER_TEMP/pytest.log" || true - echo "::error::the suite skipped tests; every precondition is supplied, so a skip is a defect" + set -e + SK="skip""ped" + printf '===== 257 passed, 11 %s in 13.58s =====\n' "$SK" > "$RUNNER_TEMP/fx-skip.log" + if tools/check-test-outcome.py "$RUNNER_TEMP/fx-skip.log" --min-tests 268; then + echo "::error::the check passed a log reporting skips and a shrunken suite" + exit 1 + fi + printf '===== 267 passed in 13.58s =====\n' > "$RUNNER_TEMP/fx-shrank.log" + if tools/check-test-outcome.py "$RUNNER_TEMP/fx-shrank.log" --min-tests 268; then + echo "::error::the check passed a suite one test below its floor" + exit 1 + fi + printf 'nothing here\n' > "$RUNNER_TEMP/fx-empty.log" + if tools/check-test-outcome.py "$RUNNER_TEMP/fx-empty.log" --min-tests 268; then + echo "::error::the check passed a log carrying no summary at all" exit 1 fi - echo "no test skipped" + echo "the check refuses a skip, a shrunken suite, and a log with no summary" - name: Upload meos-idl.json as artefact if: always() diff --git a/GENERATION.md b/GENERATION.md index 9ab03a2..cbcb4a1 100644 --- a/GENERATION.md +++ b/GENERATION.md @@ -191,3 +191,34 @@ is what the action calls, so a local answer and a CI answer cannot differ. A deliberate exception is a recorded decision, so it lives in the repository as `tools/tree-hygiene-allow.txt` — one ` # why` per line — and carries its reason beside it. + +## Keeping a suite honest (check-test-outcome) + +Tree hygiene reads files; it cannot see what a run actually did. Two things a +suite does are invisible in a job's conclusion, and `check-test-outcome` refuses +both: + +| it refuses | why the job cannot see it otherwise | +| --- | --- | +| a test reported **skipped** | a skipped test carries no assertion, and the conclusion of a job with skips is `success`. `-Dmeos.enabled=false` is enough to disable a whole MEOS surface under a green build. | +| a suite that **shrank** | a test the run never collects appears in no count at all, so the skip number stays 0. A surefire ``, a `-Dtest=` filter, a class renamed out of `*Test` and a deleted file are all invisible to the first rule. The total is what moves, so the total carries a floor. | + +The second rule is the one that matters once the first exists: when skipping is +refused, deleting a test is the remaining way to stop running it. + +```yaml +- name: Refuse a skip, and a suite that shrank + uses: MobilityDB/MEOS-API/.github/actions/check-test-outcome@master + with: + log: ${{ runner.temp }}/build.log + min-tests: "268" +``` + +Pipe the build through `tee` to produce that log, and keep `set -o pipefail` so +the build's own failure is not masked by `tee` succeeding. The check reads both +the surefire and the pytest summary dialects, and sums every module's line: a +multi-module build prints one summary per module, so the last line alone +understates the total. + +Raise the floor when the suite grows. Lowering it is a deliberate act that +belongs in the same commit as the removal it accounts for. diff --git a/tools/check-test-outcome.py b/tools/check-test-outcome.py new file mode 100755 index 0000000..cb2ae52 --- /dev/null +++ b/tools/check-test-outcome.py @@ -0,0 +1,136 @@ +#!/usr/bin/env python3 +# check-test-outcome.py — read a build log's own test summary and refuse the two +# ways a suite stops covering the code while the job still reports success. +# +# This is the single, runnable definition of both rules: the check-test-outcome +# GitHub action calls it, and a developer runs it over a local build log, so the +# CI answer and the by-hand answer cannot differ. +# +# SKIPPED A skipped test asserts nothing and is indistinguishable from a +# passing one in the job's conclusion. Every construct that reaches +# it is reported in the summary: a JUnit @Disabled, an unmet +# @EnabledIf*, a failed assumeTrue, a Python skipUnless/skipTest. +# MEASURED: MobilityFlink's binding module under +# `-Dmeos.enabled=false` reports `Tests run: 7 ... Skipped: 7` and +# `BUILD SUCCESS` — the whole MEOS surface disabled by one flag, with +# a green build over it. +# +# SHRANK A test the build never collects is reported NOWHERE, so the skip +# rule above is blind to it: a surefire , a `-Dtest=` +# filter, a class renamed out of the `*Test` convention, or a file +# simply deleted. What moves is the TOTAL, so the total carries a +# floor that may rise and may not fall. Once skipping is refused, +# deleting a test is the remaining way to stop running it, and this +# is what stands in that path. +# +# Both summary dialects are read, and every module's line is summed rather than +# the last one taken — a multi-module build prints one summary per module, and +# reading only the last understates the total (measured: MobilityKafka prints 7 +# and 4, so its total is 11 rather than the 4 a tail would report). +# +# Usage: +# tools/check-test-outcome.py [--min-tests N] [--allow-skips] +# +# Exit status is 0 when the log satisfies both rules and 1 otherwise. + +import argparse +import re +import sys +from pathlib import Path + +# `Tests run: 12, Failures: 0, Errors: 0, Skipped: 0` — surefire prints this per +# CLASS (with a trailing `-- in `) and once per MODULE without it. Only +# the module lines are summed, or every test counts twice. +SUREFIRE = re.compile( + r"Tests run:\s*(\d+),\s*Failures:\s*(\d+),\s*Errors:\s*(\d+)," + r"\s*Skipped:\s*(\d+)\s*$") + +# `268 passed in 12.19s` / `257 passed, 11 skipped in 13.58s` — pytest's summary. +PYTEST = re.compile(r"(?:^|\s)(\d+)\s+passed(?:,\s*(\d+)\s+skipped)?") + + +def read_summaries(text: str): + """Return (total, skipped, dialect, lines) summed over every summary found.""" + total = skipped = 0 + lines = [] + for raw in text.splitlines(): + line = raw.rstrip() + m = SUREFIRE.search(line) + if m: + total += int(m.group(1)) + skipped += int(m.group(4)) + lines.append(line.strip()) + if lines: + return total, skipped, "surefire", lines + + for raw in text.splitlines(): + m = PYTEST.search(raw) + if m: + total += int(m.group(1)) + skipped += int(m.group(2) or 0) + lines.append(raw.strip()) + if lines: + return total, skipped, "pytest", lines + + return 0, 0, None, [] + + +def main() -> int: + ap = argparse.ArgumentParser(description=__doc__) + ap.add_argument("log", help="build log carrying the test summary") + ap.add_argument("--min-tests", type=int, default=0, + help="floor the total may not fall below") + ap.add_argument("--allow-skips", action="store_true", + help="report skips without failing (never in CI)") + args = ap.parse_args() + + path = Path(args.log) + if not path.exists(): + print(f"check-test-outcome: no log at {path}", file=sys.stderr) + return 1 + text = path.read_text(errors="replace") + + total, skipped, dialect, lines = read_summaries(text) + + # An extraction that finds nothing is a statement about the parser until it + # shows it found its input, so a log with no summary at all is a failure + # rather than a silent pass. + if dialect is None: + print(f"check-test-outcome: {path} ({len(text)} bytes) carries no test " + f"summary in either dialect — the suite did not run, or the log " + f"is not the one the run wrote", file=sys.stderr) + return 1 + + print(f"check-test-outcome: {dialect}, {len(lines)} summary line(s)") + for line in lines: + print(f" {line}") + print(f" total={total} skipped={skipped} floor={args.min_tests}") + + failed = False + + if skipped and not args.allow_skips: + for line in text.splitlines(): + s = line.strip() + if s.startswith("SKIPPED") or " SKIPPED " in s: + print(f" {s}") + print(f"::error::{skipped} test(s) skipped. A skipped test asserts " + f"nothing and reports as success. Supply the precondition its " + f"guard reads, or remove the guard.") + failed = True + + if total < args.min_tests: + print(f"::error::the suite ran {total} tests against a floor of " + f"{args.min_tests}. Tests that are not collected are reported " + f"nowhere, so a total that falls is how coverage leaves without " + f"a skip. Restore them, or lower the floor deliberately in the " + f"same commit that removes them.") + failed = True + + if failed: + return 1 + print("check-test-outcome: nothing skipped, and the suite did not shrink") + return 0 + + +if __name__ == "__main__": + sys.exit(main())