diff --git a/frontend/app/cycle/_workspace.ts b/frontend/app/cycle/_workspace.ts new file mode 100644 index 00000000..c204f5d3 --- /dev/null +++ b/frontend/app/cycle/_workspace.ts @@ -0,0 +1,86 @@ +/** + * Empty this suite's workspace, through the routes the product publishes. + * + * The workspace is built once per *server* start — `scripts/cycle_server.sh` + * `rm -rf`s it and runs `visionset init`, and `playwright.cycle.config.ts` starts + * that script once as its `webServer`. So every attempt after the first inherits + * the one before it, and the walk's third step asserts Home's first-run + * invitation, which is gated on the workspace holding no projects at all. + * Without this, a retry could never reach whatever failed: it died on that + * assertion, three steps in, naming a screen that had nothing to do with the + * failure — and the trace and screenshot a person opens are the retry's. + * + * Deleting through the API rather than the filesystem is what makes it possible + * at all. The server owns the workspace for as long as it runs, and `webServer` + * is Playwright's to start and stop. + * + * A project delete takes its dataset, batches, jobs, sources and releases with + * it; connections are the only other workspace-level row this walk creates. + * Content blobs survive both, deliberately — they are shared and the delete + * route never removes them — so a repeated walk re-ingests the same three + * fixture images into a new dataset rather than into a new store. + * + * Paths here are origin-relative on purpose. This suite's `baseURL` ends in + * `/app/`, because the bundle is mounted under a prefix while the API owns the + * root — and a leading-slash path resolves against that base's *origin*, so + * `/projects` reaches the API where `/app/projects` would not. + */ +import { expect, type APIRequestContext } from "@playwright/test"; + +/** The shape both listings answer with. Neither takes a paging parameter. */ +interface Listing { + items: { id: string; name: string }[]; + total: number; +} + +async function listing( + request: APIRequestContext, + path: string, + headers: Record, +): Promise { + const response = await request.get(path, { headers }); + expect(response.ok(), `GET ${path} answered ${response.status()}`).toBe(true); + const page = (await response.json()) as Listing; + // A cast is not a check. A 200 whose body carries no `items` — a route + // reshaped under this helper, which is the case running on every attempt + // exists to catch — would otherwise throw a bare TypeError on the next line, + // before any message could name the route it came from. + expect(Array.isArray(page.items), `GET ${path} answered without an items array`).toBe(true); + /* + * One read is the whole collection, and that is asserted rather than assumed. + * A reset that quietly cleared only the first page would put this suite back + * exactly where it started — the next attempt failing on a stale screen three + * steps later, which is the failure this file exists to remove and is harder + * to see the second time. + */ + expect(page.items.length, `GET ${path} returned a partial page`).toBe(page.total); + return page; +} + +/** + * Delete every project and every inference connection, and assert each answer. + * + * Loud on purpose: a reset that half-succeeds must fail at the reset, where the + * cause is on screen, rather than leaving the walk to fail somewhere that cannot + * name it. + */ +export async function emptyWorkspace(request: APIRequestContext, bearer: string): Promise { + const headers = { Authorization: `Bearer ${bearer}` }; + + for (const project of (await listing(request, "/projects", headers)).items) { + // The kernel refuses to destroy data without `confirm`, and answers 409. + const response = await request.delete(`/projects/${project.id}`, { + headers, + params: { confirm: true }, + }); + expect(response.status(), `DELETE project ${project.name}`).toBe(204); + } + + for (const connection of (await listing(request, "/inference/connections", headers)).items) { + // No confirmation gate here, unlike a project: nothing holds a key to a + // connection, because an annotation copies its model's identity at write + // time. What is destroyed is a configuration. + const response = await request.delete(`/inference/connections/${connection.id}`, { headers }); + expect(response.status(), `DELETE connection ${connection.name}`).toBe(204); + } +} diff --git a/frontend/app/cycle/cycle.spec.ts b/frontend/app/cycle/cycle.spec.ts index 3b0ca2d3..748b3302 100644 --- a/frontend/app/cycle/cycle.spec.ts +++ b/frontend/app/cycle/cycle.spec.ts @@ -39,6 +39,7 @@ import { readFileSync, readdirSync } from "node:fs"; import path from "node:path"; import { saveNow } from "../e2e/_frame"; +import { emptyWorkspace } from "./_workspace"; const CYCLE_DIR = process.env["VISIONSET_CYCLE_DIR"] ?? ""; @@ -84,37 +85,47 @@ const REPINNED = "built-in stand-in, repinned"; * A project name nothing else in the workspace will collide with — **including * this same spec on another repetition**. * - * A project name is unique per workspace, case-insensitively, and the workspace - * outlives a repetition: `scripts/cycle_server.sh` rebuilds it once per *server - * start*, and `--repeat-each` reuses that one server. So the fixed literal that - * used to live here made the flag useless — repeat 2 died on - * `POST /projects → 409`, a wall standing in front of everything the suite is - * about, and the only way to run the cycle twice was two whole invocations at - * about ninety seconds of rebuild each. + * A project name is unique per workspace, case-insensitively, and a fixed + * literal here used to make `--repeat-each` useless: the workspace outlived a + * repetition, so repeat 2 died on `POST /projects → 409` before reaching + * anything the suite is about. The connection name had the same problem one + * screen later and nobody had noticed it, which is the argument against curing + * a collision one name at a time. * - * `repeatEachIndex` **and `retry`**, because a retry is the same repetition run - * again into the same workspace. Scoping only the first turns one readable - * failure into two unreadable ones: a genuine failure leaves its project behind, - * the retry dies on `POST /projects → 409`, and the report names the 409 — which - * is the exact wall the scoping exists to remove. The workspace really is fresh - * per invocation (the script - * `rm -rf`s it before `init`) and `workers: 1` means two repetitions never - * overlap, so those two indices are the whole of the uniqueness needed. + * Neither needs curing now. Every attempt begins on an empty workspace — see + * the `beforeEach` below — so no name in this walk collides with a name from + * another attempt, and none of them has to move. * - * The suffix is unconditional rather than omitted on the first, so every run's - * names have one shape and a failure message reads the same way whether or not - * somebody passed the flag. - * - * The project is the only name that has to move, and that is worth stating so - * the next collision is looked for rather than assumed: a release tag is unique - * per dataset, a batch name is not unique at all, and a source's idempotency key - * `(project, kind, path, fps)` leads with the project. All three are already - * scoped by a project that is new. + * The suffix stays anyway, and is unconditional, because it costs nothing and + * it makes a failure message name the attempt it came from. That is worth more + * now than it was: a retry can finally produce a second, *different* failure, + * and two reports that name the same project are two reports somebody has to + * tell apart by hand. */ function projectFor(info: TestInfo): string { return `browser-cycle-${info.repeatEachIndex}-${info.retry}`; } +/** + * Every attempt starts on an empty workspace — a retry included, and every + * repetition of `--repeat-each`. + * + * On the first attempt of a freshly built workspace this deletes nothing and + * costs two reads. It exists for the second: the workspace is rebuilt once per + * *server* start, not once per attempt, so without this a retry inherits the + * previous attempt's projects and dies on Home's first-run invitation — an + * assertion about the workspace, three steps in, unrelated to whatever actually + * failed. + * + * Unconditional rather than guarded on `retry`, because a repair that only runs + * on the rare attempt has the same property as the defect it repairs: nothing + * exercises it until the day it matters. Run every time, its two reads are + * proved by every run of this suite. + */ +test.beforeEach(async ({ request }) => { + await emptyWorkspace(request, token()); +}); + test("the whole cycle, from opening the app to a downloaded export", async ({ page }, info) => { test.slow(); @@ -532,11 +543,31 @@ test("the whole cycle, from opening the app to a downloaded export", async ({ pa // Born not set up, like every local connection, and made ready by the same // action — the lifecycle here is the real one, not a shortcut written for a // suite. What is different is only that there is nothing to fetch. - await expect(page.getByTestId("connection-status")).toContainText(/not set up/i); - await page.getByTestId("download-weights").click(); - await expect(page.getByTestId("connection-status")).toContainText(/ready/i, { + + // Read inside the row rather than off the screen. Both ids live inside a + // connection's own row, so an unscoped read is sound only while the + // workspace holds exactly one — and Playwright's strict mode then refuses + // the locator, naming the selector instead of the assumption. + const row = page.getByTestId(`connection-${STAND_IN}`); + await expect(row.getByTestId("connection-status")).toContainText(/not set up/i); + await row.getByTestId("download-weights").click(); + await expect(row.getByTestId("connection-status")).toContainText(/ready/i, { timeout: 15_000, }); + + /* + * One connection, asserted rather than assumed. + * + * The suggest panel further down names this connection in a sentence and + * offers a picker from two upwards, so a second connection breaks that + * assertion as a *missing string* — pointing at the suggest panel while the + * cause is an extra row on this screen. Stated here, where the count is + * decided, a violation names the count. + * + * The type badge is what is counted because it is unconditional inside a + * row, while the download button appears only before setup. + */ + await expect(page.getByTestId("connection-type")).toHaveCount(1); }); await test.step("a click in the editor comes back as a shape, from a real server", async () => { @@ -1380,7 +1411,10 @@ test("the whole cycle, from opening the app to a downloaded export", async ({ pa await page.getByTestId("rail-inference").click(); await expect(page.getByTestId("inference-screen")).toBeVisible(); - await expect(page.getByTestId("connection-status")).toContainText(/ready/i); + // Two locators because the row's id is its name and the rename moves it. + const before = page.getByTestId(`connection-${STAND_IN}`); + const after = page.getByTestId(`connection-${REPINNED}`); + await expect(before.getByTestId("connection-status")).toContainText(/ready/i); // A rename, which sends the model reference the row already has. That it is // *accepted* is the half that would have caught a PATCH carrying the kind; @@ -1395,8 +1429,8 @@ test("the whole cycle, from opening the app to a downloaded export", async ({ pa // was typed — so its absence is the first thing that says the server took // the body. await expect(page.getByTestId("connection-dialog")).toHaveCount(0); - await expect(page.getByTestId(`connection-${REPINNED}`)).toBeVisible(); - await expect(page.getByTestId("connection-status")).toContainText(/ready/i); + await expect(after).toBeVisible(); + await expect(after.getByTestId("connection-status")).toContainText(/ready/i); // And a reference that really does move. The revision is free text on a // connection — the commit-hash rule belongs to catalog entries, which is why @@ -1408,11 +1442,11 @@ test("the whole cycle, from opening the app to a downloaded export", async ({ pa await page.getByTestId("connection-submit").click(); await expect(page.getByTestId("connection-dialog")).toHaveCount(0); - await expect(page.getByTestId("connection-status")).toContainText(/not set up/i); + await expect(after.getByTestId("connection-status")).toContainText(/not set up/i); // The remedy is offered on the row it happened to, which is the other half // of what "undoes its setup" is allowed to mean. - await expect(page.getByTestId("download-weights")).toBeVisible(); + await expect(after.getByTestId("download-weights")).toBeVisible(); }); await test.step("the whole walk produced a clean console", async () => {