gate-19: a CONDITIONAL `test.skip(true, reason)` inside an `if` guard is read as a permanent skip — a test that ran and PASSED is reported as "never runs"

[rubenvdlinde]

Summary

check_e2e_coverage.py's "does this test actually run" discriminator classifies any test.skip(true, …) in a test body as an unconditional skip. But test.skip(true, reason) is also the correct Playwright form for a conditional skip written inside an if guard — the single most common defensive idiom in the fleet's e2e suites. The true is the API's "skip from this point" shape; the call site is what carries the condition.

Result: gate-19 reports "referenced only by a test that never runs" about tests that run on every reachable environment — including, provably, tests that passed in the same CI run that produced the finding.

Minimal reproduction

Two tests in one file, identical but for a guarded skip:

// @e2e openspec/specs/demo/spec.md#s1-guarded-skip-inside-an-if
test('guarded: runs whenever the app is reachable', async ({ page }) => {
	const response = await page.goto('/app').catch(() => null)
	if (!response) {
		test.skip(true, 'app not reachable')
		return
	}
	await expect(page.locator('body')).not.toContainText('Internal Server Error')
})

// @e2e openspec/specs/demo/spec.md#s2-no-skip-at-all
test('clean: no skip anywhere', async ({ page }) => {
	await page.goto('/app')
	await expect(page.locator('body')).not.toContainText('Internal Server Error')
})

$ HYDRA_GATE_BASE_REF=base python3 check_e2e_coverage.py .
demo::s1-guarded-skip-inside-an-if — @e2e tag present but the test does not run:
  referenced only by a test that never runs (tests/e2e/demo.spec.ts). …
[gate-19] e2e-coverage: FAIL — 1 scenario(s) without a running e2e test

S2 passes, S1 fails. The only difference is the guarded skip.

Live confirmation

On ConductionNL/procest PR #765, gate-19 flagged req-zak-004a and req-zak-004b. Both are tagged on:

test('cases index renders so a case dossier tab can be opened', async ({ page }) => {
	const response = await page.goto('/index.php/apps/procest/cases').catch(() => null)
	if (!response) {
		test.skip(true, 'Procest dev container not reachable')
		return
	}
	await expect(page.locator('body')).not.toContainText('Internal Server Error', { timeout: 10000 })
})

No .skip / .fixme modifier. Its only skip is inside if (!response). The E2E job on the same commit reported 87 passed / 0 failed / 38 skipped — this test is one of the 87. gate-19 said it never runs.

Why the current rule was chosen, and why the reasoning inverts here

The source comment is explicit and deliberate (around L440–L460):

test.skip(true) — an UNCONDITIONAL skip at the top of a body … so the discriminator is the argument, not the call.

That holds for test.skip(true) written at the top of a body, which is a permanent skip. It does not hold for the same call written inside a conditional block, where the guard is the condition and true is only how you tell Playwright "skip now". Both forms are legal, idiomatic Playwright and are textually identical at the call.

Blast radius

Counted with the gate's own patterns across the accessible fleet checkouts — occurrences of test.skip(true, …) that are inside an if / else / catch guard:

repo	guarded test.skip(true, …)
openregister	81
procest	22
hermiq	4
docudesk	3
nldesign	1

111 occurrences, versus 4 genuinely unconditional ones in the same checkouts (all in one procest file, now fixed). So the rule currently misfires on ~96% of the call sites it inspects, and any PR touching one of those specs gets a false finding.

Why this is worse than a normal false positive

The remediation gate-19 offers is "replace the tag with a reason-bearing @e2e exclude" — i.e. declare the scenario not e2e-covered. Complying with a false finding therefore deletes a true coverage claim and permanently marks a genuinely-tested scenario as untestable. The gate actively pushes the codebase in the wrong direction.

Suggested fix

Determine conditionality from the call site, not the argument: a test.skip(true, …) is unconditional only when it is a direct statement of the test/describe body, not nested inside an if / else if / else / catch / try block. The existing _has_own_unconditional_skip() already walks block structure to exclude skips belonging to nested tests — the same walk can note enclosing conditional blocks.

Include the reproduction above as a fixture: it fails both ways today (S1 false-positive; and a genuine top-of-body test.skip(true) must still be caught).

Secondary observation — no representation for "quarantined pending an issue"

The same procest PR converted three permanently-skipped tests to test.fixme referencing a tracking issue. gate-19 correctly notes they do not run, but its only sanctioned end state is @e2e exclude <reason>, which means "this scenario is out of scope for e2e, forever". A scenario that should be e2e-tested and is blocked on a tracked fixture gap has no way to say so. Consider accepting a tracked form (e.g. @e2e blocked <issue> <reason>) that is visible in reporting without being either a silent pass or a permanent exclusion.

Found during a fleet-wide triage of disabled e2e declarations.

[rubenvdlinde]

Fixed in #249.

Conditionality is now decided by the call site, as you proposed, and the rule is: a test.skip(true, …) switches its block off only when it is a direct statement of that block's body — not inside a nested declaration, not inside a braced block, and not the braceless body of an if / while / for / catch / else / try.

The reproduction from this issue is in the suite verbatim (S1 live, S2 live), with four controls that must stay dead or the fix would be an amnesty rather than a correction:

• a genuine top-of-body test.skip(true, 'broken since March') → dead
• a group-level test.skip(true, …) directly in a describe body → dead, and it still takes the whole group
• the braceless if (!ok) test.skip(true, …) → live
• a skip inside a catch → live

Mutation-checked: reinstating the argument-only rule turns 4 of these red; a mutant that calls every ref live turns 25 red, which is the control that the fix did not simply widen the gate.

Measured on the repos this issue named, root-commit-scoped: openregister 799 → 794 (its 5 are your braceless if (!registerId) test.skip(true, 'no test register')), procest 1181 → 1166, decidesk 991 → 984 (the if (!(await openFirstLiveMeeting(page))) { test.skip(true, …); return } idiom exactly).

procest's remaining 26 are correct: they are test.skip(true, 'Requires a seeded case fixture …') written at brace depth 0 of the test body, i.e. genuinely unconditional. The gate now separates them from the guarded ones in the same file — that file went from 41 findings to 26.

Your secondary observation

no representation for "quarantined pending an issue"

Not addressed here, deliberately — it is a policy change (a new sanctioned annotation, @e2e blocked <issue> <reason>), not a false-positive fix, and folding it in would have made this PR's fleet numbers unattributable. It is a real gap: larpingapp's 23 remaining findings are all test.fixme referencing DETAIL_ACTIONS_BLOCKER, and openbuild's 36 are test.skip('title', …) — 59 findings whose only sanctioned end state today is to declare the scenario permanently out of e2e scope. Worth a separate issue.