fix(ci): a failing seed exits 0 (#192); a tag inside describe.skip counts as live (#210) - #212
Merged
Conversation
…unts as live (#210) Two defects that destroy test evidence fleet-wide, both measured. #192 — THE SEED COULD FAIL AND THE STEP STILL WENT GREEN `playwright-seed-command` was interpolated into the step body unquoted: eval ${{ inputs.playwright-seed-command }} `${{ }}` is a TEXTUAL substitution applied before bash sees the script, so a seed of `a && b && c` became `eval a && b && c` — three commands in an `&&` list with eval receiving only the first stage. Bash's `set -e` exemption for `&&` lists ("except the command following the final &&") then meant a failing first stage aborted nothing, the tail never ran, and the step exited 0 while printing "Seed command completed." Playwright tested a half-seeded instance and produced a full, credible-looking tally. Measured on pipelinq run 30800304506: a bundle-truncation positive control that never truncated anything and reported 107 passed / 61 failed. The seed now arrives through the environment as DATA (`SEED_COMMAND:`), the expansion is quoted, the status is checked explicitly, and the diagnostic goes to STDERR — an `::error::` on stdout is swallowed whole by any caller that captures the step with `$(…)`. `eval` is retained deliberately. A survey of every fleet caller's actual value found launchpad passing `OC_PASS=… php occ user:add … || true` and openconnector passing `SEED_SCOPE=register bash …/ci-seed.sh`: assignment prefixes and `||` lists that a bare `"$SEED_COMMAND"` would exec as one argv word. The defect was the QUOTING, not the eval. THREE steps, not two. `scripts/assert-seed-step-fails-loudly.py` extracts every step named "Seed test data" — env block and run body — and EXECUTES it under the shell GitHub uses. Running the shipped text rather than grepping for a pattern found a third instance nobody had listed: the Newman job's own seed step, reading `newman-seed-command`, whose comment claimed "Use eval to support complex commands with pipes or &&". openbuild passes it `php occ app:disable openbuild && php occ app:enable openbuild` — the defect, live, in a repo shipping today. Proven both ways. `--positive-control` runs the identical battery against the pre-fix body reconstructed verbatim and requires it to FAIL; it does, on six assertions. Eight mutations of the fixed step (predicate always-true and always-false, stderr redirect dropped, explicit exit dropped, expansion unquoted, env passthrough dropped, rc capture pinned to 0, full revert) were each killed by the suite. #210 — A TAG INSIDE test.describe.skip COUNTED AS COVERAGE `_enclosing_block()` searches FORWARD from the tag, because the convention this module documents puts the tag immediately above the test it annotates. That is right for the test and blind to everything wrapping it: a tag inside a `test.describe.skip` resolves to the inner, un-skipped `test()`. Reproducing it surfaced a wider defect the issue had not seen. `_TEST_DECL_RE` could not match `test.describe.skip(` AT ALL — at `test` the modifier group finds `.describe` instead of `.skip` so the required `(` fails, and at `describe` the `(?<![.\w$])` lookbehind sees the preceding dot and refuses. The construct was invisible, not judged. So the issue's own "correctly dead" control — the tag ABOVE a skipped describe — was in fact reported LIVE too. Both are fixed: an explicit `test.` / `it.` namespace segment (named, not `\w+\.`, so `rx.test(` is still rejected) plus an outward ancestor walk. Making `test.describe(` visible handed the empty-body check a body full of OTHER TESTS, and a single nested `test.skip(true, …)` then condemned the whole group — launchpad's spec-coverage.spec.ts header tag went live→dead for one guarded test among many. `_has_own_unconditional_skip()` disowns nested occurrences while keeping Playwright's real group-level `test.skip()` dead. MEASURED AGAINST THE FLEET, old code vs new, on eight repos at origin/development: 16 refs flip live→dead — openconnector 11, scholiq 5, exactly the issue's count — and nothing regresses. One ref flips dead→live, softwarecatalog's `user-triggers-organization-export-with-toggles`: a pre-existing false positive on main where a trailing comma before the closing paren makes a real asserting test read as an empty body. That bug is untouched here and filed separately. 25 new tests, every dead assertion paired with a live control (`describe.only`, `describe.serial`, a live describe, a sibling after a closed skipped block, a runtime-conditional skip, `rx.test(`). Eleven mutations of the changed predicates — ancestor check always-true and always-false, `_is_switched_off` always-true and always-false, the pre-fix regex restored, the namespace widened to any member, `.only` and `.serial` folded into the modifier group, the xdescribe arm dropped, the encloses-test weakened to appears-earlier — were each killed. The new lint is wired into quality-resolve-probe.yml as its own job, behind the same single required guard, with its positive control first. Closes #192 Closes #210
This was referenced Aug 8, 2026
rubenvdlinde
added a commit
that referenced
this pull request
Aug 8, 2026
…, #239, #244) (#249) 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. Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two defects that silently destroy test evidence across the fleet. Both measured, both proven in the direction that matters and in the direction that would make the gate blind.
#192 — a failing seed exits 0
playwright-seed-commandwas interpolated into the step body unquoted.${{ }}is a textual substitution applied before bash sees the script, so a seed ofa && b && cbecameeval a && b && c— three commands in an&&list, withevalreceiving only the first stage. Bash'sset -eexemption for&&lists then meant a failing first stage aborted nothing, the tail never ran, and the step exited 0 while printingSeed command completed.Measured on pipelinq run
30800304506: a bundle-truncation positive control that never truncated anything and reported 107 passed / 61 failed.The seed now arrives as data (
SEED_COMMAND:underenv:), the expansion is quoted, the status is checked explicitly, and the diagnostic goes to stderr — an::error::on stdout is swallowed whole by any caller that captures the step with$(…).What the fleet actually passes — enumerated before choosing the shape
Every caller's real value, read from
origin/development, not from the local checkouts:bash apps/<app>/tests/e2e/ci-seed.shphp occ config:app:set …|| trueOC_PASS=E2eGranteePw123 php occ user:add --password-from-env e2e-grantee || trueAnd on the Newman input, which the issue did not cover:
newman-seed-commandphp occ app:disable openbuild && php occ app:enable openbuild← an&&chain, the defect live todaySEED_SCOPE=register bash apps/openconnector/tests/e2e/ci-seed.sh(assignment prefix)No repo currently passes an
&&chain toplaywright-seed-command. One passes an assignment prefix and a|| true; one passes an assignment prefix to the Newman input. Soevalis retained deliberately — a bare"$SEED_COMMAND"would exec launchpad's whole string as one argv word. The defect was the quoting, not the eval.Three steps, not two
scripts/assert-seed-step-fails-loudly.pyextracts every step namedSeed test data— env block and run body both — and executes it under the shell GitHub uses (bash -e <file>; quality.yml declares noshell:).Running the shipped text rather than grepping for a pattern found a third instance nobody had listed: the Newman job's own seed step, whose comment claimed "Use eval to support complex commands with pipes or &&". That was the assurance the defect hid behind, and openbuild ships an
&&chain to it.Proven both ways
--positive-controlruns the identical battery against the pre-fix body reconstructed verbatim and requires it to fail. It does, on six assertions. A battery that passed the known-bad body would be measuring nothing.Eight mutations of the fixed step, each killed by the suite:
exitdroppedTwo of these only started failing after the harness was strengthened, and both gaps were real: an always-true predicate still gated correctly while crying
::error::Seed command failed with exit 0on every green run, and the harness was injectingSEED_COMMANDitself, so deleting the env line changed nothing. It now reads the env block from the file and asserts that a successful seed is silent.One assertion of my own was wrong first time round and is worth recording: "did the tail run?" was grepping the log for a marker word that also appears in the
::error::line, which echoes the seed back. It reported "the tail ran" on a step where it demonstrably had not. It now answers with a filesystem side effect, and has its own control proving the sentinel can fire.#210 — a tag inside
test.describe.skipcounted as coverage_enclosing_block()searches forward from the tag, because the convention this module documents puts the tag immediately above the test it annotates. That is right for the test and blind to everything wrapping it.Reproducing it surfaced a wider defect the issue had not seen.
_TEST_DECL_REcould not matchtest.describe.skip(at all: attestthe modifier group finds.describeinstead of.skipso the required(fails, and atdescribethe(?<![.\w$])lookbehind sees the preceding dot and refuses. The construct was invisible, not judged. So the issue's own "correctly dead" control — the tag above a skipped describe — was in fact reported LIVE too:Both are fixed: an explicit
test./it.namespace segment — named rather than\w+\., sorx.test(is still rejected — plus an outward ancestor walk.A regression I introduced, caught by measuring
Making
test.describe(visible handed the empty-body check a body full of other tests, and a single nestedtest.skip(true, …)then condemned the whole group. launchpad'sspec-coverage.spec.tsheader tag went live→dead because one guarded test among many opts out on an env condition._has_own_unconditional_skip()disowns nested occurrences while keeping Playwright's real group-leveltest.skip()dead — with both tests.Measured against the fleet
Old code vs new, eight repos at
origin/development:16 live→dead — openconnector 11, scholiq 5, exactly the issue's count.
The single dead→live is
softwarecatalog::org-archimate-export::user-triggers-organization-export-with-toggles, and it is not a relaxation. It exposes a pre-existing false positive onmain: the body finder scans back from the closing paren expecting}, and a trailing comma before)makes a real asserting test read as an empty body. Demonstrated in isolation against both versions:The ref flips only because the tag now resolves outward to the enclosing
test.describe, whose body computation succeeds. That bug is untouched here and filed separately — it is a false RED, unrelated to either issue, and fixing it inside this PR would change verdicts for a third reason.Proven both ways
25 new tests. Every dead assertion is paired with a live control:
describe.only,describe.serial,describe.configure, a livedescribe, a sibling after a closed skipped block, a runtime-conditional skip, andrx.test(. Eleven mutations, each killed:_switched_off_ancestorfinds nothing_is_switched_off→ always True / always False\w+\.).onlyfolded into the modifier group.serialfolded into the modifier groupxdescribearm droppedNothing was unskipped in any consumer repo. That is a separate decision.
Resolution safety
The new lint is a file under
scripts/, called byquality-resolve-probe.yml— not byquality.yml, so no consumer below any tag can break on a path that does not exist in its checkout. It runs as its own job behind the same singleShared-workflow guardrequired check, with its positive control first, exactly as the probe does.assert-run-steps-resolvable.pystill passes: everyrun:block is under 16384 bytes, and the largest step added here is ~1 KB.Full package suite: 27 passed, 0 failed, 2 quarantined (both pre-existing, unchanged).
What consumers see on the next run
The fleet tracks
@mainunpinned per #177, so a merge here reaches all callers immediately.&&chain.|| truestill cannot fail — its seed swallows its own status. Unchanged by this PR, and worth a decision separately.Closes #192
Closes #210