test(ui): run the live Playwright suite in CI against the built UI image - #2874
Draft
toreysoloio wants to merge 20 commits into
Draft
toreysoloio wants to merge 20 commits into
toreysoloio wants to merge 20 commits into
Conversation
`ui-tests` proves the UI is self-consistent, not that it works: it runs on the in-browser mock, so a backend contract change cannot fail it. That is how kagent-dev#2638 emptied three Substrate fields with every check green. Closes kagent-dev#2657. The lane runs in `test-e2e`, after the Go end-to-end tests and on the same cluster, reusing what that job already stands up — including the `smoke` agent the schedules spec needs. The UI image joins the fan-out, `ui.replicas` is no longer zeroed, and the address is the `kagent-ui` service's MetalLB IP, the chart already publishing it as a LoadBalancer. `UI_LOOP_LIVE_URL` points the suite at an app it did not start, so what runs is the shipped artifact: nginx, the SPA fallback, and the `env-config.js` rendered from the pod's environment at start, none of which exists in front of `yarn dev`. Two invariants worth stating, both learned the hard way: - `build-ui` and `build-golang-adk` both depend on `proto-generate`, and `buf generate` writes `ui/src/generated` as well as `go/api/gen`. Run in parallel, one make writes into the directory the other is tarring as a build context, so it runs once up front with `-o proto-generate` on the fan-out. - A green live run has to have been live. Against the image there is no build-time `VITE_API_MODE` pin, so `globalSetup` asserts `/mockServiceWorker.js` 404s — which says both that fixtures cannot be served and that this is the built artifact rather than a dev server that would pass every spec. Turning the lane on meant fixing the suite it runs, because nothing had run it and every spec had rotted. `agent-lifecycle.spec.ts` drove `/agents/new`, a page removed long before, for an agent nobody creates — replaced by `agent-templates.spec.ts`, which asserts the property only a cluster can settle: admission is read from the template's status and cannot be computed in the browser. `schedules.spec.ts` drove a dialog for an editor that is a page, and named CI's fixture rather than `setup-cluster.sh`'s, so it could only pass where nothing ran it. `substrate.spec.ts` read a tile once, catching the em-dash drawn before the data lands. Two harness faults came out of the same run: `selectFirstOption` could resolve an option from a dropdown that had just been dismissed, and the live project now runs `workers: 1`, these specs sharing one cluster where every mock test owns a backend in its own page's memory. Verified against a Kind cluster running the image built from this checkout: 7 live specs pass, and the mock suite is unchanged at 159. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: Torey Scheer <[email protected]>
A create, an edit and a delete are where a fixture and a controller most easily disagree — a create wrapped one way in the fixtures and another by the API, a name sent where a ref belonged. Those journeys ran only against the fixtures, which answer whatever they were taught. `playwright/shared/` is claimed by every project, mock and live alike, and holds the portable half: the write journey for models, prompts and harnesses, plus two properties true whatever the backend holds — no conversation listed by a bare id, and a deep link resolving on a cold load. It is laid out like `tests/`, one folder per resource and app-wide specs at the root, and `conventions.test.ts` enforces that along with the rule that makes it work: a shared spec may not reach for `?mock=`, `withScenario` or `helpers/mockCalls`. Three invariants earned the hard way, each recorded where it applies: - **Counts are relative, never absolute.** The fixtures seed four models and a cluster seeds whatever it was installed with, so `before + 1` is the only form of the claim that survives both — and it is exactly as strong. - **antd draws its loading placeholder as a `tbody tr`.** Counting those read a seeded set of one for a table that had not loaded; data rows are `tbody tr.ant-table-row`. - **A one-shot read is not an assertion.** Two specs failed reading a value while it was still arriving, in opposite directions — the dashboard card against the mock, a substrate tile against the cluster. Both poll now. The duplicated driving is gone from `tests/`: those specs keep what only fixed data can settle — the seeded rows, the refresh counts, the required-field marks, and the empty and failure states no cluster can be asked for — and their titles say so. `routing` and `dashboard` are split the same way, with the SPA-fallback steps moving to `shared/` because nginx and Vite answer a deep link differently and only one of them ships. MCP servers are deliberately absent: that page cannot read its own writes against a real backend (kagent-dev#2849), and a shared spec would have had to press Refresh to get past it. `DEFERRED.md` records the deferral and what closing it looks like. `live/helpers/live.ts` keeps only what is live-specific. `loadApp`, `expectNoLoadFailure`, `rowNamed`, `dataRows` and `throwawayName` moved to `helpers/app.ts`; the first adds the mock scenario only where there is a mock backend to read it, and the two row helpers had drifted to different selectors in the two suites for no reason anybody could name. Verified against a kind cluster running the image built from this tree: 12 live specs and 169 mock specs pass, and the cluster is left with no stray resources. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: Torey Scheer <[email protected]>
…he reload `agent-templates` and `schedules` were the two resources whose journey still ran twice — once against the fixtures in `tests/`, once against a cluster in `live/` — which is the duplication the previous commit removed everywhere else. Both move to `shared/`, and what is left behind in each place is what only that place can answer. **Schedules split on a reload.** Moving the live spec wholesale did not work, and the reason is worth keeping: step 4 reloads, and the fixture backend holds writes in the page's own memory, so a reload starts a backend that has never heard of the schedule. That reload is not incidental — it is the whole claim, since everything short of a re-read could be the form showing itself its own draft. So the journey is `shared/schedules/`, clicking through on either backend, and `live/schedules.spec.ts` keeps only the persistence: a fractional timeout stored as seconds plus nanos, and a named IANA zone, both still there after a reload. The general rule is now in the README — a spec whose claim is "the backend stored it" needs a backend that stores. **Agent templates gained the edit they never had.** The shared spec created and deleted but never updated, so no suite was asserting an update against a real controller. It now changes the description in place and reads it back off the page, and it picked up the namespace-narrowing assertion the mock create was making — the line that once carried two faults at once, a redirect with no query string and a caller sending `namespace` where the list narrows on `ns`. The mock specs keep what needs fixed data: the seeded rows, the cadence and history of a schedule and the pausing that is unique to it, the template list's narrowing and sorting, the form's refusal of an unusable template, both branches of the delete warning, and the empty and failure states. Their titles say what they now cover. One thing the trim exposed: the agent-template delete was doing double duty, since removing the template also navigated back to the list, and the step after it relied on that. The hop is explicit now. The create stays in the mock template spec as setup rather than as a claim. Removing it would mean pointing the read-and-edit steps at the only seeded template that has a harness, and a later step asserts that template's description unedited — the note on `CREATED` records that, so the trade is not rediscovered. Verified against a kind cluster running the image built from this tree: 13 live specs and 173 mock specs pass, and the cluster is left with no stray resources. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: Torey Scheer <[email protected]>
Moving each write journey to `shared/` left five headers describing a spec that no longer exists, and three were stale beyond the opening line: - `agent-templates` said the created template "survives all the way to the delete that removes it" — that delete moved two commits ago. - `harnesses` carried both "There is no update half" and "Nor is the write journey here", two ways of saying the same thing once the trim had landed. - `routing` said the login route needs something to log in to. It does not: the route was tried against a deployment and passes. It stays on the fixtures for a different reason, which the header now gives — `shared/routing` already proves the SPA fallback on `/substrate` and an unknown path, so `/login` earns nothing by running twice. Shorter as well as truer, 130 lines out for 65 in. What each keeps is the knowledge that cannot be recovered from the code — why a prompt save replaces the whole ConfigMap `data` map, why `ready: false` must not be drawn as "broken", why a schedule's history outlives the schedule, and the cluster observation behind the agent-template admission property. What went was boilerplate: every one of them repeated that a video and a trace are recorded per test, which `README.md` says. Comments only; no assertion, selector or title changed. 173 mock specs pass. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: Torey Scheer <[email protected]>
Six defects, most of them only reachable against a backend slower than the
fixtures — which is to say, only in CI.
**Counts were of rows on screen.** The lists page at 25, so on a cluster holding
more than a page the created row is on page two: present, correct, and invisible to
a locator. The totals now come off the `<list>-summary` line, and `searchList`
narrows to the name this run invented so the row is found wherever it landed. The
harness tab keeps counting rows, which is right there — it sets `pagination={false}`.
**`LIFECYCLE_TIMEOUT` was the tightest budget in the live run.** A
`describe.configure` overrides the per-test default, so a shared lifecycle asking
for sixty seconds was undercutting live's own hundred and twenty. It is 180s live,
60s mock.
**Cleanups skipped their deletes.** A `count()` taken straight after a navigation
reads zero because the page has not answered yet, so "is it still there?" always
said no and the resource stayed on the cluster. `appeared()` waits first, and
returns `false` rather than throwing because a throw in a `finally` replaces the
failure the test was reporting. It treats a timeout as an answer and anything else —
a locator matching several, raising strict mode — as a broken check, said out loud
rather than silently. A mistyped test id cannot be told apart and the comment says so.
**The uploaded CI artifact was near-empty.** `github` writes annotations and no
files, so the report path had nothing in it and `if-no-files-found: ignore` hid that.
The config asks for `html` alongside it.
**`LIVE_EXTERNAL_URL` was read three inconsistent ways** — `??`, `!== undefined` and
truthiness — so an empty string meant "deployed" to one and "not deployed" to
another: it would have pointed `baseURL` at `""`, checked that for nginx, *and*
started a dev server nothing would visit. Normalised at the source.
**The one-harness admission read was a race.** Those labels are applied in an effect
that lands after the buttons first render, so a single read could take the pre-effect
state and go down the "nothing admits it" branch on a cluster where something does.
The button's disabled state is the settled signal in both directions.
Two more worth stating. The live admission assertion leaned on the controller having
reconciled before the navigation — that page fetches once on mount, so a longer
timeout only re-read a DOM that was never going to change; the reload is the refetch
now. And widening the ESLint spec rules from `tests/` to `{tests,shared,live}` found
`live/pages` and `live/substrate` importing from `@playwright/test`, so neither had
the console-error guard, in the only suite that talks to a cluster.
CI gains a gate on the `smoke` template existing. It is applied part-way through the
Go step, so a step that died before that apply would fail the browser journeys three
times each against a cluster with no agent to schedule.
Verified against a kind cluster running the image built from this tree: 13 live and
173 mock specs pass, with no stray resources left behind.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
An audit of what each test actually asserts, rather than whether it is green.
Seven passed while claiming more than they checked.
The live sweep's error check ran before the page had asked the controller
anything, so its claim was "the shell rendered". Gating on each page's summary
was not enough either: it renders for a frame before the answer lands, and a
read delayed 1.5s and then failed passed both orders while the page ended up
saying "Could not load agents · HTTP 500". A networkidle gate never settles,
SWR retrying the failed read. So the sweep watches responses instead — any
non-2xx under /api/, asserted per page and again at the end, which is the one
that catches a failure landing after its page was checked.
Stale claims in the same file: `not.toContainText("__NS__")` under "resolves
its model", a string this repo has never rendered, on a table that has had no
model column since the rewrite; and a "tool count" step that only re-checked
`agents-error`. They assert the harness each row runs on, the provider tag, and
the summary's numbers rather than the static word "server".
`shared/harnesses` asserted that an unpinned image is refused while the
snapshot location was also empty — the button was disabled either way, as the
next line proved by leaving it disabled with a valid digest. It now fills a
complete draft, asserts the button is enabled, and breaks one field at a time.
The "admits no templates" warning is asserted to appear, which nothing did.
`shared/dashboard` ties its label loop to the state the poll settled on: a
clean cluster has no conversations, so in CI the loop body never ran.
Four guards named test ids that have never existed in src, and so could not
fail however the control came back. They read the chrome's own ids, search the
dialog by role, and assert there is no switch on the page. Substrate names its
five tiles rather than one invented absence.
Verified by mutation, each reverted: a create-looking id added to the header
fails the shell guard, widening the dialog regex to a control that is there
fails on count 1, and the induced late failure now fails the sweep in 2.2s.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
…from nothing Six findings from the review of the previous commit, each verified before and after. The live sweep watched for a failure the controller does not produce. The app speaks gRPC-Web, so a refused read answers HTTP 200 and puts the reason in a `grpc-status` trailer — measured against a cluster, a missing library came back `200 ok=true grpc-status=5`. Only nginx's own 502s would ever have reached the non-2xx check, so it reads both now. `pressUntil`'s budget was shorter than one attempt in the live project, where `settled` is an assertion on a thirty-second `expect` timeout. Measured: a 20s attempt under a 15s budget runs exactly once, under 90s it runs five times. So every live Delete was a single press with no retry, which is the whole reason that helper exists, and a swallowed one left the resource on the cluster. Five cleanups called `loadApp`, `searchList` or `page.goto` inside a `finally`, all of which throw — replacing the failure the test was reporting and skipping the delete underneath. They go through `helpers/cleanup.ts`, which warns. `appeared` was written for this and the blocks around it were not. `live/schedules.spec.ts` is a journey on the global budget while every shared one takes `LIFECYCLE_TIMEOUT`; the agent-templates journey read its row off an unnarrowed list that pages at 25, which is what `searchList` exists for; and six `expectNoLoadFailure` calls sat where they always pass. The helper now says what it must be called after. Verified on a cluster built from nothing: 13 live, 173 mock, 498 unit. The fresh run exercised what the old one could not — the dashboard's empty branch (no conversations, so the naming claim is untested there and now says so), the schedules empty-state gate, and lists of one. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: Torey Scheer <[email protected]>
Second review round, five findings, each checked before acting on it. The live admission poll could not retry. `expect.poll` calls its callback outside its own try/catch, so a throw ends the poll rather than failing one round — measured: a callback that throws twice then succeeds gives up after 4ms, where one that returns is retried to its deadline. Both `textContent` on a page still drawing its Skeleton and `expectSettled` after the reload throw, so the ninety-second budget and its message were decoration. The callback now returns the reason it could not answer. The harnesses cleanup was the last `finally` still calling `loadApp` and `confirmDelete` directly, which is what `helpers/cleanup.ts` exists to stop: thrown from there they replace the failure the test was reporting and skip the delete underneath. `expectNoLoadFailure` matched `-error` only, while four pages say a failed read with `-unavailable` instead — the dashboard's recent list, the schedules table, the tools chart, the schedule history. Each of those pages happens to draw its `-error` sibling from the same condition, so nothing was slipping through today; matching both means the claim no longer rests on that. Per-assertion budgets were sixty seconds against a mock `LIFECYCLE_TIMEOUT` of sixty, so an assertion could never exhaust its own budget and a broken one was reported as "Test timeout exceeded" rather than by name. `READ_TIMEOUT` sizes a read below the journey that contains it. And the CI gate: five UI steps hung on one un-retried `kubectl get`, where a transient hiccup skipped the whole browser lane with the job still green — the shape of kagent-dev#2638. It retries, and a lane that does not run says so with a warning annotation. Verified: 13 live against a cluster built from nothing, 173 mock, 31 conventions. A dashboard whose read was aborted now fails the sweep. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: Torey Scheer <[email protected]>
Every list page drew its summary on `!error && !isLoading`, and SWR reports `isLoading` false on the first paint — its fetcher runs in an effect, after it. So a page that had not yet asked the controller anything rendered "0 of 0 harnesses" and held it there. Measured on the harnesses tab at 300ms intervals: "0 of 0" for the first 600ms, then "4 of 4". A count of nothing is a claim, and until the read lands the page has not earned it — the same distinction this codebase already makes about a failed read, which must not be drawn as "there is nothing here". The summary now waits for `data`, so the six lists say nothing until they have something to say. Found by the live suite: a journey that counted the tab before creating a harness read 0 against a tab holding four, and then asserted "one more than nothing". Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: Torey Scheer <[email protected]>
…CI for what it has not got Third review round, four findings, each measured before acting. The tool-server assertion could not pass in CI. The e2e job installs with `--set kagent-tools.enabled=false --set grafana-mcp.enabled=false`, and the chart's only RemoteMCPServer is gated on the first, so that cluster has none — where `setup-cluster.sh` leaves the default and has two. "At least one server", added last round to stop the assertion being vacuous, would have failed every CI run while passing on a laptop. Both counts are now required to be numbers the page computed, and the per-server check runs only where there are servers. Cleanup moved out of `finally` and into `afterEach`. Measured with a four-second test: after a timeout the body's `finally` is refused with "Target page, context or browser has been closed" while a hook still drives the page. A timed-out journey is the likeliest live failure and was the one case these blocks could never handle — so every resource they were meant to remove stayed on the cluster. Three journeys waited for a row before counting, which quietly required the cluster to already own one of that resource; a shared spec may not assume that. They read the total off the summary, which renders at "0 of 0" for a read that succeeded — and which now only renders once the read has landed, the flash that exposed being fixed in the commit before this one. The total is still read twice and has to agree with itself. And `expectListTotal`, `expectListLoaded` and `appeared` kept their hardcoded sixty seconds while their callers moved to `READ_TIMEOUT`, so a stuck read still reported the test timeout rather than naming what it waited for. Verified: 173 mock across both engines, 13 live against the fresh cluster running an image rebuilt from this branch, 498 unit, 31 conventions. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: Torey Scheer <[email protected]>
A `/* … */` written among a JSX element's children is a text node, not a comment. The gating comment added in 8bae718 landed inside `trailing={…}` on three pages, where it is a comment, and among a `<Space>`'s children on the other three, where it is not — so Agents, Templates and Harnesses each rendered a paragraph about SWR into their filter bars. Nothing caught it: `tsc` and eslint both accept it, and no assertion in this suite read that region. 173 mock, 13 live and 498 unit tests all passed over it. So the pages are now asked directly, in `app-shell`, over all six surfaces including the two tabs — two of the three were tabs. The check waits for each page's table before asserting the absence, which is the difference between a check and a formality: asked on arrival it passed over a harnesses tab broken on purpose, because "there is no source on this page" is true of a page that has not drawn yet. With the wait it fails, naming the route. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: Torey Scheer <[email protected]>
Two of the fixes in this branch reached around a tool instead of for it. `expect.poll` ends on a throwing callback, so the live admission check wrapped its callback in a try/catch that returned a string to keep the poll alive. `toPass` is the primitive for retrying a callback that throws — `pressUntil` was already using it two files away. The failure is now the assertion's own rather than a sentence composed to survive the wrong mechanism. And the list total was read twice and required to agree, because every summary drew "0 of 0" for the 600ms before its read landed. That is fixed where it belongs now, so the double read was a test carrying a workaround for a bug that no longer exists. One read again; a return of the flash fails the journeys that count before and after a create, which is how it was found. Two rules added to the README for the pattern behind most of this branch's defects: never assert an absence before the thing could appear, and clean up in a hook rather than a `finally`. Verified: 173 mock, 13 live, 31 conventions. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: Torey Scheer <[email protected]>
Three conflicts, each resolved toward the side that still holds: - `ci.yaml` timeout: main raised its own to 40; the UI image and browser suite add about ten, so 50. - `ci.yaml` builder and cache: main's, whole. It now reuses Blacksmith's persistent builder rather than creating one, which supersedes the create-once-before-the-fan-out care this branch added, and it dropped the per-image gha cache with it. `-o proto-generate` stays: that is about two builds writing `ui/src/generated` at once, not about caching. - `live/schedules.spec.ts`: this branch's. Main's is the older whole-journey version driven by labels; here the journey lives in `shared/` and this file keeps only the claim a reload can settle. - `tests/substrate/inventory.spec.ts`: this branch's named tile set, which catches an ate-api tile returning under any id, where main asserts one absent. `live/agent-lifecycle.spec.ts` stays deleted: this branch moved that journey to `shared/agent-templates/`, and main has not touched the file since the base. Verified on the merged tree: 183 mock across both engines, 530 unit, lint and typecheck clean. The live lane is not verified here — main moved substrate from 0.0.29 to 0.2.0-beta2, so the cluster this branch was tested against no longer matches what CI installs. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: Torey Scheer <[email protected]>
The merge took this branch's `live/schedules.spec.ts` over main's, which asserted two things nothing else here did. Both are answerable on either backend, so they land in the shared journey rather than where they came from. A blank time zone is not blank — the form stores `timeZone || "UTC"` — so the cadence line has to say which it is. Filled, cleared, and put back, one field at a time. With the fallback removed the line reads "Weekdays at 09:00 ()", which is what a reader would have been left to interpret. And a schedule that has never run says so. The page keeps three states apart — no executions, no search match, and a failed read — and the mock spec could only ever assert the absence of the first; a journey that creates a paused schedule is the one place it can be produced. Verified on a cluster rebuilt on substrate 0.2.0-beta2, which is what main now installs: 13 live, 183 mock, no resources left behind. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: Torey Scheer <[email protected]>
Read against kagent-dev#2828, which tracks these entries and asks that each be closed or re-argued when its blocker moves. This branch moves one of them. Two entries said the missing coverage "belongs in `playwright/live/`". That lane now exists and runs in CI, so on a plain reading they are ready to write — and whoever tried would find the real obstacle underneath: an `AgentInstance` exists only once a message has been sent, a streamed reply needs a model that answers, and neither cluster has one. CI installs with `OPENAI_API_KEY: fake` and `setup-cluster.sh` sets no key at all. Both entries now say so. And the cleanup note claimed each live spec deletes what it made in a `finally`, which is both out of date and the opposite of what was measured: a timed-out test has a closed page, so a `finally` cannot clean up after the failure most likely to leave something behind. They are hooks now. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: Torey Scheer <[email protected]>
Read against kagent-dev#2828. The entry deferring this said the approval controls needed a product decision. That decision landed in kagent-dev#2714 and the controls shipped with it — per-tool approve and reject, a reason on a rejection, a submit — and `chat-approval` appeared nowhere in `playwright/`. Not a deferred gap: an untested surface behind a note that had gone out of date. Two scenarios carry it, both parking the turn at the `ChatClient` boundary the `asks` scenarios already park it at. That is the seam the entry argued for keeping; what stays out is wire frames, which unit tests cover over real bytes. `approves` offers two tools so one can be allowed and the other denied in a single submission — a single-tool fixture cannot show that the controls belong to their row rather than to the form. The fixture reads the decisions out of the payload it received and says them back, so the spec asserts what went to the agent rather than what the form looked like. Checked by breaking it both ways: sending every decision as an approval fails on the rejected tool, and dropping the rejection reason fails on the reason. `asks-unknown` parks on a request this build cannot render — a turn started without the HITL extension, carrying prose and no correlation id. The page says so and offers only the discard, and that sentence was untested too. Verified: 186 mock, 530 unit, 31 conventions. One firefox failure per full run, a different test each time and passing 6/6 in isolation, at load average 50-58 with the kind cluster running; it happens with these changes stashed, so it is the contention the config documents rather than these. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: Torey Scheer <[email protected]>
…vered One tool is a different prompt rather than a shorter one — Approve and Reject on the prompt itself, a rejection opening its reason with the caret in it — and the two-tool fixture cannot reach any of it. A second scenario covers it, and the approval surface now has no undriven control. Then the file itself. Three sections recited what the chat, MCP server, prompt and form-validation specs cover, which those specs describe better; they are one paragraph now, keeping the lesson they carried — two of them had been listed as blocked on pages that already existed, and a stale "blocked on" costs more than no entry. The approval entry keeps its argument and loses its recital. One correction rather than a compression: the entry on what was lost when agents became AgentInstances says no single surface shows an agent's readiness reason end to end. True, and neither half is covered either — `AgentDetailsPage`, the record with `instance-state`, `instance-operation` and `instance-failure`, has no spec at all. Found by listing every test id in `src` against every one the suite references: 188 of 490 are never driven, and that page is the largest of them. Verified: 36 chat specs, 530 unit, 31 conventions. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: Torey Scheer <[email protected]>
A file whose only value is being true, checked claim by claim: every path, identifier, spec name and request shape it names, then the prose. Most of it holds. `ListModelConfigsRequest` really is empty and `ListPromptTemplatesRequest` really does take only a namespace, so the paging table is accurate; `GetSubstrateStatus` really is gone; `ListTasks` is there to carry the auto-titling argument; the MCP entry's two `Refresh` presses are still in the spec, so kagent-dev#2849's acceptance test still reads. Of the three files it names that do not exist, two are the point — old specs it records as lost. Three corrections: - "Not started by request: app extension-point specs" said the opposite of the paragraph two above it, which lists them as covered. Both specs exist and run, and between them assert every point the app declares. - The cleanup row still said each live spec removes what it made in a `finally`, corrected everywhere else in this file already. - `agent-lifecycle.spec.ts` is named for a fault it had; it is gone, replaced by `shared/agent-templates/`, and now says so. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: Torey Scheer <[email protected]>
Found by listing every test id in `src` against every one the suite references: `AgentDetailsPage` was the largest undriven surface in the app. Its fixtures were already there — `instances.failed` is commented "with a reason the conversation's record page shows" — so the fixture was written for this page and the spec never followed. Two tests. The record reports its state in words rather than in the controller's vocabulary, says what that state means, and links to the agent and the template instead of restating what they hold — which is the claim `DEFERRED.md` makes for why replacing the old agent details page was not a reduction: an instance has no spec. Then the three readings the page must keep apart: a failure carries the reason it failed, somebody else's conversation reads as not found because an instance is read as its creator, and a read that failed offers a retry rather than a way back to the list. Checked by breaking both substantive claims: pointing the agent link at the list fails the href, and showing the wire value in the state tag fails on "ready" where a word was expected. Verified: 191 mock, 31 conventions. The two firefox checkpoints failures in the full run pass 6/6 in isolation and were already shown to fail with these changes stashed — the contention the config documents. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: Torey Scheer <[email protected]>
…from nothing Seven findings, none blocking, two with teeth. `detailURL` was recorded after the heading assertion, so a create that reached the controller while the detail page was slow to draw left the hook with no address — a real Schedule on the cluster, which is the litter the hook exists to prevent. Both schedules specs record the address first now. And the fixture had grown a second payload parser that drifted permissive: it took an empty `approvals` array and an empty `rejection_reason` where the app's own reader rejects both, so it would have acknowledged payloads the app refuses. It uses `readToolApprovalResponse` now. The rest: the live sweep walks eight pages on one budget and had the project default, so a slow cluster reported "Test timeout exceeded" rather than naming the page; `agent-details` applied its mock scenario twice and worked only because `URLSearchParams.get` returns the first; an orphaned comment; and two documents each contradicting something true in the same file — the README said `finally` eleven lines above the rule saying never `finally`, and a CI comment claimed `buildx-create` runs once when nothing passes it `-o`. Verified: 193 mock across both engines, 13 live against a cluster deleted and rebuilt from nothing on substrate 0.2.0-beta2 with images from this tree, 530 unit, 31 conventions. No `e2e-live-*` left on the cluster afterwards. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: Torey Scheer <[email protected]>
github-actions
Bot
added
testing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 written by Claude (start)
The browser suite only ever ran against fixtures, so nothing checked the app an operator actually gets.
playwright/live/now runs in thetest-e2ejob against the image built fromui/Dockerfile— nginx, the SPA fallback and the runtime config — on the same cluster as the Go end-to-end tests.Closes #2657
Refs #2828
What runs where
tests/is mock-only andlive/needs a cluster;shared/is the journeys that assert the same thing against both, one create-read-edit-delete per resource. That split is the point: a spec inshared/may not name a fixture or a?mock=scenario, whichconventions.test.tsenforces.The product fix in here
Every list page drew its summary on
!error && !isLoading, and SWR reportsisLoadingfalse on the first paint — so a page that had not yet asked the controller anything rendered "0 of 0 harnesses" for about 600ms. The six summaries now wait fordata.The deferred coverage in #2828
tests/chat/approvals.spec.tsdoesAlso closed: a turn parked without the HITL extension, and three claims in
DEFERRED.mdthat had gone stale — including one section saying the extension-point specs were never started, two paragraphs below the list of what they cover.Testing
Mock suite, no cluster needed:
cd ui && yarn test:pwLive, against a real backend:
scripts/setup-cluster/setup-cluster.sh— it leaves the UI onlocalhost:8080.cd ui && UI_LOOP_LIVE_URL=http://localhost:8080 yarn test:pw:livekubectl get agenttemplate,harness,modelconfig -A | grep e2e-liveshould find nothing, since each journey removes what it made.In CI, the
test-e2ejob runs the same lane against the LoadBalancer address and uploadsplaywright-report/on failure.🤖 written by Claude (end)