Skip to content

fix(gate-19): a GUARDED test.skip(true, …) is a test that RUNS - #283

Closed
rubenvdlinde wants to merge 1 commit into
mainfrom
fix/gate-19-guarded-skip-is-not-permanent
Closed

fix(gate-19): a GUARDED test.skip(true, …) is a test that RUNS#283
rubenvdlinde wants to merge 1 commit into
mainfrom
fix/gate-19-guarded-skip-is-not-permanent

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Closes #239.

The bug

gate-19's liveness check treated every test.skip(true, …) as permanent. That is not the syntax for "this test is switched off" — it is Playwright's skip-from-this-point form, and the condition lives at the call site:

if (!response) { test.skip(true, 'container not reachable'); return }

That test runs, and passes, whenever the guard is false. So the gate reported "referenced only by a test that never runs" about tests that ran and passed in the same CI run.

Measured blast radius

test.skip(true, …) call sites across 11 repos 118
of those, guarded 114
genuinely unconditional 4 (all in one procest file)

The rule misfired on ~96% of the sites it inspected.

On openregister alone: of 205 @e2e refs across 63 spec files, 16 flip dead → live under this fix.

Why this was worse than an ordinary false positive

The gate's prescribed remedy is @e2e exclude. So complying with a false finding deletes a true coverage claim and marks a tested scenario permanently untestable. The gate would have laundered away real coverage, and the resulting number would have looked like an improvement.

The fix

_skip_is_guarded() walks backwards tracking brace depth to the innermost still-open {, asks what introduced it, and repeats outward — so a skip nested several blocks deep inside an if is still guarded.

finally is deliberately not a guard opener. A finally block always runs, so a skip there is unconditional. That case is the sharpest control: it proves the check discriminates between block openers rather than treating any enclosing brace as a guard.

It also caught a real bug in my first draft — a 120-character look-back window matched the try in } finally { and called a finally-block guarded, the exact inversion this check exists to prevent. The opener regex is now anchored to the token immediately preceding the brace.

Verified in both directions

78 tests pass. The controls:

state result
_skip_is_guarded stubbed to pre-fix behaviour FAILtest_guarded_skip_is_live, rc 1
restored rc 0
bare test.skip(true) still dead
skip inside finally still dead

A fix that only makes things live is a fix that turned the rule off. The negative controls are what distinguish the two.

gate-19's liveness check treated every `test.skip(true, …)` as permanent. But
that is not the syntax for "this test is switched off" — it is Playwright's
skip-from-this-point form, and the condition lives at the CALL SITE:

    if (!response) { test.skip(true, 'container not reachable'); return }

That test runs, and passes, whenever the guard is false. So the gate reported
"referenced only by a test that never runs" about tests that ran and passed in
the same CI run.

MEASURED
--------
Across 11 repos: 118 `test.skip(true, …)` call sites — 114 guarded, and only
4 genuinely unconditional (all 4 in one procest file). The rule misfired on
~96% of the sites it inspected.

On openregister alone: of 205 `@e2e` refs across 63 spec files, 16 flip
dead -> live under this fix.

WHY IT MATTERED MORE THAN A NORMAL FALSE POSITIVE
-------------------------------------------------
The gate's prescribed remedy is `@e2e exclude`. Complying with a false finding
therefore DELETES a true coverage claim and marks a tested scenario
permanently untestable — the gate would have laundered away real coverage.

THE FIX
-------
`_skip_is_guarded()` walks backwards tracking brace depth to the innermost
still-open `{`, then asks what introduced it, repeating outward so a skip
nested several blocks deep inside an `if` is still guarded.

`finally` is deliberately NOT a guard opener: a finally block always runs, so
a skip there is unconditional. That case is the sharpest control — it proves
the check discriminates BETWEEN block openers rather than treating any
enclosing brace as a guard. It also caught a real bug in the first draft: a
120-char look-back window matched the `try` in `} finally {`. The opener regex
is now anchored to the token immediately preceding the brace.

VERIFIED BOTH DIRECTIONS
------------------------
78 tests pass. With `_skip_is_guarded` stubbed to the pre-fix behaviour,
`test_guarded_skip_is_live` FAILS (rc 1); restored, rc 0. The negative
controls — a bare `test.skip(true)` and a skip inside `finally` — must stay
dead, and do.

Closes #239
@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Closing — already fixed on main, and I should have checked before building this.

main moved substantially while I was working (the gate overhaul in #275/#276/#279/#280/#282). Its current check_e2e_coverage.py carries a _is_guarded() method and its test suite already covers these cases — the comment at test_check_e2e_coverage.py:1288 cites the same measurement I arrived at independently: 111 guarded call sites in the fleet against 4 genuinely unconditional. Issue #239 is CLOSED.

Verified: main's suite is 105 tests, all passing.

What I built here is redundant, and merging it would have meant re-litigating a solved problem against a rewritten module — my branch's conflict was in the module docstring precisely because main had already reworked this area.

One thing from this attempt worth keeping regardless of whose fix landed: the finally case is the sharpest control. A skip inside finally runs unconditionally, so it must stay dead. My first draft used a 120-character look-back window and matched the try in } finally { — calling a finally-block guarded, which is the exact inversion the check exists to prevent. Only the token immediately preceding the brace should decide. If main's implementation doesn't already discriminate that case, it's worth a test.

Lesson on my side: I measured the bug, wrote the fix, and proved it both directions — but never re-checked whether main had moved under me. Same class as the stale-checkout error recorded earlier in this programme.

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.

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"

1 participant