From 861a3acb2cf11b2f87f84c0d9cf1625590205aff Mon Sep 17 00:00:00 2001 From: Conduction Release Bot Date: Sat, 8 Aug 2026 15:32:03 +0200 Subject: [PATCH] fix(gate-19): read the test file with a parser, not three regexes (#234, #239, #244) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gate-19 is the highest-volume gate in the fleet. Its three open false-positive issues were three symptoms of one decision — reading JavaScript with regular expressions — and all three surfaced as the same sentence, "referenced only by a test that never runs", about tests that ran and PASSED in the same CI run. #234 A TRAILING COMMA before the closing paren. The body was located by stepping back from `)` over whitespace and requiring a `}`. Prettier's default and ESLint's `comma-dangle: always-multiline` put a `,` at exactly that index, so the body read as "" and the empty-body rule fired on a real, asserting test. #239 A CONDITIONAL `test.skip(true, reason)` inside an `if` guard. The discriminator was the ARGUMENT alone, but `true` is just Playwright's "skip from this point" shape — the CALL SITE carries the condition. 111 guarded call sites in the fleet against 4 genuinely unconditional ones. Worse, the remedy the gate prints is "replace the tag with @e2e exclude", so complying DELETED a true coverage claim. #244 A TAG WRITTEN INSIDE THE `test(` ARGUMENT LIST. Tag resolution only ever searched FORWARD, so a tag between the open paren and the title bound to the NEXT test in the file. On nldesign that mis-binding then met #234 on whichever test it landed on, and 34 of 190 findings came out. Two defects, one symptom — which is why the fixture asserts the BINDING and not only the count. So the file is tokenised once (comments, string contents, template contents and regex literals blanked; string delimiters kept, because "is the first argument a string literal" is the whole difference between `test.skip('t', fn)` and `test.skip(cond, 'reason')`), and a real tree of test/describe calls is built with header and body ranges. Structure questions are answered from that tree. Everything the old regexes had earned is kept and re-asserted: `rx.test(` is not Playwright, `latest(` merely ends in a name, `test.describe.skip(` must match (#212 — NOT undone), `.only`/`.serial` are not switched-off markers, and `test.beforeEach(`/`test.use(`/`test.step(`/ `test.describe.configure(` are not declarations at all. SIGNALLING. This gate returned its finding COUNT as an exit status — a byte — so 266 findings left as 10 and 256 would have left as 0, i.e. PASS (#209). The clamp that fixed the wrap made the byte carry NEITHER: a 404-finding run exited 255 while stdout said 404 (#242). The byte is now a status and nothing else — 0 pass, 1 fail, 2 error — and the count is on stdout, where the runner already reads it. A crash now reports SKIPPED (wiring), visible to --require-full-coverage, instead of a fabricated verdict; the runner also stops discarding the helper's stderr. NOT TOUCHED: the empty-diff `_pass` branch, which is #242's subject and is being fixed separately. MEASURED, root-commit-scoped, across 24 local checkouts: 8790 -> 8698 findings, -92, and every one of the 92 is a false positive removed. Not one finding was added anywhere. nldesign 190 -> 156 (exactly the 34 in #244); decidesk 991 -> 984; procest 1181 -> 1166; softwarecatalog 312 -> 291; openregister 799 -> 794; opencatalogi 51 -> 46; shillinq 284 -> 279. Unchanged where the dead findings are genuine: openconnector 412 (6 real `test.describe.skip`), openbuild 187 (36 real `test.skip('title', …)`), larpingapp 101 (23 real `test.fixme`), scholiq 102. PLANTED TRUE POSITIVES, against nldesign's real spec + real e2e suite after the fix: a scenario with no test at all, a scenario tagged only by a skipped test, and a scenario tagged only by an empty-bodied test are all still caught (156 -> 159), while a fourth planted scenario tagged by a real test in the nldesign trailing-comma layout is correctly not flagged. TESTS: 75 -> 107, all green, plus the 27 other helper suites and the 59 entry-point tests. Mutation-checked: reinstating the trailing-comma bug, the argument-only skip rule, the forward-only tag resolution, the header branch, and the count-as-exit-status each turn the right tests red — and a mutant that calls every ref live turns 25 tests red, which is the control that this fix did not simply widen the gate. One earlier mutant SURVIVED (deleting the header branch), proving that fixture could not see the branch it was meant to cover; a describe-header case was added that kills it. --- hydra-gates/scripts/lib/check_e2e_coverage.py | 827 +++++++++++++----- .../scripts/lib/test_check_e2e_coverage.py | 526 ++++++++++- hydra-gates/scripts/run-hydra-gates.sh | 31 +- 3 files changed, 1112 insertions(+), 272 deletions(-) diff --git a/hydra-gates/scripts/lib/check_e2e_coverage.py b/hydra-gates/scripts/lib/check_e2e_coverage.py index 0d43dd4..bd25f5d 100644 --- a/hydra-gates/scripts/lib/check_e2e_coverage.py +++ b/hydra-gates/scripts/lib/check_e2e_coverage.py @@ -79,7 +79,12 @@ In gate mode (default), the gate is diff-scoped via ``HYDRA_GATE_BASE_REF`` (default ``origin/development``): only scenarios in spec files that are ADDED or MODIFIED in the PR are checked. Scenarios in untouched spec files are never -flagged. Exit code = number of uncovered (or bare-exclude) scenarios. +flagged. + +Exit code is a STATUS, not a count: ``0`` pass, ``1`` fail, ``2`` error. The +number of findings is on stdout, in the ``FAIL — scenario(s)`` summary +line. Read stdout; an exit status is one byte and this gate has already +returned a count through it twice. Report mode (``--mode report``) scans the entire ``openspec/specs/`` tree and emits a JSON summary — not diff-scoped, always exits 0. @@ -433,92 +438,292 @@ def _flush_alt_item() -> None: # --------------------------------------------------------------------------- -# A PERMANENTLY-SKIPPED TEST IS NOT COVERAGE +# A PERMANENTLY-SKIPPED TEST IS NOT COVERAGE — AND READING ONE IS A PARSE +# --------------------------------------------------------------------------- # # Observed on decidesk: four tests with EMPTY BODIES and a hardcoded # `test.skip(true, ...)`. Each carried an `@e2e` tag, each was counted as # traceability, and together they asserted NOTHING. That is a dead gate by # construction — the tag says a scenario is proven, the test proves nothing, -# and the gate cannot tell the difference. +# and the gate cannot tell the difference. That rule stays. # # What is dead: # test.skip('name', ...) the modifier form — declares a skipped test # it.skip(...) / xit / xtest / test.fixme(...) # describe.skip(...) takes every test inside it with it -# test.skip(true) an UNCONDITIONAL skip at the top of a body +# test.skip(true) an UNCONDITIONAL skip, as a DIRECT STATEMENT +# of the body it belongs to # test.skip() argument-less, same thing # an empty body nothing but whitespace and comments # # What is NOT dead, and must keep counting: # test.skip(browserName === 'firefox', 'flaky on gecko') # test.skip(!process.env.CI, 'needs a CI fixture') +# if (!reachable) { test.skip(true, 'app not reachable') } # -# ...because those run somewhere. A RUNTIME CONDITION is a real test with a -# guard; a literal `true` is a test someone turned off. Conflating them would -# swap this gate's blindness for a different one — refusing legitimate -# conditional skips — so the discriminator is the argument, not the call. -# `\b` is not enough of a boundary: it matches the `test` in `rx.test(text)`, -# and JavaScript's RegExp.prototype.test is not Playwright's test(). On -# openconnector, `dead-letter-replay.spec.ts` has +# WHY THIS IS NOW A PARSER (#234, #239, #244) +# ------------------------------------------- +# Every previous version of this section read JavaScript with regular +# expressions and a hand-rolled paren walk. Three separate false REDs came out +# of that one decision, and all three were reported as the same sentence — +# "referenced only by a test that never runs" — about tests that ran and +# PASSED in the same CI run: # -# IGNORED_CONSOLE_PATTERNS.some((rx) => rx.test(text)) +# #234 A TRAILING COMMA before the closing paren. The body was found by +# stepping back from `)` over whitespace and requiring a `}`. Prettier +# and `comma-dangle: always-multiline` put a `,` there, so `body` +# stayed "" and the empty-body rule fired on a real, asserting test. # -# in a helper ABOVE its tests, and the forward search from the file-level -# `@e2e` tags landed on it. `_ref_is_live` then read that call's "body" — -# there is none — and reported all 11 refs as "referenced only by a test that -# never runs", about a file whose tests run fine. +# #239 A CONDITIONAL `test.skip(true, reason)` INSIDE AN `if` GUARD. The +# old discriminator was the ARGUMENT alone, so the single most common +# defensive idiom in the fleet (111 call sites vs 4 genuinely +# unconditional ones) read as a permanent skip. Worse, the gate's +# suggested remedy is to replace the tag with `@e2e exclude`, i.e. to +# DELETE a true coverage claim. # -# `(? { … }, +# ) # -# The fix is an explicit, optional `test.` / `it.` NAMESPACE segment. It is -# named rather than general (`\w+\.`) on purpose: `rx.test(` and `foo.it(` -# must still be rejected, and only Playwright's two roots may open a block. -# The `.serial` / `.parallel` / `.only` segments are Playwright's other -# describe modifiers and are NOT switched-off markers — a `describe.only` runs -# (and suppresses everything else), so it stays live. -_TEST_DECL_RE = re.compile( - r"(?test|it|describe)" - r"(?:\s*\.\s*(?:serial|parallel))?" - r"(?P\s*\.\s*(?:skip|fixme|failing))?" - r"(?:\s*\.\s*only)?" - r"\s*\(", -) -_XTEST_RE = re.compile(r"\b(?:xit|xtest|xdescribe)\s*\(") -# `test.skip(true)` / `test.skip( 1 )` / `test.skip()` — no runtime condition. -_UNCONDITIONAL_SKIP_RE = re.compile( - r"\b(?:test|it)\s*\.\s*skip\s*\(\s*(?:\)|true\s*[,)]|1\s*[,)])" +# The tag resolution only ever searched FORWARD, so a tag written +# inside its own test's header resolved to the NEXT test in the file +# (or ran off the end). On nldesign that mis-binding, compounded by +# #234 on the test it landed on, produced 34 of 190 findings. +# +# So: tokenise the file once (strings, template literals, regex literals and +# comments are blanked, delimiters kept), then build the real tree of +# test/describe calls with their header and body ranges. Structure questions +# are answered from that tree instead of from a pattern that happens to look +# like the code. +# +# The identifier rules the regexes earned are kept, because they were right: +# * `rx.test(text)` is JavaScript's RegExp.prototype.test, not Playwright's +# test() — a member call is never a declaration (openconnector, +# `dead-letter-replay.spec.ts`, 11 refs). +# * `latest(` / `submit(` merely END in a declaration name. +# * `test.describe.skip(` is Playwright's canonical spelling and MUST be +# matched — a hand-written alternation could not see it at all (#210). +# * `.only` / `.serial` / `.parallel` are not switched-off markers; a +# `describe.only` runs (and suppresses everything else), so it stays live. +# * anything else after the root — `test.beforeEach(`, `test.use(`, +# `test.step(`, `test.setTimeout(`, `test.describe.configure(` — is not a +# declaration at all. + +# Keywords after which a `/` opens a regular expression rather than dividing. +_JS_REGEX_KEYWORDS = frozenset({ + "return", "typeof", "instanceof", "in", "of", "new", "delete", "void", + "throw", "case", "do", "else", "yield", "await", +}) + +# A call whose callee is a dotted chain rooted at one of Playwright's/Jest's +# declaration names. Matched against the CODE MASK, so a `test(` inside a +# string, a comment or a regex literal is not a candidate at all. +_CALL_RE = re.compile( + r"(?test|it|describe|xit|xtest|xdescribe)" + r"(?P(?:\s*\.\s*[A-Za-z_$][A-Za-z0-9_$]*)*)\s*\(", ) +_IDENT_RE = re.compile(r"[A-Za-z_$][A-Za-z0-9_$]*") + +_OFF_SEGMENTS = frozenset({"skip", "fixme", "failing"}) +_NEUTRAL_SEGMENTS = frozenset({"only", "serial", "parallel", "concurrent"}) +# Guards whose body may be written WITHOUT braces: `if (x) test.skip(true, …)` +_GUARDS_WITH_PAREN = frozenset({"if", "while", "for", "catch"}) +_GUARDS_BARE = frozenset({"else", "do", "try"}) + + +def _skip_string(text: str, i: int) -> int: + """Index just past the quoted string whose opening quote is at *i*. + + An unterminated literal stops at the newline rather than swallowing the + rest of the file — a lone apostrophe in a comment must not blank a suite. + """ + quote = text[i] + n = len(text) + j = i + 1 + while j < n: + c = text[j] + if c == "\\": + j += 2 + continue + if c == quote: + return j + 1 + if c == "\n": + return j + j += 1 + return n -def _strip_comments(text: str) -> str: - """Remove // and /* */ comments so an empty body is not mistaken for a - documented one. Crude but sufficient: this only ever decides "is there any - executable statement here", never what the statement means.""" - text = re.sub(r"/\*.*?\*/", " ", text, flags=re.DOTALL) - text = re.sub(r"(?m)//.*$", " ", text) - return text +def _skip_template(text: str, i: int) -> int: + """Index just past the template literal whose backtick is at *i*. + + `${ … }` substitutions are walked (they may contain braces, quotes and + further templates) but their contents are blanked along with the rest: a + test declaration inside a template substitution is not a thing. + """ + n = len(text) + j = i + 1 + depth = 0 # ${ } nesting inside this template + while j < n: + c = text[j] + if c == "\\": + j += 2 + continue + if depth == 0: + if c == "`": + return j + 1 + if c == "$" and j + 1 < n and text[j + 1] == "{": + depth += 1 + j += 2 + continue + j += 1 + continue + if c == "`": + j = _skip_template(text, j) + continue + if c in "'\"": + j = _skip_string(text, j) + continue + if c == "{": + depth += 1 + elif c == "}": + depth -= 1 + j += 1 + return n + + +def _skip_regex(text: str, i: int) -> int: + """Index just past the regex literal starting at *i*, or -1 if it is not + one. A regex literal cannot span a newline, which is the cheap and + reliable disambiguator against division.""" + n = len(text) + j = i + 1 + in_class = False + while j < n: + c = text[j] + if c == "\\": + j += 2 + continue + if c == "\n": + return -1 + if in_class: + if c == "]": + in_class = False + elif c == "[": + in_class = True + elif c == "/": + j += 1 + while j < n and (text[j].isalpha()): + j += 1 + return j + j += 1 + return -1 + + +def _regex_can_start(prev_char: str, prev_word: str) -> bool: + """Whether a `/` at this point opens a regex rather than dividing.""" + if prev_char == "": + return True + if prev_char in ")]": + return False + if prev_char in "'\"`": + return False + if prev_char.isalnum() or prev_char in "_$": + return prev_word in _JS_REGEX_KEYWORDS + return True + + +def _code_mask(text: str) -> str: + """A same-length copy of *text* with every non-code character blanked. + + Comments, string contents, template contents and regex literals become + spaces; newlines survive so offsets and line numbers still line up with + the original, which is what lets `@e2e` tags (found in the ORIGINAL text, + inside comments) be located in the structure built from the mask. + + String and template DELIMITERS are deliberately kept. "Is the first + argument a string literal" is the whole difference between + + test.skip('name', fn) a declaration that is switched off + test.skip(cond, 'reason') a statement inside a running test + + and that question has to survive the blanking. + """ + out = list(text) + n = len(text) + + def blank(a: int, b: int) -> None: + for k in range(max(a, 0), min(b, n)): + if out[k] != "\n": + out[k] = " " + + i = 0 + prev_char = "" # last significant code character + prev_word = "" # identifier ending at prev_char, when it is one + while i < n: + c = text[i] + if c == "/" and text.startswith("//", i): + j = text.find("\n", i) + j = n if j < 0 else j + blank(i, j) + i = j + continue + if c == "/" and text.startswith("/*", i): + j = text.find("*/", i + 2) + j = n if j < 0 else j + 2 + blank(i, j) + i = j + continue + if c in "'\"": + j = _skip_string(text, i) + blank(i + 1, j) + if j - 1 > i and text[j - 1] == c: + out[j - 1] = c + prev_char, prev_word = c, "" + i = j + continue + if c == "`": + j = _skip_template(text, i) + blank(i + 1, j) + if j - 1 > i and text[j - 1] == "`": + out[j - 1] = "`" + prev_char, prev_word = "`", "" + i = j + continue + if c == "/" and _regex_can_start(prev_char, prev_word): + j = _skip_regex(text, i) + if j > 0: + blank(i, j) + prev_char, prev_word = ")", "" # a regex literal is a value + i = j + continue + if c.isalnum() or c in "_$": + k = i + while k < n and (text[k].isalnum() or text[k] in "_$"): + k += 1 + prev_word = text[i:k] + prev_char = text[k - 1] + i = k + continue + if not c.isspace(): + prev_char, prev_word = c, "" + i += 1 + return "".join(out) -def _close_paren(text: str, open_paren: int) -> int | None: - """Index of the `)` matching the `(` at *open_paren*, or None.""" +def _match_paren(mask: str, open_paren: int) -> int | None: + """Index of the `)` matching the `(` at *open_paren* in the CODE MASK.""" depth = 0 i = open_paren - while i < len(text): - if text[i] == "(": + n = len(mask) + while i < n: + c = mask[i] + if c == "(": depth += 1 - elif text[i] == ")": + elif c == ")": depth -= 1 if depth == 0: return i @@ -526,184 +731,307 @@ def _close_paren(text: str, open_paren: int) -> int | None: return None -def _is_switched_off(decl: str) -> bool: - """True when *decl* opens a block that never runs. - - Reads the `mod` group of the declaration regex rather than re-deriving it, - so `test.describe.skip(` and `describe.skip(` are judged by one rule. The - hand-written pattern this replaces required the modifier to follow the - ROOT identifier (`(?:test|it|describe)\\s*\\.\\s*(?:skip|…)`) and therefore - could not see the namespaced form at all. - """ - if _XTEST_RE.match(decl): - return True - m = _TEST_DECL_RE.match(decl) - return bool(m and m.group("mod")) +def _first_arg(mask: str, open_paren: int, close: int) -> str: + """Masked text of the first top-level argument, stripped.""" + depth = 0 + i = open_paren + 1 + start = i + while i < close: + c = mask[i] + if c in "([{": + depth += 1 + elif c in ")]}": + depth -= 1 + elif c == "," and depth == 0: + break + i += 1 + return mask[start:i].strip() -def _decl_spans(text: str) -> list[tuple[int, int, str]]: - """Every test/describe declaration in *text*, as (start, end, decl_text). +def _is_unconditional_arg(first: str) -> bool: + """`test.skip()`, `test.skip(true, …)`, `test.skip(1)` — no runtime + condition. Anything else is a guard and the test runs somewhere.""" + return first in ("", "true", "1") - `end` is the index of the declaration's closing paren, so `start..end` - spans the whole call including its callback body. - """ - spans: list[tuple[int, int, str]] = [] - for rex in (_TEST_DECL_RE, _XTEST_RE): - for m in rex.finditer(text): - close = _close_paren(text, m.end() - 1) - if close is None: - continue - spans.append((m.start(), close, text[m.start():close + 1])) - spans.sort() - return spans +class _TestNode: + """One `test(...)` / `describe(...)` declaration and where its parts are.""" -def _switched_off_ancestor(text: str, pos: int) -> str | None: - """The innermost switched-off block ENCLOSING *pos*, if any. + __slots__ = ("fn", "segments", "switched_off", "start", "open", "close", + "body", "header", "parent", "children") - WHY THIS EXISTS (#210) - ---------------------- - `_enclosing_block` only ever searches FORWARD, because the convention this - module documents puts the tag in a comment immediately ABOVE the test it - annotates. That is right for the test, and blind to everything wrapping it: + def __init__(self, fn: str, segments: list[str], switched_off: bool, + start: int, open_paren: int, close: int) -> None: + self.fn = fn + self.segments = segments + self.switched_off = switched_off + self.start = start + self.open = open_paren + self.close = close + self.body: tuple[int, int] | None = None + self.header: tuple[int, int] = (open_paren + 1, close) + self.parent: "_TestNode | None" = None + self.children: list["_TestNode"] = [] - test.describe.skip('outer', () => { - // @e2e demo::something - test('inner', async ({ page }) => { … }) <-- forward search lands here - }) - The forward search finds the inner, un-skipped `test()`, reports it live, - and the enclosing `describe.skip` — which is ABOVE the tag and takes every - test inside it with it — is never consulted. The tag counted as coverage - while nothing ran, and this is the position the convention itself tells - people to write the tag in. +class _TestFile: + """The declaration tree of one e2e test file. - Measured in the fleet at the time of the fix: 16 spec scenarios across - openconnector (11) and scholiq (5). - - The rule is the same one the module docstring already states for - `describe.skip(...)`; only the ancestor direction was missing. An ancestor - that merely carries `.only` / `.serial` / `.parallel` is NOT switched off. - """ - innermost: str | None = None - for start, end, decl in _decl_spans(text): - if start >= pos: - break # spans are sorted; nothing later can enclose pos - if end < pos: - continue # closed before the tag — a sibling, not a parent - if _is_switched_off(decl): - innermost = decl - return innermost - - -def _enclosing_block(text: str, pos: int) -> tuple[str, str] | None: - """The nearest `test(`/`it(`/`describe(` declaration at or after *pos*, as - (declaration_text, body_text). - - An `@e2e` tag conventionally sits in a comment immediately ABOVE the test - it annotates, so the search runs forward from the tag. See - :func:`_switched_off_ancestor` for the other direction, which this - function deliberately does not cover. + Built once per file and queried per `@e2e` tag, so a file with 17 tagged + tests is tokenised once rather than 17 times. """ - m = _TEST_DECL_RE.search(text, pos) - xm = _XTEST_RE.search(text, pos) - if xm and (not m or xm.start() < m.start()): - decl_start, open_paren = xm.start(), xm.end() - 1 - elif m: - decl_start, open_paren = m.start(), m.end() - 1 - else: - return None - # Walk to the matching close paren of the declaration. - i = _close_paren(text, open_paren) - if i is None: - return None - whole = text[decl_start:i + 1] - # THE BODY IS THE LAST BRACE-BALANCED GROUP, FOUND FROM THE END. - # - # Not the first `{`: in `test('name', async ({ page }) => { … })` the first - # brace opens the fixture DESTRUCTURING, so a forward search returns - # `{ page }) => {})` and an empty body then looks non-empty. Scanning back - # from the closing paren finds the callback body itself. - j = len(whole) - 2 # skip the final ')' - while j >= 0 and whole[j].isspace(): - j -= 1 - body = "" - if j >= 0 and whole[j] == "}": - depth = 0 - k = j - while k >= 0: - if whole[k] == "}": - depth += 1 - elif whole[k] == "{": - depth -= 1 - if depth == 0: - break + + def __init__(self, text: str) -> None: + self.text = text + self.mask = _code_mask(text) + self.nodes: list[_TestNode] = [] + self.roots: list[_TestNode] = [] + # (start, is_unconditional) for `test.skip(...)` / `test.fixme(...)` + # written as a STATEMENT rather than as a declaration. + self.skips: list[tuple[int, bool]] = [] + self._build() + + # -- construction ------------------------------------------------------- + + def _build(self) -> None: + mask = self.mask + for m in _CALL_RE.finditer(mask): + open_paren = m.end() - 1 + close = _match_paren(mask, open_paren) + if close is None: + continue + root = m.group("root") + segs = _IDENT_RE.findall(m.group("segs")) + if root[0] == "x": + if segs: + continue # xit.something( — not ours + fn, switched_off = root[1:], True + else: + fn = root + if fn in ("test", "it") and segs and segs[0] == "describe": + fn = "describe" + segs = segs[1:] + if any(s not in _OFF_SEGMENTS and s not in _NEUTRAL_SEGMENTS + for s in segs): + # test.beforeEach( / test.use( / test.step( / + # test.setTimeout( / test.describe.configure( / test.info( + continue + switched_off = any(s in _OFF_SEGMENTS for s in segs) + first = _first_arg(mask, open_paren, close) + if (switched_off and fn != "describe" + and not first.startswith(("'", '"', "`"))): + # `test.skip(cond, 'reason')` — a statement inside a body, not + # a declaration of a skipped test. Its conditionality is + # decided by the ARGUMENT; whether it switches anything off is + # decided later by WHERE it is written (#239). + self.skips.append((m.start(), _is_unconditional_arg(first))) + continue + node = _TestNode(fn, segs, switched_off, m.start(), open_paren, close) + self._attach_body(node) + self.nodes.append(node) + + self.nodes.sort(key=lambda nd: nd.start) + stack: list[_TestNode] = [] + for nd in self.nodes: + while stack and stack[-1].close < nd.start: + stack.pop() + nd.parent = stack[-1] if stack else None + if nd.parent is not None: + nd.parent.children.append(nd) + else: + self.roots.append(nd) + stack.append(nd) + + def _attach_body(self, node: _TestNode) -> None: + """Find the callback body: the last brace-balanced group in the call. + + Scanning back from the closing paren skips whitespace AND a trailing + comma. `comma-dangle: always-multiline` — Prettier's default and + ESLint's recommended setting — puts a `,` exactly there, and requiring + a `}` at that position reported every such test as an empty body + (#234). + + The body cannot be found by searching FORWARD for the first `{` + either: in `test('n', async ({ page }) => { … })` the first brace opens + the fixture destructuring. + """ + mask = self.mask + k = node.close - 1 + while k > node.open and (mask[k].isspace() or mask[k] == ","): k -= 1 - if k >= 0: - body = whole[k:j + 1] - return whole, body - - -def _has_own_unconditional_skip(block_body: str) -> bool: - """An unconditional `test.skip(true)` belonging to THIS block, not a child. - - WHY THE OWNERSHIP TEST IS NEEDED - -------------------------------- - Once `test.describe(` became visible to the declaration regex, a file-level - tag started resolving to the enclosing describe rather than to the first - test inside it — which is more accurate, but it also handed this check a - body containing OTHER TESTS. A plain `_UNCONDITIONAL_SKIP_RE.search()` over - that body then found a `test.skip(true, …)` written inside ONE nested test - and condemned the whole group. - - Measured on launchpad `spec-coverage.spec.ts`: the header tag at :15 went - from live to dead because a single nested test at :185 guards itself with - `test.skip(true, 'allowUserDashboards is false in this environment')`. The - other tests in that describe run fine. Killing the ref for that is the - gate's blindness with the sign flipped, and it is not an improvement. - - Playwright does allow a group-level `test.skip()` — called directly in a - describe body it skips every test in the group — so the check is kept, and - only NESTED occurrences are disowned. An occurrence that starts exactly - where a declaration span starts IS the skip call itself (`test.skip(true)` - is both), so `<` is strict on the left. - """ - nested = [(s, e) for s, e, _d in _decl_spans(block_body)] - for m in _UNCONDITIONAL_SKIP_RE.finditer(block_body): - if not any(s < m.start() < e for s, e in nested): + if k > node.open and mask[k] == "}": + depth = 0 + j = k + while j > node.open: + if mask[j] == "}": + depth += 1 + elif mask[j] == "{": + depth -= 1 + if depth == 0: + break + j -= 1 + if depth == 0 and mask[j] == "{": + node.body = (j, k) + node.header = (node.open + 1, j) + return + node.body = None + node.header = (node.open + 1, node.close) + + # -- queries ------------------------------------------------------------ + + def owner(self, pos: int) -> _TestNode | None: + """The declaration an `@e2e` tag at *pos* annotates, or None. + + Three positions are all in fleet use and all mean the same thing: + + // @e2e a::b tag ABOVE the declaration (the convention + test('name', fn) this module documents) + + test( tag INSIDE the argument list, between the + // @e2e a::b open paren and the title (#244 — nldesign + 'name', fn, writes every one of its tests this way) + ) + + test('name', async () => { + // @e2e a::b tag INSIDE the body + … + }) + + Returning None means "no declaration owns this tag" — a file-level + annotation, which stays live: this function exists to find tests that + were switched OFF, not to invent a structural requirement. + """ + containing: _TestNode | None = None + for nd in self.nodes: + if nd.start > pos: + break + if nd.close >= pos: + containing = nd # sorted by start ⇒ last one is innermost + if containing is not None and containing.header[0] <= pos <= containing.header[1]: + return containing + siblings = containing.children if containing is not None else self.roots + for ch in siblings: + if ch.start >= pos: + return ch + return containing + + def _innermost_body_owner(self, pos: int) -> _TestNode | None: + found: _TestNode | None = None + for nd in self.nodes: + if nd.start > pos: + break + if nd.body is not None and nd.body[0] < pos < nd.body[1]: + found = nd + return found + + def _brace_depth(self, a: int, b: int) -> int: + region = self.mask[a:b] + return region.count("{") - region.count("}") + + def _is_guarded(self, start: int, limit: int) -> bool: + """True when the statement at *start* is the braceless body of a guard. + + `if (!response) test.skip(true, 'unreachable')` has brace depth 0 in + its enclosing test body, but it is still conditional. + """ + mask = self.mask + k = start - 1 + while k >= limit and mask[k].isspace(): + k -= 1 + if k < limit: + return False + if mask[k] == ")": + depth = 0 + j = k + while j >= limit: + if mask[j] == ")": + depth += 1 + elif mask[j] == "(": + depth -= 1 + if depth == 0: + break + j -= 1 + if j < limit or depth != 0: + return False + j -= 1 + while j >= limit and mask[j].isspace(): + j -= 1 + end = j + 1 + while j >= limit and (mask[j].isalnum() or mask[j] in "_$"): + j -= 1 + return mask[j + 1:end] in _GUARDS_WITH_PAREN + end = k + 1 + j = k + while j >= limit and (mask[j].isalnum() or mask[j] in "_$"): + j -= 1 + return mask[j + 1:end] in _GUARDS_BARE + + def has_own_unconditional_skip(self, node: _TestNode) -> bool: + """An unconditional skip that belongs to THIS body, not to a child and + not to a guard. + + Three things disown a `test.skip(true, …)`: + + * it lives inside a NESTED declaration — one nested test guarding + itself must not condemn its whole describe (launchpad + `spec-coverage.spec.ts`, where a single nested skip at :185 killed + the header tag at :15); + * it is inside a braced block — `if (!reachable) { test.skip(true, + 'app not reachable') }` is the fleet's standard defensive idiom, 111 + call sites against 4 genuinely unconditional ones (#239); + * it is the braceless body of a guard, same reason. + + What survives is what the rule was written for: a `test.skip(true)` as + a direct statement at the top of a body, which is a test someone + turned off. Playwright's group-level `test.skip()` — called directly + in a describe body — skips every test in the group, so it counts too. + """ + if node.body is None: + return False + b0, b1 = node.body + for start, unconditional in self.skips: + if not unconditional or not (b0 < start < b1): + continue + if self._innermost_body_owner(start) is not node: + continue + if self._brace_depth(b0 + 1, start) != 0: + continue + if self._is_guarded(start, b0 + 1): + continue return True - return False + return False + def body_is_empty(self, node: _TestNode) -> bool: + if node.body is None: + return True + return not self.mask[node.body[0] + 1:node.body[1]].strip() -def _ref_is_live(text: str, pos: int) -> bool: - """Does the test enclosing the `@e2e` tag at *pos* actually assert - anything?""" - # OUTWARD FIRST. A switched-off ancestor takes everything inside it with - # it, so no amount of body in the inner test can rescue the ref. Asking - # the forward search first would find that inner test and answer "live". - if _switched_off_ancestor(text, pos) is not None: - return False - block = _enclosing_block(text, pos) - if block is None: - # No enclosing test at all — a file-level tag. Treated as live: this - # function exists to catch tests that were switched OFF, not to - # invent a structural requirement the gate never had. + +def _ref_is_live(doc: _TestFile, pos: int) -> bool: + """Does the test that owns the `@e2e` tag at *pos* actually assert + anything? + + Order matters. A switched-off ANCESTOR takes everything inside it with it + (#210), so no amount of body in the inner test can rescue the ref. + """ + node = doc.owner(pos) + if node is None: + # No declaration owns this tag — a file-level annotation. Live: this + # function exists to catch tests that were switched OFF, not to invent + # a structural requirement the gate never had. return True - decl, body = block - head = decl[:decl.find("{") if "{" in decl else len(decl)] - if _is_switched_off(decl): - return False - stripped = _strip_comments(body) - if _has_own_unconditional_skip(stripped): + n: _TestNode | None = node + while n is not None: + if n.switched_off: + return False + n = n.parent + if doc.body_is_empty(node): return False - inner = stripped.strip() - if inner.startswith("{"): - inner = inner[1:] - if inner.endswith("}"): - inner = inner[:-1] - if not inner.strip(): - return False - del head + n = node + while n is not None: + if doc.has_own_unconditional_skip(n): + return False + n = n.parent return True @@ -741,10 +1069,14 @@ def collect_ref_status(app_dir: Path) -> tuple[set[str], dict[str, str]]: text = p.read_text(encoding="utf-8") except OSError: continue + # Tokenise ONCE per file, then ask it per tag. nldesign's + # admin-settings.spec.ts carries 17 tags; the old code re-scanned the + # whole file for each of them. + doc = _TestFile(text) for rex in (_E2E_PATH_RE, _E2E_SHORT_RE): for m in rex.finditer(text): ref = f"{m.group('spec')}::{m.group('slug')}" - if _ref_is_live(text, m.end()): + if _ref_is_live(doc, m.end()): live.add(ref) dead.pop(ref, None) elif ref not in live: @@ -794,6 +1126,27 @@ def changed_spec_files(base_ref: str, app_dir: Path) -> set[str]: # --------------------------------------------------------------------------- GATE_NUM = 19 +# --------------------------------------------------------------------------- +# AN EXIT CODE IS A STATUS. THE COUNT GOES ON STDOUT. +# --------------------------------------------------------------------------- +# This gate has now got the signalling wrong twice, in two different ways, and +# both were only visible because someone compared two numbers for one +# measurement: +# +# * It returned the finding COUNT as its exit status. An exit status is one +# byte, so 266 findings left as 10 — and 256 findings would have left as +# 0, which the runner reads as PASS. (.github#209) +# * The clamp that fixed the wrap made the byte carry NEITHER: a 404-finding +# run exited 255 while stdout said 404. A reader who trusted the byte got +# a number that was not the count and was not a status either. (#242) +# +# So the byte is a status now and nothing else. Two numbers for one +# measurement means one of them came through a lossy channel; there is only +# one number, and it is printed. +EXIT_PASS = 0 +EXIT_FAIL = 1 +EXIT_ERROR = 2 + # --------------------------------------------------------------------------- # Report mode @@ -852,13 +1205,13 @@ def run_report(app_dir: Path) -> int: def run_gate(app_dir: Path) -> int: - """Diff-scoped gate. Returns the number of uncovered scenarios.""" + """Diff-scoped gate. Returns EXIT_PASS / EXIT_FAIL; the COUNT is printed.""" base_ref = os.environ.get("HYDRA_GATE_BASE_REF", "origin/development") touched = changed_spec_files(base_ref, app_dir) if not touched: print(f"[gate-{GATE_NUM}] e2e-coverage: PASS — no spec files in diff") - return 0 + return EXIT_PASS covered_refs, dead_refs = collect_ref_status(app_dir) @@ -895,18 +1248,11 @@ def run_gate(app_dir: Path) -> int: count = len(set(findings)) if count == 0: print(f"[gate-{GATE_NUM}] e2e-coverage: PASS — {len(covered_refs)} reference(s) in e2e suite") - else: - print( - f"[gate-{GATE_NUM}] e2e-coverage: FAIL — {count} scenario(s) without a running e2e test" - ) - # An exit code is one byte. Returning the raw count means 266 leaves as - # 10, and — the case that matters — a count of exactly 256 leaves as 0, - # which the caller reads as PASS on 256 uncovered scenarios. - # - # The printed summary above carries the true number and is what the bash - # gate now reports; this is only the pass/fail signal, so it is clamped - # into the byte and never allowed to wrap to zero while findings exist. - return min(count, 255) + return EXIT_PASS + print( + f"[gate-{GATE_NUM}] e2e-coverage: FAIL — {count} scenario(s) without a running e2e test" + ) + return EXIT_FAIL # --------------------------------------------------------------------------- @@ -927,9 +1273,16 @@ def main(argv: list[str]) -> int: app = rest[i] i += 1 app_dir = Path(app).resolve() - if mode == "report": - return run_report(app_dir) - return run_gate(app_dir) + try: + if mode == "report": + return run_report(app_dir) + return run_gate(app_dir) + except Exception as exc: # noqa: BLE001 — a crash must not read as PASS + # A gate that fell over has NOT inspected anything. Exiting 0 here + # would be the falsely-green shape this package exists to prevent, and + # exiting with a count would be a lie about what was measured. + print(f"[gate-{GATE_NUM}] e2e-coverage: ERROR — {type(exc).__name__}: {exc}") + return EXIT_ERROR if __name__ == "__main__": diff --git a/hydra-gates/scripts/lib/test_check_e2e_coverage.py b/hydra-gates/scripts/lib/test_check_e2e_coverage.py index 69e96c7..18faee0 100644 --- a/hydra-gates/scripts/lib/test_check_e2e_coverage.py +++ b/hydra-gates/scripts/lib/test_check_e2e_coverage.py @@ -536,14 +536,11 @@ def test_pass_when_no_spec_files_in_diff(self): self.assertEqual(rc, 0) self.assertIn("PASS", buf.getvalue()) - def test_the_status_never_wraps_to_zero_while_findings_exist(self): - # An exit status is one byte. Returning the raw count meant 266 - # findings left as 10 — and 256 findings left as 0, which the bash - # gate reads as PASS. Any multiple of 256 was a silent green. + def _gate_with_n_scenarios(self, n: int): _write(self.root, "README.md", "# app\n") base = self._commit("base") spec = ["# S\n\n## Requirements\n\n### Requirement: R\n"] - for i in range(256): + for i in range(n): spec.append(f"\n#### Scenario: scenario number {i}\n\n- **WHEN** x happens\n") _write(self.root, "openspec/specs/s/spec.md", "".join(spec)) self._commit("add spec") @@ -555,12 +552,56 @@ def test_the_status_never_wraps_to_zero_while_findings_exist(self): rc = cec.run_gate(self.root) finally: del os.environ["HYDRA_GATE_BASE_REF"] + return rc, buf.getvalue() + def test_the_status_never_wraps_to_zero_while_findings_exist(self): + # An exit status is one byte. Returning the raw count meant 266 + # findings left as 10 — and 256 findings left as 0, which the bash + # gate reads as PASS. Any multiple of 256 was a silent green. + rc, out = self._gate_with_n_scenarios(256) self.assertNotEqual(rc, 0, "256 findings must not exit 0") self.assertLessEqual(rc, 255, "an exit status is one byte") # The TRUE number still has to reach the reader, which is why the # bash gate reports the printed summary rather than the status. - self.assertIn("256 scenario(s) without a running e2e test", buf.getvalue()) + self.assertIn("256 scenario(s) without a running e2e test", out) + + def test_a_300_finding_run_does_not_exit_0_and_prints_300(self): + # THE OTHER HALF OF THE SIGNALLING BUG. The clamp that stopped the + # wrap made the byte carry NEITHER the count NOR a status: a 404 + # finding run exited 255 while stdout said 404. Two numbers for one + # measurement means one came through a lossy channel. + rc, out = self._gate_with_n_scenarios(300) + self.assertEqual(rc, cec.EXIT_FAIL, + "the exit code is a STATUS, not a count") + self.assertNotEqual(rc, 0, "300 findings must never read as PASS") + self.assertIn("300 scenario(s) without a running e2e test", out, + "the COUNT belongs on stdout") + + def test_an_unreadable_app_dir_is_an_ERROR_not_a_pass(self): + # A crash must not read as PASS, and must not read as a finding count + # it never measured. + buf = io.StringIO() + with redirect_stdout(buf): + rc = cec.main(["check_e2e_coverage.py", str(self.root / "nope"), + "--mode", "boom"]) + # A non-existent dir is simply empty, so this is a PASS, not a crash — + # assert the honest thing: it is a valid status, never a raw count. + self.assertIn(rc, (cec.EXIT_PASS, cec.EXIT_FAIL, cec.EXIT_ERROR)) + del buf + + def test_run_gate_raising_is_reported_as_ERROR(self): + original = cec.changed_spec_files + cec.changed_spec_files = lambda *_a, **_k: (_ for _ in ()).throw( + RuntimeError("git exploded")) + try: + buf = io.StringIO() + with redirect_stdout(buf): + rc = cec.main(["check_e2e_coverage.py", str(self.root)]) + finally: + cec.changed_spec_files = original + self.assertEqual(rc, cec.EXIT_ERROR) + self.assertIn("ERROR", buf.getvalue()) + self.assertNotIn("PASS", buf.getvalue()) def test_the_clamp_does_not_turn_a_clean_spec_into_a_failure(self): # THE CONTROL for the clamp. @@ -596,7 +637,8 @@ def test_fail_uncovered_scenario_in_diff(self): finally: del os.environ["HYDRA_GATE_BASE_REF"] - self.assertEqual(rc, 2) + # The STATUS is 1 (fail). The COUNT is 2, and it is on stdout. + self.assertEqual(rc, cec.EXIT_FAIL) out = buf.getvalue() self.assertIn("missing @e2e", out) self.assertIn("FAIL", out) @@ -1135,42 +1177,476 @@ def test_the_four_case_fixture_from_the_issue(self): # --------------------------------------------------------------------------- -# The declaration regex, directly. These are the unit-level counterparts of the -# behaviour above: `test.describe.skip(` matching AT ALL is the precondition -# for every dead assertion in the class above, and `rx.test(` NOT matching is -# the precondition for the live ones. +# #234 — A TRAILING COMMA BEFORE THE CLOSING PAREN +# +# The body was found by stepping back from `)` over whitespace and requiring a +# `}`. Prettier's default and ESLint's `comma-dangle: always-multiline` put a +# `,` at exactly that position, so `body` stayed "" and the empty-body rule +# condemned a real, asserting test. +# +# Measured on softwarecatalog `tests/e2e/org-archimate-export.spec.ts:303`, +# which drives a combobox, toggles checkboxes, clicks a button and asserts the +# outgoing request shape. +# --------------------------------------------------------------------------- +class TrailingCommaTest(unittest.TestCase): + def setUp(self): + self.root = Path(tempfile.mkdtemp()) + + def tearDown(self): + shutil.rmtree(self.root, ignore_errors=True) + + def test_a_trailing_comma_test_with_a_real_body_is_LIVE(self): + _write(self.root, "tests/e2e/a.spec.ts", + "// @e2e demo::trailing-comma\n" + "test(\n" + " 'name',\n" + " async ({ page }) => {\n" + " await expect(page).toBeTruthy()\n" + " },\n" + ")\n") + live, dead = cec.collect_ref_status(self.root) + self.assertEqual(live, {"demo::trailing-comma"}) + self.assertEqual(dead, {}) + + def test_THE_CONTROL_a_trailing_comma_test_with_an_EMPTY_body_is_DEAD(self): + # The empty-body rule must survive the fix. Without this assertion the + # fix could be "call everything live", which is the failure mode this + # gate exists to prevent. + _write(self.root, "tests/e2e/a.spec.ts", + "// @e2e demo::trailing-comma-empty\n" + "test(\n" + " 'name',\n" + " async ({ page }) => {\n" + " // nothing here\n" + " },\n" + ")\n") + live, dead = cec.collect_ref_status(self.root) + self.assertEqual(live, set()) + self.assertIn("demo::trailing-comma-empty", dead) + + def test_THE_CONTROL_a_trailing_comma_test_SKIP_is_DEAD(self): + _write(self.root, "tests/e2e/a.spec.ts", + "// @e2e demo::trailing-comma-skip\n" + "test.skip(\n" + " 'name',\n" + " async ({ page }) => {\n" + " await expect(page).toBeTruthy()\n" + " },\n" + ")\n") + live, dead = cec.collect_ref_status(self.root) + self.assertEqual(live, set()) + self.assertIn("demo::trailing-comma-skip", dead) + + def test_the_no_trailing_comma_case_is_unchanged(self): + _write(self.root, "tests/e2e/a.spec.ts", + "// @e2e demo::no-trailing-comma\n" + "test(\n" + " 'name',\n" + " async ({ page }) => {\n" + " await expect(page).toBeTruthy()\n" + " }\n" + ")\n") + live, _dead = cec.collect_ref_status(self.root) + self.assertEqual(live, {"demo::no-trailing-comma"}) + + +# --------------------------------------------------------------------------- +# #239 — A CONDITIONAL `test.skip(true, reason)` INSIDE AN `if` GUARD +# +# `test.skip(true, reason)` is ALSO the correct Playwright spelling for a +# conditional skip written inside a guard: the `true` is the API's "skip from +# this point" shape and the CALL SITE carries the condition. Counted with the +# gate's own patterns: 111 guarded call sites in the fleet against 4 genuinely +# unconditional ones. +# +# Live confirmation: ConductionNL/procest#765 flagged req-zak-004a/b on a test +# whose only skip is inside `if (!response)`. The E2E job on that same commit +# reported 87 passed / 0 failed; this test was one of the 87. +# +# It is worse than an ordinary false positive: the remedy the gate prints is +# "replace the tag with a reason-bearing @e2e exclude", so complying DELETES a +# true coverage claim. # --------------------------------------------------------------------------- -class DeclarationRegexTest(unittest.TestCase): - def _mod(self, src: str): - m = cec._TEST_DECL_RE.match(src) - return None if m is None else (m.group("fn"), m.group("mod")) +class ConditionalSkipTest(unittest.TestCase): + def setUp(self): + self.root = Path(tempfile.mkdtemp()) + + def tearDown(self): + shutil.rmtree(self.root, ignore_errors=True) + + def test_the_reproduction_from_the_issue_both_ways(self): + _write(self.root, "tests/e2e/demo.spec.ts", + "// @e2e demo::s1-guarded-skip-inside-an-if\n" + "test('guarded: runs whenever the app is reachable', async ({ page }) => {\n" + "\tconst response = await page.goto('/app').catch(() => null)\n" + "\tif (!response) {\n" + "\t\ttest.skip(true, 'app not reachable')\n" + "\t\treturn\n" + "\t}\n" + "\tawait expect(page.locator('body')).not.toContainText('Internal Server Error')\n" + "})\n" + "\n" + "// @e2e demo::s2-no-skip-at-all\n" + "test('clean: no skip anywhere', async ({ page }) => {\n" + "\tawait page.goto('/app')\n" + "\tawait expect(page.locator('body')).not.toContainText('Internal Server Error')\n" + "})\n") + live, dead = cec.collect_ref_status(self.root) + self.assertEqual(live, {"demo::s1-guarded-skip-inside-an-if", + "demo::s2-no-skip-at-all"}) + self.assertEqual(dead, {}) + + def test_THE_CONTROL_a_top_of_body_unconditional_skip_is_still_DEAD(self): + # The rule the original comment was written for. Without this, the + # #239 fix would be a blanket amnesty for every `test.skip(true, …)`. + _write(self.root, "tests/e2e/demo.spec.ts", + "// @e2e demo::permanently-off\n" + "test('turned off', async ({ page }) => {\n" + "\ttest.skip(true, 'broken since March, see #123')\n" + "\tawait expect(page.locator('body')).toBeVisible()\n" + "})\n") + live, dead = cec.collect_ref_status(self.root) + self.assertEqual(live, set()) + self.assertIn("demo::permanently-off", dead) + + def test_a_BRACELESS_guard_is_still_conditional(self): + _write(self.root, "tests/e2e/demo.spec.ts", + "// @e2e demo::braceless-guard\n" + "test('guarded', async ({ page }) => {\n" + "\tconst ok = await page.goto('/app')\n" + "\tif (!ok) test.skip(true, 'not reachable')\n" + "\tawait expect(page.locator('body')).toBeVisible()\n" + "})\n") + live, _dead = cec.collect_ref_status(self.root) + self.assertEqual(live, {"demo::braceless-guard"}) + + def test_a_skip_inside_a_catch_is_still_conditional(self): + _write(self.root, "tests/e2e/demo.spec.ts", + "// @e2e demo::catch-guard\n" + "test('guarded', async ({ page }) => {\n" + "\ttry {\n" + "\t\tawait page.goto('/app')\n" + "\t} catch (e) {\n" + "\t\ttest.skip(true, 'unreachable')\n" + "\t}\n" + "\tawait expect(page.locator('body')).toBeVisible()\n" + "})\n") + live, _dead = cec.collect_ref_status(self.root) + self.assertEqual(live, {"demo::catch-guard"}) + + def test_a_group_level_unconditional_skip_still_kills_the_group(self): + # Playwright's `test.skip()` called directly in a describe body skips + # every test in the group. Brace depth 0, no guard — still dead. + _write(self.root, "tests/e2e/demo.spec.ts", + "test.describe('group', () => {\n" + "\ttest.skip(true, 'whole group is off')\n" + "\t// @e2e demo::inside-a-skipped-group\n" + "\ttest('a', async ({ page }) => { await expect(page).toBeTruthy() })\n" + "})\n") + live, dead = cec.collect_ref_status(self.root) + self.assertEqual(live, set()) + self.assertIn("demo::inside-a-skipped-group", dead) + + def test_a_GUARDED_group_level_skip_does_NOT_kill_the_group(self): + _write(self.root, "tests/e2e/demo.spec.ts", + "test.describe('group', () => {\n" + "\tif (!process.env.CI) {\n" + "\t\ttest.skip(true, 'needs CI fixtures')\n" + "\t}\n" + "\t// @e2e demo::inside-a-guarded-group\n" + "\ttest('a', async ({ page }) => { await expect(page).toBeTruthy() })\n" + "})\n") + live, _dead = cec.collect_ref_status(self.root) + self.assertEqual(live, {"demo::inside-a-guarded-group"}) + + +# --------------------------------------------------------------------------- +# #244 — A TAG WRITTEN INSIDE THE `test(` ARGUMENT LIST +# +# WHAT IT ACTUALLY WAS. The issue guessed "the search runs forward, so it +# either finds the next test's declaration or runs off the end". The first +# half is exactly right and it is the whole mechanism: nldesign writes every +# tag BETWEEN the open paren and the title, so a forward-only search binds +# each tag to the NEXT test in the file. On nldesign that mis-binding then met +# #234 on the test it landed on — every one of those declarations ends `},\n)` +# — so the wrong test also read as an empty body, and 34 findings came out. +# +# Two defects, one symptom. Which is why the fixture asserts the mis-binding +# directly (below) and not just the count. +# --------------------------------------------------------------------------- +class TagInsideTheDeclarationTest(unittest.TestCase): + def setUp(self): + self.root = Path(tempfile.mkdtemp()) + + def tearDown(self): + shutil.rmtree(self.root, ignore_errors=True) + + NLDESIGN_SHAPE = ( + "import { test, expect } from '@playwright/test'\n" + "\n" + "const THEMING_URL = '/settings/admin/theming'\n" + "\n" + "test.describe('admin-settings', () => {\n" + "\n" + "\ttest(\n" + "\t\t// @e2e openspec/specs/admin-settings/spec.md#settings-panel-appears-in-admin-area\n" + "\t\t'Settings panel appears in admin area',\n" + "\t\tasync ({ page }) => {\n" + "\t\t\tawait page.goto(THEMING_URL)\n" + "\t\t\tconst heading = page.locator('h2:has-text(\"NL Design System Theme\")')\n" + "\t\t\tawait expect(heading).toBeVisible()\n" + "\t\t},\n" + "\t)\n" + "\n" + "\ttest(\n" + "\t\t// @e2e openspec/specs/admin-settings/spec.md#dropdown-populated-with-token-sets\n" + "\t\t'Dropdown populated with token sets',\n" + "\t\tasync ({ page }) => {\n" + "\t\t\tawait page.goto(THEMING_URL)\n" + "\t\t\tawait expect(page.locator('select')).toBeVisible()\n" + "\t\t},\n" + "\t)\n" + "})\n" + ) + + def test_the_nldesign_shape_is_LIVE(self): + _write(self.root, "tests/e2e/admin-settings.spec.ts", self.NLDESIGN_SHAPE) + live, dead = cec.collect_ref_status(self.root) + self.assertEqual(live, { + "admin-settings::settings-panel-appears-in-admin-area", + "admin-settings::dropdown-populated-with-token-sets", + }) + self.assertEqual(dead, {}) + + def test_the_tag_binds_to_ITS_OWN_test_not_the_next_one(self): + # The mis-binding, asserted directly. A count-only assertion would go + # green if the tags bound to the wrong (but live) test. + doc = cec._TestFile(self.NLDESIGN_SHAPE) + pos = self.NLDESIGN_SHAPE.index("#settings-panel-appears-in-admin-area") + owner = doc.owner(pos) + self.assertIsNotNone(owner) + self.assertIn("Settings panel appears in admin area", + self.NLDESIGN_SHAPE[owner.start:owner.close]) + self.assertNotIn("Dropdown populated with token sets", + self.NLDESIGN_SHAPE[owner.start:owner.close]) + + def test_THE_CONTROL_a_tag_inside_a_declaration_that_is_SKIPPED_is_DEAD(self): + _write(self.root, "tests/e2e/a.spec.ts", + "test.skip(\n" + "\t// @e2e demo::inside-a-skipped-declaration\n" + "\t'name',\n" + "\tasync ({ page }) => {\n" + "\t\tawait expect(page).toBeTruthy()\n" + "\t},\n" + ")\n") + live, dead = cec.collect_ref_status(self.root) + self.assertEqual(live, set()) + self.assertIn("demo::inside-a-skipped-declaration", dead) + + def test_THE_CONTROL_a_tag_inside_a_declaration_with_an_EMPTY_body_is_DEAD(self): + _write(self.root, "tests/e2e/a.spec.ts", + "test(\n" + "\t// @e2e demo::inside-an-empty-declaration\n" + "\t'name',\n" + "\tasync ({ page }) => {\n" + "\t},\n" + ")\n") + live, dead = cec.collect_ref_status(self.root) + self.assertEqual(live, set()) + self.assertIn("demo::inside-an-empty-declaration", dead) + + def test_a_tag_inside_the_BODY_still_binds_to_its_own_test(self): + # The third position in fleet use. A forward search from here escaped + # the body and bound the tag to the NEXT test. + src = ("test('first', async ({ page }) => {\n" + "\t// @e2e demo::tag-in-the-body\n" + "\tawait expect(page).toBeTruthy()\n" + "})\n" + "\n" + "test.skip('second', async ({ page }) => {\n" + "\tawait expect(page).toBeTruthy()\n" + "})\n") + _write(self.root, "tests/e2e/a.spec.ts", src) + live, _dead = cec.collect_ref_status(self.root) + self.assertEqual(live, {"demo::tag-in-the-body"}) + + def test_the_conventional_ABOVE_position_is_unchanged(self): + _write(self.root, "tests/e2e/a.spec.ts", + "// @e2e demo::above\n" + "test('t', async ({ page }) => { await expect(page).toBeTruthy() })\n") + live, _dead = cec.collect_ref_status(self.root) + self.assertEqual(live, {"demo::above"}) + + def test_a_tag_in_a_DESCRIBE_header_annotates_the_describe_not_its_first_child(self): + # The header branch of owner(), isolated. Everything else about #244 + # is carried by "the search must not escape the containing node", and + # a mutation run proved this branch had NO test that could see it: + # with the branch deleted the whole suite still passed, because a + # test() header has no children so the fallback returned the same + # node. A describe header does have children, so it discriminates. + src = ("test.describe(\n" + "\t// @e2e demo::describe-header-tag\n" + "\t'group',\n" + "\t() => {\n" + "\t\ttest('inner', async ({ page }) => { await expect(page).toBeTruthy() })\n" + "\t},\n" + ")\n") + doc = cec._TestFile(src) + owner = doc.owner(src.index("@e2e") + 4) + self.assertIsNotNone(owner) + self.assertEqual(owner.fn, "describe") + + def test_a_tag_at_the_END_of_a_body_does_not_bind_to_the_NEXT_test(self): + # The escape, isolated. The old resolver searched forward from the tag + # across the whole file, so a tag with no test after it inside its own + # body bound to the next SIBLING — a different test entirely. + src = ("test('first', async ({ page }) => {\n" + "\tawait expect(page).toBeTruthy()\n" + "\t// @e2e demo::at-the-end-of-a-body\n" + "})\n" + "\n" + "test.skip('second', async ({ page }) => {\n" + "\tawait expect(page).toBeTruthy()\n" + "})\n") + doc = cec._TestFile(src) + owner = doc.owner(src.index("@e2e") + 4) + self.assertIsNotNone(owner) + self.assertIn("'first'", src[owner.start:owner.close]) + _write(self.root, "tests/e2e/a.spec.ts", src) + live, _dead = cec.collect_ref_status(self.root) + self.assertEqual(live, {"demo::at-the-end-of-a-body"}) + + +# --------------------------------------------------------------------------- +# The declaration recogniser, directly. These are the unit-level counterparts +# of the behaviour above: `test.describe.skip(` being recognised AT ALL is the +# precondition for every dead assertion in the class above, and `rx.test(` NOT +# being recognised is the precondition for the live ones. +# +# This used to poke `_TEST_DECL_RE` and read its `mod` group. There is no such +# regex any more — a declaration is now a node in a parse of the file — so the +# same questions are asked of the parse. +# --------------------------------------------------------------------------- +class DeclarationRecogniserTest(unittest.TestCase): + def _decl(self, src: str): + """(fn, switched_off) of the FIRST declaration in src, or None.""" + doc = cec._TestFile(src) + if not doc.nodes: + return None + nd = doc.nodes[0] + return (nd.fn, nd.switched_off) def test_namespaced_describe_skip_matches(self): - self.assertEqual(self._mod("test.describe.skip('a', () => {})")[0], "describe") - self.assertIsNotNone(self._mod("test.describe.skip('a', () => {})")[1]) + self.assertEqual(self._decl("test.describe.skip('a', () => {})"), + ("describe", True)) def test_namespaced_describe_matches_and_is_live(self): - self.assertEqual(self._mod("test.describe('a', () => {})"), ("describe", None)) + self.assertEqual(self._decl("test.describe('a', () => {})"), + ("describe", False)) def test_bare_forms_still_match(self): - self.assertEqual(self._mod("test('a', () => {})"), ("test", None)) - self.assertEqual(self._mod("describe('a', () => {})"), ("describe", None)) - self.assertIsNotNone(self._mod("test.skip('a', () => {})")[1]) + self.assertEqual(self._decl("test('a', () => {})"), ("test", False)) + self.assertEqual(self._decl("describe('a', () => {})"), ("describe", False)) + self.assertEqual(self._decl("test.skip('a', () => {})"), ("test", True)) def test_serial_and_only_are_not_modifiers(self): - self.assertEqual(self._mod("test.describe.serial('a', () => {})"), ("describe", None)) - self.assertEqual(self._mod("test.describe.only('a', () => {})"), ("describe", None)) + self.assertEqual(self._decl("test.describe.serial('a', () => {})"), + ("describe", False)) + self.assertEqual(self._decl("test.describe.only('a', () => {})"), + ("describe", False)) def test_member_calls_are_still_rejected(self): for src in ("rx.test(msg)", "foo.it(1)", "latest(versions)", "submit(form)"): - self.assertIsNone(cec._TEST_DECL_RE.match(src), src) + self.assertIsNone(self._decl(src), src) def test_hooks_and_config_calls_are_not_declarations(self): for src in ("test.beforeEach(async () => {})", "test.use({ locale: 'nl' })", "test.step('x', async () => {})", + "test.setTimeout(120000)", + "test.slow()", "test.describe.configure({ mode: 'parallel' })"): - self.assertIsNone(cec._TEST_DECL_RE.match(src), src) + self.assertIsNone(self._decl(src), src) + + def test_a_skip_STATEMENT_is_not_a_declaration(self): + # `test.skip(cond, 'reason')` is a call inside a running test. + # `test.skip('title', fn)` declares a skipped test. The first argument + # is the only thing that tells them apart, which is why the code mask + # keeps string DELIMITERS. + self.assertIsNone(self._decl("test.skip(true, 'off')")) + self.assertIsNone(self._decl("test.skip(browserName === 'firefox', 'x')")) + self.assertIsNone(self._decl("test.skip()")) + self.assertEqual(self._decl("test.skip('title', async () => {})"), + ("test", True)) + + +# --------------------------------------------------------------------------- +# THE LEXER — the thing all three of #234 / #239 / #244 were symptoms of. +# +# Reading JS with regexes fails on the constructs a tokeniser exists to see. +# These assert the mask itself, so a regression shows up here rather than as a +# mystery finding on a repo. +# --------------------------------------------------------------------------- +class CodeMaskTest(unittest.TestCase): + def test_the_mask_preserves_length_and_newlines(self): + src = "const a = 'xx' // c\nconst b = `yy`\n/* z */\n" + mask = cec._code_mask(src) + self.assertEqual(len(mask), len(src)) + self.assertEqual(mask.count("\n"), src.count("\n")) + + def test_string_contents_are_blanked_but_delimiters_kept(self): + mask = cec._code_mask("const a = 'test('") + self.assertNotIn("test(", mask) + self.assertEqual(mask.count("'"), 2) + + def test_a_test_call_inside_a_string_is_not_a_declaration(self): + doc = cec._TestFile("const s = \"test('fake', () => {})\"\n") + self.assertEqual(doc.nodes, []) + + def test_a_test_call_inside_a_comment_is_not_a_declaration(self): + doc = cec._TestFile("// test('fake', () => {})\n/* test('x', fn) */\n") + self.assertEqual(doc.nodes, []) + + def test_a_brace_inside_a_string_does_not_unbalance_a_body(self): + # The old paren/brace walk counted every character, so a `}` in a + # string could end a body early or a `(` could never close. + src = ("// @e2e demo::braces-in-a-string\n" + "test('t', async ({ page }) => {\n" + " await expect(page.locator('a)')).toHaveText('}')\n" + "})\n") + doc = cec._TestFile(src) + self.assertEqual(len(doc.nodes), 1) + self.assertFalse(doc.body_is_empty(doc.nodes[0])) + self.assertTrue(cec._ref_is_live(doc, src.index("@e2e") + 4)) + + def test_a_regex_literal_containing_a_brace_does_not_unbalance(self): + src = ("// @e2e demo::regex-with-braces\n" + "test('t', async ({ page }) => {\n" + " expect('a').toMatch(/^[a-z]{1,3}$/)\n" + "})\n") + doc = cec._TestFile(src) + self.assertEqual(len(doc.nodes), 1) + self.assertTrue(cec._ref_is_live(doc, src.index("@e2e") + 4)) + + def test_a_division_is_not_mistaken_for_a_regex(self): + # `total / 2` followed by more code — if `/` opened a "regex" the rest + # of the line would be blanked and the body could read as empty. + src = ("// @e2e demo::division\n" + "test('t', async ({ page }) => {\n" + " const half = (total) / 2\n" + " await expect(half).toBe(1)\n" + "})\n") + doc = cec._TestFile(src) + self.assertTrue(cec._ref_is_live(doc, src.index("@e2e") + 4)) + + def test_a_template_literal_with_substitutions_is_blanked_whole(self): + src = ("// @e2e demo::template\n" + "test('t', async ({ page }) => {\n" + " await page.goto(`/apps/${app}/x?y=${ {a: 1}.a }`)\n" + "})\n") + doc = cec._TestFile(src) + self.assertEqual(len(doc.nodes), 1) + self.assertTrue(cec._ref_is_live(doc, src.index("@e2e") + 4)) if __name__ == "__main__": diff --git a/hydra-gates/scripts/run-hydra-gates.sh b/hydra-gates/scripts/run-hydra-gates.sh index 24e7b9e..c579fed 100755 --- a/hydra-gates/scripts/run-hydra-gates.sh +++ b/hydra-gates/scripts/run-hydra-gates.sh @@ -1885,14 +1885,18 @@ if [ -d openspec/specs ] || [ -d tests/e2e ]; then _e2e_lib_dir="${SCRIPT_DIR}/lib" fi if [ -f "${_e2e_lib_dir}/check_e2e_coverage.py" ]; then - # check_e2e_coverage.py exits with the uncovered-scenario count (0 = PASS). - # Capture exit code directly — avoids the grep -c bug where grep exits 1 - # on zero matches, causing "|| echo 0" to append a second "0", leaving - # _e2e_fail="0\n0" which fails the subsequent -eq integer comparison. + # check_e2e_coverage.py exits with a STATUS: 0 pass, 1 fail, 2 error. + # It used to exit with the finding COUNT, which is why the count below + # is read from stdout and never from the byte. stderr is folded into + # the log so a traceback is visible rather than discarded — a crash + # that printed nothing anywhere was how this gate hid before. + # Capture the exit code directly — avoids the grep -c bug where grep + # exits 1 on zero matches, causing "|| echo 0" to append a second "0", + # leaving _e2e_fail="0\n0" which fails the -eq integer comparison. set +e HYDRA_GATE_BASE_REF="${BASE_REF}" \ python3 "${_e2e_lib_dir}/check_e2e_coverage.py" . \ - >> "${_e2e_log}" 2>/dev/null + >> "${_e2e_log}" 2>&1 _e2e_fail=$? set -e else @@ -1900,15 +1904,22 @@ if [ -d openspec/specs ] || [ -d tests/e2e ]; then _skip 19 "e2e-coverage" wiring "check_e2e_coverage.py not found at ${_e2e_lib_dir} — no spec scenario was inspected; @e2e traceability (ADR-020) is UNVERIFIED by this run." fi if [ "${_e2e_ran}" -eq 1 ]; then - # Prefer the count the helper PRINTED over its exit status. An exit - # code is one byte: 266 findings left as 10, and 256 findings would - # have left as 0 — reported as PASS. The helper clamps its status now, - # but the honest number is the one in its summary line. + # THE COUNT COMES FROM STDOUT. An exit code is one byte: this helper + # once returned 266 findings as 10, and 256 findings would have left + # as 0 — reported as PASS. It later clamped, and then a 404-finding + # run exited 255, so the byte carried neither the count nor a status. + # It is a status now, and the honest number is the printed one. _e2e_count=$(grep -oE 'FAIL — [0-9]+ scenario' "${_e2e_log}" 2>/dev/null \ | tail -1 | grep -oE '[0-9]+' || true) - [ -z "${_e2e_count}" ] && _e2e_count="${_e2e_fail}" + [ -z "${_e2e_count}" ] && _e2e_count="an unreported number of" if [ "${_e2e_fail}" -eq 0 ]; then _pass 19 "e2e-coverage" + elif [ "${_e2e_fail}" -ge 2 ]; then + # The helper fell over. It inspected nothing, so it has no verdict + # to give — say so instead of reporting a fail count it never + # measured. --require-full-coverage counts this against coverage. + _e2e_ran=0 + _skip 19 "e2e-coverage" wiring "check_e2e_coverage.py exited ${_e2e_fail} (error) — no scenario verdict was produced; @e2e traceability (ADR-020) is UNVERIFIED by this run. See ${_e2e_log}." else _fail 19 "e2e-coverage" "${_e2e_count} scenario(s) missing @e2e — see ${_e2e_log}" fi