Compare commits

..
Author SHA1 Message Date
logan e5d0f2714b test(ui): make ui-visual-update honour its file filter
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 2m27s
CI / e2e (pull_request) Successful in 9m27s
vitest parses a bare `--update` as taking the next positional as its
value, so `make ui-visual-update UI_ARGS=<path>` handed the path to the
flag and ran with no filter at all: 99 files, every baseline in the repo
re-recorded, any stale one blessed in silence. Two paths were worse
still — the first was eaten and only the second ran.

That is #196's own hazard living in the tool meant to resolve it: the
rule is "refresh the reference your change moved and never one you did
not cause", and the documented way to refresh one refreshed the set.

`--update=true` is the whole fix, with the reason beside it because
`=true` reads like something to tidy away. `make ui-visual` and
`make ui-test` are unaffected — their `$(UI_ARGS)` follows `run`, with
no flag to swallow it — and no other target interpolates a variable
after a boolean flag.

Closes #204
2026-08-23 04:36:46 -04:00
8 changed files with 9 additions and 96 deletions
+1 -8
View File
@@ -158,7 +158,7 @@ only climb when it cannot.
| You changed | Run | Cost |
|---|---|---|
| A Lit component, a store, the shortcut service | `make ui-test` | ~2 s, no app |
| …and it renders differently | `make ui-visual` | + 10 baselines, opt-in, never gates |
| …and it renders differently | `make ui-visual` | + 6 baselines, opt-in |
| Any Go code | `make test` | 3 passes, ~2 min |
| A service that emits events | `make test` — assert on the payload, see `backend/queue/emit_test.go` | in-process, no app |
| A bound method or a bound struct field | `make bindings` then `make ui-test` | ~1.5 s + 2 s |
@@ -180,13 +180,6 @@ less than it looks.)
Two rules about climbing:
- **If you moved a component's geometry, run `make ui-visual` and
refresh that component's baseline in the same commit.** Nothing else
will: it is the one tier in this repo no hook and no CI job runs, and
it cannot be one — its references are machine-specific, measured in
[references/ui-tier.md](references/ui-tier.md). Four of them drifted
across three merges before anyone noticed (#196). Read the image;
never bless a reference you did not cause.
- **A component test passing is not the app rendering.** If you touched
anything in `frontend/src`, verify it in the real app too — start it
headless, `screenshot --filename=/tmp/shot.png`, and *read the PNG*.
@@ -78,58 +78,9 @@ synchronously.
Microtasks and not a timer, deliberately: a timer hangs forever under
the suites that install fake ones.
## The visual tier does not gate, and that is measured (#196)
`make ui-visual` is the same suite with nine `toMatchScreenshot`
baselines switched on. **Nothing runs it but a person**, deliberately,
and the reason is a number rather than a preference: the committed
baselines were recorded on Arch, and replayed in a bare `ubuntu:24.04`
container — CI's `check` image — three of them fail for reasons that
have nothing to do with any component.
| baseline | Arch | ubuntu:24.04 |
|---|---|---|
| `page-header` filtered-by-search | passes | ratio 0.03 differ, against a 0.02 allowance |
| `track-info` | passes | ratio 0.03 differ |
| `seek-bar` | 1152×18 | 1152×17 |
The two references that were genuinely stale did not even agree about
their *new* size — `now-playing` renders 1152×65 on Arch and 1152×64 in
the container. So moving CI's `check` job from `make ui-test` to
`make ui-visual` is not a one-line change: it needs a second,
container-recorded baseline set, which every local run would then fail
against. That is the same trap the other way round, and a pre-push hook
is the same fault again — one machine's baselines against everybody
else's renderer.
So the tier stays local and opt-in, and the rule that replaces the gate
is:
- **A change that moves a component's geometry refreshes that
component's reference in the same commit, having read the image.**
Look at the PNG; the dimensions in the failure message are the cheap
half of the answer.
- **Never refresh a reference you did not cause.** #196 exists because
four of them drifted across three unrelated merges, and every red run
made the next person likelier to stop running the tier than to read
it.
- **State the world the shot is taken in.** The stores are singletons,
so a visual case that sets nothing photographs whatever the previous
case left behind — which is how the sidebar's baseline came to have
Tracks lit and `now-playing`'s to be playing from a dynamic mix.
- **`make ui-visual-update UI_ARGS=<path>` does not filter** and
re-records *every* baseline, blessing any stale one in silence:
vitest's `--update` takes the following positional as its value. Until
#204 lands, record one file with
`cd frontend && YJ_VISUAL=1 npx vitest run --update=true <path>`, and
check `git status` before committing either way.
What the tier is worth, for the record: it is a *layout* check, blind to
colour (the component tier has no `:root`, so it renders the fallbacks —
`make ui-visual` passed unchanged through a whole palette rewrite,
twice), and it has caught one thing nothing else could — swapping
`library-status-indicator`'s `<button>` for a `<span>` lost the UA
stylesheet's `box-sizing` and grew the badge 36→38px.
Visual baselines are font-hinting and compositing sensitive, which is
why they are opt-in: they only mean anything on the machine that
recorded them.
## Bindings
-19
View File
@@ -3424,25 +3424,6 @@ Pre-commit hooks verify generated code is fresh — always run `make generate` a
Tests use `database.NewTestDB(t)` for in-memory SQLite, built by the same
`applySchema` production uses so the two cannot diverge. Test audio fixtures live in `test_data/music_library_test/`. Table-driven tests are the norm.
**`make ui-visual` is the one tier nothing but a person runs, and it
cannot become one.** Its ten `toMatchScreenshot` baselines were recorded
on a developer's Arch box; replayed in a bare `ubuntu:24.04` container
— CI's `check` image — three of them fail on font metrics and
compositing alone (`track-info` and one `page-header` shot at a 0.03
mismatch ratio against a 0.02 allowance, `seek-bar` one pixel shorter),
and two components disagree about their own height between the two
machines. So CI keeps running `make ui-test`, which is the same suite
with the comparisons off, and a pre-push hook would be the same fault
with the machines swapped. What replaces the gate is a rule, in
`.pi/skills/yellowjacket-dev/references/ui-tier.md`: **a change that
moves a component's geometry refreshes that component's baseline in the
same commit, having read the image, and never one it did not cause**.
That is #196, which was four stale references accumulated across three
unrelated merges — a red tier nobody could read, which is how it stayed
red. A visual case must also **state the world it photographs**, since
the stores are singletons and a case that sets nothing records whatever
the previous one left in them.
## Git Workflow
Feature branches and PRs are the only way in: **`main` is a protected
+5 -2
View File
@@ -160,8 +160,11 @@ ui-watch: ## Same suite, in watch mode
ui-visual: ## Run the suite including screenshot comparisons
@cd frontend && YJ_VISUAL=1 npx vitest run $(UI_ARGS)
ui-visual-update: ## Re-record the screenshot baselines
@cd frontend && YJ_VISUAL=1 npx vitest run --update $(UI_ARGS)
# `--update=true`, never a bare `--update`: vitest takes the following
# positional as the flag's value, so `--update <path>` swallows the path
# and re-records every baseline in the repo instead of the one named.
ui-visual-update: ## Re-record the screenshot baselines (UI_ARGS=<path> to filter)
@cd frontend && YJ_VISUAL=1 npx vitest run --update=true $(UI_ARGS)
ui-setup: ## Install the Vitest browser provider's own Chromium (once)
@cd frontend && pnpm install && npx playwright install chromium
Binary file not shown.

Before

Width:  |  Height:  |  Size: 14 KiB

After

Width:  |  Height:  |  Size: 15 KiB

Binary file not shown.

Before

Width:  |  Height:  |  Size: 6.7 KiB

After

Width:  |  Height:  |  Size: 4.4 KiB

-5
View File
@@ -137,11 +137,6 @@ describe('<app-sidebar>', () => {
});
it('looks the way it did last time', async () => {
// Stated rather than inherited: `activeViewStore` is a singleton, so
// without this the shot records whichever view the *previous* case
// left in it and the reference moves when the file is reordered.
activeViewStore.setView('home', true);
const el = await fixture('app-sidebar');
await visual(el, 'app-sidebar');
@@ -366,16 +366,6 @@ describe('<now-playing>', () => {
const el = await fixture('now-playing');
emit(Events.TrackChanged, { ...TRACK, trackChangeId: 6 });
// Stated rather than inherited: the queue store is a singleton, so
// without this the shot records whichever source the *previous*
// case left in it and the reference moves when the file is
// reordered. Three lines is what the bar renders while playing
// from somewhere, which is the arrangement worth recording.
setQueue([queueTrack(1, 'Ashes to Ashes')], 0, {
type: 'album',
id: 7,
label: 'Scary Monsters',
});
await flush();
await el.updateComplete;