2026-07-28 17:44:35 -06:00
|
|
|
# Contributing to Civitai
|
|
|
|
|
|
|
|
|
|
Thanks for contributing. This document covers the things that are easy to get
|
|
|
|
|
wrong here and hard to discover on your own — what CI actually checks, how to
|
|
|
|
|
verify your work locally, and a few conventions that have bitten us before.
|
|
|
|
|
|
|
|
|
|
For getting the app running at all, see [README.md](README.md).
|
|
|
|
|
|
|
|
|
|
## Fork PRs: what CI does and doesn't check
|
|
|
|
|
|
|
|
|
|
`civitai/civitai` is public, and a `pull_request` from a fork gets no repository
|
|
|
|
|
secrets. That is deliberate and is not going to change. The practical
|
|
|
|
|
consequences:
|
|
|
|
|
|
|
|
|
|
- **Workflows don't start on their own.** A fork PR parks at `action_required`
|
|
|
|
|
until a maintainer clicks *Approve and run workflows*. If your PR looks like
|
|
|
|
|
nothing is happening, that's why — please just ping in the PR.
|
|
|
|
|
- **Two things block.** Typecheck (full repo), and ESLint errors + Prettier on
|
|
|
|
|
files your PR **adds**. New files start clean, so holding them to the rules is
|
|
|
|
|
free. Typecheck runs on fork PRs too — it needs no secret, because the
|
|
|
|
|
`event-engine-common` submodule is public and fetched over HTTPS.
|
|
|
|
|
- **Everything else is report-only.** ESLint and Prettier on files you *modify*
|
|
|
|
|
run with `continue-on-error`, because a formatter has no changed-line
|
|
|
|
|
granularity — 789 of 4,116 `src` files fail `prettier --check` today, so
|
|
|
|
|
blocking on them would turn a three-line bugfix into a 200-line reformat. You
|
|
|
|
|
will see a failed-but-ignored marker in the checks list. That's expected.
|
|
|
|
|
- **Unit tests run but don't block yet.** CI runs the unit suite (676 files,
|
|
|
|
|
~8,900 tests) report-only while we establish its pass rate on a CI runner — a
|
|
|
|
|
handful of tests are slow enough to trip the 60s per-test timeout under load,
|
|
|
|
|
though they pass in isolation. So a red unit job is a signal to look, not proof
|
|
|
|
|
you broke something, and a green overall check doesn't mean the tests passed.
|
|
|
|
|
Read the job output.
|
fix(tests): unzero the `preview / component-tests` tier, and make a zero-collected run say so (#4531)
* fix(tests): unzero the component tier — one mock factory was aborting the whole run
`preview / component-tests` was reporting `failure` on `main` while executing ZERO
tests. Reproduced deterministically (3/3 at d353f785c3, and in isolation): the whole
`component` project aborts before any reporter prints, with no `Test Files` line, no
per-file results, and exit 1.
Root cause, one file:
`src/tests/pages/apps/review/review-queue-nav.browser.test.tsx:30` mocks
`~/providers/FeatureFlagsProvider` with a WHOLESALE factory naming only
`useFeatureFlags`. The review queue page's row now renders the review entry point,
which reads flags through `useOptionalFeatureFlags`, so the named import has nothing
to bind to:
SyntaxError: The requested module '/src/providers/FeatureFlagsProvider.tsx'
does not provide an export named 'useOptionalFeatureFlags'
In the node `unit` project that would fail ONE file. In BROWSER mode it kills the run:
vitest resolves a manual mock over the browser-to-node channel inside a Playwright
route handler that does not catch (`@vitest/browser-playwright/dist/index.js`,
`await module.resolve()` inside `page.route`), so the rejection escapes as an
Unhandled Rejection in the orchestrator.
The printed error names neither the file nor the real cause. It is wrapped twice --
once by the browser mocker, once again on the node side -- and the innermost `cause`
is dropped in transit, so all you see is the generic "[vitest] There was an error when
mocking a module ... make sure there are no top level variables inside", which points
at hoisting and is wrong. The root cause above was recovered by temporarily patching
`createHelpfulError` in the browser tester bundle to inline `cause.stack`; the file was
then confirmed by bisecting the 50 candidate files down to one.
This is the SECOND time this class has bitten (see the header of
`src/components/AppBlocks/__tests__/featureFlagsMockCompleteness.test.ts`, which fixed
six sibling suites in the AppBlocks directory and deliberately scoped its guard there).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(ci): make the component tier fail LOUDLY on zero collected, not silently
The `preview / component-tests` tier could report `failure` having executed ZERO
tests, and nothing anywhere said so. The shared `npm-report-only-suite` Tekton task
computes its verdict from the runner's EXIT CODE alone, so an abort that collected
nothing and a genuine list of red assertions both render as `component:fail` /
"Component suite failed" -- same words, same colour, same place. That is the shape
that trains people to click through a tier.
`pnpm test:component` now runs through `scripts/test-component-run.mjs`, which asks
vitest for a JSON report and hands it to `scripts/ci/assert-component-suite-ran.mjs`.
That gate prints a ledger -- `N executed, N skipped, across N files; N failed suites,
N failed tests` -- and fails when nothing was collected, or when the executed count
falls below a floor (1240, ~55% of the 2254 measured on a full green run of 201 files
on 2026-08-31).
Deliberate limits, each of which is a way this could have been wrong:
- It can only ever ADD a failure. Vitest's own exit code is passed straight through
when the ledger is satisfied, so a red suite stays red for its own reason.
- EXECUTED, not TOTAL. `numTotalTests` counts skipped tests, so a suite that
self-skipped wholesale would satisfy a total-based floor having run nothing.
- `failed` counts as executed. A guard that scored a red run as "did not run" would
fire on every genuine failure, and the tier would then be red for two different
reasons that nobody could tell apart -- the exact confusion this removes.
- A signal-killed runner short-circuits the gate entirely. The CI task wraps this in
`timeout(1)` and distinguishes "timeout" from "fail" on purpose; a killed run also
writes no report, so relabelling it "collected nothing" would be a wrong answer
rather than a missing one.
- A narrowed run (a file argument, or `-t`) skips the FLOOR but not the zero check:
the collected count is then a property of the filter, but a single-file run that
collects nothing is the cheapest reproduction of the abort and is precisely when
someone is debugging it.
The message enumerates every cause it cannot tell apart rather than asserting one --
an absence is the observable the most causes share, and a guard that names the wrong
one sends the next reader hunting a bug that is not there. All three named have been
observed on this suite; two of them were observed while writing this change, and the
browser-crash one arrives wearing the mock error's headline with the real cause on
the `Caused by:` line.
Verification:
- 16 unit tests over JSON fixtures (`scripts/__tests__/`), covering zero, the floor
boundary both sides, all-red, all-skipped, narrowed both ways, import-failed
suites, and a missing/unparseable report.
- Mutation-checked: 9 mutants of the gate, ALL KILLED, each by the specific named
test written for it (verified per-test, not "some test failed"), against a green
16/16 baseline.
- End-to-end negative control: reverting the one-line fix in the previous commit and
running through the wrapper produces the ledger's abort message and exit 1, from
the missing-report branch.
- End-to-end positive: the full suite through `pnpm run test:component`.
Docs corrected while here: CONTRIBUTING said component tests "don't run in CI at all"
and put the file count at 106; they do run, report-only, in the preview pipeline, and
there are 201 files with ~2,250 tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(tests): the zero-collected message overstated a PARTIAL abort
The headline said "THE COMPONENT SUITE COLLECTED NOTHING ... nothing executed", and
the code contradicts it in the case that actually happens most: the JSON reporter
writes at the END of a run, so an abort part-way through leaves no report at all.
Measured while writing this: a run that aborted 68 files into 201 had 68 files
scrolled past as green, wrote nothing, and got told nothing executed.
Both halves of that were wrong to assert. Tests HAD executed, and a reader who
scrolls up and sees green lines is entitled to believe the message is confused --
or, worse, to believe the green lines are coverage.
So the headline is now "THIS RUN PRODUCED NO ACCOUNTABLE RESULT", which is true of
both shapes, and the diagnosis says explicitly that an abort can land part-way and
that whatever scrolled past is unaccounted for rather than confirmed. The
missing-report branch says why the report is absent (the reporter writes at the end)
and that a partial run and a run that never started are indistinguishable from
there -- which is the reason neither counts as one that ran.
No behaviour change: the same runs fail, with the same exit code. The four test
assertions that pinned the old headline move with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(tests): close six findings from the adversarial audit of this PR
An adversarial audit of #4531 found two ways the new guard reproduced the very
pathology it was written to remove, plus four smaller gaps. All six are fixed here;
nothing about the payload fix in c32aa06e8d changes.
1. A SIGNAL WAS RELABELLED AS A TEST FAILURE. `child.on('exit')` returned a hardcoded
143 for EVERY signal. `report-only-suite-task.yaml` branches on 137 to report
`oom-killed` -- "this is an OUT-OF-MEMORY kill, not a timeout; raise the task's
memory limit" -- and before this wrapper existed an OOM-killer SIGKILL on vitest
reached that task as 137, because pnpm re-raises. With a constant 143 it matched no
branch and fell through to `RC=1`, verdict `fail`, rendered "Component suite
failed". A memory problem reported as a test failure, on the same tier, in the same
words. Now `128 + os.constants.signals[sig]`, so 137/143/130 come out right and the
mapping cannot drift from the names node hands back. The comment above the branch
claimed it passed the status through; it does now.
2. A CALLER `--outputFile` MADE A GREEN RUN REPORT "COLLECTED NOTHING". Measured by
the auditor: `pnpm test:component <file> --outputFile=/tmp/x.json` wrote an 18/18
green report to the caller's path, left the wrapper's path empty, and printed the
full "this tier verified NOTHING on this commit" diagnosis naming three causes,
none of them real. `.github/workflows/lint.yml` runs the SIBLING unit tier with
exactly that flag, so it is a copy-paste away. Now refused up front with the fix in
the message.
3. `isNarrowed` WAS WRONG IN BOTH DIRECTIONS ON SPACE-SEPARATED FLAGS. `--max-workers 1`
-- the form CONTRIBUTING steers people towards for sizing a run on a shared box --
put `1` in a positional slot, so the run scored "narrowed" and THE FLOOR WAS SILENTLY
TURNED OFF; same for `--reporter`, `--retry`, `--bail`, `--project`, `--pool`. In the
other direction `--shard=1/4` scored NOT narrowed, so a healthy sharded run would
fail the floor while being told "Do NOT lower the floor to make this green" --
misdirection, not merely a false red. Value-taking flags now consume their value, and
`--shard`/`--changed`/`--related` narrow explicitly.
4. WINDOWS. `spawn()` of `vitest.cmd` without `shell` has failed since the node
18.20.2/20.12.2 CVE fix; `scripts/test-unit-run.mjs` already sets
`shell: process.platform === 'win32'` for this reason. Added. And a spawn failure
(rc 127) now short-circuits instead of handing the gate a missing report and getting
forty lines about mock factories and dead browsers.
5. THE REPORT IS NOW DELETED BEFORE THE RUN, not only after. Cleanup after the run is
skipped by exactly the paths that leave a stale report behind (a signal death, a
throw), so a healthy 2254-test report could survive into a later run that aborted
before writing one -- the gate reading it, printing a green ledger, and passing:
silently inert in precisely its own use case.
6. THE GATE COMPUTED A FILE COUNT AND NEVER CHECKED IT. Replaced with a LEDGER, which
is the stronger of the two checks: it walks `src/` for every `*.browser.test.tsx` and
fails when one is absent from the report, NAMING it. The test floor sits at ~55%, so
~45% of the suite could stop being collected while the gate stayed green -- and the
incident this whole guard descends from is exactly that shape (six files contributing
0 of 438, nothing red). The expectation is re-derived every run, so there is no
constant to go stale. Skipped when narrowed, and when the walk finds nothing -- an
empty walk is not a measurement, and it SAYS so rather than passing quietly.
Also: the fixed factory now names all THREE hooks the flags module exports.
`useFeatureFlagsReady` is the third and has four live consumers; none is in this page's
graph today, which is the only reason naming two loads at all. The comment said "BOTH
hooks", which reads as "the module has exactly two" -- and it is the template the PR
proposes copying to fifty files.
Verification:
- 27 unit tests (was 16), including the on-disk ledger against a fixture tree that
contains a `node_modules/` and a non-browser `.test.tsx` neither of which may count.
- Mutation-checked: 17 mutants, ALL KILLED, each by its own named test. One SURVIVED on
the first sweep -- `out.length > 0 ? out : null` was unreachable from the fixture,
which had no `src/` at all, so the walk returned early. A second case with a `src/`
that holds no browser tests reaches it; an empty array is TRUTHY, so that mutant would
otherwise have armed a ledger over zero expected files and passed everything.
- The on-disk ledger run against the REAL tree: 201/201 green, and dropping one real
file from the report fails and names it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(tests): close audit round 2 — two regressions the round-1 fixes introduced
A delta re-audit of `6ceac37f33..7f005f0482` confirmed 4 of the 7 claims outright
(the signal mapping, the pre-run clear, the file ledger, the fixed factory) with its
own positive and negative controls, and found that two of the fixes had introduced
new defects of their own. Both are here, with the smaller findings.
🔴 THE ON-DISK LEDGER FALSE-FAILED A LEGITIMATE RUN, WITH A MESSAGE FORBIDDING THE FIX.
`--exclude`, `--dir` and `--root` were added to VALUE_FLAGS but not to
NARROWING_FLAGS. All three take a value AND genuinely shrink the collected file set,
so `pnpm test:component --exclude 'src/tests/**'` scored as a FULL run and the ledger
failed it naming up to 200 files -- asserting the include broke or the run died, and
telling the reader "Do NOT silence this by narrowing the walk", which is the only
thing that would have fixed it. `--config` joins them: it can replace the project's
`include` outright, which is the assumption the walk is built on. This is precisely
the shape the round was convened to prevent, produced by the round's own fix.
🔴 `shell: win32` WENT ON THE SHARED `run()`, SO IT ALSO WRAPPED THE GATE SPAWN.
The claim said this matched `scripts/test-unit-run.mjs`; it did not -- that file puts
`shell` on its vitest spawn only and deliberately leaves its `process.execPath` spawn
alone. Node with `shell: true` concatenates argv UNESCAPED (DEP0190), and the gate is
spawned as `process.execPath`, which on Windows is `C:\Program Files\nodejs\node.exe`.
So on the one platform the option exists to support, every `pnpm test:component` would
have failed at the gate step with a cmd.exe parse error rather than any of this
wrapper's messages. `shell` is now per-call.
Also:
- KEBAB AND CAMEL ARE THE SAME FLAG TO VITEST (cac camelCases every option key), so a
hand-enumerated list has a hole wherever it carries one spelling and not the other --
and it did: `--max-workers`/`--maxWorkers` were both listed, `--test-timeout` was
not, so the kebab form silently disabled both checks. Spellings are now canonicalised
rather than enumerated, and the test asserts them in PAIRS.
- `--output-file` (kebab) was NOT refused, so the exact defect the refusal exists for
was still reachable -- under a test named "catches every spelling". Meanwhile
`--outputFile.junit=` and `--outputFile.html=` WERE refused, though neither touches
the `.json` key: object-form output paths are per-reporter, so that was over-strict
and the stated reason ("the bare form sets the path for EVERY reporter") is true only
of the bare form. Both directions fixed.
- The rc-127 short-circuit is keyed on a spawn-failure FLAG, not on the number. 127 is
an exit code a runner can produce on its own, and relabelling that as "the binary
could not be started" is a second wrong answer that also skips the gate on a run that
happened.
- `--repo-root <dir>` did not consume its value, so with the flag FIRST the directory
was read as the report path: "EISDIR: illegal operation on a directory" plus the
whole abort diagnosis. Every test passed it last, which is why nothing caught it; the
ledger tests now pass it first.
- `--related` removed. It is a vitest SUBCOMMAND, and this wrapper always spawns
`vitest run …`, so it cannot arrive -- the entry and its assertion were both inert,
the assertion pinning behaviour for an input the runner cannot receive.
- The factory now names all FOUR runtime exports of the flags module, including
`FeatureFlagsProvider`. The comment said "EVERY runtime hook", which was narrower
than the module -- and "not in this graph today" is exactly the reasoning that put
this file in the diff.
- A comment the previous round made false: it said the positional rule would catch
`-t foo` anyway. It stopped being true the moment `-t` was listed as value-taking --
the value would now be CONSUMED -- so the explicit narrowing branch is load-bearing.
- CONTRIBUTING documents the refusal, the file ledger, and which flags narrow.
Coverage for the two claims that shipped with none: `main` now takes its collaborators
as injected defaults, so the ORDER of effects can be asserted rather than inspected.
The two `clearReport()` calls are byte-identical statements -- only their position
carries the meaning, so a refactor moving the first below the run reopens "a stale
report satisfies the next run's ledger" with every other test green.
Verification: 35 unit tests (was 27). Mutation-checked at 17 mutants, ALL KILLED, zero
survivors -- including re-running round 1's guards, because an audit fix resets the
gate. Real-tree controls re-run with the new argument parsing, flag first and flag
absent: 201/201 green, and dropping one real file fails and names it. ESLint on the
touched files is back to the base count; two `no-empty-function` errors this change
introduced are fixed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(tests): close audit round 3 — the canonicaliser re-opened the class it closed
Round 3 verified 8 of round 2's 9 claims against the diff or against vitest's own
source (it read cac's `setDotProp` to confirm the `--outputFile.junit=` allowance is
genuinely safe, and `cliOptionsConfig` to confirm `-r`/`-c` really are root/config).
It found one regression and one coverage hole, both here.
🔴 A BOOLEAN FLAG SWALLOWED THE FILENAME AFTER IT — the same shape round 2 fixed,
re-introduced by round 2's own mechanism. `canonicalFlag` collapsed `.subkey` onto the
parent, so every dot-subkey inherited its parent's value-consuming behaviour, and the
two entries `--coverage.reporter`/`--coverage.provider` became a single `coverage` that
also matched the BARE `--coverage` — which takes no value (`argument: ""` in vitest's
`cliOptionsConfig`). So `pnpm test:component --coverage <file>` ate the FILE as
`--coverage`'s value, scored the run as full, and failed an 18/18 green single-file run
naming ~200 files as ABSENT while telling the reader not to narrow the walk. Same for
`--coverage.enabled <file>` and `--browser.headless <file>`. Measured base-vs-head, all
three flipped `true` to `false`. Matching is now on the full canonical PATH, so a
subkey is value-taking only if it is listed as one.
🔴 THREE MUTANTS SURVIVED A GREEN 35-TEST SUITE, AND EACH WAS THIS PR'S OWN HEADLINE
FAILURE. The injected fakes took no parameter, so nothing could observe the argv `main`
builds — the seam between the two modules this change exists to wire together. Dropping
`--narrowed` from the gate argv makes every narrowed run hard-fail both the floor and
the on-disk ledger; dropping `--outputFile.json=` makes every run report "does not
exist" plus the whole abort diagnosis; dropping `...argv` makes
`pnpm test:component <file>` silently run the entire suite behind a healthy-looking
ledger. Testing `isNarrowed` in isolation cannot see whether its answer is ever USED.
The fakes now capture their argument and three cases assert it; all three mutants die.
Also:
- `--repo-root` with no value was a silent fall-through to the real repo root, so a
fixture report got graded against the 201 real files and failed with a confident
diagnosis about the include breaking — produced by a typo. Now a usage error (exit 2).
- CONTRIBUTING: the sample ledger omitted `measured <date>`, which the real output
carries; and "skips both checks but not the zero check" counted a different pair than
the two enumerated four lines above. The checks are now numbered and the sentence
names which ones a narrowed run skips.
Verification: 39 unit tests (was 35). Mutation-checked at 19 mutants, ALL KILLED, zero
survivors — the three that survived round 3, the round-3 fixes themselves, and every
guard from rounds 1 and 2 re-run, because an audit fix resets the gate. Real-tree
controls re-run: 201/201 green, one missing file still fails. ESLint clean on the
changed test file.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(tests): close audit round 4 — replace the flag LIST with a shape rule
Round 4 measured `isNarrowed` at base vs head across every one of vitest 4.1.11's 72
boolean options and 73 value-taking ones, and found the flag list had traded 22 loud
wrong answers for 25 quiet ones. Splitting `coverage` into two subkeys left
`--retry.count 2`, `--browser.name chromium` and sixteen more `coverage.*` paths
reading their VALUE as a filename, scoring a FULL run as narrowed and switching the
file ledger and the floor off with a one-line note. That is the direction this file's
own comments repeatedly name as the worse of the two.
🔴 THE LIST WAS THE PROBLEM, AND THREE ROUNDS WERE SPENT ON IT. Enumerating flag NAMES
missed `--test-timeout` next to `--testTimeout`. Canonicalising spellings then made
`--coverage <file>` swallow the file. Splitting into subkeys produced the 25 above.
vitest 4.1.11 has a 164-path option tree; a hand-maintained copy of it is wrong the day
it is written, and each fix moved the wrongness rather than removing it.
So the rule is now about the ARGUMENT, not the flag: a positional is a file filter if
it looks like a path, or if nothing before it could have been expecting a value. A
non-path token straight after a flag is that flag's value, WHATEVER the flag is —
correct for all 73 value-taking options without naming one of them, and correct for all
72 booleans too. `VALUE_FLAGS` is deleted. What remains is `NARROWING_FLAGS` (flags that
genuinely shrink the run and must be named, because omitting one fails LOUDLY) and a
five-entry `PATH_VALUE_FLAGS` for the only residual the shape cannot decide: a value
that is itself a path.
🔴 AND THE DECISION IS NOW A SENTENCE, NOT A BOOLEAN. No rule over an unknowable flag
list is right always; what must never happen is being wrong SILENTLY, because
`--narrowed` disables the two checks this whole change exists to add. `narrowingReason`
returns why, and the runner prints it: `--retry.count 2` reading `2` as a file filter is
obvious on sight and invisible otherwise.
Also:
- `canonicalFlag` camelCases only the FIRST dot segment, matching cac's own
`camelcaseOptionName` (`name.split(".").map((v,i) => i===0 ? camelcase(v) : v)`).
Camel-casing all of them made this wrapper accept `--coverage.reports-directory`,
a spelling vitest does not — so the two would disagree about the next token.
- `--repo-root=<dir>` is parsed. Matching only the space form left the inline spelling
falling through BOTH branches to the real repo root — byte-for-byte the failure the
missing-value guard was added to close, reachable through one extra character. A
value that is itself a flag is rejected too.
Verification: 45 unit tests (was 44 after the round's own additions; 39 before).
Mutation-checked at 23 mutants, ALL KILLED, zero survivors, each by its OWN named test.
🔴 THE FIRST SWEEP REPORTED FOUR SURVIVORS AND WAS WRONG ABOUT ALL FOUR, WHICH IS WORTH
RECORDING BECAUSE THE HARNESS HAD THIS PR'S OWN DEFECT. Two were mislabelled — killed by
a different test than the one named, which the sweep scores as SURVIVED. Of the other
two, one was a real gap (`--reporter=json AppNameCrumb`, now covered) and one was
`canonicalFlag`'s dot handling, which is equivalent for every input that has no dashed
subkey (also now covered). Along the way the harness itself was found reusing a JSON
report path without clearing it — the same stale-report defect fixed in the product two
commits ago — and now clears it and verifies the mutation reached disk before running.
A mutant reported SURVIVED is a claim about the instrument until the instrument has been
controlled.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* wip(tests): stop guessing which vitest flags take values
Safety commit of in-progress round-5 rework so it is not lost; the agent was
interrupted mid mutation-sweep. Verification is NOT complete — the sweep, the
red/green pair and the merged-tree re-run have not been reported. Do not merge
on this commit.
Round 5 measured both prior approaches against vitest 4.1.11's real option
table (170 long options, 74 boolean, 96 value-taking): the hand-maintained
list was wrong 73 times, the shape heuristic 74. The error count never moved,
only its direction. This removes the question instead of answering it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AEa6GDJyTiu2R146ndYsLK
* test(ci): finish the verification bacb8a2cf5 was pushed without
bacb8a2cf5 landed as a rescue commit marked `wip` because the session that wrote it
died with the change only in a working tree. The change itself is unaltered; this is
the verification it was missing, plus the merge of a main that moved twice underneath
it. Nothing here modifies the rule.
WHY THE RULE CHANGED, WITH THE NUMBER THAT JUSTIFIES IT. Round 5 of the audit
enumerated vitest 4.1.11's REAL CLI option table -- by calling `createCLI()` and reading
each cac option's `isBoolean` rather than by hand -- and ran `isNarrowed(['--<flag>',
'VALUE'])` against both revisions imported side by side:
hand-maintained flag list wrong on 73 options (every value-taking one) QUIET
shape heuristic wrong on 74 options (every boolean one) LOUD
73 versus 74. The heuristic did not beat the list; it moved the wrongness off one half
of the table onto the other. The direction improved, which is worth something, but the
error count did not -- and `pnpm test:component --coverage AppNameCrumb` would run one
test and then be failed against the 1240 floor with "the include broke or the run died".
That is a mis-posed question, so it is no longer asked: any argument at all means
narrowed.
Re-measured against the same table on the merged tree, with the same method:
164 long options (72 boolean, 92 value-taking)
UNSAFE-LOUD -- rule claims FULL so the floor and ledger fire on a partial run: 0
no-argument invocation (what CI runs): NOT narrowed, so all three checks arm
(My enumeration walks the global + `run` commands and sees 164/72/92 where round 5's
saw 170/74/96; the traversal differs slightly, the conclusion does not.)
🔴 The comparison is one-directional ON PURPOSE, and the other direction is a real cost
rather than a rounding error: all 164 options now score as narrowed, so an arg-ful run
does not get the floor or the file ledger even when it was genuinely full. That is
affordable for one measured reason -- `pr-preview-pipeline.yaml` invokes
`pnpm run test:component` with NO arguments, so CI is the `argv.length === 0` path and
always gets all three. What is given up is enforcing a floor on an ad-hoc local run.
`VITEST_MAX_WORKERS=4` in the environment sizes a run without giving that up.
Verification on the MERGED tree (d77dd394db), all at load <= 16 with no browser-session
errors in any run quoted:
no-arg full run (the CI path) 202 files / 2257 tests, exit 0, no NARROWED line,
floor 1240 and the 202-file ledger both ARMED
guard red arm exit 1, "THIS RUN PRODUCED NO ACCOUNTABLE RESULT",
failing on the missing-report branch
guard green arm exit 0, "2 executed, 0 skipped, across 1 files"
gate unit tests 37 passed
mutation sweep 20 mutants, ALL KILLED, zero survivors
🔴 The red arm was run NARROWED, deliberately: it proves `--narrowed` disables the floor
and the ledger and NOT the zero-collected check, which is the one failure this whole
change exists to catch.
THE MERGE. main moved twice; both times the only conflict was `package.json`, and both
times it was the same semantic hazard -- main editing `test:lint-rules` and this branch
editing `test:component`, adjacent lines of one object, where taking either side
wholesale silently reverts the other and makes the entire guard inert with every test
green. Resolved by parsing the merged JSON, not by reading the diff: `test:lint-rules`
now matches origin/main byte-for-byte and `test:component` is the wrapper. The wiring
guard added for exactly this was then checked against the bad resolution -- applying
main's `package.json` wholesale fails one test, the one written for it.
Round 5's two remaining 🟢 items are closed by the rework itself rather than separately:
the stale JSDoc described `VALUE_FLAGS`, which no longer exists, and the test whose
description claimed "a BOOLEAN flag does not swallow the filename after it" while
passing entirely through the path check is gone with the heuristic it tested. That one
was load-bearing -- a description asserting coverage its body did not provide is what
let the class through -- so its replacement asserts a property with no free parameters
instead, across both halves of the option table.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* style(tests): format the gate test file
The rescue commit was pushed before a prettier pass could run over the last edit to
this file, and `ESLint + Prettier (changed files)` caught it: the file is ADDED by this
PR, so it is covered by the added-files prettier gate. One line.
This red was MINE, not inherited — unlike `Unit tests (1)/(2)` (a redis integration
test that arrived from main) and `preview / smoke-tests` (#4516 changing
`reaction.toggle`), both of which are verified as pre-existing and are left alone.
CONTRIBUTING.md also reports unformatted, and that one is NOT actionable here: it is
already unformatted on origin/main, and it is modified rather than added, so the gate
(which reads `added.txt`) does not cover it. Reformatting it would bury this PR under an
unrelated whole-file diff.
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-01 10:57:49 -05:00
|
|
|
- **Component tests run in the PR-preview pipeline, report-only.** The 201
|
|
|
|
|
`*.browser.test.tsx` files need real Chromium; they do not run in GitHub Actions,
|
|
|
|
|
but the in-cluster preview pipeline runs them and posts a
|
|
|
|
|
`preview / component-tests` commit status. It never blocks, so read it rather
|
|
|
|
|
than relying on it.
|
2026-07-28 17:44:35 -06:00
|
|
|
|
|
|
|
|
A green check on a fork PR therefore means much less than it looks like. Verify
|
|
|
|
|
locally.
|
|
|
|
|
|
|
|
|
|
## The `event-engine-common` submodule
|
|
|
|
|
|
|
|
|
|
The repo depends on a submodule at `event-engine-common/`. It is public and
|
|
|
|
|
fetchable over HTTPS, but it is **not** checked out automatically:
|
|
|
|
|
|
|
|
|
|
```bash
|
|
|
|
|
git submodule update --init event-engine-common
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
Without it, `pnpm typecheck` fails with a wall of `Cannot find module` errors and
|
|
|
|
|
several dozen test files fail to *collect* — noise that looks like your change
|
|
|
|
|
broke something when it didn't. Same applies to any new git worktree; worktrees
|
|
|
|
|
don't check out submodules for you.
|
|
|
|
|
|
|
|
|
|
### If you cloned before the URL moved to HTTPS
|
|
|
|
|
|
|
|
|
|
Run this once:
|
|
|
|
|
|
|
|
|
|
```bash
|
|
|
|
|
git submodule sync --recursive
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
`git submodule init` writes the URL into `.git/config` the first time it runs and
|
|
|
|
|
never overwrites it afterwards — **including when that first attempt failed.** So
|
|
|
|
|
if you ever tried `--init` while the submodule was still private and got an auth
|
|
|
|
|
error, your clone has the old SSH URL recorded and `--init` will keep failing with
|
|
|
|
|
the identical error no matter how many times you pull. `sync` is the fix, and for
|
|
|
|
|
that case it is required, not optional.
|
|
|
|
|
|
|
|
|
|
The recorded URL also lives in the *shared* config rather than per-worktree, so
|
|
|
|
|
every existing and future worktree of that clone inherits it until you sync.
|
|
|
|
|
|
|
|
|
|
## Verifying locally
|
|
|
|
|
|
flake: own the dev toolchain, guard the pins, one command to a running app (#4107)
* feat(flake): make the Nix flake own the dev toolchain, and add one command to start
The flake shipped nodejs_22 while package.json declares engines.node
">=24.0.0 <25", .nvmrc pins 24.19.0 and the Dockerfile builds production on
node:24.19.0-alpine3.24. A NixOS developer was running a major the repo does
not support, and nothing said so.
Toolchain:
- node and pnpm are now DERIVED from .nvmrc and package.json's packageManager
rather than named twice. .nvmrc is treated as the authority because it is what
every workflow's actions/setup-node reads and what the Dockerfile tracks.
- flake.lock moved 2026-04-23 -> 2026-08-18 (117 days). At that rev nodejs_24 is
exactly 24.19.0, which is what made agreeing with .nvmrc possible at all.
- pnpm now comes from `pnpm_10`, not the unversioned `pkgs.pnpm`. At the new
rev the unversioned attribute resolves to 11.21.0 -- a major bump that
rewrites pnpm-lock.yaml -- so this bump would otherwise have shipped pnpm 11
to every dev shell silently.
- postgresql_16 -> postgresql_17, matching the primary `db` container. The
postgres/redis/clickhouse entries are CLIENTS for the compose-hosted servers;
that is now stated in the file instead of left to be guessed.
- npm_config_manage_package_manager_versions=false. Measured: without it, pnpm
downloads and re-execs the exact version from the packageManager field, so the
flake's pnpm pin was being defeated at runtime (`pnpm --version` returns
10.28.1 with the var unset, 10.34.5 with it set).
Guards (`nix flake check`, 4 checks):
- toolchain-pins: the flake's node must satisfy engines.node and equal .nvmrc,
and its pnpm must share a major with packageManager. Deliberately does NOT
re-check the .nvmrc/Dockerfile/engines triangle -- node-version-consistency.test.ts
already owns that, and a predicate open-coded twice starts disagreeing.
- prisma-pin: re-derives the resolved @prisma/client AND its engine commit from
pnpm-lock.yaml and compares them to the values flake.nix hardcodes. These were
correct but unguarded: package.json declares `^6.3.0`, a caret range, so a
routine lockfile refresh moves the client while the flake's engines stay put,
and the failure surfaces at runtime in every dev shell.
- pin-guards-selftest: breaks each pin on purpose and requires the guard that
owns it to fire while the others stay silent.
- dev-scripts: builds the shell entrypoints, which is what runs their shellcheck.
(`nix flake check` builds checks.* but only EVALUATES packages.*, measured.)
Entrypoints:
- `nix run .#dev` - docker preflight, submodule, .env.development, compose up,
wait for postgres, pnpm install, then `next dev`. Every step idempotent and
non-destructive; migrations and seeding stay opt-in.
- `nix run .#dev-server` - runs the dev-server CLI on the flake's node. The
daemon re-execs itself with process.execPath, so whichever node starts the CLI
is the node it runs on until it is restarted.
- `nix run .#doctor` - the same pin checks against the working tree.
Compose project is pinned to `civitai` so every worktree shares the one local
stack instead of each spawning a duplicate that fails on the port binds.
* fix(flake): give `nix run` the same env as the dev shell, not just the shell
Found by running the bootstrap on a genuinely clean worktree rather than
reasoning about it. `mkShell`'s `env` applies to `nix develop` only, so both
values it carried were absent from `nix run .#dev`:
- `pnpm install`'s postinstall runs `prisma generate`. Without
PRISMA_QUERY_ENGINE_LIBRARY et al, prisma tried to fetch an engine for
platform `linux-nixos` and the bootstrap died on
`404 ... /linux-nixos/libquery_engine.so.node.sha256`.
- pnpm re-execed itself as 10.28.1 from the packageManager field even though
PATH pointed at the flake's 10.34.5, so the app reported a pnpm the flake had
not pinned.
The env is now one attrset (`devEnv`) rendered two ways: `env` for the shell and
an `export` preamble for the apps, so they cannot drift. `nix run .#dev-server`
gets it too -- the daemon runs `pnpm install` / `db:generate` on its own when it
sees the lockfile move, which would have hit the identical 404.
* docs: describe the toolchain the repo actually has, not the one it used to
Every claim below was checked against the code before rewriting, and the
measurements are quoted where they are load-bearing.
README.md
- "Node.js (version 20 or later)" -> 24.19.0, with .nvmrc named as the authority.
- `make init` was DEAD, not merely awkward: it ran `npm i`, and package.json's
`preinstall` runs `only-allow pnpm`, which exits 1 under an npm user agent
(measured, with the pnpm-user-agent control exiting 0). Both bootstrap paths
the README offered went through it.
- MinIO console is on :9001, not :9000 (:9000 is the S3 API). The instructions
sent people to the wrong port to mint the keys the next step needs.
- `git submodule update --recursive` -> `--init`; without `--init` it is a no-op
on a fresh clone, which is precisely when it is being run.
- Data Migrations step 1 pointed at `schema.prisma`, which is gitignored and
regenerated from `schema.full.prisma` on every `db:generate`, so edits to it
were silently discarded.
- Adds the Nix path (`nix run .#dev`) and a real non-Nix sequence.
- engines.node is ADVISORY, stated plainly: pnpm 10.34.5 under node 26.7.0
against ">=24.0.0 <25" prints `WARN Unsupported engine` and exits 0. An
earlier draft of this very README claimed it refuses. It does not, and that is
the reason the drift survived so long.
Makefile
- `npm i` -> `pnpm install` (see above). `npm-install` kept as an alias.
- `gen-prisma` ran a bare `prisma generate`, which reads the gitignored slim
schema that does not exist yet on a fresh clone; now `pnpm run db:generate`,
which generates it first.
- `dev` ran bare `cross-env`/`next`, requiring the caller to put
node_modules/.bin on PATH by hand; now via `pnpm exec`.
- `docker-compose` (EOL v1) -> `docker compose`.
- COMPOSE_PROJECT_NAME pinned to `civitai`. Reproduced first: `make start` in a
worktree died with `Bind for :::15434 failed: port is already allocated`
because compose named the project after the directory.
.envrc.example (new, tracked) + .gitignore
- `.env*` matched `.envrc` too, so nothing tracked in the repo mentioned the
flake at all -- the only reference was a line in CLAUDE.md filed under
worktree hygiene. Placeholders only; the real .envrc stays ignored.
.claude/skills/dev-server/SKILL.md
- The skill said nothing about node. The daemon is spawned with
`process.execPath` (cli.mjs:66, console.mjs:87) and hands its env to every
`next dev` it supervises, so the first shell to run a CLI verb decides the
node for everything, indefinitely. Measured on this box: daemon on 26.7.0,
with no pnpm on PATH at all. Documents `nix run .#dev-server` and how to check.
- `npm run dev:daemon` -> `pnpm run dev:daemon`, in a repo that bans npm.
src/__tests__/node-version-consistency.test.ts
- Comment-only. It said flake.nix "is on a different major" and could not be
aligned because the pinned nixpkgs had no Node 24 this new. Both halves are
now false, and a comment a maintainer might act on is worth correcting.
Also: docs/pnpm-migration.md's "Node.js 18.x or later"; the generated-header
line in scripts/generate-slim-schema.js telling readers to run `npm run
db:generate`; CLAUDE.md's local-dev section (no node version, no services) and
its stale "flake's 22.22.2" figure.
NOT changed, because it could not be exercised here: the devcontainer pins
typescript-node:1-22 (Node 22, outside engines.node). Flagged in README with the
tag to use -- there is no `1-24`, the template major moved on, so `3-24`.
* docs(flake): the four postgres containers are not all one version
prisma-pit and db are postgres 17; notification-db and logical-db are 15. The
comment justifying postgresql_17 read as though they were uniform, which would
have made the next person's version decision from the wrong premise.
* docs: keep the non-Nix path the default, demote the flake to optional
The flake is used by one maintainer. Everyone else uses Docker + nvm, and that
has to stay the path a contributor lands on. The previous revision inverted
that: README's Installation section led with "With Nix (recommended...)" and
titled the standard path "Without Nix" — framing the majority workflow as the
fallback. CLAUDE.md opened "From nothing to a running app, one command:" with
`nix run .#dev`, and the dev-server skill led its fix with "Start it through the
flake and this cannot happen".
None of that made Nix *required* — verified: `.github/` is untouched by this
branch, no workflow references Nix (the apparent hits are substrings of
`eslint-unix.json` and `--format unix`), and `nix flake check` is not wired to
any CI gate. It was purely an ordering-and-emphasis problem, which is the kind
that costs a new contributor twenty minutes before they find the section that
applies to them.
Changes, all editorial:
- README: `#### Standard setup` now precedes `#### Optional: Nix flake`, and the
Nix section opens with a blockquote saying it is not the supported default,
that nothing requires it, and why it exists at all (NixOS has no published
`linux-nixos` Prisma engine, so a flake is the practical way to work there).
The signals/buzz instructions lead with `docker compose up -d` and mention
`nix run .#dev -- --full` parenthetically.
- CLAUDE.md: the bootstrap block is now the nvm/docker sequence, labelled as the
default path, with the flake shown after it as NixOS-only and explicitly
flagged as something not to assume a contributor has. The dev-server step no
longer instructs going through `nix run .#dev-server`; it states the
requirement (a shell whose node matches `.nvmrc`) and notes the flake does that
for you on NixOS.
- dev-server SKILL.md: the fix is now stated setup-agnostically — start the
daemon from a shell whose node matches `.nvmrc` with pnpm on PATH, which
`nvm use` gives you — with the flake wrapper presented as the optional NixOS
convenience, and an explicit note that nothing in the document depends on Nix.
No behaviour, tooling or gate changes: the Makefile, flake, guards and their
tests are untouched by this commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-19 19:48:54 -05:00
|
|
|
All of these need the repo's own toolchain — node `24.19.0` (the version in
|
|
|
|
|
`.nvmrc`) and pnpm 10.x. **Nothing stops you running them on the wrong node**:
|
|
|
|
|
`engines.node` is advisory, so `pnpm install` prints `WARN Unsupported engine`
|
|
|
|
|
and continues. That is why this is worth stating rather than leaving to the
|
|
|
|
|
tooling — the wrong major shows up as spurious test failures attributed to your
|
|
|
|
|
branch, not as an error at install time. On NixOS there is a second, louder
|
|
|
|
|
failure: outside the dev shell there are no `PRISMA_*_ENGINE_*` paths, so Prisma
|
|
|
|
|
goes looking for a `linux-nixos` engine that has never been published.
|
|
|
|
|
|
|
|
|
|
`nvm use` picks the right node up from `.nvmrc`; with Nix, prefix any of these
|
|
|
|
|
with `nix develop -c`, or use direnv (`cp .envrc.example .envrc && direnv allow`).
|
|
|
|
|
|
2026-07-28 17:44:35 -06:00
|
|
|
```bash
|
|
|
|
|
pnpm typecheck # full repo
|
|
|
|
|
pnpm test:unit:run # ~8,900 unit tests, node env
|
fix(tests): unzero the `preview / component-tests` tier, and make a zero-collected run say so (#4531)
* fix(tests): unzero the component tier — one mock factory was aborting the whole run
`preview / component-tests` was reporting `failure` on `main` while executing ZERO
tests. Reproduced deterministically (3/3 at d353f785c3, and in isolation): the whole
`component` project aborts before any reporter prints, with no `Test Files` line, no
per-file results, and exit 1.
Root cause, one file:
`src/tests/pages/apps/review/review-queue-nav.browser.test.tsx:30` mocks
`~/providers/FeatureFlagsProvider` with a WHOLESALE factory naming only
`useFeatureFlags`. The review queue page's row now renders the review entry point,
which reads flags through `useOptionalFeatureFlags`, so the named import has nothing
to bind to:
SyntaxError: The requested module '/src/providers/FeatureFlagsProvider.tsx'
does not provide an export named 'useOptionalFeatureFlags'
In the node `unit` project that would fail ONE file. In BROWSER mode it kills the run:
vitest resolves a manual mock over the browser-to-node channel inside a Playwright
route handler that does not catch (`@vitest/browser-playwright/dist/index.js`,
`await module.resolve()` inside `page.route`), so the rejection escapes as an
Unhandled Rejection in the orchestrator.
The printed error names neither the file nor the real cause. It is wrapped twice --
once by the browser mocker, once again on the node side -- and the innermost `cause`
is dropped in transit, so all you see is the generic "[vitest] There was an error when
mocking a module ... make sure there are no top level variables inside", which points
at hoisting and is wrong. The root cause above was recovered by temporarily patching
`createHelpfulError` in the browser tester bundle to inline `cause.stack`; the file was
then confirmed by bisecting the 50 candidate files down to one.
This is the SECOND time this class has bitten (see the header of
`src/components/AppBlocks/__tests__/featureFlagsMockCompleteness.test.ts`, which fixed
six sibling suites in the AppBlocks directory and deliberately scoped its guard there).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(ci): make the component tier fail LOUDLY on zero collected, not silently
The `preview / component-tests` tier could report `failure` having executed ZERO
tests, and nothing anywhere said so. The shared `npm-report-only-suite` Tekton task
computes its verdict from the runner's EXIT CODE alone, so an abort that collected
nothing and a genuine list of red assertions both render as `component:fail` /
"Component suite failed" -- same words, same colour, same place. That is the shape
that trains people to click through a tier.
`pnpm test:component` now runs through `scripts/test-component-run.mjs`, which asks
vitest for a JSON report and hands it to `scripts/ci/assert-component-suite-ran.mjs`.
That gate prints a ledger -- `N executed, N skipped, across N files; N failed suites,
N failed tests` -- and fails when nothing was collected, or when the executed count
falls below a floor (1240, ~55% of the 2254 measured on a full green run of 201 files
on 2026-08-31).
Deliberate limits, each of which is a way this could have been wrong:
- It can only ever ADD a failure. Vitest's own exit code is passed straight through
when the ledger is satisfied, so a red suite stays red for its own reason.
- EXECUTED, not TOTAL. `numTotalTests` counts skipped tests, so a suite that
self-skipped wholesale would satisfy a total-based floor having run nothing.
- `failed` counts as executed. A guard that scored a red run as "did not run" would
fire on every genuine failure, and the tier would then be red for two different
reasons that nobody could tell apart -- the exact confusion this removes.
- A signal-killed runner short-circuits the gate entirely. The CI task wraps this in
`timeout(1)` and distinguishes "timeout" from "fail" on purpose; a killed run also
writes no report, so relabelling it "collected nothing" would be a wrong answer
rather than a missing one.
- A narrowed run (a file argument, or `-t`) skips the FLOOR but not the zero check:
the collected count is then a property of the filter, but a single-file run that
collects nothing is the cheapest reproduction of the abort and is precisely when
someone is debugging it.
The message enumerates every cause it cannot tell apart rather than asserting one --
an absence is the observable the most causes share, and a guard that names the wrong
one sends the next reader hunting a bug that is not there. All three named have been
observed on this suite; two of them were observed while writing this change, and the
browser-crash one arrives wearing the mock error's headline with the real cause on
the `Caused by:` line.
Verification:
- 16 unit tests over JSON fixtures (`scripts/__tests__/`), covering zero, the floor
boundary both sides, all-red, all-skipped, narrowed both ways, import-failed
suites, and a missing/unparseable report.
- Mutation-checked: 9 mutants of the gate, ALL KILLED, each by the specific named
test written for it (verified per-test, not "some test failed"), against a green
16/16 baseline.
- End-to-end negative control: reverting the one-line fix in the previous commit and
running through the wrapper produces the ledger's abort message and exit 1, from
the missing-report branch.
- End-to-end positive: the full suite through `pnpm run test:component`.
Docs corrected while here: CONTRIBUTING said component tests "don't run in CI at all"
and put the file count at 106; they do run, report-only, in the preview pipeline, and
there are 201 files with ~2,250 tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(tests): the zero-collected message overstated a PARTIAL abort
The headline said "THE COMPONENT SUITE COLLECTED NOTHING ... nothing executed", and
the code contradicts it in the case that actually happens most: the JSON reporter
writes at the END of a run, so an abort part-way through leaves no report at all.
Measured while writing this: a run that aborted 68 files into 201 had 68 files
scrolled past as green, wrote nothing, and got told nothing executed.
Both halves of that were wrong to assert. Tests HAD executed, and a reader who
scrolls up and sees green lines is entitled to believe the message is confused --
or, worse, to believe the green lines are coverage.
So the headline is now "THIS RUN PRODUCED NO ACCOUNTABLE RESULT", which is true of
both shapes, and the diagnosis says explicitly that an abort can land part-way and
that whatever scrolled past is unaccounted for rather than confirmed. The
missing-report branch says why the report is absent (the reporter writes at the end)
and that a partial run and a run that never started are indistinguishable from
there -- which is the reason neither counts as one that ran.
No behaviour change: the same runs fail, with the same exit code. The four test
assertions that pinned the old headline move with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(tests): close six findings from the adversarial audit of this PR
An adversarial audit of #4531 found two ways the new guard reproduced the very
pathology it was written to remove, plus four smaller gaps. All six are fixed here;
nothing about the payload fix in c32aa06e8d changes.
1. A SIGNAL WAS RELABELLED AS A TEST FAILURE. `child.on('exit')` returned a hardcoded
143 for EVERY signal. `report-only-suite-task.yaml` branches on 137 to report
`oom-killed` -- "this is an OUT-OF-MEMORY kill, not a timeout; raise the task's
memory limit" -- and before this wrapper existed an OOM-killer SIGKILL on vitest
reached that task as 137, because pnpm re-raises. With a constant 143 it matched no
branch and fell through to `RC=1`, verdict `fail`, rendered "Component suite
failed". A memory problem reported as a test failure, on the same tier, in the same
words. Now `128 + os.constants.signals[sig]`, so 137/143/130 come out right and the
mapping cannot drift from the names node hands back. The comment above the branch
claimed it passed the status through; it does now.
2. A CALLER `--outputFile` MADE A GREEN RUN REPORT "COLLECTED NOTHING". Measured by
the auditor: `pnpm test:component <file> --outputFile=/tmp/x.json` wrote an 18/18
green report to the caller's path, left the wrapper's path empty, and printed the
full "this tier verified NOTHING on this commit" diagnosis naming three causes,
none of them real. `.github/workflows/lint.yml` runs the SIBLING unit tier with
exactly that flag, so it is a copy-paste away. Now refused up front with the fix in
the message.
3. `isNarrowed` WAS WRONG IN BOTH DIRECTIONS ON SPACE-SEPARATED FLAGS. `--max-workers 1`
-- the form CONTRIBUTING steers people towards for sizing a run on a shared box --
put `1` in a positional slot, so the run scored "narrowed" and THE FLOOR WAS SILENTLY
TURNED OFF; same for `--reporter`, `--retry`, `--bail`, `--project`, `--pool`. In the
other direction `--shard=1/4` scored NOT narrowed, so a healthy sharded run would
fail the floor while being told "Do NOT lower the floor to make this green" --
misdirection, not merely a false red. Value-taking flags now consume their value, and
`--shard`/`--changed`/`--related` narrow explicitly.
4. WINDOWS. `spawn()` of `vitest.cmd` without `shell` has failed since the node
18.20.2/20.12.2 CVE fix; `scripts/test-unit-run.mjs` already sets
`shell: process.platform === 'win32'` for this reason. Added. And a spawn failure
(rc 127) now short-circuits instead of handing the gate a missing report and getting
forty lines about mock factories and dead browsers.
5. THE REPORT IS NOW DELETED BEFORE THE RUN, not only after. Cleanup after the run is
skipped by exactly the paths that leave a stale report behind (a signal death, a
throw), so a healthy 2254-test report could survive into a later run that aborted
before writing one -- the gate reading it, printing a green ledger, and passing:
silently inert in precisely its own use case.
6. THE GATE COMPUTED A FILE COUNT AND NEVER CHECKED IT. Replaced with a LEDGER, which
is the stronger of the two checks: it walks `src/` for every `*.browser.test.tsx` and
fails when one is absent from the report, NAMING it. The test floor sits at ~55%, so
~45% of the suite could stop being collected while the gate stayed green -- and the
incident this whole guard descends from is exactly that shape (six files contributing
0 of 438, nothing red). The expectation is re-derived every run, so there is no
constant to go stale. Skipped when narrowed, and when the walk finds nothing -- an
empty walk is not a measurement, and it SAYS so rather than passing quietly.
Also: the fixed factory now names all THREE hooks the flags module exports.
`useFeatureFlagsReady` is the third and has four live consumers; none is in this page's
graph today, which is the only reason naming two loads at all. The comment said "BOTH
hooks", which reads as "the module has exactly two" -- and it is the template the PR
proposes copying to fifty files.
Verification:
- 27 unit tests (was 16), including the on-disk ledger against a fixture tree that
contains a `node_modules/` and a non-browser `.test.tsx` neither of which may count.
- Mutation-checked: 17 mutants, ALL KILLED, each by its own named test. One SURVIVED on
the first sweep -- `out.length > 0 ? out : null` was unreachable from the fixture,
which had no `src/` at all, so the walk returned early. A second case with a `src/`
that holds no browser tests reaches it; an empty array is TRUTHY, so that mutant would
otherwise have armed a ledger over zero expected files and passed everything.
- The on-disk ledger run against the REAL tree: 201/201 green, and dropping one real
file from the report fails and names it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(tests): close audit round 2 — two regressions the round-1 fixes introduced
A delta re-audit of `6ceac37f33..7f005f0482` confirmed 4 of the 7 claims outright
(the signal mapping, the pre-run clear, the file ledger, the fixed factory) with its
own positive and negative controls, and found that two of the fixes had introduced
new defects of their own. Both are here, with the smaller findings.
🔴 THE ON-DISK LEDGER FALSE-FAILED A LEGITIMATE RUN, WITH A MESSAGE FORBIDDING THE FIX.
`--exclude`, `--dir` and `--root` were added to VALUE_FLAGS but not to
NARROWING_FLAGS. All three take a value AND genuinely shrink the collected file set,
so `pnpm test:component --exclude 'src/tests/**'` scored as a FULL run and the ledger
failed it naming up to 200 files -- asserting the include broke or the run died, and
telling the reader "Do NOT silence this by narrowing the walk", which is the only
thing that would have fixed it. `--config` joins them: it can replace the project's
`include` outright, which is the assumption the walk is built on. This is precisely
the shape the round was convened to prevent, produced by the round's own fix.
🔴 `shell: win32` WENT ON THE SHARED `run()`, SO IT ALSO WRAPPED THE GATE SPAWN.
The claim said this matched `scripts/test-unit-run.mjs`; it did not -- that file puts
`shell` on its vitest spawn only and deliberately leaves its `process.execPath` spawn
alone. Node with `shell: true` concatenates argv UNESCAPED (DEP0190), and the gate is
spawned as `process.execPath`, which on Windows is `C:\Program Files\nodejs\node.exe`.
So on the one platform the option exists to support, every `pnpm test:component` would
have failed at the gate step with a cmd.exe parse error rather than any of this
wrapper's messages. `shell` is now per-call.
Also:
- KEBAB AND CAMEL ARE THE SAME FLAG TO VITEST (cac camelCases every option key), so a
hand-enumerated list has a hole wherever it carries one spelling and not the other --
and it did: `--max-workers`/`--maxWorkers` were both listed, `--test-timeout` was
not, so the kebab form silently disabled both checks. Spellings are now canonicalised
rather than enumerated, and the test asserts them in PAIRS.
- `--output-file` (kebab) was NOT refused, so the exact defect the refusal exists for
was still reachable -- under a test named "catches every spelling". Meanwhile
`--outputFile.junit=` and `--outputFile.html=` WERE refused, though neither touches
the `.json` key: object-form output paths are per-reporter, so that was over-strict
and the stated reason ("the bare form sets the path for EVERY reporter") is true only
of the bare form. Both directions fixed.
- The rc-127 short-circuit is keyed on a spawn-failure FLAG, not on the number. 127 is
an exit code a runner can produce on its own, and relabelling that as "the binary
could not be started" is a second wrong answer that also skips the gate on a run that
happened.
- `--repo-root <dir>` did not consume its value, so with the flag FIRST the directory
was read as the report path: "EISDIR: illegal operation on a directory" plus the
whole abort diagnosis. Every test passed it last, which is why nothing caught it; the
ledger tests now pass it first.
- `--related` removed. It is a vitest SUBCOMMAND, and this wrapper always spawns
`vitest run …`, so it cannot arrive -- the entry and its assertion were both inert,
the assertion pinning behaviour for an input the runner cannot receive.
- The factory now names all FOUR runtime exports of the flags module, including
`FeatureFlagsProvider`. The comment said "EVERY runtime hook", which was narrower
than the module -- and "not in this graph today" is exactly the reasoning that put
this file in the diff.
- A comment the previous round made false: it said the positional rule would catch
`-t foo` anyway. It stopped being true the moment `-t` was listed as value-taking --
the value would now be CONSUMED -- so the explicit narrowing branch is load-bearing.
- CONTRIBUTING documents the refusal, the file ledger, and which flags narrow.
Coverage for the two claims that shipped with none: `main` now takes its collaborators
as injected defaults, so the ORDER of effects can be asserted rather than inspected.
The two `clearReport()` calls are byte-identical statements -- only their position
carries the meaning, so a refactor moving the first below the run reopens "a stale
report satisfies the next run's ledger" with every other test green.
Verification: 35 unit tests (was 27). Mutation-checked at 17 mutants, ALL KILLED, zero
survivors -- including re-running round 1's guards, because an audit fix resets the
gate. Real-tree controls re-run with the new argument parsing, flag first and flag
absent: 201/201 green, and dropping one real file fails and names it. ESLint on the
touched files is back to the base count; two `no-empty-function` errors this change
introduced are fixed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(tests): close audit round 3 — the canonicaliser re-opened the class it closed
Round 3 verified 8 of round 2's 9 claims against the diff or against vitest's own
source (it read cac's `setDotProp` to confirm the `--outputFile.junit=` allowance is
genuinely safe, and `cliOptionsConfig` to confirm `-r`/`-c` really are root/config).
It found one regression and one coverage hole, both here.
🔴 A BOOLEAN FLAG SWALLOWED THE FILENAME AFTER IT — the same shape round 2 fixed,
re-introduced by round 2's own mechanism. `canonicalFlag` collapsed `.subkey` onto the
parent, so every dot-subkey inherited its parent's value-consuming behaviour, and the
two entries `--coverage.reporter`/`--coverage.provider` became a single `coverage` that
also matched the BARE `--coverage` — which takes no value (`argument: ""` in vitest's
`cliOptionsConfig`). So `pnpm test:component --coverage <file>` ate the FILE as
`--coverage`'s value, scored the run as full, and failed an 18/18 green single-file run
naming ~200 files as ABSENT while telling the reader not to narrow the walk. Same for
`--coverage.enabled <file>` and `--browser.headless <file>`. Measured base-vs-head, all
three flipped `true` to `false`. Matching is now on the full canonical PATH, so a
subkey is value-taking only if it is listed as one.
🔴 THREE MUTANTS SURVIVED A GREEN 35-TEST SUITE, AND EACH WAS THIS PR'S OWN HEADLINE
FAILURE. The injected fakes took no parameter, so nothing could observe the argv `main`
builds — the seam between the two modules this change exists to wire together. Dropping
`--narrowed` from the gate argv makes every narrowed run hard-fail both the floor and
the on-disk ledger; dropping `--outputFile.json=` makes every run report "does not
exist" plus the whole abort diagnosis; dropping `...argv` makes
`pnpm test:component <file>` silently run the entire suite behind a healthy-looking
ledger. Testing `isNarrowed` in isolation cannot see whether its answer is ever USED.
The fakes now capture their argument and three cases assert it; all three mutants die.
Also:
- `--repo-root` with no value was a silent fall-through to the real repo root, so a
fixture report got graded against the 201 real files and failed with a confident
diagnosis about the include breaking — produced by a typo. Now a usage error (exit 2).
- CONTRIBUTING: the sample ledger omitted `measured <date>`, which the real output
carries; and "skips both checks but not the zero check" counted a different pair than
the two enumerated four lines above. The checks are now numbered and the sentence
names which ones a narrowed run skips.
Verification: 39 unit tests (was 35). Mutation-checked at 19 mutants, ALL KILLED, zero
survivors — the three that survived round 3, the round-3 fixes themselves, and every
guard from rounds 1 and 2 re-run, because an audit fix resets the gate. Real-tree
controls re-run: 201/201 green, one missing file still fails. ESLint clean on the
changed test file.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(tests): close audit round 4 — replace the flag LIST with a shape rule
Round 4 measured `isNarrowed` at base vs head across every one of vitest 4.1.11's 72
boolean options and 73 value-taking ones, and found the flag list had traded 22 loud
wrong answers for 25 quiet ones. Splitting `coverage` into two subkeys left
`--retry.count 2`, `--browser.name chromium` and sixteen more `coverage.*` paths
reading their VALUE as a filename, scoring a FULL run as narrowed and switching the
file ledger and the floor off with a one-line note. That is the direction this file's
own comments repeatedly name as the worse of the two.
🔴 THE LIST WAS THE PROBLEM, AND THREE ROUNDS WERE SPENT ON IT. Enumerating flag NAMES
missed `--test-timeout` next to `--testTimeout`. Canonicalising spellings then made
`--coverage <file>` swallow the file. Splitting into subkeys produced the 25 above.
vitest 4.1.11 has a 164-path option tree; a hand-maintained copy of it is wrong the day
it is written, and each fix moved the wrongness rather than removing it.
So the rule is now about the ARGUMENT, not the flag: a positional is a file filter if
it looks like a path, or if nothing before it could have been expecting a value. A
non-path token straight after a flag is that flag's value, WHATEVER the flag is —
correct for all 73 value-taking options without naming one of them, and correct for all
72 booleans too. `VALUE_FLAGS` is deleted. What remains is `NARROWING_FLAGS` (flags that
genuinely shrink the run and must be named, because omitting one fails LOUDLY) and a
five-entry `PATH_VALUE_FLAGS` for the only residual the shape cannot decide: a value
that is itself a path.
🔴 AND THE DECISION IS NOW A SENTENCE, NOT A BOOLEAN. No rule over an unknowable flag
list is right always; what must never happen is being wrong SILENTLY, because
`--narrowed` disables the two checks this whole change exists to add. `narrowingReason`
returns why, and the runner prints it: `--retry.count 2` reading `2` as a file filter is
obvious on sight and invisible otherwise.
Also:
- `canonicalFlag` camelCases only the FIRST dot segment, matching cac's own
`camelcaseOptionName` (`name.split(".").map((v,i) => i===0 ? camelcase(v) : v)`).
Camel-casing all of them made this wrapper accept `--coverage.reports-directory`,
a spelling vitest does not — so the two would disagree about the next token.
- `--repo-root=<dir>` is parsed. Matching only the space form left the inline spelling
falling through BOTH branches to the real repo root — byte-for-byte the failure the
missing-value guard was added to close, reachable through one extra character. A
value that is itself a flag is rejected too.
Verification: 45 unit tests (was 44 after the round's own additions; 39 before).
Mutation-checked at 23 mutants, ALL KILLED, zero survivors, each by its OWN named test.
🔴 THE FIRST SWEEP REPORTED FOUR SURVIVORS AND WAS WRONG ABOUT ALL FOUR, WHICH IS WORTH
RECORDING BECAUSE THE HARNESS HAD THIS PR'S OWN DEFECT. Two were mislabelled — killed by
a different test than the one named, which the sweep scores as SURVIVED. Of the other
two, one was a real gap (`--reporter=json AppNameCrumb`, now covered) and one was
`canonicalFlag`'s dot handling, which is equivalent for every input that has no dashed
subkey (also now covered). Along the way the harness itself was found reusing a JSON
report path without clearing it — the same stale-report defect fixed in the product two
commits ago — and now clears it and verifies the mutation reached disk before running.
A mutant reported SURVIVED is a claim about the instrument until the instrument has been
controlled.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* wip(tests): stop guessing which vitest flags take values
Safety commit of in-progress round-5 rework so it is not lost; the agent was
interrupted mid mutation-sweep. Verification is NOT complete — the sweep, the
red/green pair and the merged-tree re-run have not been reported. Do not merge
on this commit.
Round 5 measured both prior approaches against vitest 4.1.11's real option
table (170 long options, 74 boolean, 96 value-taking): the hand-maintained
list was wrong 73 times, the shape heuristic 74. The error count never moved,
only its direction. This removes the question instead of answering it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AEa6GDJyTiu2R146ndYsLK
* test(ci): finish the verification bacb8a2cf5 was pushed without
bacb8a2cf5 landed as a rescue commit marked `wip` because the session that wrote it
died with the change only in a working tree. The change itself is unaltered; this is
the verification it was missing, plus the merge of a main that moved twice underneath
it. Nothing here modifies the rule.
WHY THE RULE CHANGED, WITH THE NUMBER THAT JUSTIFIES IT. Round 5 of the audit
enumerated vitest 4.1.11's REAL CLI option table -- by calling `createCLI()` and reading
each cac option's `isBoolean` rather than by hand -- and ran `isNarrowed(['--<flag>',
'VALUE'])` against both revisions imported side by side:
hand-maintained flag list wrong on 73 options (every value-taking one) QUIET
shape heuristic wrong on 74 options (every boolean one) LOUD
73 versus 74. The heuristic did not beat the list; it moved the wrongness off one half
of the table onto the other. The direction improved, which is worth something, but the
error count did not -- and `pnpm test:component --coverage AppNameCrumb` would run one
test and then be failed against the 1240 floor with "the include broke or the run died".
That is a mis-posed question, so it is no longer asked: any argument at all means
narrowed.
Re-measured against the same table on the merged tree, with the same method:
164 long options (72 boolean, 92 value-taking)
UNSAFE-LOUD -- rule claims FULL so the floor and ledger fire on a partial run: 0
no-argument invocation (what CI runs): NOT narrowed, so all three checks arm
(My enumeration walks the global + `run` commands and sees 164/72/92 where round 5's
saw 170/74/96; the traversal differs slightly, the conclusion does not.)
🔴 The comparison is one-directional ON PURPOSE, and the other direction is a real cost
rather than a rounding error: all 164 options now score as narrowed, so an arg-ful run
does not get the floor or the file ledger even when it was genuinely full. That is
affordable for one measured reason -- `pr-preview-pipeline.yaml` invokes
`pnpm run test:component` with NO arguments, so CI is the `argv.length === 0` path and
always gets all three. What is given up is enforcing a floor on an ad-hoc local run.
`VITEST_MAX_WORKERS=4` in the environment sizes a run without giving that up.
Verification on the MERGED tree (d77dd394db), all at load <= 16 with no browser-session
errors in any run quoted:
no-arg full run (the CI path) 202 files / 2257 tests, exit 0, no NARROWED line,
floor 1240 and the 202-file ledger both ARMED
guard red arm exit 1, "THIS RUN PRODUCED NO ACCOUNTABLE RESULT",
failing on the missing-report branch
guard green arm exit 0, "2 executed, 0 skipped, across 1 files"
gate unit tests 37 passed
mutation sweep 20 mutants, ALL KILLED, zero survivors
🔴 The red arm was run NARROWED, deliberately: it proves `--narrowed` disables the floor
and the ledger and NOT the zero-collected check, which is the one failure this whole
change exists to catch.
THE MERGE. main moved twice; both times the only conflict was `package.json`, and both
times it was the same semantic hazard -- main editing `test:lint-rules` and this branch
editing `test:component`, adjacent lines of one object, where taking either side
wholesale silently reverts the other and makes the entire guard inert with every test
green. Resolved by parsing the merged JSON, not by reading the diff: `test:lint-rules`
now matches origin/main byte-for-byte and `test:component` is the wrapper. The wiring
guard added for exactly this was then checked against the bad resolution -- applying
main's `package.json` wholesale fails one test, the one written for it.
Round 5's two remaining 🟢 items are closed by the rework itself rather than separately:
the stale JSDoc described `VALUE_FLAGS`, which no longer exists, and the test whose
description claimed "a BOOLEAN flag does not swallow the filename after it" while
passing entirely through the path check is gone with the heuristic it tested. That one
was load-bearing -- a description asserting coverage its body did not provide is what
let the class through -- so its replacement asserts a property with no free parameters
instead, across both halves of the option table.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* style(tests): format the gate test file
The rescue commit was pushed before a prettier pass could run over the last edit to
this file, and `ESLint + Prettier (changed files)` caught it: the file is ADDED by this
PR, so it is covered by the added-files prettier gate. One line.
This red was MINE, not inherited — unlike `Unit tests (1)/(2)` (a redis integration
test that arrived from main) and `preview / smoke-tests` (#4516 changing
`reaction.toggle`), both of which are verified as pre-existing and are left alone.
CONTRIBUTING.md also reports unformatted, and that one is NOT actionable here: it is
already unformatted on origin/main, and it is modified rather than added, so the gate
(which reads `added.txt`) does not cover it. Reformatting it would bury this PR under an
unrelated whole-file diff.
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-01 10:57:49 -05:00
|
|
|
pnpm test:component # ~2,250 component tests in real Chromium, slower
|
2026-07-28 17:44:35 -06:00
|
|
|
pnpm exec prettier --check <files you added>
|
|
|
|
|
pnpm exec eslint <files you added>
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
Use `pnpm exec prettier --write <file>`, **not** `pnpm prettier:write -- <file>`.
|
|
|
|
|
The latter ignores the argument and reformats the entire repository.
|
|
|
|
|
|
perf(tests): uncap vitest workers, cap via VITEST_MAX_WORKERS (#3953)
* perf(tests): uncap vitest workers, cap via VITEST_MAX_WORKERS
The flat cap of 8 (#3900) existed because several agents each running a full
suite at once saturated the box. The dev-server test queue now serialises
full-suite runs at concurrency 1 (#3947), so one suite has the machine to
itself and can use all of it.
Measured on a 32-core Windows box, alternating capped/uncapped runs through
that queue: 8 workers 507.3s / 526.9s, uncapped (31 workers) 281.4s / 295.8s.
Every run reported the same 16 failed / 16738 passed across 6 files (the known
Windows path-separator baseline) and the same 347 tests collected from
blocks.router.workflow.test.ts.
VITEST_MAX_WORKERS is the name Vitest itself honours, so the number is the same
whichever layer reads it; it is read here as well because getThreadsCount
consults only the project's own maxWorkers, never the root, so the browser
project has to be handed it individually.
CI is unaffected: it runs on 4-vCPU ubuntu-latest, where the old cap's
`cpus > 9` guard already made it inert.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(tests): correct what the worker knob does and where it applies
Review of #3953 found the documented escape hatch was inert on the machine it
matters on. With CIVITAI_TEST_QUEUE set, test:unit:run hands the run to the
dev-server daemon, which spawns it with the daemon's environment, so
VITEST_MAX_WORKERS=8 pnpm run test:unit:run runs at the full pool while reading
as capped. The CLI flag is forwarded through the queue and does work; verified
with a 40-file probe recording VITEST_POOL_ID (3 distinct workers under
--max-workers=3, against 31 unset).
Three further corrections, all to claims rather than behaviour:
- The number is not clamped to the browser pool's min(12, cpus - 1).
getThreadsCount returns project.config.maxWorkers unchanged, so a value above
12 raises the Chromium instance count. It sizes the pool, it does not only
shrink it.
- Only test:unit:run is queued. test:component, test:packages:run,
test:apps:run and test:lint-rules call vitest directly, so a queued unit run
and an unqueued component run still overlap.
- The comment justifying COMPONENT_GROUP_ORDER by a groupSpecs throw is stale:
with no per-project maxWorkers that differs from the others, the throw can no
longer happen. The group is kept for what it still does — serialising the two
projects in a bare vitest run.
Also notes that a CPU-quota container resolves workers from host cores, which
nothing in this repo invokes but a pipeline outside it might.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-14 23:40:09 -06:00
|
|
|
Both suites use Vitest's own worker count (`cpus - 1`, or `min(12, cpus - 1)` for
|
|
|
|
|
the browser one). To leave the machine usable while a suite runs, size it for that
|
|
|
|
|
run with `--max-workers=8`, or with `VITEST_MAX_WORKERS=8` in the environment. The
|
|
|
|
|
number applies to every project and is not clamped, so a value above 12 raises the
|
|
|
|
|
Chromium instance count rather than lowering it.
|
|
|
|
|
|
2026-07-28 17:44:35 -06:00
|
|
|
### Compare against a baseline, not against zero
|
|
|
|
|
|
|
|
|
|
`pnpm test:component` can report two extra failing files on a cold
|
|
|
|
|
`optimizeDeps` cache, both `Vitest failed to find the runner`. Adding any
|
|
|
|
|
`*.browser.test.tsx` perturbs `optimizeDeps.entries` — see the comment at
|
|
|
|
|
`vitest.config.mts:98-124`. Re-run and it settles. If a file passes in isolation,
|
|
|
|
|
that's what happened.
|
|
|
|
|
|
|
|
|
|
The reliable method for any of these commands is to run it on unmodified `main`
|
|
|
|
|
first, save the output, then compare. A failure that also happens on `main` isn't
|
|
|
|
|
yours.
|
|
|
|
|
|
fix(tests): unzero the `preview / component-tests` tier, and make a zero-collected run say so (#4531)
* fix(tests): unzero the component tier — one mock factory was aborting the whole run
`preview / component-tests` was reporting `failure` on `main` while executing ZERO
tests. Reproduced deterministically (3/3 at d353f785c3, and in isolation): the whole
`component` project aborts before any reporter prints, with no `Test Files` line, no
per-file results, and exit 1.
Root cause, one file:
`src/tests/pages/apps/review/review-queue-nav.browser.test.tsx:30` mocks
`~/providers/FeatureFlagsProvider` with a WHOLESALE factory naming only
`useFeatureFlags`. The review queue page's row now renders the review entry point,
which reads flags through `useOptionalFeatureFlags`, so the named import has nothing
to bind to:
SyntaxError: The requested module '/src/providers/FeatureFlagsProvider.tsx'
does not provide an export named 'useOptionalFeatureFlags'
In the node `unit` project that would fail ONE file. In BROWSER mode it kills the run:
vitest resolves a manual mock over the browser-to-node channel inside a Playwright
route handler that does not catch (`@vitest/browser-playwright/dist/index.js`,
`await module.resolve()` inside `page.route`), so the rejection escapes as an
Unhandled Rejection in the orchestrator.
The printed error names neither the file nor the real cause. It is wrapped twice --
once by the browser mocker, once again on the node side -- and the innermost `cause`
is dropped in transit, so all you see is the generic "[vitest] There was an error when
mocking a module ... make sure there are no top level variables inside", which points
at hoisting and is wrong. The root cause above was recovered by temporarily patching
`createHelpfulError` in the browser tester bundle to inline `cause.stack`; the file was
then confirmed by bisecting the 50 candidate files down to one.
This is the SECOND time this class has bitten (see the header of
`src/components/AppBlocks/__tests__/featureFlagsMockCompleteness.test.ts`, which fixed
six sibling suites in the AppBlocks directory and deliberately scoped its guard there).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(ci): make the component tier fail LOUDLY on zero collected, not silently
The `preview / component-tests` tier could report `failure` having executed ZERO
tests, and nothing anywhere said so. The shared `npm-report-only-suite` Tekton task
computes its verdict from the runner's EXIT CODE alone, so an abort that collected
nothing and a genuine list of red assertions both render as `component:fail` /
"Component suite failed" -- same words, same colour, same place. That is the shape
that trains people to click through a tier.
`pnpm test:component` now runs through `scripts/test-component-run.mjs`, which asks
vitest for a JSON report and hands it to `scripts/ci/assert-component-suite-ran.mjs`.
That gate prints a ledger -- `N executed, N skipped, across N files; N failed suites,
N failed tests` -- and fails when nothing was collected, or when the executed count
falls below a floor (1240, ~55% of the 2254 measured on a full green run of 201 files
on 2026-08-31).
Deliberate limits, each of which is a way this could have been wrong:
- It can only ever ADD a failure. Vitest's own exit code is passed straight through
when the ledger is satisfied, so a red suite stays red for its own reason.
- EXECUTED, not TOTAL. `numTotalTests` counts skipped tests, so a suite that
self-skipped wholesale would satisfy a total-based floor having run nothing.
- `failed` counts as executed. A guard that scored a red run as "did not run" would
fire on every genuine failure, and the tier would then be red for two different
reasons that nobody could tell apart -- the exact confusion this removes.
- A signal-killed runner short-circuits the gate entirely. The CI task wraps this in
`timeout(1)` and distinguishes "timeout" from "fail" on purpose; a killed run also
writes no report, so relabelling it "collected nothing" would be a wrong answer
rather than a missing one.
- A narrowed run (a file argument, or `-t`) skips the FLOOR but not the zero check:
the collected count is then a property of the filter, but a single-file run that
collects nothing is the cheapest reproduction of the abort and is precisely when
someone is debugging it.
The message enumerates every cause it cannot tell apart rather than asserting one --
an absence is the observable the most causes share, and a guard that names the wrong
one sends the next reader hunting a bug that is not there. All three named have been
observed on this suite; two of them were observed while writing this change, and the
browser-crash one arrives wearing the mock error's headline with the real cause on
the `Caused by:` line.
Verification:
- 16 unit tests over JSON fixtures (`scripts/__tests__/`), covering zero, the floor
boundary both sides, all-red, all-skipped, narrowed both ways, import-failed
suites, and a missing/unparseable report.
- Mutation-checked: 9 mutants of the gate, ALL KILLED, each by the specific named
test written for it (verified per-test, not "some test failed"), against a green
16/16 baseline.
- End-to-end negative control: reverting the one-line fix in the previous commit and
running through the wrapper produces the ledger's abort message and exit 1, from
the missing-report branch.
- End-to-end positive: the full suite through `pnpm run test:component`.
Docs corrected while here: CONTRIBUTING said component tests "don't run in CI at all"
and put the file count at 106; they do run, report-only, in the preview pipeline, and
there are 201 files with ~2,250 tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(tests): the zero-collected message overstated a PARTIAL abort
The headline said "THE COMPONENT SUITE COLLECTED NOTHING ... nothing executed", and
the code contradicts it in the case that actually happens most: the JSON reporter
writes at the END of a run, so an abort part-way through leaves no report at all.
Measured while writing this: a run that aborted 68 files into 201 had 68 files
scrolled past as green, wrote nothing, and got told nothing executed.
Both halves of that were wrong to assert. Tests HAD executed, and a reader who
scrolls up and sees green lines is entitled to believe the message is confused --
or, worse, to believe the green lines are coverage.
So the headline is now "THIS RUN PRODUCED NO ACCOUNTABLE RESULT", which is true of
both shapes, and the diagnosis says explicitly that an abort can land part-way and
that whatever scrolled past is unaccounted for rather than confirmed. The
missing-report branch says why the report is absent (the reporter writes at the end)
and that a partial run and a run that never started are indistinguishable from
there -- which is the reason neither counts as one that ran.
No behaviour change: the same runs fail, with the same exit code. The four test
assertions that pinned the old headline move with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(tests): close six findings from the adversarial audit of this PR
An adversarial audit of #4531 found two ways the new guard reproduced the very
pathology it was written to remove, plus four smaller gaps. All six are fixed here;
nothing about the payload fix in c32aa06e8d changes.
1. A SIGNAL WAS RELABELLED AS A TEST FAILURE. `child.on('exit')` returned a hardcoded
143 for EVERY signal. `report-only-suite-task.yaml` branches on 137 to report
`oom-killed` -- "this is an OUT-OF-MEMORY kill, not a timeout; raise the task's
memory limit" -- and before this wrapper existed an OOM-killer SIGKILL on vitest
reached that task as 137, because pnpm re-raises. With a constant 143 it matched no
branch and fell through to `RC=1`, verdict `fail`, rendered "Component suite
failed". A memory problem reported as a test failure, on the same tier, in the same
words. Now `128 + os.constants.signals[sig]`, so 137/143/130 come out right and the
mapping cannot drift from the names node hands back. The comment above the branch
claimed it passed the status through; it does now.
2. A CALLER `--outputFile` MADE A GREEN RUN REPORT "COLLECTED NOTHING". Measured by
the auditor: `pnpm test:component <file> --outputFile=/tmp/x.json` wrote an 18/18
green report to the caller's path, left the wrapper's path empty, and printed the
full "this tier verified NOTHING on this commit" diagnosis naming three causes,
none of them real. `.github/workflows/lint.yml` runs the SIBLING unit tier with
exactly that flag, so it is a copy-paste away. Now refused up front with the fix in
the message.
3. `isNarrowed` WAS WRONG IN BOTH DIRECTIONS ON SPACE-SEPARATED FLAGS. `--max-workers 1`
-- the form CONTRIBUTING steers people towards for sizing a run on a shared box --
put `1` in a positional slot, so the run scored "narrowed" and THE FLOOR WAS SILENTLY
TURNED OFF; same for `--reporter`, `--retry`, `--bail`, `--project`, `--pool`. In the
other direction `--shard=1/4` scored NOT narrowed, so a healthy sharded run would
fail the floor while being told "Do NOT lower the floor to make this green" --
misdirection, not merely a false red. Value-taking flags now consume their value, and
`--shard`/`--changed`/`--related` narrow explicitly.
4. WINDOWS. `spawn()` of `vitest.cmd` without `shell` has failed since the node
18.20.2/20.12.2 CVE fix; `scripts/test-unit-run.mjs` already sets
`shell: process.platform === 'win32'` for this reason. Added. And a spawn failure
(rc 127) now short-circuits instead of handing the gate a missing report and getting
forty lines about mock factories and dead browsers.
5. THE REPORT IS NOW DELETED BEFORE THE RUN, not only after. Cleanup after the run is
skipped by exactly the paths that leave a stale report behind (a signal death, a
throw), so a healthy 2254-test report could survive into a later run that aborted
before writing one -- the gate reading it, printing a green ledger, and passing:
silently inert in precisely its own use case.
6. THE GATE COMPUTED A FILE COUNT AND NEVER CHECKED IT. Replaced with a LEDGER, which
is the stronger of the two checks: it walks `src/` for every `*.browser.test.tsx` and
fails when one is absent from the report, NAMING it. The test floor sits at ~55%, so
~45% of the suite could stop being collected while the gate stayed green -- and the
incident this whole guard descends from is exactly that shape (six files contributing
0 of 438, nothing red). The expectation is re-derived every run, so there is no
constant to go stale. Skipped when narrowed, and when the walk finds nothing -- an
empty walk is not a measurement, and it SAYS so rather than passing quietly.
Also: the fixed factory now names all THREE hooks the flags module exports.
`useFeatureFlagsReady` is the third and has four live consumers; none is in this page's
graph today, which is the only reason naming two loads at all. The comment said "BOTH
hooks", which reads as "the module has exactly two" -- and it is the template the PR
proposes copying to fifty files.
Verification:
- 27 unit tests (was 16), including the on-disk ledger against a fixture tree that
contains a `node_modules/` and a non-browser `.test.tsx` neither of which may count.
- Mutation-checked: 17 mutants, ALL KILLED, each by its own named test. One SURVIVED on
the first sweep -- `out.length > 0 ? out : null` was unreachable from the fixture,
which had no `src/` at all, so the walk returned early. A second case with a `src/`
that holds no browser tests reaches it; an empty array is TRUTHY, so that mutant would
otherwise have armed a ledger over zero expected files and passed everything.
- The on-disk ledger run against the REAL tree: 201/201 green, and dropping one real
file from the report fails and names it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(tests): close audit round 2 — two regressions the round-1 fixes introduced
A delta re-audit of `6ceac37f33..7f005f0482` confirmed 4 of the 7 claims outright
(the signal mapping, the pre-run clear, the file ledger, the fixed factory) with its
own positive and negative controls, and found that two of the fixes had introduced
new defects of their own. Both are here, with the smaller findings.
🔴 THE ON-DISK LEDGER FALSE-FAILED A LEGITIMATE RUN, WITH A MESSAGE FORBIDDING THE FIX.
`--exclude`, `--dir` and `--root` were added to VALUE_FLAGS but not to
NARROWING_FLAGS. All three take a value AND genuinely shrink the collected file set,
so `pnpm test:component --exclude 'src/tests/**'` scored as a FULL run and the ledger
failed it naming up to 200 files -- asserting the include broke or the run died, and
telling the reader "Do NOT silence this by narrowing the walk", which is the only
thing that would have fixed it. `--config` joins them: it can replace the project's
`include` outright, which is the assumption the walk is built on. This is precisely
the shape the round was convened to prevent, produced by the round's own fix.
🔴 `shell: win32` WENT ON THE SHARED `run()`, SO IT ALSO WRAPPED THE GATE SPAWN.
The claim said this matched `scripts/test-unit-run.mjs`; it did not -- that file puts
`shell` on its vitest spawn only and deliberately leaves its `process.execPath` spawn
alone. Node with `shell: true` concatenates argv UNESCAPED (DEP0190), and the gate is
spawned as `process.execPath`, which on Windows is `C:\Program Files\nodejs\node.exe`.
So on the one platform the option exists to support, every `pnpm test:component` would
have failed at the gate step with a cmd.exe parse error rather than any of this
wrapper's messages. `shell` is now per-call.
Also:
- KEBAB AND CAMEL ARE THE SAME FLAG TO VITEST (cac camelCases every option key), so a
hand-enumerated list has a hole wherever it carries one spelling and not the other --
and it did: `--max-workers`/`--maxWorkers` were both listed, `--test-timeout` was
not, so the kebab form silently disabled both checks. Spellings are now canonicalised
rather than enumerated, and the test asserts them in PAIRS.
- `--output-file` (kebab) was NOT refused, so the exact defect the refusal exists for
was still reachable -- under a test named "catches every spelling". Meanwhile
`--outputFile.junit=` and `--outputFile.html=` WERE refused, though neither touches
the `.json` key: object-form output paths are per-reporter, so that was over-strict
and the stated reason ("the bare form sets the path for EVERY reporter") is true only
of the bare form. Both directions fixed.
- The rc-127 short-circuit is keyed on a spawn-failure FLAG, not on the number. 127 is
an exit code a runner can produce on its own, and relabelling that as "the binary
could not be started" is a second wrong answer that also skips the gate on a run that
happened.
- `--repo-root <dir>` did not consume its value, so with the flag FIRST the directory
was read as the report path: "EISDIR: illegal operation on a directory" plus the
whole abort diagnosis. Every test passed it last, which is why nothing caught it; the
ledger tests now pass it first.
- `--related` removed. It is a vitest SUBCOMMAND, and this wrapper always spawns
`vitest run …`, so it cannot arrive -- the entry and its assertion were both inert,
the assertion pinning behaviour for an input the runner cannot receive.
- The factory now names all FOUR runtime exports of the flags module, including
`FeatureFlagsProvider`. The comment said "EVERY runtime hook", which was narrower
than the module -- and "not in this graph today" is exactly the reasoning that put
this file in the diff.
- A comment the previous round made false: it said the positional rule would catch
`-t foo` anyway. It stopped being true the moment `-t` was listed as value-taking --
the value would now be CONSUMED -- so the explicit narrowing branch is load-bearing.
- CONTRIBUTING documents the refusal, the file ledger, and which flags narrow.
Coverage for the two claims that shipped with none: `main` now takes its collaborators
as injected defaults, so the ORDER of effects can be asserted rather than inspected.
The two `clearReport()` calls are byte-identical statements -- only their position
carries the meaning, so a refactor moving the first below the run reopens "a stale
report satisfies the next run's ledger" with every other test green.
Verification: 35 unit tests (was 27). Mutation-checked at 17 mutants, ALL KILLED, zero
survivors -- including re-running round 1's guards, because an audit fix resets the
gate. Real-tree controls re-run with the new argument parsing, flag first and flag
absent: 201/201 green, and dropping one real file fails and names it. ESLint on the
touched files is back to the base count; two `no-empty-function` errors this change
introduced are fixed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(tests): close audit round 3 — the canonicaliser re-opened the class it closed
Round 3 verified 8 of round 2's 9 claims against the diff or against vitest's own
source (it read cac's `setDotProp` to confirm the `--outputFile.junit=` allowance is
genuinely safe, and `cliOptionsConfig` to confirm `-r`/`-c` really are root/config).
It found one regression and one coverage hole, both here.
🔴 A BOOLEAN FLAG SWALLOWED THE FILENAME AFTER IT — the same shape round 2 fixed,
re-introduced by round 2's own mechanism. `canonicalFlag` collapsed `.subkey` onto the
parent, so every dot-subkey inherited its parent's value-consuming behaviour, and the
two entries `--coverage.reporter`/`--coverage.provider` became a single `coverage` that
also matched the BARE `--coverage` — which takes no value (`argument: ""` in vitest's
`cliOptionsConfig`). So `pnpm test:component --coverage <file>` ate the FILE as
`--coverage`'s value, scored the run as full, and failed an 18/18 green single-file run
naming ~200 files as ABSENT while telling the reader not to narrow the walk. Same for
`--coverage.enabled <file>` and `--browser.headless <file>`. Measured base-vs-head, all
three flipped `true` to `false`. Matching is now on the full canonical PATH, so a
subkey is value-taking only if it is listed as one.
🔴 THREE MUTANTS SURVIVED A GREEN 35-TEST SUITE, AND EACH WAS THIS PR'S OWN HEADLINE
FAILURE. The injected fakes took no parameter, so nothing could observe the argv `main`
builds — the seam between the two modules this change exists to wire together. Dropping
`--narrowed` from the gate argv makes every narrowed run hard-fail both the floor and
the on-disk ledger; dropping `--outputFile.json=` makes every run report "does not
exist" plus the whole abort diagnosis; dropping `...argv` makes
`pnpm test:component <file>` silently run the entire suite behind a healthy-looking
ledger. Testing `isNarrowed` in isolation cannot see whether its answer is ever USED.
The fakes now capture their argument and three cases assert it; all three mutants die.
Also:
- `--repo-root` with no value was a silent fall-through to the real repo root, so a
fixture report got graded against the 201 real files and failed with a confident
diagnosis about the include breaking — produced by a typo. Now a usage error (exit 2).
- CONTRIBUTING: the sample ledger omitted `measured <date>`, which the real output
carries; and "skips both checks but not the zero check" counted a different pair than
the two enumerated four lines above. The checks are now numbered and the sentence
names which ones a narrowed run skips.
Verification: 39 unit tests (was 35). Mutation-checked at 19 mutants, ALL KILLED, zero
survivors — the three that survived round 3, the round-3 fixes themselves, and every
guard from rounds 1 and 2 re-run, because an audit fix resets the gate. Real-tree
controls re-run: 201/201 green, one missing file still fails. ESLint clean on the
changed test file.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(tests): close audit round 4 — replace the flag LIST with a shape rule
Round 4 measured `isNarrowed` at base vs head across every one of vitest 4.1.11's 72
boolean options and 73 value-taking ones, and found the flag list had traded 22 loud
wrong answers for 25 quiet ones. Splitting `coverage` into two subkeys left
`--retry.count 2`, `--browser.name chromium` and sixteen more `coverage.*` paths
reading their VALUE as a filename, scoring a FULL run as narrowed and switching the
file ledger and the floor off with a one-line note. That is the direction this file's
own comments repeatedly name as the worse of the two.
🔴 THE LIST WAS THE PROBLEM, AND THREE ROUNDS WERE SPENT ON IT. Enumerating flag NAMES
missed `--test-timeout` next to `--testTimeout`. Canonicalising spellings then made
`--coverage <file>` swallow the file. Splitting into subkeys produced the 25 above.
vitest 4.1.11 has a 164-path option tree; a hand-maintained copy of it is wrong the day
it is written, and each fix moved the wrongness rather than removing it.
So the rule is now about the ARGUMENT, not the flag: a positional is a file filter if
it looks like a path, or if nothing before it could have been expecting a value. A
non-path token straight after a flag is that flag's value, WHATEVER the flag is —
correct for all 73 value-taking options without naming one of them, and correct for all
72 booleans too. `VALUE_FLAGS` is deleted. What remains is `NARROWING_FLAGS` (flags that
genuinely shrink the run and must be named, because omitting one fails LOUDLY) and a
five-entry `PATH_VALUE_FLAGS` for the only residual the shape cannot decide: a value
that is itself a path.
🔴 AND THE DECISION IS NOW A SENTENCE, NOT A BOOLEAN. No rule over an unknowable flag
list is right always; what must never happen is being wrong SILENTLY, because
`--narrowed` disables the two checks this whole change exists to add. `narrowingReason`
returns why, and the runner prints it: `--retry.count 2` reading `2` as a file filter is
obvious on sight and invisible otherwise.
Also:
- `canonicalFlag` camelCases only the FIRST dot segment, matching cac's own
`camelcaseOptionName` (`name.split(".").map((v,i) => i===0 ? camelcase(v) : v)`).
Camel-casing all of them made this wrapper accept `--coverage.reports-directory`,
a spelling vitest does not — so the two would disagree about the next token.
- `--repo-root=<dir>` is parsed. Matching only the space form left the inline spelling
falling through BOTH branches to the real repo root — byte-for-byte the failure the
missing-value guard was added to close, reachable through one extra character. A
value that is itself a flag is rejected too.
Verification: 45 unit tests (was 44 after the round's own additions; 39 before).
Mutation-checked at 23 mutants, ALL KILLED, zero survivors, each by its OWN named test.
🔴 THE FIRST SWEEP REPORTED FOUR SURVIVORS AND WAS WRONG ABOUT ALL FOUR, WHICH IS WORTH
RECORDING BECAUSE THE HARNESS HAD THIS PR'S OWN DEFECT. Two were mislabelled — killed by
a different test than the one named, which the sweep scores as SURVIVED. Of the other
two, one was a real gap (`--reporter=json AppNameCrumb`, now covered) and one was
`canonicalFlag`'s dot handling, which is equivalent for every input that has no dashed
subkey (also now covered). Along the way the harness itself was found reusing a JSON
report path without clearing it — the same stale-report defect fixed in the product two
commits ago — and now clears it and verifies the mutation reached disk before running.
A mutant reported SURVIVED is a claim about the instrument until the instrument has been
controlled.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* wip(tests): stop guessing which vitest flags take values
Safety commit of in-progress round-5 rework so it is not lost; the agent was
interrupted mid mutation-sweep. Verification is NOT complete — the sweep, the
red/green pair and the merged-tree re-run have not been reported. Do not merge
on this commit.
Round 5 measured both prior approaches against vitest 4.1.11's real option
table (170 long options, 74 boolean, 96 value-taking): the hand-maintained
list was wrong 73 times, the shape heuristic 74. The error count never moved,
only its direction. This removes the question instead of answering it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AEa6GDJyTiu2R146ndYsLK
* test(ci): finish the verification bacb8a2cf5 was pushed without
bacb8a2cf5 landed as a rescue commit marked `wip` because the session that wrote it
died with the change only in a working tree. The change itself is unaltered; this is
the verification it was missing, plus the merge of a main that moved twice underneath
it. Nothing here modifies the rule.
WHY THE RULE CHANGED, WITH THE NUMBER THAT JUSTIFIES IT. Round 5 of the audit
enumerated vitest 4.1.11's REAL CLI option table -- by calling `createCLI()` and reading
each cac option's `isBoolean` rather than by hand -- and ran `isNarrowed(['--<flag>',
'VALUE'])` against both revisions imported side by side:
hand-maintained flag list wrong on 73 options (every value-taking one) QUIET
shape heuristic wrong on 74 options (every boolean one) LOUD
73 versus 74. The heuristic did not beat the list; it moved the wrongness off one half
of the table onto the other. The direction improved, which is worth something, but the
error count did not -- and `pnpm test:component --coverage AppNameCrumb` would run one
test and then be failed against the 1240 floor with "the include broke or the run died".
That is a mis-posed question, so it is no longer asked: any argument at all means
narrowed.
Re-measured against the same table on the merged tree, with the same method:
164 long options (72 boolean, 92 value-taking)
UNSAFE-LOUD -- rule claims FULL so the floor and ledger fire on a partial run: 0
no-argument invocation (what CI runs): NOT narrowed, so all three checks arm
(My enumeration walks the global + `run` commands and sees 164/72/92 where round 5's
saw 170/74/96; the traversal differs slightly, the conclusion does not.)
🔴 The comparison is one-directional ON PURPOSE, and the other direction is a real cost
rather than a rounding error: all 164 options now score as narrowed, so an arg-ful run
does not get the floor or the file ledger even when it was genuinely full. That is
affordable for one measured reason -- `pr-preview-pipeline.yaml` invokes
`pnpm run test:component` with NO arguments, so CI is the `argv.length === 0` path and
always gets all three. What is given up is enforcing a floor on an ad-hoc local run.
`VITEST_MAX_WORKERS=4` in the environment sizes a run without giving that up.
Verification on the MERGED tree (d77dd394db), all at load <= 16 with no browser-session
errors in any run quoted:
no-arg full run (the CI path) 202 files / 2257 tests, exit 0, no NARROWED line,
floor 1240 and the 202-file ledger both ARMED
guard red arm exit 1, "THIS RUN PRODUCED NO ACCOUNTABLE RESULT",
failing on the missing-report branch
guard green arm exit 0, "2 executed, 0 skipped, across 1 files"
gate unit tests 37 passed
mutation sweep 20 mutants, ALL KILLED, zero survivors
🔴 The red arm was run NARROWED, deliberately: it proves `--narrowed` disables the floor
and the ledger and NOT the zero-collected check, which is the one failure this whole
change exists to catch.
THE MERGE. main moved twice; both times the only conflict was `package.json`, and both
times it was the same semantic hazard -- main editing `test:lint-rules` and this branch
editing `test:component`, adjacent lines of one object, where taking either side
wholesale silently reverts the other and makes the entire guard inert with every test
green. Resolved by parsing the merged JSON, not by reading the diff: `test:lint-rules`
now matches origin/main byte-for-byte and `test:component` is the wrapper. The wiring
guard added for exactly this was then checked against the bad resolution -- applying
main's `package.json` wholesale fails one test, the one written for it.
Round 5's two remaining 🟢 items are closed by the rework itself rather than separately:
the stale JSDoc described `VALUE_FLAGS`, which no longer exists, and the test whose
description claimed "a BOOLEAN flag does not swallow the filename after it" while
passing entirely through the path check is gone with the heuristic it tested. That one
was load-bearing -- a description asserting coverage its body did not provide is what
let the class through -- so its replacement asserts a property with no free parameters
instead, across both halves of the option table.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* style(tests): format the gate test file
The rescue commit was pushed before a prettier pass could run over the last edit to
this file, and `ESLint + Prettier (changed files)` caught it: the file is ADDED by this
PR, so it is covered by the added-files prettier gate. One line.
This red was MINE, not inherited — unlike `Unit tests (1)/(2)` (a redis integration
test that arrived from main) and `preview / smoke-tests` (#4516 changing
`reaction.toggle`), both of which are verified as pre-existing and are left alone.
CONTRIBUTING.md also reports unformatted, and that one is NOT actionable here: it is
already unformatted on origin/main, and it is modified rather than added, so the gate
(which reads `added.txt`) does not cover it. Reformatting it would bury this PR under an
unrelated whole-file diff.
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-01 10:57:49 -05:00
|
|
|
### `pnpm test:component` fails on ZERO COLLECTED, not only on red tests
|
|
|
|
|
|
|
|
|
|
The browser suite has a failure mode that runs **no tests at all** and, until you
|
|
|
|
|
read the numbers, looks exactly like an ordinary failure. `pnpm test:component`
|
|
|
|
|
runs through `scripts/test-component-run.mjs`, which asks vitest for a JSON report
|
|
|
|
|
and hands it to `scripts/ci/assert-component-suite-ran.mjs`. That gate prints a
|
|
|
|
|
ledger and applies three checks:
|
|
|
|
|
|
|
|
|
|
```
|
|
|
|
|
test:component ledger: 2254 executed, 0 skipped, across 201 files; 0 failed suites,
|
|
|
|
|
0 failed tests (baseline 2254 tests / 201 files measured 2026-08-31; 201 on disk;
|
|
|
|
|
floor 1240)
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
1. **Nothing collected fails** — always, on every invocation.
|
|
|
|
|
2. **A `*.browser.test.tsx` on disk that is absent from the report fails, and is
|
|
|
|
|
named.** A file that stops being collected reports as *absence*, so no failure
|
|
|
|
|
count and no per-test list can show it.
|
|
|
|
|
3. **A floor on executed tests**, as a backstop for a partial collapse that still
|
|
|
|
|
leaves every file present.
|
|
|
|
|
|
|
|
|
|
The gate can only ever *add* a failure — vitest's own exit code is passed straight
|
|
|
|
|
through otherwise.
|
|
|
|
|
|
|
|
|
|
Three things to know before you run it with arguments:
|
|
|
|
|
|
|
|
|
|
- **Any argument at all skips checks 2 and 3. Check 1 always applies.** Passing
|
|
|
|
|
*anything* — a filename, `--shard`, even `--max-workers 4` — means what the run
|
|
|
|
|
should have collected is not knowable from the arguments, so the floor and the file
|
|
|
|
|
ledger are not asserted. Run with **no arguments** to get all three; that is what CI
|
|
|
|
|
does. To size a run without giving up the checks, use the environment instead:
|
|
|
|
|
`VITEST_MAX_WORKERS=4 pnpm test:component`.
|
|
|
|
|
(This is deliberately not a parser. Deciding which vitest flags consume the next
|
|
|
|
|
token was tried twice and lost twice — measured against vitest's real option table,
|
|
|
|
|
a hand-maintained flag list was wrong on 73 options and the shape heuristic that
|
|
|
|
|
replaced it was wrong on 74. The rule above cannot be wrong about any of them.)
|
|
|
|
|
- **`--outputFile` is refused** (exit 2), in every spelling that would collide with
|
|
|
|
|
the report the gate reads. Run `pnpm exec vitest run --project component` directly
|
|
|
|
|
if you want your own report.
|
|
|
|
|
|
|
|
|
|
Why it exists: a `vi.mock` factory that throws is resolved inside a Playwright
|
|
|
|
|
route handler that does not catch, so the rejection escapes as an
|
|
|
|
|
`Unhandled Rejection` in the orchestrator and kills the whole run — no
|
|
|
|
|
`Test Files` line, no per-file results, exit 1. The `preview / component-tests`
|
|
|
|
|
tier reads only the exit code, so that abort and a genuine list of red assertions
|
|
|
|
|
rendered identically. One such file zeroed all 201 suites on `main` at
|
|
|
|
|
`d353f785c3`. A browser crash under host load produces the same shape, wearing the
|
|
|
|
|
same misleading headline. The gate's message enumerates the causes it cannot tell
|
|
|
|
|
apart; read the error printed above it.
|
|
|
|
|
|
|
|
|
|
The commonest cause is a **wholesale** factory that stops naming an export
|
|
|
|
|
something in the file's module graph imports. `local-rules/no-wholesale-module-mock`
|
|
|
|
|
guards that for a listed set of modules, and
|
|
|
|
|
`src/components/AppBlocks/__tests__/featureFlagsMockCompleteness.test.ts` guards
|
|
|
|
|
the feature-flags module — but only under `src/components/AppBlocks`, so neither
|
|
|
|
|
covers you by default.
|
|
|
|
|
|
2026-07-28 17:44:35 -06:00
|
|
|
## Where tests go
|
|
|
|
|
|
|
|
|
|
**Never put test files under `src/pages`.** Next.js treats every `.ts`/`.tsx`
|
|
|
|
|
file there as a route — including nested `__tests__/` directories — and
|
|
|
|
|
`next build` runs a route-type validator over them. A Vitest file in that tree
|
|
|
|
|
fails the build with `Property 'default' is missing`, and **only `next build`
|
|
|
|
|
catches it**: typecheck, vitest and every CI job pass. It reaches the preview
|
|
|
|
|
build before anyone notices.
|
|
|
|
|
|
|
|
|
|
Put handler tests in a `__tests__/` directory outside `src/pages` (e.g.
|
|
|
|
|
`src/server/__tests__/`) and import the handler through the `~/pages/...` alias.
|
|
|
|
|
|
|
|
|
|
## Database migrations
|
|
|
|
|
|
|
|
|
|
**We do not use `prisma migrate deploy`.** Migrations are applied by hand, per
|
|
|
|
|
environment. Files in `packages/civitai-db-schema/prisma/migrations/` exist for
|
|
|
|
|
review and history; they are never auto-run, and the `_prisma_migrations` table
|
|
|
|
|
is not a source of truth.
|
|
|
|
|
|
|
|
|
|
So: write the SQL, commit it, and say so in your PR description — a maintainer
|
|
|
|
|
applies it. Don't suggest `prisma migrate deploy` or `prisma migrate resolve`.
|
|
|
|
|
|
|
|
|
|
Create migrations with `pnpm run db:migrate:empty "brief description"`. They must
|
|
|
|
|
land in `packages/civitai-db-schema/prisma/migrations/`, not the `prisma/migrations/`
|
|
|
|
|
directory at the repo root, which predates the monorepo and Prisma no longer reads.
|
|
|
|
|
|
|
|
|
|
## Branching: no stacked PRs
|
|
|
|
|
|
|
|
|
|
Base every PR directly on `main` (or on a feature integration branch), **never on
|
|
|
|
|
another open PR's branch.**
|
|
|
|
|
|
|
|
|
|
Stacked PRs mis-merge silently here: a squash-merged parent doesn't retarget its
|
|
|
|
|
child, so the child lands on the orphaned parent branch instead of the real base
|
|
|
|
|
and its changes vanish. This has cost us real work.
|
|
|
|
|
|
|
|
|
|
If your change depends on an unmerged PR, wait for it to merge and branch off the
|
|
|
|
|
updated base, or fold both changes into one PR.
|
|
|
|
|
|
|
|
|
|
## Scope and PR size
|
|
|
|
|
|
|
|
|
|
Smaller is genuinely better here — reviewer attention is the bottleneck. If a PR
|
|
|
|
|
contains one clearly-correct one-line fix plus a larger feature, split it. The
|
|
|
|
|
one-liner will merge in a day; the feature might take a week, and there's no
|
|
|
|
|
reason for the fix to wait.
|
|
|
|
|
|
|
|
|
|
If you find a second bug while fixing the first, prefer a separate issue or PR
|
|
|
|
|
over widening the one you're in.
|
|
|
|
|
|
|
|
|
|
## Writing it up
|
|
|
|
|
|
|
|
|
|
A good PR description explains **why the change is correct**, not just what it
|
|
|
|
|
does. Especially valuable:
|
|
|
|
|
|
|
|
|
|
- What you verified, and how. "Ran X, got Y" beats "should work".
|
|
|
|
|
- What you *didn't* change and why — deliberate omissions read as oversights
|
|
|
|
|
otherwise.
|
|
|
|
|
- Anything you're unsure about. Flagging a shaky assumption is more useful than
|
|
|
|
|
quietly hoping nobody checks.
|
|
|
|
|
|
|
|
|
|
If you discover your description was wrong after opening the PR, correct it in a
|
|
|
|
|
comment. That's a normal and welcome thing to do, not an admission of anything.
|
|
|
|
|
|
|
|
|
|
## Comments in code
|
|
|
|
|
|
|
|
|
|
Bias toward none. Comment the non-obvious *why* — a rationale, tradeoff, gotcha,
|
|
|
|
|
or workaround a reader can't recover from the code. Never narrate what the next
|
|
|
|
|
line does, and don't describe the current behaviour of nearby code; that's
|
|
|
|
|
exactly what goes stale. Comments aren't type-checked, so they rot silently.
|
|
|
|
|
|
|
|
|
|
## Getting help
|
|
|
|
|
|
|
|
|
|
Open an issue, or join the
|
|
|
|
|
[Community Development Team](https://civitai.com/articles/7782).
|