Skip to content

Commit c0429dd

Browse files
committed
ci(timings): give the test-timing parser self-test teeth in lint.yml, not continue-on-error in ci.yml
`scripts/report-test-timings.mjs` ships a real self-test — 61 cases across 4 batteries, a naive prefix-only reference parser plus three controls that assert it gets the answer wrong exactly where the real parser gets it right. ci.yml named it, so `check:self-test-wired` was green. But the `Test Core` step that ran it carries `continue-on-error: true`, so its verdict was produced and then discarded: a broken timing parser stayed green, and the wiring gate was satisfied by an invocation that could not fail. That `continue-on-error` cannot simply be removed. #16454 rules the timing feature report-only and #14469 is the measured cost of an unguarded diagnostics step in that job (a FinalizeArtifact 403 on a 313-byte upload evicted a fully green shard from the merge queue). Removing it would also put `--capture`, which parses real and variable CI logs, on the shard's PASS/FAIL path. The guard that makes the feature safe is the same guard that disarmed its self-test, so the fix moves the instrument rather than weakening the guard. - lint.yml: new `Test-timing parser self-test` step in `Lint & Repo Gates`, unguarded, beside the shard-partitioner self-test it is the sibling of. - ci.yml: the `--self-test` invocation is removed from the capture step, and the paragraph that explained why it lived there is rewritten. `--capture` and `--merge` keep `continue-on-error`; that property is untouched. - report-test-timings.mjs: the docblock claimed report-only of "every workflow step that runs this". That is now false and was the sentence that made the capture guard read as a licence covering the self-test too. `check:self-test-wired` stays green with the invocation moved: lint.yml is a workflow, and the gate credits any workflow that names the script. Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU Co-authored-by: Claude <noreply@anthropic.com>
1 parent 66e34d1 commit c0429dd

3 files changed

Lines changed: 51 additions & 12 deletions

File tree

.github/workflows/ci.yml

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -928,12 +928,17 @@ jobs:
928928
# here is fine; moving them below that pair is not — and ⛔ that pair does
929929
# not acquire an `if:` or a `continue-on-error:` to "match" these.
930930
#
931-
# The `--self-test` runs here because this is where CI names the script,
932-
# which is what check:self-test-wired requires. ⚠ Under
933-
# `continue-on-error` its verdict is VISIBLE but not enforcing: inside
934-
# this workflow, no step this card may add is permitted to fail anything.
935-
# Giving that self-test teeth means a `check:` step in lint.yml, which is
936-
# deliberately NOT taken here.
931+
# ⛔ The `--self-test` is deliberately NOT invoked here (#17097). It ran
932+
# in this step until then, where `continue-on-error` discarded its verdict:
933+
# the self-test reached an answer nothing could act on, and
934+
# `check:self-test-wired` was satisfied by an invocation that could not
935+
# fail. A self-test with no teeth is worse than no self-test, because its
936+
# presence is what stops anyone from adding a real one. It now runs in
937+
# lint.yml's `Lint & Repo Gates` job, as `Test-timing parser self-test`,
938+
# with no `continue-on-error` — so a broken timing parser reddens there
939+
# while this workflow keeps the report-only property #16454 ruled and
940+
# #14469 measured the cost of losing. ⛔ Do not re-add it here; giving it
941+
# teeth in `ci.yml` is exactly the trade-off those two cards refuse.
937942
- name: Capture this shard's test timings
938943
if: always()
939944
continue-on-error: true
@@ -942,7 +947,6 @@ jobs:
942947
echo "No test log — the test step did not get far enough to produce one."
943948
exit 0
944949
fi
945-
node scripts/report-test-timings.mjs --self-test
946950
node scripts/report-test-timings.mjs --capture \
947951
--log "$RUNNER_TEMP/test-core.log" \
948952
--summaries .turbo/runs \

.github/workflows/lint.yml

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3943,6 +3943,31 @@ jobs:
39433943
- name: Shard partitioner self-test
39443944
run: node scripts/partition-test-shards.mjs --self-test
39453945

3946+
# A self-test that RUNS but cannot fail anything (#17097) — the sibling of
3947+
# the defect above, one turn further out. `scripts/report-test-timings.mjs`
3948+
# ships a real self-test: 61 cases across 4 batteries, a naive prefix-only
3949+
# reference parser plus three controls that assert it gets the answer WRONG
3950+
# exactly where the real parser gets it right. ci.yml names it, so
3951+
# `check:self-test-wired` was green. But every `Test Core` step that runs
3952+
# that script carries `continue-on-error: true`, and MUST: #16454 rules the
3953+
# timing feature report-only, and #14469 is the measured cost of an
3954+
# unguarded diagnostics step there (a FinalizeArtifact 403 on a 313-byte
3955+
# upload evicted a fully green shard from the merge queue). So the guard
3956+
# that makes the feature safe was the same guard that disarmed its
3957+
# self-test — the verdict was produced and then discarded, and the wiring
3958+
# gate was satisfied by an invocation that could not fail.
3959+
#
3960+
# ⇒ the self-test runs HERE, where it has teeth, and ⛔ is no longer
3961+
# invoked from ci.yml: ONE enforcing run, not one enforcing and one
3962+
# decorative. ci.yml's own `continue-on-error` on the capture and merge
3963+
# steps is untouched by this — the report-only property belongs to that
3964+
# workflow, not to this script.
3965+
# Invoked as `node` rather than through a `pnpm check:*` alias for the same
3966+
# reason as the steps above: see the GATE INVOCATION IDIOM note at the top
3967+
# of this file. In-process fixtures, no IO, milliseconds.
3968+
- name: Test-timing parser self-test
3969+
run: node scripts/report-test-timings.mjs --self-test
3970+
39463971
# Hand-written `.d.mts` mirrors (#10549). `scripts/js-comment-mask.mjs`
39473972
# and `scripts/check-regen-pending.mjs` are untyped `.mjs` that each ship
39483973
# a hand-written declaration beside them, and both files say "keep this in

scripts/report-test-timings.mjs

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,21 @@
1818
* summaries (`.turbo/runs/*.json`, uploaded per shard) carry per-PACKAGE
1919
* execution windows and nothing finer.
2020
*
21-
* REPORT ONLY. Nothing here may redden `Test Core` -- that is the required
22-
* branch-protection context, and a diagnostics step that can fail it buys a
23-
* measurement at the price of the merge queue. Every workflow step that runs
24-
* this carries `if: always()` and `continue-on-error: true`, and the tool
25-
* itself degrades to a named refusal rather than throwing.
21+
* REPORT ONLY IN `ci.yml`. Nothing in that workflow may redden `Test Core` --
22+
* that is the required branch-protection context, and a diagnostics step that
23+
* can fail it buys a measurement at the price of the merge queue. Every
24+
* `ci.yml` step that runs this carries `if: always()` and
25+
* `continue-on-error: true`, and the tool itself degrades to a named refusal
26+
* rather than throwing.
27+
*
28+
* -- and that is a property of `ci.yml`, NOT of this script (#17097). This
29+
* docblock used to claim it of "every workflow step that runs this", which
30+
* made the `--capture`/`--merge` guard read as a licence covering the
31+
* self-test too. `--self-test` runs in lint.yml's `Lint & Repo Gates` job with
32+
* NO `continue-on-error`: a red verdict there is the whole point, and it is
33+
* the only instrument watching the parsing rules below -- a clean tree cannot
34+
* tell a working parser from a weakened one, which is what the naive-parser
35+
* controls in the battery exist to say out loud.
2636
*
2737
* ## THE MEASUREMENT THAT CHOSE THE ROUTE (route (a): parse the stream)
2838
*

0 commit comments

Comments
 (0)