diff --git a/.pi/skills/yellowjacket-dev/SKILL.md b/.pi/skills/yellowjacket-dev/SKILL.md index 4aba9a8..c3b738f 100644 --- a/.pi/skills/yellowjacket-dev/SKILL.md +++ b/.pi/skills/yellowjacket-dev/SKILL.md @@ -96,6 +96,15 @@ reference, because you need them *before* the failure, not after. Run against the `bulk` seed a measurement session left behind and a third of them fail (13 of 36, when it was measured), in a list that reads exactly like a regression in whatever you are holding. `make dev-headless SEED=default` first. +- **The catalog is stubbed out locally now, like CI.** + `dev-headless.sh` defaults `YJ_CORE_INDEX_URL` to a dead address + because it was the only launcher that did not — `seed-sandbox.sh` and + `ci.yml` always have. Without it the app downloads the real ~1M-row + Explore catalog into the run's `YJ_HOME`, and specs that stage their + own catalog rows then search a million real ones and fail *locally + only*, which reads as a regression and is an environment. Pass + `YJ_CORE_INDEX_URL=` when you want the real catalog to + explore by hand. - **…and the suite spends state it cannot always give back.** `view-lifecycle.spec.ts` **skips an autotag album** on every run, out of the eleven the seed has, and does not put it back — so around the diff --git a/.planning/NOTES.md b/.planning/NOTES.md index 030ffcc..e734a5d 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -2888,17 +2888,44 @@ Three smaller things worth keeping: (`update(el, {})`), which is only visible from `tsc`, not from a failing test. -### A pre-existing failure this uncovered but did not cause +### The local e2e tier was not running the same app CI runs -`requested-badge.spec.ts` fails two of its three tests, **on clean -`main` as well** (verified by stashing every change and re-running). -The symptom is `download.Service.AddRequest` not settling in 10s. +`requested-badge.spec.ts` failed two of three tests locally while CI was +green, and the reason is worth more than the fix: **`dev-headless.sh` +was the only place that did not neutralise `YJ_CORE_INDEX_URL`.** +`seed-sandbox.sh` and `ci.yml` both point it at `127.0.0.1:1`; the dev +launcher did not, so the app downloaded and built the real ~1M-row +Explore catalog into the run's `YJ_HOME`, and a local `make e2e` then +ran against a world CI never sees. -What is now known, and narrows it for whoever picks it up: the same -method with the same arguments, called straight at the runtime endpoint -with `curl`, **returns in 4ms** (it inserted, and answered `2`). So it -is not the backend and not the documented read-pool trap — `Queries` is -built over the writer, and `Reconciler.Trigger` is a non-blocking -select. It is the page-side path: `__yjEvents.call` → the `fetch` hook -→ `/wails/runtime`. A third test in that file fails only *after* those -two, so it is state, not a third bug. +Found by reading the failure screenshot: the spec had searched Explore +for its fixture album and the page was full of *real* ones — Real +Estate, Arrested Youth, The Yes Album. The staged row was there and +invisible among a million others. + +`dev-headless.sh` now defaults the variable to the dead address and +takes an explicit one if you want the real catalog for exploring by +hand. `make e2e` locally: 97 passed / 3 failed before, 100 passed +after. + +The second half of the same problem is that **the backend is one shared +process with one database, and specs leave rows in it.** +`explore-shelves` staged its catalog only `IfEmpty`, so a single album +row left behind by `requested-badge` satisfied that gate, the shelves +were drawn from one foreign row, and the artist card the spec clicks did +not exist. It fails on the *second* local run and passes on the first, +which is the least useful order, and never in CI, where every run gets a +fresh `YJ_HOME`. + +"Is the catalog empty" was the wrong question; "are my rows there" is +the right one. The staging is unconditional now (`INSERT OR IGNORE` +keyed on the MBID) and the assertion moved from *this insert wrote a +row* to *every fixture row is present* — which is both idempotent and a +stronger check, since an MBID failing `CHECK(length(mbid) = 16)` is +silently dropped by OR IGNORE and would otherwise show up as an empty +page rather than a failed setup. + +**Verified: the full suite runs twice against the same app, 100 passed +both times.** That is the property to keep — a spec tier whose second +run differs from its first is a tier that will one day blame the wrong +commit. diff --git a/e2e/specs/explore-shelves.spec.ts b/e2e/specs/explore-shelves.spec.ts index 40aff8d..9b5bae5 100644 --- a/e2e/specs/explore-shelves.spec.ts +++ b/e2e/specs/explore-shelves.spec.ts @@ -26,9 +26,7 @@ import type { Page } from '@playwright/test'; */ test.describe('Explore before anyone has typed', () => { test.beforeEach(async ({ app }) => { - // Idempotent, so running it per test costs one count query when a - // catalog is already there — which is every developer machine. - await stageCatalogIfEmpty(app); + await stageCatalog(app); await app.getByTestId('nav-explore').click(); await expect(app.getByTestId('main-content')).toHaveAttribute( @@ -125,8 +123,7 @@ test.describe('Explore before anyone has typed', () => { }); /** - * Give the app a catalog if it has none, so the empty-index environment - * still exercises the shelves rather than skipping them. + * Give the app the catalog these shelves are written against. * * Deliberately shaped: two artists with albums (one of them with * three), and a third with none. The three albums are what "one album @@ -135,9 +132,23 @@ test.describe('Explore before anyone has typed', () => { * above it and correctly skipped — the first version of this fixture * had only two, and the artists shelf was rightly omitted, which read * as a broken page. + * + * **Unconditional, and it used to ask whether the catalog was empty.** + * "Any rows at all" is the wrong question: the backend is one shared + * process with one database, so a *single* row left by another spec + * file — `requested-badge` stages one album — satisfies that gate and + * this suite then draws a shelf page with no artist card on it and + * times out looking for one. It survives a suite run, so it is the + * second local `make e2e` that fails and the first that passes, which + * is the least useful order. CI never sees it: every run there is a + * fresh YJ_HOME. + * + * The inserts are `INSERT OR IGNORE` keyed on the MBID, so running + * this per test is idempotent, and adding seven low-popularity rows to + * a developer machine's real million-row catalog changes nothing the + * shelves show. */ -async function stageCatalogIfEmpty(app: Page): Promise { - if ((await catalogRows(app)) > 0) return; +async function stageCatalog(app: Page): Promise { // The catalog stores an MBID as its 16 raw bytes and an entity type // as a small integer, so a staged row has to be spelled the way the @@ -185,15 +196,51 @@ async function stageCatalogIfEmpty(app: Page): Promise { expect(result.status, `staging failed: ${result.body}`).toBe(200); // …and `OR IGNORE` means a 200 is not a write. A CHECK the row - // violates is *ignored*, not reported, so the count below is the + // violates is *ignored*, not reported, so the check below is the // only thing that can tell staging from silence. + // + // It is `0 or 1`, not `1`, because this helper is now + // unconditional: the second call of a run legitimately writes + // nothing. What must hold either way is that the rows are *there*, + // which is what the assertion after the loop says — a stronger + // statement than "this insert wrote something", and the one that + // actually protects the fixture. expect( (JSON.parse(result.body) as { rowsAffected?: number }).rowsAffected, - `staged nothing: ${result.body}`, - ).toBe(1); + `staging error: ${result.body}`, + ).toBeLessThanOrEqual(1); } - expect(await catalogRows(app)).toBeGreaterThan(0); + // Every staged row is present, whoever put it there. An MBID that + // fails `CHECK(length(mbid) = 16)` is silently dropped by OR IGNORE, + // and this is where that shows up. + expect(await stagedRowCount(app), 'the staged catalog is incomplete') + .toBe(rows.length); +} + +/** How many of the staged fixture rows are in the catalog. */ +async function stagedRowCount(app: Page): Promise { + const result = await app.evaluate(async () => { + const res = await fetch('/__test/sql', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + sql: `SELECT COUNT(*) AS n FROM explore_index + WHERE artist_name IN ('Staged Alpha', 'Staged Beta', + 'Staged Gamma')`, + }), + }); + + return { status: res.status, body: await res.text() }; + }); + + expect(result.status, `count failed: ${result.body}`).toBe(200); + + const parsed = JSON.parse(result.body) as { + rows?: { n?: number }[]; + }; + + return parsed.rows?.[0]?.n ?? 0; } /** diff --git a/scripts/dev-headless.sh b/scripts/dev-headless.sh index 7851a59..22ff732 100755 --- a/scripts/dev-headless.sh +++ b/scripts/dev-headless.sh @@ -157,7 +157,22 @@ fi # YJ_TESTCTL mounts backend/testctl's /__test/ endpoints. It is opt-in # rather than implied by the dev build so that a human's `make dev` does # not carry an arbitrary-SQL endpoint on a listening port. +# +# YJ_CORE_INDEX_URL points at a dead address, which is what +# seed-sandbox.sh and ci.yml already do and what this script was the +# only one *not* doing. Without it the app downloads and builds the +# real ~1M-row Explore catalog into the run's YJ_HOME, so a local `make +# e2e` runs against a different world than CI: the specs that stage +# their own catalog rows (requested-badge) then search a catalog full +# of real albums, fail to find their fixture, and report it as a +# regression in whatever was last changed. A spec tier whose result +# depends on what a previous run downloaded is not a result -- the same +# rule as the emulator's `-no-snapshot`. +# +# Set YJ_CORE_INDEX_URL yourself to opt back in, for exploring Explore +# by hand. YJ_TESTCTL=1 \ + YJ_CORE_INDEX_URL="${YJ_CORE_INDEX_URL:-http://127.0.0.1:1/none.tar.zst}" \ WAILS_SERVER_PORT="$PORT" \ YJ_LOG_LEVEL="$LOG_LEVEL" setsid dbus-run-session -- \ "$BIN" \