Skip to content

fix(ci): Frontend Tests (unit) sat outside the fleet's only required check (#190) - #229

Merged
rubenvdlinde merged 1 commit into
mainfrom
fix/190-frontend-tests-outside-the-required-check
Aug 8, 2026
Merged

fix(ci): Frontend Tests (unit) sat outside the fleet's only required check (#190)#229
rubenvdlinde merged 1 commit into
mainfrom
fix/190-frontend-tests-outside-the-required-check

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

The issue's premise no longer holds — measured before changing anything

#190 says "0 of 20 fleet repos gate on Playwright, and 17 have no branch protection at all". Re-measured across 25 repos on 2026-08-08:

  • All 25 have ACTIVE rulesets on main, beta and development. The "17 with no protection" figure is stale — the legacy /branches/*/protection API reads mostly empty because the gating moved to rulesets.
  • Main Branch Protection and Beta Branch Protection each require exactly two contexts: branch-protection / check-branch and quality / Quality Report.
  • Development Branch Protection requires zero checks.

development is unprotected by design and stays that way. Admin merges into development are this team's routine, authorised workflow; protecting it would block the thing the team does every day. Nothing in this PR touches it. Recording that here so it does not get "fixed" later.

E2E already gates main and beta — verified on real runs, not read off the YAML

Quality Report is not a pass-through. It carries if: ${{ always() && contains(needs.*.result, 'failure') }}exit 1.

repo run Playwright Quality Report
pipelinq 31181419092 failure failure
scholiq 31164073861 failure failure
nldesign 31252937998 failure failure

The one counter-example — pipelinq 31181387613, Playwright cancelled, Quality Report success — is not a hole. That run's head SHA ce44766c was superseded by 47a1a590 twenty-nine seconds later, and the cancelled-job guard deliberately stays quiet for a superseded run because the newer run carries the verdict. The newer runs on 47a1a590 did report failure. (Checking whether the smoking gun also appears in a legitimately passing scenario is what stopped this becoming a wrong bug report.)

Why no explicit E2E context was added

Requiring quality / E2E Tests (Playwright) would be redundant with a gate measured to work, and it carries the failure mode this fleet has already been bitten by: four repos — openklant, opentalk, openzaak, shillinq — leave enable-playwright UNSET, so the job is SKIPPED there. A required check that does not report leaves PRs permanently pending, which is worse than no protection because it looks like caution.

Branch protection is outward-facing. Adding a duplicate context that can wedge four repos' merge queues is not an improvement over a gate that already works.

What was actually broken

Quality Report's needs: named fifteen jobs. The workflow has sixteen. frontend-testsFrontend Tests (unit) — was not in it.

Both gating steps read needs.*.result. A job absent from needs: is absent from needs.*. So a real unit suite, with no continue-on-error, could fail — or be cancelled mid-run — and the only meaningful required check on main and beta across all 25 repos would stay green.

A failing frontend unit suite could not block a merge to main or beta anywhere in the fleet.

Latent, not active — and said so

Sampled across ten repos, Frontend Tests (unit) is green in every run (with some supersession cancellations). A search for a run with Frontend Tests (unit) = failure alongside Quality Report = success found none. Nothing has been waved through yet.

A gate that would not catch a failure is still a dead gate, and "it has not bitten yet" is not a verdict.

The fix and the guard

frontend-tests joins the list, and scripts/assert-quality-report-gates-every-leg.py asserts that every top-level job is either the report job or inside its needs:. An exemption needs an ALLOWLIST entry with a stated reason, so the list can only shrink and it says why.

Proven in both directions against the real document:

against origin/main   -> FAILS, naming 'frontend-tests'
against this commit   -> passes (all 16 legs gated)
--positive-control    -> drops a job from the list in memory and must notice; it names the job it dropped

It also refuses to pass on a degenerate parse (< 5 jobs) or a missing report job — the latter being its own outage, since renaming that job would make quality / Quality Report stop reporting and wedge every PR in the fleet.

It runs as its own job in quality-resolve-probe.yml behind the same single Shared-workflow guard required check, with its positive control first, and it lives in .github's own CI so it cannot itself go permanently pending.

What consumers see on the next run

The fleet tracks @main unpinned (#177), so this reaches all callers immediately.

  • enable-frontend: false → job SKIPPED → not failurenothing changes.
  • Suite passes → nothing changes.
  • Only a genuine frontend unit failure, or a frontend-tests job cancelled while its run carries on, newly blocks a merge to main or beta. That is the point.

No ruleset was modified in any repo. No waiver, no continue-on-error, no weakened assertion.

Refs #190

…check (#190)

MEASURED FIRST — the issue's premise no longer holds

#190 says "0 of 20 fleet repos gate on Playwright, and 17 have no branch
protection at all". Re-measured across 25 repos on 2026-08-08, the second half
is simply false and the first half is misleading:

  * ALL 25 repos have ACTIVE rulesets on main, beta AND development.
  * `Main Branch Protection` and `Beta Branch Protection` each require exactly
    two contexts: `branch-protection / check-branch` and
    `quality / Quality Report`.
  * `Development Branch Protection` requires ZERO checks. That is correct and
    deliberate — development is unprotected BY DESIGN, because admin merges
    into it are this team's routine workflow. Nothing here changes it.

And E2E already gates main and beta, transitively. `Quality Report` is not a
pass-through: it exits 1 on `contains(needs.*.result, 'failure')`. Verified on
real runs rather than read off the YAML —

  pipelinq 31181419092   Playwright failure -> Quality Report failure
  scholiq  31164073861   Playwright failure -> Quality Report failure
  nldesign 31252937998   Playwright failure -> Quality Report failure

The one counter-example, pipelinq 31181387613 (Playwright CANCELLED, Quality
Report SUCCESS), is not a hole: that run's head SHA ce44766c was superseded by
47a1a590 twenty-nine seconds later, and the cancelled-job guard deliberately
stays quiet for a superseded run because the newer run carries the verdict.
The newer runs on 47a1a590 did report failure.

So no new required context was added. Requiring
`quality / E2E Tests (Playwright)` explicitly would be redundant with a gate
measured to work, and it carries the risk the fleet has already been bitten by
three times: four repos (openklant, opentalk, openzaak, shillinq) leave
`enable-playwright` UNSET, so the job is SKIPPED there, and a required check
that does not report leaves PRs permanently PENDING — worse than no protection
because it looks like caution.

WHAT WAS ACTUALLY BROKEN

`Quality Report`'s `needs:` list named fifteen jobs. The workflow has sixteen.
`frontend-tests` ("Frontend Tests (unit)") was not in it.

Both of that job's gating steps read `needs.*.result`. A job absent from
`needs:` is absent from `needs.*`, so `Frontend Tests (unit)` — a real unit
suite with no `continue-on-error` — could fail, or be cancelled mid-run, and
the only meaningful required check on main and beta across all 25 repos would
stay green. A failing frontend unit suite could not block a merge to main or
beta anywhere in the fleet.

LATENT, NOT ACTIVE, AND SAID SO. Sampled across ten repos the job is currently
green in every run (with some supersession cancellations), and a search for a
run with `Frontend Tests (unit) = failure` alongside `Quality Report = success`
found none. Nothing has been waved through yet. A gate that would not catch a
failure is still a dead gate, and "it has not bitten yet" is not a verdict.

THE FIX AND THE GUARD

`frontend-tests` joins the list, and
`scripts/assert-quality-report-gates-every-leg.py` now asserts that EVERY
top-level job is either the report job or inside its `needs:`. An exemption
requires an ALLOWLIST entry with a stated reason, so the list can only shrink
and it says why.

Proven in both directions, against the real document:
  * run against `origin/main` -> FAILS, naming `frontend-tests`
  * run against this commit   -> passes
  * `--positive-control` drops a job from the list in memory and requires the
    check to notice; it names the job it dropped

It runs as its own job in quality-resolve-probe.yml, behind the same single
`Shared-workflow guard` required check, with its positive control first — and
it lives in .github's own CI, so it cannot itself go permanently pending.

BLAST RADIUS FOR CONSUMERS

The fleet tracks @main unpinned (#177), so this reaches all callers on their
next run. Repos where `enable-frontend` is false see a SKIPPED job, which is
not 'failure' and changes nothing. Repos where the suite passes see nothing.
Only a genuine frontend unit failure — or a frontend-tests job cancelled while
its run carries on — newly blocks a merge to main or beta. That is the point.

Refs #190
@rubenvdlinde
rubenvdlinde merged commit 0bb4c0c into main Aug 8, 2026
29 checks passed
rubenvdlinde added a commit that referenced this pull request Aug 8, 2026
…e threshold never gated (#189) (#253)

* fix(ci): a security failure DELETED the test tier (#194); the coverage threshold never gated (#189)

#194 — gating the test tier on the security tier silently deletes all test
evidence. phpunit, newman, playwright and journeydoc-capture each carried
`&& needs.security.result != 'failure'`. Because composer audit queries the
LIVE Packagist feed, CVE-2026-67434 against squizlabs/php_codesniffer — a code
formatter that never runs in production — turned the whole fleet's test tier
into 'skipped' on 2026-08-06 with no commit anywhere. A skipped job is a grey
tick, not a red X, so nothing counted it and a review filed a false 'fully
green' report.

Option E of the issue: decouple AND render loudly.
  * the four test jobs no longer read needs.security.result; 'needs:' is kept
    for ordering only, suppressed by the existing !cancelled().
  * security still blocks the merge unchanged, via the required
    'quality / Quality Report' check. Nothing is weakened at the merge gate.
  * Quality Report gains a third gate: an ENABLED test job in state 'skipped'
    is the ABSENCE OF A VERDICT — it hard-fails and says so in words, and
    distinguishes 'tests passed, security failed' from 'tests never ran'.
  * scripts/assert-no-producer-deletes-a-verdict.py makes it an invariant, and
    closes the direction #229 left open: #229 asserted every job can REACH the
    required check, this asserts no job can be DELETED before it gets there.

Proved live, not argued: run 31259774225 on fixture/issue-194-evidence-deletion.
Identical failing test job under the two conditions —
  gated on security   -> skipped   (evidence gone, old tally GREEN)
  decoupled           -> failure   (verdict exists)
  new invariant       -> failure   ('TEST TIER NOT EXECUTED — NO VERDICT EXISTS')

#189 — playwright-coverage-threshold has never gated. Three defects, all fixed:
  1. below-threshold emitted ::warning:: and exited 0, so the knob was
     decorative. It now ::error::s and exits non-zero.
  2. the metric was count(test() calls) / count(scenario headings) — two
     independent totals never compared to each other, which ten unrelated
     tests raised as much as covering ten scenarios did, and which could
     exceed 100% while covering nothing. Replaced with real per-scenario
     matching against @e2e references in gate-19's dialect. The old ratio is
     kept and reported as testsPerScenarioPercent, never gated on.
  3. zero scenarios scored 100%. Zero enforceable scenarios is now NOT
     MEASURABLE and fails — a measurement that could not be taken is not a pass.

Default threshold 75 -> 0 to bound the blast radius: measured across all 31
fleet callers, exactly one repo (pipelinq) enables this, and it sets its own
value. Gating is now opt-in by setting a number, which is what a threshold
input should mean.

scripts/test-spec-coverage-gate.py extracts the shipped program out of
quality.yml and runs it against fixtures — including the one #189 says cannot
currently exist: coverage below threshold turning the job red. 24 assertions;
the positive control neuters the gate to warning-only and 5 of them flip to
FAIL, so its clean pass is a verdict.

Both new scripts are wired into quality-resolve-probe.yml. A checker with no
callers is not a checker.

* test(ci): mutation battery for the spec-coverage gate; state the gate-19 liveness gap

Peer review asked for a mutation standard rather than a single positive
control. Seven mutants, each reintroducing one specific defect, plus an
anti-widening control that reworks a log string nothing asserts on and which
the suite must NOT notice — without it a suite that failed on any edit would
score a perfect kill rate while being worthless.

It earned its keep on the first run: 'exclude-directive-read-as-reference'
SURVIVED. The fixture used '@e2e exclude <slug>' space-separated, and under
that form the guarded and unguarded regexes are indistinguishable — both
capture 'exclude', which contains no '#' or '::' and so resolves to no slug.
The assertion had been passing while proving nothing. The guard is load-bearing
only for '@e2e exclude::<slug>' and '@e2e exclude#<slug>', where the unguarded
regex marks the named scenario COVERED; the fixture now uses those forms and
the mutant dies. 7 of 7 killed, control survives.

An unapplied mutant is reported as SKIPPED (wiring) and FAILS the run rather
than counting as a kill — an anchor that has drifted means the battery measures
less than it claims.

Also documents a real limitation rather than leaving it to be discovered: #249
rewrote gate-19 to parse test files with a real JS parser, so it will not count
an @e2e reference inside a describe.skip or an empty test body. This step reads
the annotation as text and will, so its number is an UPPER BOUND on real
coverage. A passing threshold here is not evidence that gate-19 would pass.

---------

Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant