Compare commits
7
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
a82d29abd7 | ||
|
|
d365850321 | ||
|
|
3a2d3e8ef8 | ||
|
|
871a3b7aac | ||
|
|
f3207e8bf9 | ||
|
|
772c71c49f | ||
|
|
e5d0f2714b |
@@ -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` | + 6 baselines, opt-in |
|
||||
| …and it renders differently | `make ui-visual` | + 10 baselines, opt-in, never gates |
|
||||
| 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,6 +180,13 @@ 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,9 +78,58 @@ synchronously.
|
||||
Microtasks and not a timer, deliberately: a timer hangs forever under
|
||||
the suites that install fake ones.
|
||||
|
||||
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.
|
||||
## 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.
|
||||
- **Record one file with `make ui-visual-update UI_ARGS=<path>`**, and
|
||||
check `git status` before committing either way. That filter is only
|
||||
honoured since #204: the recipe was a bare `--update`, and vitest
|
||||
takes the following positional as the flag's value, so the path was
|
||||
swallowed and *every* baseline was re-recorded — blessing any stale
|
||||
one in silence.
|
||||
|
||||
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.
|
||||
|
||||
## Bindings
|
||||
|
||||
|
||||
@@ -262,6 +262,26 @@ real store code. A binding carries an **ID**, not a name
|
||||
`yellowjacket/backend/home.Service.GetShelves`), so the fake derives
|
||||
that map from the generated tree rather than writing it down.
|
||||
|
||||
**A test file does not get its own origin, so `setup.ts` clears
|
||||
`localStorage` between tests.** `@vitest/browser-playwright` opens one
|
||||
BrowserContext per session and runs several files in it one after
|
||||
another, so everything a component persists — the track list's sort and
|
||||
column widths, the cover size, `now-playing`'s scroll mode — is still
|
||||
there when the next file mounts the same component. Which files share a
|
||||
tab, and in what order, changes run to run, so the symptom is a spec
|
||||
that fails about one test in three and passes every time it is run on
|
||||
its own: #138 cost three scheduled runs, one of them a PR whose diff
|
||||
held no frontend code at all. Measured on the build before the fix, a
|
||||
single full run started **24** tests with storage already set. Two
|
||||
things follow. The clear is safe precisely because the leak is
|
||||
sequential — files in a session do not overlap, so it cannot wipe
|
||||
storage a concurrently-running file is in the middle of using — and it
|
||||
belongs in `setup.ts` rather than in the specs that write, because the
|
||||
spec that *reads* is never the one that knows. And a spec whose
|
||||
assertion depends on an order still **states that order** rather than
|
||||
inheriting a default, or the next change to a default is the same
|
||||
mystery again.
|
||||
|
||||
**`frontend/bindings/` is generated by `wails3`, not `go generate`**, so
|
||||
the pre-commit codegen check does not cover it. `make bindings-check`
|
||||
(~3.5 s warm, ~20 s on a cold build cache, also a pre-commit hook)
|
||||
@@ -3451,6 +3471,25 @@ 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
|
||||
|
||||
@@ -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
|
||||
|
||||
BIN
Binary file not shown.
|
Before Width: | Height: | Size: 15 KiB After Width: | Height: | Size: 14 KiB |
BIN
Binary file not shown.
|
Before Width: | Height: | Size: 4.4 KiB After Width: | Height: | Size: 6.7 KiB |
@@ -137,6 +137,11 @@ 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,6 +366,16 @@ 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;
|
||||
|
||||
|
||||
@@ -161,6 +161,15 @@ describe('double-clicking a row in the track list', () => {
|
||||
localStorage.removeItem('track-list-column-widths');
|
||||
|
||||
el = await fixture<LitElement>('track-list', { externalTracks: LIST });
|
||||
|
||||
// Say which order is being asserted rather than inheriting one.
|
||||
// `restoreSortPreferences()` runs in `connectedCallback`, so the
|
||||
// list opens in whatever sort was last persisted -- and
|
||||
// `track-11` sorts before `track-3` by title, which is the shape
|
||||
// of #138. The row below is the fixture's third track only while
|
||||
// nothing is sorting the list.
|
||||
(el as unknown as { sortField: string | null }).sortField = null;
|
||||
|
||||
el.style.display = 'block';
|
||||
el.style.height = '600px';
|
||||
await flush();
|
||||
@@ -172,6 +181,10 @@ describe('double-clicking a row in the track list', () => {
|
||||
const rows = shadowAll(el, '.track-row');
|
||||
const row = rows.find((r) => r.getAttribute('data-index') === '3');
|
||||
|
||||
// Stated first, so a list that is not in the order this asserts
|
||||
// fails by saying so rather than as an off-by-eight file path.
|
||||
expect(row?.getAttribute('data-file-path')).toBe('/music/track-3.mp3');
|
||||
|
||||
dblclick(row!);
|
||||
await flush();
|
||||
|
||||
|
||||
@@ -72,6 +72,23 @@ document.body.style.margin = '0';
|
||||
|
||||
beforeEach(() => {
|
||||
resetHarness();
|
||||
|
||||
// A test file does not get its own origin. `@vitest/browser-playwright`
|
||||
// opens one BrowserContext per session and runs several files in it,
|
||||
// one after another, so everything a component persists — the track
|
||||
// list's sort and column widths, the cover size, `now-playing`'s
|
||||
// scroll mode — is still there when the next file mounts the same
|
||||
// component. That is invisible until it is intermittent, because
|
||||
// which files share a tab and in what order changes run to run: it
|
||||
// cost #138 three scheduled runs, one of them a PR with no frontend
|
||||
// code in it at all.
|
||||
//
|
||||
// Clearing here rather than in the specs that write is deliberate —
|
||||
// the spec that *reads* is never the one that knows. It is safe for
|
||||
// the same reason the leak exists: files in a session are
|
||||
// sequential, so this cannot wipe storage a concurrent file is in
|
||||
// the middle of using.
|
||||
localStorage.clear();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
|
||||
Reference in New Issue
Block a user