From 7a47528de8f80856afe0057855f6d52dd350532f Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Mon, 24 Aug 2026 07:44:53 +0200 Subject: [PATCH] feat(quality): read the e2e REPORT, so a skipped test stops passing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every e2e check in this repo reads tests/e2e/** source. gate-19 parses test.skip(...) statically and is careful about it — unconditional vs conditional, and #239 tightened the unconditional-inside-an-if case. What no source reader can settle is the form the fleet actually uses: const present = await someLiveQuery(page) test.skip(!present, 'Members tab not deployed on this instance') At source level that is indistinguishable from a test that runs. Only the report of an actual run knows. Nothing read the report. MEASURED across 20 apps' latest GREEN development runs: 298 / 3210 tests skipped (9.3%) 27 spec files executed ZERO tests 61 skips defer to a deploy state CI itself decides 142 skips carry no reason at all decidiq 57/191 (29.8%) dossiq 38/137 (27.7%) buildiq 68/259 (26.3%) A zero-test spec file is still accepted by gate-19 as the @e2e anchor for an openspec scenario, so a scenario can be "covered" by a file that never runs. The 61 deploy-state skips were all written for a shared dev instance, where the deployed build genuinely can lag the test. CI is not that. About the app under test the claim is impossible — the app IS the head commit, so "not deployed" can only mean missing or broken. About a pinned dependency it is deterministic — decidiq's 46 all read "leaves PR not deployed yet", which the lockfile decides identically every run. Either way the guard is an unconditional escape hatch in the one environment where the condition is not a matter of luck. Note the shape of the worst one: `partial registry: 27/29 providers` skips an exact-count assertion, so a registry that is 93% correct reports as "not deployed" rather than as a diff. NOT reported: a skip naming an optional app or external service the instance genuinely lacks ("No chat backend reachable"). Those describe a real absence the app does not control. Precedence is asserted both ways in the tests. Report-only per app, exactly as app:check-code: eight apps carry findings today and turning them all red at once makes it a gate nobody can turn on. `e2e-skip-blocking` flips one app to a hard failure once it is worked down. An absent report exits non-zero rather than 0 — an absent artifact is an absent measurement, and that is the one failure mode that would quietly turn this whole gate into a no-op. Asserted in the suite. Paired controls, 10/10, auto-discovered by run-helper-suites.sh: each finding proven present on a defect fixture AND absent on a clean one, with the enforce-mode exit code asserted in both directions. Verified against real reports too: decidiq exits 1, launchpad and versioniq exit 0. The report regex tolerates further attributes on the script tag. Pinning the exact adjacency would let one added attribute silently turn the gate into a no-op, which reads the same as a clean fleet. --- .github/workflows/quality.yml | 69 ++++ hydra-gates/scripts/lib/check_e2e_skips.py | 387 ++++++++++++++++++ .../scripts/lib/test_check_e2e_skips.sh | 209 ++++++++++ 3 files changed, 665 insertions(+) create mode 100644 hydra-gates/scripts/lib/check_e2e_skips.py create mode 100755 hydra-gates/scripts/lib/test_check_e2e_skips.sh diff --git a/.github/workflows/quality.yml b/.github/workflows/quality.yml index 59b44804..8e2c370a 100644 --- a/.github/workflows/quality.yml +++ b/.github/workflows/quality.yml @@ -402,6 +402,11 @@ on: required: false type: string default: "[]" + e2e-skip-blocking: + description: "Make the E2E skip-discipline check a hard failure for this app. It reads the RUN's Playwright report — the only place a runtime `test.skip(cond, reason)` is visible — and reports three things: a spec file that executed ZERO tests, a skip deferring to a deploy state CI itself decides ('not deployed', 'deploy drift', 'predates'), and a skip carrying no reason. A skip naming a genuinely absent optional app or external service is allowed. Default false because eight apps carry findings today (measured 2026-08-24: 298/3210 tests skipped fleet-wide, 27 zero-test spec files) and a gate that turns them all red at once is a gate nobody can turn on — same reasoning as check-code-blocking. Flip it per app once that app is worked down; it can then only stay clean." + required: false + type: boolean + default: false enable-coverage-guard: description: "Enforce the coverage ratchet — READ-ONLY, never commits. On a PULL REQUEST the merge base is measured in the phpunit job and the change must not reduce coverage against it; the floor is measured, not typed, so it cannot go stale and there is nothing for an author to keep up to date. On a PUSH the committed .coverage-baseline is enforced as a conservative fail-safe floor (below it fails; above it is fine). PRs may raise that floor but never lower it. Requires scripts/coverage-guard.php supporting --against; the workflow probes for it and fails loudly rather than silently downgrading the check." required: false @@ -4856,6 +4861,70 @@ jobs: SPEC_COVERAGE_JS node /tmp/spec-coverage.js + # ── e2e skip discipline: read the REPORT, not the source ─────────────── + # + # Every other e2e check in this file reads `tests/e2e/**` source. gate-19 + # parses `test.skip(...)` statically and is careful about it. What no + # source reader can settle is the form the fleet actually uses: + # + # const present = await someLiveQuery(page) + # test.skip(!present, 'Members tab not deployed on this instance') + # + # At source level that is indistinguishable from a test that runs. Only + # the report of an actual run knows. Nothing read the report, so measured + # 2026-08-24 across 20 apps' latest GREEN `development` runs: 298 of 3210 + # tests skipped (9.3%), 27 spec files executing ZERO tests, 61 skips + # deferring to a deploy state CI itself decides, and 142 carrying no + # reason at all. decidiq 29.8%, dossiq 27.7%, buildiq 26.3%. + # + # A zero-test spec file is still accepted by gate-19 as the `@e2e` anchor + # for an openspec scenario, so a scenario can be "covered" by a file that + # never runs. + # + # `!cancelled()` and not `always()`: a cancelled run has no verdict to + # explain, and a capture hung off a cancelled job produces nothing exactly + # when you need it. It deliberately runs when the suite PASSED — a green + # suite is where skips hide, and running only on failure would reproduce + # the blindness this step exists to remove. + # + # REPORT, NOT FAIL, on day one, exactly as app:check-code above: eight + # apps carry findings today, and turning them all red at once makes it a + # gate nobody can turn on. `e2e-skip-blocking` flips it per app as that + # app is worked down. + - name: E2E skip discipline + if: ${{ !cancelled() }} + continue-on-error: ${{ !inputs.e2e-skip-blocking }} + run: | + set -euo pipefail + GATE="" + for candidate in \ + "server/apps/${{ inputs.app-name }}/vendor/conduction/hydra-gates/scripts/lib/check_e2e_skips.py" \ + "hydra-gates/scripts/lib/check_e2e_skips.py"; do + if [ -f "${candidate}" ]; then GATE="${candidate}"; break; fi + done + if [ -z "${GATE}" ]; then + echo "::warning::check_e2e_skips.py not found (hydra-gates not vendored here yet) — skip discipline NOT measured on this run." + exit 0 + fi + + REPORT="" + for candidate in \ + "server/apps/${{ inputs.app-name }}/playwright-report" \ + "server/apps/${{ inputs.app-name }}/tests/e2e/playwright-report"; do + if [ -f "${candidate}/index.html" ]; then REPORT="${candidate}"; break; fi + done + if [ -z "${REPORT}" ]; then + # An absent report is an absent measurement, never a pass. Said + # loudly, because this is the one failure mode that would quietly + # turn the whole gate into a no-op. + echo "::error::no Playwright HTML report found — skip discipline could not be measured. Ensure the 'html' reporter is configured." + exit 1 + fi + + MODE=report + if [ "${{ inputs.e2e-skip-blocking }}" = "true" ]; then MODE=enforce; fi + python3 "${GATE}" --report "${REPORT}" --mode "${MODE}" --summary "${GITHUB_STEP_SUMMARY}" + - name: Upload Playwright report if: always() # `tests/e2e/…` is included because that is where the scaffolded config diff --git a/hydra-gates/scripts/lib/check_e2e_skips.py b/hydra-gates/scripts/lib/check_e2e_skips.py new file mode 100644 index 00000000..1deee693 --- /dev/null +++ b/hydra-gates/scripts/lib/check_e2e_skips.py @@ -0,0 +1,387 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: EUPL-1.2 +"""E2E skip discipline — reads the RUN's Playwright report, not the source. + +Every other e2e gate in this repo reads `tests/e2e/**` source. That is exactly +what this gate cannot do, and why it exists. + +`check_e2e_coverage.py` (gate-19) already parses `test.skip(...)` statically and +is careful about it: it separates an unconditional `test.skip(true)` from a +conditional `test.skip(cond, reason)`, and issue #239 tightened the case of an +unconditional skip hidden inside an `if` guard. What no source reader can settle +is the form the fleet actually uses: + + const present = await someLiveQuery(page) + test.skip(!present, 'Members tab not deployed on this instance') + +At source level that is indistinguishable from a test that runs. Only the report +of an actual run knows whether it executed. Nothing read the report, so the +fleet accumulated 298 skipped tests across 3210 (9.3%) and 27 spec files that +execute ZERO tests — every one of them on a GREEN run. + +MEASURED 2026-08-24 by this script, against the latest green `development` run +of each of 20 fleet apps. Fleet totals: 298/3210 skipped, V1=27, V2=61, V3=142. +The eight apps carrying it: + + buildiq 68/259 (26.3%) 6 zero-test specs + decidiq 57/191 (29.8%) 1 + dossiq 38/137 (27.7%) 5 + pipelinq 40/324 (12.3%) 3 + learniq 27/457 (5.9%) 6 + shillinq 21/345 (6.1%) 3 + integriq 16/191 (8.4%) 2 + filinq 9/124 (7.3%) 1 + +(An earlier hand-rolled count of mine read double these, by summing the +report's own `report.json` roll-up alongside the per-file entries. This script +skips `report.json` for that reason — the per-file entries are the whole truth +and the roll-up repeats them.) + +Three findings, in the order they matter. + +**V1 — a spec file that executed zero tests.** It contributes nothing and reads +as coverage. Worse, gate-19 accepts it as the `@e2e` anchor for an openspec +scenario, so a scenario can be "covered" by a file that never runs. + +**V2 — a skip that defers to a deploy state CI fully determines.** These are +real, copied verbatim from green runs: + + "decidiq not deployed on the shared instance — live-run deferred" + "Members tab not deployed on this instance" + "Deploy drift: the deployed decidiq predates minutes-ui-v1" + "Sub-cases tab not present in the deployed build (deploy mismatch)" + "leaves PR not deployed yet" + +Every one was written for a SHARED DEV INSTANCE, where the deployed build really +can lag the test. CI is not that, and the claim fails there in one of two ways: + +* About the app under test, it is **impossible**. The app IS the head commit, so + "not deployed" and "deploy drift" cannot happen, and the same sentence can + only mean the feature is missing or broken. +* About a dependency (decidiq's 46 all say "leaves PR not deployed yet", meaning + the pinned `nextcloud-vue`), it is **deterministic**. The lockfile decides it, + identically on every run. A test that quietly stands down because the pinned + dependency is too old is not waiting for anything: either bump the dependency + or drop the test. + +Either way the guard is an unconditional escape hatch in the one environment +where the condition is not a matter of luck. Note the shape of decidiq's worst +case — `partial registry: 27/29 providers` skips an exact-count assertion, so a +registry that is 93% correct reads as "not deployed" rather than as a diff. + +**V3 — a skip with no reason.** 142 of the 298 carry no description at all. A +reason is already required by gate-16 and gate-19 for their exclusions; a skip +is an exclusion decided at run time and the same rule applies. Without one there +is nothing to review and nothing to expire. + +NOT a finding: a skip that names an optional Nextcloud app or external service +the CI instance genuinely does not have ("No chat backend reachable", "requires +a TaskProcessing LLM provider"). Those describe a real absence the app does not +control. They must still carry a reason (V3), but they are allowed to skip. + +Modes, following the `app:check-code` precedent in quality.yml: `report` always +exits 0 and prints the table, `enforce` exits 1 on any violation. Day one is +report-only per app, because a gate that turns eight apps red at once is a gate +nobody can turn on. +""" + +from __future__ import annotations + +import argparse +import base64 +import io +import json +import os +import re +import sys +import zipfile +from pathlib import Path + +# Reasons that defer to a deploy state CI fully determines — impossible for the +# app under test, deterministic for a pinned dependency. Each entry was taken +# from a real skip annotation on a green fleet run, not invented. Matched +# case-insensitively against the whole reason string. +DEPLOY_STATE = ( + "not deployed", + "not yet deployed", + "deploy drift", + "deploy mismatch", + "not present in the deployed build", + "predates", + "not yet wired", + "deployed build", + "on the shared instance", + "in target instance", + "on this instance", +) + +# A reason may name a genuinely absent optional dependency even though it also +# trips a phrase above ("no Talk backend reachable on this instance"). These +# phrases mark that shape and are checked FIRST, so a real environment gap is +# never reported as a deploy claim. +ENVIRONMENT_ABSENCE = ( + "not reachable", + "no chat backend", + "unreachable", + "credential", + "llm provider", + "not provisioned", + "not installed", + "session does not survive", +) + +# Tolerate further attributes between the id and the closing bracket. Playwright +# writes `' + "" +) +io.open(os.path.join(out, "index.html"), "w", encoding="utf-8").write(html) +PY +} + +# expect