diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 55af445d23..cdd4acc0e9 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -42,9 +42,7 @@ jobs: VERSION: v0.0.1-test SUBSTRATE_VERSION: 0.2.0-beta5 runs-on: blacksmith-4vcpu-ubuntu-2404 - # 50: main's own 40, kept as headroom. The job itself takes around 10 minutes, - # UI image and browser suite included, on a warm buildx cache. - timeout-minutes: 50 + timeout-minutes: 40 steps: - name: Checkout repository uses: actions/checkout@v6 @@ -120,6 +118,7 @@ jobs: KMCP_ENABLED: "false" KAGENT_HELM_EXTRA_ARGS: >- --cleanup-on-fail=false + --set ui.replicas=0 --set kagent-tools.enabled=false --set grafana-mcp.enabled=false --set controller.substrate.enabled=true @@ -137,21 +136,11 @@ jobs: BUILDX_BUILDER_NAME=$(docker buildx inspect | awk '$1 == "Name:" { print $2; exit }') test -n "$BUILDX_BUILDER_NAME" export BUILDX_BUILDER_NAME - # Before the fan-out. Each `build-*` runs it again — it is not passed `-o` the - # way `proto-generate` is below — which is harmless only because the target - # documents itself as safe to run concurrently. make buildx-create - # Likewise once, and for a sharper reason: `build-ui` and `build-golang-adk` both - # depend on it, and `buf generate` writes `ui/src/generated` as well as - # `go/api/gen` — so in parallel one make would write into the directory the - # other is tarring as a build context. `-o proto-generate` below says it is done. - make proto-generate - # `ui` is here so the browser suite below runs against the image an operator - # gets: nginx, the SPA fallback, and the env-config.js rendered at pod start. - printf '%s\n' controller ui golang-adk claude-harness codex-harness byo-a2a | xargs -P5 -n1 bash -c ' + printf '%s\n' controller golang-adk claude-harness codex-harness byo-a2a | xargs -P4 -n1 bash -c ' image="$1" DOCKER_BUILD_ARGS="--platform=linux/amd64 --push" \ - make -o proto-generate GIT_COMMIT=e2e BUILD_DATE=1970-01-01 "build-${image}" + make GIT_COMMIT=e2e BUILD_DATE=1970-01-01 "build-${image}" ' _ make helm-install-provider kubectl rollout status deployment/kagent-controller -n kagent --timeout=120s @@ -192,111 +181,6 @@ jobs: export KAGENT_E2E_OTLP_LISTEN_ADDRESS=:14317 go test -v github.com/kagent-dev/kagent/go/core/test/e2e -failfast -shuffle=on -parallel 4 -count=1 - # The `smoke` template is applied part-way through the step above, after four - # digest lookups. The browser journeys below need an agent to schedule and a - # template to list, so a step that died before that apply would fail them for a - # reason that has nothing to do with the UI — three retries each, against an empty - # cluster. `continue-on-error`, because a missing template means the Go step has - # already failed the job and this only decides whether the UI steps are worth - # running: `outcome` still reports the failure the gate below reads. - - name: Check the cluster has the smoke agent - id: smoke-agent - if: ${{ !cancelled() && steps.install-kagent.outcome == 'success' }} - continue-on-error: true - # Retried, because everything below is gated on this one command. Asked once, a - # transient API-server or kubeconfig hiccup skips the whole browser lane and the - # job still reports green — which is the shape of #2638, a suite that measured - # nothing while every check passed. - run: | - for attempt in $(seq 1 10); do - if kubectl get agenttemplate smoke -n kagent; then exit 0; fi - echo "attempt ${attempt}: not there yet" - sleep 6 - done - kubectl get agenttemplate -A || true - exit 1 - - # And when it is genuinely absent, said out loud on the run. A skipped lane and a - # passing one look identical in the checks list otherwise. - - name: Say so if the UI suite was skipped - if: ${{ !cancelled() && steps.smoke-agent.outcome != 'success' }} - run: | - echo "::warning title=UI live suite skipped::No smoke agent on the cluster, so the live Playwright lane did not run." - - # The UI, against this same cluster and the image built above. `ui-tests` runs on - # the in-browser mock, so a backend contract change cannot fail it — which is how - # #2638 emptied three Substrate fields with every check green. - # - # After the Go tests, not beside them: both create and delete resources in the same - # namespace, and these journeys read the lists back. - # - # Gated on the cluster being usable rather than on `success()`, so one failed Go - # test under `-failfast` does not also cost every UI signal. No cluster or no - # agent, no run. - - name: Setup Node.js - if: ${{ !cancelled() && steps.smoke-agent.outcome == 'success' }} - uses: actions/setup-node@v7 - with: - node-version-file: ui/.nvmrc - - # Before the cache step, as in `ui-tests`: package.json pins Yarn 4, and the - # runner's own shim cannot read this lock file. - - name: Enable Corepack - if: ${{ !cancelled() && steps.smoke-agent.outcome == 'success' }} - run: corepack enable - - - name: Cache Yarn downloads - if: ${{ !cancelled() && steps.smoke-agent.outcome == 'success' }} - uses: actions/cache@v6 - with: - path: ui/.yarn/cache - key: yarn-${{ runner.os }}-${{ hashFiles('ui/yarn.lock') }} - restore-keys: yarn-${{ runner.os }}- - - - name: Prepare the live browser suite - if: ${{ !cancelled() && steps.smoke-agent.outcome == 'success' }} - working-directory: ./ui - # Chromium only: the live suite declares one project. The mock suite's second - # engine is there to disagree about layout, which no backend contract rests on. - run: | - yarn install --immutable - yarn playwright install --with-deps chromium - - - name: Run live browser tests - if: ${{ !cancelled() && steps.smoke-agent.outcome == 'success' }} - working-directory: ./ui - run: | - kubectl -n kagent rollout status deploy/kagent-ui --timeout=5m - # Already a LoadBalancer on a MetalLB cluster, so this is the address the - # controller e2e step takes — not a port-forward to keep alive for the run. - UI_IP="$(kubectl get svc -n kagent kagent-ui -o jsonpath='{.status.loadBalancer.ingress[0].ip}')" - test -n "$UI_IP" - export UI_LOOP_LIVE_URL="http://${UI_IP}:8080" - echo "UI_LOOP_LIVE_URL: $UI_LOOP_LIVE_URL" - # `globalSetup` refuses the run if that address answers with fixtures, or is a - # dev server rather than the built image. - yarn test:pw:live - - - name: Upload live browser report - # Any finished run, not just a failed one: CI retries twice, so a spec that - # fails and then passes leaves a green job and no trace of the failure — which - # is the run whose trace is worth the most. A clean run uploads the HTML report - # and nothing else, since `test-results` only has content when something failed. - if: ${{ !cancelled() && steps.install-kagent.outcome == 'success' }} - uses: actions/upload-artifact@v5 - with: - name: ui-live-playwright-report - # The trace is the only record of what the cluster answered — there is no fixed - # fixture to re-read afterwards, as there would be for the mock suite. The HTML - # report is what links one to the other, and it exists because the config asks - # for `html` alongside `github`; `github` on its own writes annotations and no - # files, which is how this path came to be uploaded empty. - path: | - ui/playwright-report - ui/test-results - retention-days: 7 - if-no-files-found: ignore - - name: fail print info if: failure() run: | @@ -309,10 +193,6 @@ jobs: kubectl get harnesses,agenttemplates -n kagent echo "::error::Kubectl logs -n kagent deployment/kagent-controller" kubectl logs -n kagent deployment/kagent-controller - # nginx logs every proxied request, so a browser failure that was really a - # backend failure says so here, as does an init.sh that rejected a value. - echo "::error::Kubectl logs -n kagent deployment/kagent-ui" - kubectl logs -n kagent deployment/kagent-ui --tail=200 || true kubectl get pods,actortemplates,workerpools -A echo "::error::Substrate logs" kubectl logs -n ate-system -l app --all-containers --prefix --tail=200 || true diff --git a/ui/eslint.config.mjs b/ui/eslint.config.mjs index 718c0ca22a..b4d07a2335 100644 --- a/ui/eslint.config.mjs +++ b/ui/eslint.config.mjs @@ -74,14 +74,9 @@ export default tseslint.config( * * `playwright/README.md` states these; a convention nothing checks is one that * regrows as an exception. - * - * Every folder that holds specs, not only `tests/`: `shared/` runs in every project - * and `live/` is the only suite that talks to a cluster, so a spec there trusting its - * own green matters more rather than less. Scoping this to `tests/` alone left both - * outside the guard from the day they were added. */ { - files: ["playwright/{tests,shared,live}/**/*.spec.ts"], + files: ["playwright/tests/**/*.spec.ts"], rules: { "no-restricted-imports": [ "error", diff --git a/ui/playwright.config.ts b/ui/playwright.config.ts index e473ff34d1..ece9ddc181 100644 --- a/ui/playwright.config.ts +++ b/ui/playwright.config.ts @@ -27,21 +27,6 @@ const EXTENSION_BASE_URL = `http://localhost:${EXTENSION_PORT}`; /** Specs that need an extension installed opt in by filename. */ const EXTENSION_SPECS = /\.withExtension\.spec\.ts$/; -/** - * Specs that run in both suites, claimed by every project below. - * - * Only what holds on either backend — no `?mock=` scenario, no fixture by name. That - * narrowness is the point and the risk: assertions true of both are the weakest ones, - * so this folder stays a smoke sweep rather than growing lifecycles. - * `conventions.test.ts` fails a spec here that reaches for a scenario. - * - * A glob, not a regex: Playwright matches a `testMatch` regex against the *absolute* - * path, so `/shared\//` also matches every spec in a checkout that happens to live - * under a directory of that name. A glob resolves against `testDir`, which is the - * thing actually meant here. - */ -const SHARED_SPECS = "shared/**/*.spec.ts"; - /** * The suite is the acceptance bar, so what it runs against cannot depend on the * shell it was started from: both servers are pinned to the in-browser mock @@ -91,71 +76,33 @@ const LIVE = process.env.UI_LOOP_LIVE === "true"; * server is about to want, or vice versa. */ const LIVE_PORT = Number(process.env.UI_LOOP_LIVE_PORT ?? 8301); - -/** - * An app already running — in CI, the `kagent-ui` service — tested instead of one - * this config starts. That is the deployed image, so nginx, the SPA fallback and the - * `env-config.js` rendered at pod start are under test rather than approximated by - * Vite. A developer still gets the dev server by default; the alternative is building - * an image to run a test. - * - * Empty counts as absent, and the `||` is what makes that one question rather than - * three. Three things below ask it — what `baseURL` is, whether `globalSetup` checks - * for a deployed image, whether a dev server is started — and they had asked it three - * ways: a `??`, a `!== undefined` and a truthiness test. An exported-but-empty - * variable, which is what a `kubectl get svc` that found nothing leaves behind, then - * pointed the run at `""`, checked that for nginx, *and* started a dev server nothing - * would ever visit. - */ -const LIVE_EXTERNAL_URL = process.env.UI_LOOP_LIVE_URL || undefined; - -const LIVE_BASE_URL = LIVE_EXTERNAL_URL ?? `http://localhost:${LIVE_PORT}`; +const LIVE_BASE_URL = `http://localhost:${LIVE_PORT}`; /** Read by `playwright/globalSetup.ts` to decide what to verify about a server. */ export const LIVE_PROJECT = "chromium-live"; /** - * Whether this live run is against a deployed app rather than a dev server, which - * `globalSetup` checks one more thing for — see `verifyLiveWiring`. - */ -export const LIVE_IS_DEPLOYED = LIVE_EXTERNAL_URL !== undefined; - -/** - * How the *dev server* is configured for a live run; a deployment is configured by its - * chart instead. It proxies `/api` the way nginx does in a cluster, so the app uses the - * same relative URLs either way — standing in for nginx rather than being it, which is - * the gap `UI_LOOP_LIVE_URL` closes. `VITE_API_MODE` is pinned at build time as well as - * at runtime, being the one thing an inherited `.env` cannot override. + * A live run reaches the backend through Vite's proxy, exactly as a deployed + * build reaches it through nginx — so the app uses the same relative URLs either + * way and this mode tests the addressing a real deployment uses. + * + * `VITE_API_MODE` is pinned as well as the runtime flag: the build-time pin is + * the one thing an inherited `.env` cannot override, and a live suite that + * silently answered from fixtures would be worse than a red one. */ const LIVE_APP = { VITE_API_MODE: "live", ENABLE_MOCK_UI: "false" }; -/** How the live server is started. Unused when `UI_LOOP_LIVE_URL` names one already. */ -const LIVE_COMMAND = `yarn dev --port ${LIVE_PORT}`; - /** - * The servers a live run starts, which is none when it was handed one: there is no - * process to own — the app is a pod. `globalSetup` checks the address serves the app. + * How the live server is started. + * + * Named for the same reason the three env pins above are: a branch whose backend + * needs more than a dev server — a credential minted per run, a port-forward + * probed before Vite starts — replaces this line rather than the block below. */ -const LIVE_WEB_SERVERS = LIVE_EXTERNAL_URL - ? [] - : [ - { - command: LIVE_COMMAND, - url: LIVE_BASE_URL, - reuseExistingServer: false, - timeout: 120_000, - // Whatever starts the live server is the most useful output a failed - // live run has — something that cannot reach the backend says so there, - // and Playwright discards a web server's stdout unless asked to pass it - // through. - stdout: "pipe" as const, - stderr: "pipe" as const, - env: LIVE_APP, - }, - ]; +const LIVE_COMMAND = `yarn dev --port ${LIVE_PORT}`; export default defineConfig({ - testDir: "./playwright", + testDir: "./playwright/tests", // Both servers have to be rendering, not merely listening, before any test // navigates — see the file for what goes wrong otherwise. globalSetup: "./playwright/globalSetup.ts", @@ -183,15 +130,7 @@ export default defineConfig({ * of the machine than the local runs that provoked it. */ workers: process.env.CI ? "50%" : undefined, - /* - * Both in CI: `github` writes the annotations that put a failure on the diff, and it - * writes no files at all — so the report CI uploads as an artifact has to come from - * somewhere, and for a live failure the trace is the only account of what the cluster - * answered. - */ - reporter: process.env.CI - ? [["github"], ["html", { open: "never" }]] - : [["list"]], + reporter: process.env.CI ? "github" : "list", /* * A real backend behind a port-forward answers in tens of seconds where the * in-browser mock answers in milliseconds, so the defaults that suit the mock @@ -204,20 +143,7 @@ export default defineConfig({ * and a mock-backed suite that needs more than thirty seconds for one test is saying * something is stuck, which is worth hearing rather than absorbing. */ - ...(LIVE - ? { - timeout: 120_000, - expect: { timeout: 30_000 }, - /* - * One at a time. Every mock test owns a backend in its own page's memory; these - * share a cluster, so a spec creating a resource while another counts them is a - * failure with no defect behind it. A dozen tests, and one cluster to run them - * against — it costs little. - */ - workers: 1, - fullyParallel: false, - } - : {}), + ...(LIVE ? { timeout: 120_000, expect: { timeout: 30_000 } } : {}), use: { trace: "on-first-retry", screenshot: "only-on-failure", @@ -227,7 +153,7 @@ export default defineConfig({ ? [ { name: LIVE_PROJECT, - testMatch: ["live/**/*.spec.ts", SHARED_SPECS], + testDir: "./playwright/live", use: { ...devices["Desktop Chrome"], baseURL: LIVE_BASE_URL, @@ -241,7 +167,6 @@ export default defineConfig({ : [ { name: "chromium", - testMatch: ["tests/**/*.spec.ts", SHARED_SPECS], testIgnore: EXTENSION_SPECS, use: { ...devices["Desktop Chrome"], baseURL: BASE_URL }, }, @@ -257,7 +182,6 @@ export default defineConfig({ // The extension split below is a build-time difference, not a browser one, // so it stays on one engine rather than doubling for no new signal. name: "firefox", - testMatch: ["tests/**/*.spec.ts", SHARED_SPECS], testIgnore: EXTENSION_SPECS, use: { ...devices["Desktop Firefox"], baseURL: BASE_URL }, }, @@ -276,7 +200,21 @@ export default defineConfig({ // loud startup error instead; set UI_LOOP_PORT / UI_LOOP_EXTENSION_PORT to run // alongside a dev server you want to keep. webServer: LIVE - ? LIVE_WEB_SERVERS + ? [ + { + command: LIVE_COMMAND, + url: LIVE_BASE_URL, + reuseExistingServer: false, + timeout: 120_000, + // Whatever starts the live server is the most useful output a failed + // live run has — something that cannot reach the backend says so there, + // and Playwright discards a web server's stdout unless asked to pass it + // through. + stdout: "pipe", + stderr: "pipe", + env: LIVE_APP, + }, + ] : [ { command: `yarn dev --port ${PORT}`, diff --git a/ui/playwright/DEFERRED.md b/ui/playwright/DEFERRED.md index 2a2e95b26b..19311ec925 100644 --- a/ui/playwright/DEFERRED.md +++ b/ui/playwright/DEFERRED.md @@ -1,157 +1,208 @@ # Deferred specs -Coverage this suite does not have, and what each gap is waiting on. Kept as prose -rather than as skipped tests, because a skipped spec reads as coverage and this list -does not. - -An entry is in one of three states, and saying which is most of the value here: -**deferred** (blocked on something nameable), **not planned** (a decision, so it is not -re-argued every time somebody notices the gap), or **closed**. - -Three rules for editing it, all learned the hard way here: - -- **A stale entry costs more than no entry.** Several entries were once listed as - blocked on pages that already existed, which stops somebody porting work that is - already possible. When something lands, close it in the same change. -- **Close, do not archive.** An entry describing a page that no longer exists, or a - gap since covered, belongs in *Closed* below as one line — or deleted. -- **Do not defer a decision.** If the suite is not going to cover something, say so and - say why. An entry that reads as queued is one somebody will pick up. - -## Closed - -Nothing below is a gap. They are listed only so they are not looked for again. - -- **Onboarding.** There is no onboarding wizard on this architecture and none planned. -- **A cleanup spec.** Each mock test gets a fresh browser context, and each live spec - deletes what it made in a `test.afterEach`. A run killed outright still leaks, which - is why `throwawayName` puts the process and a timestamp in every name: anything - matching `e2e-live-*` in `kagent` is litter and safe to remove by hand. A sweep spec - stays the wrong shape for that, being one bad selector away from deleting real work. -- **Agent-create validation.** There is no agent form. An agent is an `AgentTemplate` - paired with a `Harness`, materialised by admission — `router/routes.ts` records why - there is no `agentNew` and no `agentEdit`. Nothing creates one, so there is nothing - to validate. The create-and-read-back property that lived in the removed agent-create - spec is now `shared/harnesses/` and `shared/agent-templates/`, which create against - either backend — `tests/harnesses/` only checks when the button is enabled. -- **The REST path tests.** `src/api/{readPaths,writePaths}.test.ts` drove the client over - REST URLs that no longer exist; the controller serves gRPC-Web. `src/api/operations.test.ts` - replaces them against the real generated descriptors and covers strictly more. -- **The extension-point specs.** `extension-points-absent.spec.ts` and - `extension-points.withExtension.spec.ts` both run and between them assert every point - the app declares. -- **Tool approval, and a question asked without the extension.** Both shipped with #2714 - and are driven by `tests/chat/approvals.spec.ts` — several tools decided independently - behind a Submit, one tool decided on the prompt itself, and the unanswerable turn that - says so rather than inventing controls. The decisions are read back off the reply, not - off the form. -- **`AgentDetailsPage`.** Covered by `tests/agents/agent-details.spec.ts`: the state and - what it means, the links out to the template and the agent, the failure, and the - difference between a record that is missing and a read that failed. The page used to - show a `SandboxAgent`'s spec — its model, its tool bindings, its `Ready` condition — - and shows an `AgentInstance` record instead. That is not a reduction to restore: an - instance genuinely has no spec, and the configuration lives on the `AgentTemplate` and - `Harness` surfaces, which have their own specs. -- **The live suite running anywhere.** `playwright/live/` runs in the `test-e2e` job - against the image built from `ui/Dockerfile`. `README.md` has the wiring. -- **Chat and its error journeys, the chat-message extension point, MCP servers in mock, - prompt libraries, and form validation for every resource with a form.** Each is a spec - now, and a spec describes itself better than a list of what it covers. - -## Deferred: MCP servers stay mock-only — the list cannot read its own writes - -Every other resource has its create/read/change/delete journey in `shared/`, running -against both backends. MCP servers do not, and the reason is a defect rather than an -awkward fixture: **#2849**. - -The page's two halves use different stores. `CreateToolServer` writes a Kubernetes -`RemoteMCPServer`; `ListToolServers` reads the PostgreSQL `toolserver` table; the only -writer of that table is the reconciler, after it has tried to connect to the server. So -the list lags a create by however long discovery takes. Measured on a cluster: - -``` -CreateToolServer OK -ListToolServers OK <- 57ms later; the new server is not in it - (nothing further) -ListToolServers OK <- a fresh page load a minute on; now present -``` - -Delete has the same shape in reverse, and `DeleteToolServer` resolves the server's kind -from that same projection — so while a new server is invisible it is also undeletable. - -A shared spec was written and did pass, by pressing **Refresh** after the create and -after the delete. It was withdrawn rather than landed: a spec that presses through a -defect to stay green is how the defect stops being noticed, and the press would have -needed removing anyway. The mock lifecycle in `tests/mcp-servers/mcp-servers.spec.ts` -keeps its full coverage meanwhile. - -**Revisit when #2849 lands.** The spec is a short port of the mock one — create with a -URL the cluster can resolve, read the row back, delete it — and the acceptance test is -that it needs no Refresh. - -Worth recording for its own sake: the fixtures cannot show this class of bug at all. They -answer from the page's own memory and are therefore always immediately consistent, so a -mock backend has no write that is not yet a read. Only a cluster has one. - -## Not planned: the `resuming` and `suspending` lifecycle stages - -A decision rather than a queue entry, recorded so it is not re-argued each time -somebody notices the gap. - -`chat.spec.ts` drives the lifecycle indicator at rest and through `running`, because a -turn produces both. The other two stages come from `AgentInstance.operation`, which the -controller claims and clears as it works. The mock backend serves a static record, and -faking one would prove only that a fixture can hold a string; a live journey — suspend an -instance from the agents list with a chat page open on it, and watch the indicator follow -— needs a model that can answer, which neither cluster this suite runs against has. - -**What tips it from deferred to not planned** is that the part with the logic in it is -already covered, and it is covered where the logic lives: -`src/components/chat/lifecycleReading.test.ts` exercises every reading exhaustively, -including the case worth guarding hardest — that **no stage is claimed when a turn ends**, +The old suite had 13 specs. Everything it covered that still has a page is ported, +plus a good deal it did not have: a lifecycle spec per resource, routing, auth, the +two extension-point specs. The rest is listed here rather than committed as skipped +tests, because a skipped or vacuous spec reads as coverage and this list does not. + +Where an entry below names a spec that no longer exists under that filename, the +coverage moved rather than went; `README.md` has the current layout. + +Each entry names the surface that has to exist before the spec can assert +anything real. In every case the data layer is already in place — what is +missing is the page. + +| Old spec | Blocked on | Already available | +|---|---|---| +| `onboarding/onboarding.spec.ts` | No onboarding wizard exists on this architecture | — nothing; drop it unless the flow is rebuilt | +| `cleanup.spec.ts` | Not applicable while the suite runs on the mock backend: each test gets a fresh browser context, so there is nothing to sweep. Revisit if the suite gains a live-backend mode. | — | + +## Ported since: chat + +`chat/chat.spec.ts` and `chat/chat-errors.spec.ts` are live. The chat page was +rebuilt on the `ChatClient` port, so both journeys assert against the real page: +history, sending, streaming deltas, tool call and result rendering, a failed +turn with retry, cancelling mid-stream, and the session list failing on its own. + +The chat-message extension point is covered too, now that the example mounts a +component there: `extension-points.withExtension.spec.ts` asserts one slot per message +and four *distinguishable* contributions, so per-message context is proven rather +than assumed. Every extension point the app declares now has a runtime +assertion. + +## Covered: form validation, and every resource's lifecycle + +Each resource's lifecycle spec asserts its own form's gate in the create step — the +submit refused while a required field is empty, the refusal naming the field, the +address staying on the form — and checks the required *marks* against that gate with +`expectRequired`. antd draws the mark from `required` on a `Form.Item` while these +forms gate their submit in code, so the two are separate statements about the same +field and only a test keeps them agreeing. + +**What is still missing is the agent form, because there is no agent form.** An agent +is an `AgentTemplate` paired with a `Harness` and is not created, so the old +"declarative agent create blocks submit" assertion has no page to run against. If a +create-an-agent surface lands, its validation belongs in that change. + +## Not started by request + +App extension-point specs. The framework is still being edited and its +contract is not frozen; the team lead will ask for these once it lands. + +## Ported since: MCP servers and prompt libraries + +`mcp-servers/mcp-servers.spec.ts` and `prompts/prompts.spec.ts` are live: one lifecycle +spec apiece, with the failure states among the steps. + +**These were listed above as blocked on pages that did not exist. The pages did +exist** — `McpServersPage`, `PromptsPage` and `PromptDetailPage` are all real, and were +before the specs were written. The entries were simply stale, which is worth recording: +this file is only useful while it is true, and a stale "blocked on" entry costs more +than no entry at all, because it stops somebody porting work that is already possible. + +The specs cover the list, the per-server tool count including a server that discovered +none, the filter, the step through to a library's fragments and the include expression +a reader copies, and both failure journeys. The detail page's two failure states are +asserted apart — a library that could not be loaded and a library that does not exist +lead to different actions, and the page distinguishes them. + +One thing they needed from the harness: a spec can now declare console output it +provokes on purpose, with `test.use({ expectedNoise: [...] })`. The not-found journey +makes the browser log a 404, and forgiving 404s for the whole suite would have blunted +the guard — a 404 is also what a missing asset looks like, and this repository has +shipped one to production that way before. + +## Lost with the REST path tests, and where it went instead + +`src/api/readPaths.test.ts` and `src/api/writePaths.test.ts` are gone. They drove the API +client over REST URLs against the MSW fixture backend, and neither the URLs nor that +backend's REST routes exist any more — the controller serves its application API as +gRPC-Web. `src/api/operations.test.ts` replaces them, against the real generated service +descriptors served in-process, and covers strictly more of what those two were for: which +RPC each operation invokes, with what identity in the request message, and what the +response converts to. + +**One property could not live there, and it now lives in a browser spec instead.** The old +write tests read each create *back through its list* — "the create returned 200" and "the +thing exists" are different claims, and only a stateful backend can check the second. The +in-process router is stateless per test, so `operations.test.ts` cannot. That property is now +`playwright/tests/harnesses/harnesses.spec.ts`: create a harness, land back on the tab it was +created from, and find it in the list — and find it reported "not ready yet", which is the +state a cluster reports for one the controller has not observed. Nothing about it is +deferred any more. + +It lived in an agent-create spec until that page was removed: an agent is not something +anybody creates, so the form that appeared to create one went, and the read-back property +moved to the nearest thing that is genuinely created. + +Two things worth keeping from writing it, because both cost time and neither is guessable: + +- **Stay inside one browsing context.** The fixture backend keeps writes in the page's own + memory, deliberately, so one spec's creates cannot leak into the next one's list. A + `page.goto` therefore starts a backend that has never heard of the thing just created, + and the failure reads as "the create did not stick" when nothing is wrong. Click through + from the list. +- **The second read is the point.** `chat-capabilities-toggle` is asserted rather than the + heading or the panel, because those render from the URL and would appear for an agent + that does not exist. That button renders only when the per-agent read resolved a row, so + it is what distinguishes "the list re-fetched" from "the thing exists". The weaker + version of this spec passes and proves less than it looks like it does. + +**Every resource reads its create back through its list now**, not only harnesses — see +*Covered: form validation, and every resource's lifecycle* above, which also records the create-cache defect +that used to make that impossible for three of them and how the fix was proved. + +--- + +## Lost when agents became AgentInstances + +One thing the suite used to cover no longer exists, and it should not be replaced by a +passing test of something adjacent. Two others that were listed here — the capabilities +panel and the sharing loop — have since come back and are covered. + +### An agent's own tools, model and readiness on its details page + +The details page showed a `SandboxAgent`'s spec: its model resolved from a `ModelConfig`, +its tool bindings, and its `Ready` condition with a reason. It now shows the +`AgentInstance` record instead — state, operation, the pair it was cut from, the prepared +revision, the A2A authority and the failure — which is the whole of what the API knows +about an instance. + +That is not a reduction to fix: an instance genuinely has no spec. The configuration +belongs on the `AgentTemplate` and `Harness` surfaces, and those exist now — the agents +landing page carries all three as tabs, and a conversation's record links out to the +template and to the agent rather than duplicating either. What is still not covered in a +browser is that an agent's readiness *reason* is readable end to end, because the +`AgentInstance` record reports a failure message and the template reports a condition, and +no single surface shows both. + +## What the chat fixes could not be covered against + +Three gaps left by the work on the reader's own message, the artifact-append streaming +and the lifecycle indicator. Each is a *mock* gap: the mock backend cannot produce the +state the assertion would need, and inventing one would make the fixture the thing being +tested. + +### The suspending stage of the lifecycle indicator + +`chat.spec.ts` drives the indicator through its resting reading and through `running`, +because a turn produces both. It never sees `resuming` or `suspending`: those come from +`AgentInstance.operation`, which the controller claims and clears as it works, and the +mock backend serves a static record. Faking one would prove only that a fixture can hold +a string. + +The reading itself is covered exhaustively in `src/components/chat/lifecycleReading.test.ts` +— including the case worth guarding hardest, that **no stage is claimed when a turn ends**, since a substrate agent really does suspend itself then and nothing in the API reports it. -What a browser test would add is that a string the controller sets reaches an element, -for two stages out of four, at the cost of the only spec in this suite needing its own -API key. - -Revisit if the indicator grows behaviour of its own, rather than reading a field. - -## Deferred: streaming, end to end - -The one chat gap still worth a spec, and it needs a cluster with a model that answers: -CI installs with `OPENAI_API_KEY: fake` and `setup-cluster.sh` sets no key at all, so it -is reachable today only by a developer with their own. - -The client honours an artifact's `append` flag, which is how this runtime streams: one -`artifactId` for the reply, one frame per token, `append` on every frame after the -first, then a closing frame repeating the whole answer. That shape -is pinned in `src/api/chat/a2aGrpcChatClient.test.ts` against frames captured from the -controller on 2026-08-24. The mock chat client streams `delta` events — the port's -vocabulary rather than the wire's — so no browser test exercises the artifact path. -Teaching the fixture to emit artifact frames would make it an A2A server rather than a -`ChatClient`, which is the wrong seam; the transport is already covered over real bytes. -The gap is a live spec that sends a message and watches the reply grow before the turn -completes. - -## Open, but a product decision rather than coverage - -The `ask_user` payload still renders as JSON in the transcript beside the answerable -prompt. That is duplication rather than a defect, and collapsing it needs a decision -about whether a tool call with an interactive rendering should show its raw form at all. - -## Deferred: proving a share token actually travels - -`tests/chat/sharing.spec.ts` covers the loop — create a link, see it listed once, revoke -it, open one — and can prove the page spends a token and reports a refusal. It cannot -prove the header reaches a backend: chat in mock mode is served by a client-side fake -that builds no request, so the spec reads the registration directly rather than seeing -what travelled. - -Only a live spec closes it, and unlike the two chat gaps above this one needs no model -that answers — a share is over an instance, and an instance exists as soon as a -conversation is opened. What it needs is a second identity: the point of the check is -that the A2A gateway reads the instance as the share's *owner*, which a visitor who is -the same signed-in reader cannot demonstrate. +What is missing is a browser journey that suspends an instance from the agents list while a +chat page is open on it and watches the indicator follow. That belongs in `playwright/live/`, +where the operation is real. + +### Streaming, end to end, against a controller + +The client now honours an artifact's `append` flag, which is how this runtime streams: one +`artifactId` for the reply, one frame per token, `append` on every frame after the first, +then a closing frame repeating the whole answer. That shape is pinned in +`src/api/chat/a2aGrpcChatClient.test.ts` against frames captured from the controller on +2026-08-24, and it was confirmed by hand — `grpcurl` at the gateway, and a throwaway +Playwright run against a live instance that rendered the reply. + +**The mock chat client does not reproduce that shape.** It streams with `delta` events, +which is the port's own vocabulary rather than the wire's, so no browser test exercises the +artifact path. Teaching the fixture to emit artifact frames would mean it stopped being a +`ChatClient` and started being an A2A server, which is the wrong seam — the transport is +already covered by unit tests over real bytes. The browser-level gap is a `playwright/live/` +spec that sends a message and asserts the reply grows on screen before the turn completes. + +A related gap worth naming rather than leaving implicit: the mock backend serves one +instance per conversation and never *changes* an instance's `operation`, so the lifecycle +indicator's `resuming` and `suspending` stages have no browser coverage either. Both +belong in the same live spec. + +### Tool approval, and a question asked without the extension + +`ask_user` is now answerable end to end: the question renders with its choices, the +answer names the parked turn and carries the extension payload, and the agent uses it. +What is left are the two neighbouring cases, both of which the UI *recognises* and says +plainly rather than guessing at. + +**A `tool_approval_request`** carries `tools[]` and a `hint` and is answered with +`tool_approval_response` / `approvals[]` — a different payload, and a different control: +per-tool approve or reject, with a rejection reason. The prompt names the tools and +offers only the discard, which is honest. Building the approval controls needs the +product decision about what a reader is being asked to vouch for, and it should not be +guessed from the shape of the payload. + +**A turn parked without the HITL extension activated** has no payload at all — the +question exists only as prose and carries no correlation id, so no answer can be routed +to it. The prompt says so and offers the discard. This build always activates the +extension, so it can only arise from a turn started by something else (a `kubectl`-driven +send, an older client). It is not worth engineering around; it is worth not lying about. + +**The `ask_user` payload still renders as JSON in the transcript**, beside the answerable +prompt — the tool call and its result are structured data and are shown as such. That is +now duplication rather than a defect, and collapsing it needs a decision about whether a +tool call that has an interactive rendering should still show its raw form at all. ## Blocked on the API: server-side paging, searching and sorting — for every list @@ -183,16 +234,16 @@ Two capabilities remain deferred until Substrate supports them: **Which actor is on a worker is not deferred; it is not available.** ate-api's `Worker` carries capacity and allocation and no actor reference — the binding lives on the actor — so the workers table has no Actor column. `busyWorkerCount` counts workers with a positive -allocated actor count reported by Substrate. A column would need the walk per page. +allocated actor count reported by Substrate. A column would need the +walk per page. **A single-message read is defensible only while the message really holds everything.** `GetSubstrateStatus` is the read that failed this way once: a cluster of 410,110 actors produces a response gRPC refused to send, which is why the substrate page was split into -three reads in the first place. That endpoint has been removed from `SystemService`. For -the three reads at the top of this table that do still answer with everything, **the -moment one starts paging — or starts truncating to survive — its client-side search and -sort must be labelled or removed in the same change**, because an unlabelled filter over -a page reports "no matches" about a row on page nine. +three reads in the first place. That endpoint has been removed from `SystemService`. For the three reads at the top of this table that do still answer with +everything, **the moment one starts paging — or starts truncating to survive — its +client-side search and sort must be labelled or removed in the same change**, because an +unlabelled filter over a page reports "no matches" about a row on page nine. The prompts page is a partial exception worth not losing: `ListPromptTemplates` takes a namespace, so `usePrompts` fans out one call per namespace and its **namespace filter is @@ -210,25 +261,25 @@ pages, and none of their totals is `rows.length`. Counting what arrived and call total is the lie a separate summary read exists to prevent, which is what `GetSubstrateSummary` is for. -## Not deferred coverage: auto-titling costs a read per row +--- -A cost decision with a server-side fix, rather than a spec somebody owes. What the -table renders today is pinned by `agents/agent-page.spec.ts` — that it is never a bare -UUID, and that the derived title appears where the transcript is in hand — so the -behaviour is covered; what is open is making a better behaviour possible. +## Auto-titling costs a read per row, so the table still does not do it A conversation is named by the reader, and an unnamed one can be titled from its first message — `ListTasks{ContextID: instanceId}` returns the history. That is **free on the chat page**, which has already read the transcript because it is rendering it. The **rail** now pays for the rest, bounded at thirty: every row but the open one used to -read `Untitled · 50b46891`, which made the list very nearly unusable. Failures are per-row -and silent, because a title is a convenience over an id that already identifies the row. +read `Untitled · 50b46891`, which made the list very nearly unusable — the one row a +reader could identify was the one they were already looking at. Thirty reads for a rail +somebody is navigating by is a trade worth making; failures are per-row and silent, +because a title is a convenience over an id that already identifies the row. The agent's conversation **table** still falls back to `Untitled · `, and that is a decision rather than an omission: it is the surface that could hold hundreds of rows, and one read per row to put a label on them is the cost the rail's budget of thirty -exists to bound. +exists to bound. What narrows it is described two sections down — the read is paged and +this client follows every page, and the search and sort are the browser's. Two ways it could stop being a trade-off, both server-side and neither invented here: @@ -238,27 +289,25 @@ Two ways it could stop being a trade-off, both server-side and neither invented - **`ListAgentInstances` gains a field mask** for it, so callers that want it pay and callers that do not are unaffected. -Neither has landed, checked at the source rather than here: `AgentInstance` in -`proto/kagent/api/v1alpha1/agent_instances.proto` gained `name` (field 13, the -reader-supplied title) and `context_id`, and carries nothing derived from the -transcript. - -## Not a defect yet: an agent's conversation search is over what was fetched +Either would let a list show what the chat page already shows. Until then, what a list +renders for an unnamed conversation is pinned by `agents/agent-page.spec.ts` — +both that it is never a bare UUID, and that the derived title appears where the +transcript is in hand. -A tripwire rather than a gap, and the distinction is the whole entry: the search is -honest today and stops being honest on a change somebody will make for other reasons. +## An agent's conversation search is over what was fetched, and the page-following is why `ListAgentInstances` narrows to one agent **on the server**: it takes `agent_template` and `harness` and resolves them through the prepared revision. That is the narrowing that matters, because it is the one the paging is applied after. What the request does -**not** carry is a search term or a sort field — `ListAgentInstancesRequest` is -`all_creators`, `page`, `agent_template` and `harness`, and nothing else — so the agent -page's search box and column sorts run in the browser. +**not** carry is a search term or a sort field, so the agent page's search box and column +sorts run in the browser. That is honest here for a reason worth stating, because it is the one read on the list above that is paged at all: the client follows every page token before rendering anything (`INSTANCE_PAGE_LIMIT` in `api/grpc/operations.ts`), so what is in the browser is every -conversation with that agent rather than the first fifty. +conversation with that agent rather than the first fifty. The page used to say so under +the table; that note was removed as commentary a reader has no use for, which leaves this +file as the record. **If that page-following is ever removed** — and it should be, once an agent can have thousands of conversations — the search and the sort must go server-side in the same @@ -287,9 +336,11 @@ harness that will run it, when the cluster has exactly one. **Why:** the fixtures carry more than one harness on purpose — one of them exists specifically so a template can be admitted by *two*, which is what makes an agent list show two rows for one template. A single-harness cluster is therefore not a state these -fixtures can be in, and the default correctly does nothing against them. The opposite -half *is* covered: with several harnesses nothing is chosen for the reader, and a -template no harness admits says so. +fixtures can be in, and the default correctly does nothing against them. + +The opposite half *is* covered: with several harnesses nothing is chosen for the reader, +and a template no harness admits says so ("creating one, and being told when nothing +will run it"). **How it was checked instead:** against the live cluster, which has one harness (`kagent`) — the same shape the default exists for. @@ -297,3 +348,22 @@ template no harness admits says so. **What would close it:** a fixture scenario with a single harness. Worth doing when something else needs one; a scenario knob added for one assertion is a second fixture backend to keep honest. + +## A broken create takes that resource's failure states with it + +Each resource spec runs its empty, failure and retry states after the lifecycle, and a +journey is ordered — so a create that breaks aborts the three steps least likely to be +broken by the same change. `agent-templates` did exactly that during this port: step 10 +failed and steps 11 to 14 never ran. + +The README justifies the position by the fixture reset — reaching those states needs +`?mock=`, which is per-navigation and discards what the lifecycle created. That is a +reason they cannot sit in the *middle*; it does not choose an end, because the reload +starts a fresh backend whichever end they are at. + +Moving them first is not the fix either: they would then run against a pristine backend, +which is not the state they are about, and the lifecycle would start from one a +navigation had just reset. What actually removes the coupling is a second `test` in the +same file — one recording for the lifecycle, one for the states, neither able to abort +the other. That costs `conventions.test.ts` its "one spec, one test" rule, so it is an +amendment to the convention rather than a reshuffle, and belongs in its own change. diff --git a/ui/playwright/README.md b/ui/playwright/README.md index ed84918a0f..42cf3794d8 100644 --- a/ui/playwright/README.md +++ b/ui/playwright/README.md @@ -13,9 +13,8 @@ UI_LOOP_PORT=8012 yarn test:pw # when something else owns the default port Nothing else is needed — no cluster, no port-forward, no provider key. -There is a second suite that does need a cluster, and that CI runs against the -built UI image; see [Live runs](#live-runs-against-a-real-backend) at the foot of -this file. +There is a second suite that does need all three; see +[Live runs](#live-runs-against-a-real-backend) at the foot of this file. ## What changed from the old suite @@ -33,8 +32,8 @@ served. Worth stating plainly because it is a change in how contributors work, and because it is a trade: the suite no longer exercises the real controller, so it proves the UI behaves, not that the backend contract still holds. Contract drift -is caught by the Go tests and by the live suite, which CI runs on the same cluster -as the Go end-to-end tests — not here. +is caught by the Go tests and by whatever runs against a live cluster in CI — not +here. ## What it runs against @@ -72,47 +71,9 @@ playwright/ chat, controls, style, mockCalls fixtures/test.ts import { test, expect } from here — never @playwright/test live/ the live suite: specs, plus helpers/ of its own - shared/ specs that run in both suites — laid out like tests/, one folder - per resource and app-wide specs at the root — see below - DEFERRED.md coverage this suite does not have, and what each gap waits on + DEFERRED.md the specs not yet portable, and what each one is waiting on ``` -**`shared/` runs in every project**, mock and live alike. What goes in it is narrow: no -`?mock=` scenario, no fixture named, nothing assuming a populated backend. -`conventions.test.ts` fails a spec here that reaches for one. - -It holds two kinds of spec. A property true whatever the backend holds — no conversation -is listed by a bare id, a deep link renders on a cold load. And **the write path of each -resource**: create, read back, change, delete. - -That second kind is where a fixture and a controller most easily disagree, and it is why -every resource that can have its journey here does: `models`, `prompts`, `harnesses`, -`agent-templates` and `schedules`. The names are `throwawayName`d and every one cleans up -in a `test.afterEach`, because live they are real — see the rule below for why not a -`finally`. - -**One move rules a spec out of here: a reload.** The fixture backend keeps writes in the -page's own memory, so a reload starts a backend that has never heard of what was just -created. Where a claim needs one, it splits: `shared/schedules/` clicks through the whole -journey on either backend, and `live/schedules.spec.ts` keeps the one thing only a real -backend can answer — that the values survive a re-read, since everything short of that -could be the form showing itself its own draft. - -It is laid out like `tests/`: one folder per resource holding one spec holding one test, -titled for its folder, and app-wide specs (`dashboard`, `routing`) at the root. -`conventions.test.ts` checks that too — all of it except the empty-and-error rule, which -needs the `?mock=` a spec here may not touch. - -**MCP servers are deliberately not among them.** That page cannot read its own writes -against a real backend — see `DEFERRED.md` and #2849 — so a shared spec would have had to -press Refresh to get past a defect, which is the sort of workaround that keeps one alive. -It stays mock-only until the fix lands. - -What stays in `tests/` for each of those is what needs the fixtures: the exact seeded -rows, the required-field marks, the refresh counts, and the empty and failure states no -cluster can be asked for. Navigate with `loadApp`, which adds the mock scenario only -where there is a mock backend to read it. - The resources with a lifecycle spec are **models**, **MCP servers**, **prompt libraries**, **agent templates**, **harnesses** and **schedules**. Two are narrower than CRUD because the product is: an MCP server's address is its identity, so @@ -205,12 +166,6 @@ conventions below: the shared fixture import, and antd's class names. The trade is deliberate: a failed step stops the ones after it, so a broken create hides whether delete works. That is the right way round — a resource whose create is broken is broken, and the recording shows where it stopped. -- **Never assert an absence before the thing could appear.** "No error", "no rows", - "no source text on the page" are all true of a page that has not drawn yet, so each - one needs a positive signal in front of it — a summary, a table, a settled state. - Four defects on this suite were that shape, every one of them green. -- **Clean up in a hook, never in a `finally`.** A timed-out test has a closed page, so - everything in its `finally` throws and the resource stays on the cluster. - **Keep the writes in one browsing context.** The fixture backend keeps writes in the page's own memory, so a `page.goto` starts a backend that has never heard of the thing just created, and the failure reads as "the create did not stick" when nothing @@ -257,40 +212,15 @@ conventions below: the shared fixture import, and antd's class names. cd ui yarn test:pw:live UI_LOOP_LIVE_PORT=8312 yarn test:pw:live # to run beside something on 8301 - -# Against an app that is already serving, rather than a dev server this starts: -UI_LOOP_LIVE_URL=http://127.0.0.1:8080 yarn test:pw:live ``` -Unlike `yarn test:pw`, this one **does** need a cluster. The specs live in -`playwright/live/`, and the coverage deliberately left out of it is in -`DEFERRED.md`. - -**Two things can be at the other end.** Without `UI_LOOP_LIVE_URL` the suite starts -`yarn dev` and proxies to the controller, which needs nothing built and is what a -developer wants. With it, it drives an app already serving — in CI the image from -`ui/Dockerfile`, where nginx, the SPA fallback and the `env-config.js` rendered at pod -start are real rather than approximated by Vite. - -### In CI - -The `test-e2e` job runs it, after the Go end-to-end tests and on the same cluster, -reusing what that job stands up rather than building a second path — including the -`smoke` agent from `lifecycle.yaml.tmpl`. The address is the `kagent-ui` service's -MetalLB IP, the chart already publishing it as a LoadBalancer. +Unlike `yarn test:pw`, this one **does** need a cluster, with the controller +port-forwarded. It is not run in CI. The specs live in `playwright/live/`, and the +coverage deliberately left out of it is in `DEFERRED.md`. -After the Go tests, not beside them, and at `workers: 1`: these journeys create real -resources and read lists back, where `go test -parallel 4` — and each other — would be -writing to the same namespace at the same time. - -**A live spec has to work on both shapes of cluster.** `setup-cluster.sh` installs one -harness and an `assistant` agent; CI's fixture installs five and a `smoke`. Not -cosmetic: the template form applies a harness's labels unasked when there is only one. -So a spec takes whatever the cluster offers and reads the state it is in. - -**Chat stays on the mock backend.** Its journeys need deterministic streaming deltas, -tool ordering, cancellation and a failed turn with retry — none of which a real model -gives reliably, all of which the mock suite already asserts. +A live run reaches the controller through Vite's proxy, exactly as a deployed +build reaches it through nginx, so the app uses the same relative URLs either way +and this mode tests the addressing a real deployment uses. **Why a separate mode rather than a third project.** `UI_LOOP_LIVE=true` swaps the whole `projects`/`webServer` pair in `playwright.config.ts` instead of appending to @@ -302,20 +232,11 @@ would cost every live run the time to boot Vite twice for nothing. The two runs are disjoint. The live project also gets its own port, 8301, far from the mock servers' 8001/8051 for the same reason those two are 50 apart. -**A green live run has to have been live.** A live suite that quietly answered from -fixtures would be worse than a red one, since a green one gets taken as evidence -the cluster works — so `globalSetup` asks the page what settings it was actually -handed, and refuses the run if they are not the live ones. - -The guarantee is made twice, the two ends needing different arguments: - -- **Against the dev server,** `VITE_API_MODE` is pinned at build time as well as at - runtime — the one thing an inherited `.env` cannot override. -- **Against a deployment** there is no such pin, but the mock backend is not in the - image at all: the build deletes `dist/mockServiceWorker.js` and nginx 404s the path. - `globalSetup` asserts that 404, which says both that fixtures cannot be served and - that this is the built artifact rather than a `yarn dev` that would pass every spec. - -Traces are kept on failure — there is no fixed fixture to re-read afterwards, so the -trace is the only record of what the cluster answered. CI uploads them as -`ui-live-playwright-report`. +**A green live run has to have been live.** `VITE_API_MODE` is pinned at build +time as well as at runtime, because a build-time pin is the one thing an inherited +`.env` cannot override — and a live suite that quietly answered from fixtures +would be worse than a red one, since a green one gets taken as evidence the +cluster works. `globalSetup` asks the page what settings it was actually handed +and refuses the run if they are not the live ones. Traces are kept on failure: +unlike the mock suite there is no fixed fixture to re-read afterwards, so the +trace is the only record of what the cluster answered. diff --git a/ui/playwright/conventions.test.ts b/ui/playwright/conventions.test.ts index ab8ab0196d..29252746d8 100644 --- a/ui/playwright/conventions.test.ts +++ b/ui/playwright/conventions.test.ts @@ -2,15 +2,6 @@ import { readdirSync, readFileSync, statSync } from "node:fs"; import { join } from "node:path"; import { describe, expect, it } from "vitest"; -/** - * The exports of `helpers/app` that are seeded mock rows rather than helpers. - * - * A list because the distinction is not visible in the name: `routes` and `loadApp` - * hold on either backend, `instances` and `agents` are ids only the fixture backend - * has. - */ -const FIXTURE_EXPORTS = ["instances", "agents"]; - /** * The layout rules from `README.md`, checked. * @@ -31,7 +22,6 @@ const FIXTURE_EXPORTS = ["instances", "agents"]; */ const TESTS = join(__dirname, "tests"); -const SHARED = join(__dirname, "shared"); /** * Folders whose subject is the application rather than a resource it manages. @@ -51,29 +41,6 @@ function specsIn(dir: string): string[] { return readdirSync(dir).filter((name) => name.endsWith(".spec.ts")); } -/** Folders under `shared/`, which are resources for the same reason `tests/` ones are. */ -function sharedFolders(): string[] { - return readdirSync(SHARED).filter( - (name) => !name.startsWith(".") && statSync(join(SHARED, name)).isDirectory(), - ); -} - -/** - * Every spec under `shared/`, nested ones included. - * - * Recursive deliberately: the resource journeys moved a directory down into - * `shared//`, and a top-level-only read would have quietly stopped applying - * the rule below to exactly the specs it matters most for. - */ -function sharedSpecs(): string[] { - return [ - ...specsIn(SHARED).map((name) => join(SHARED, name)), - ...sharedFolders().flatMap((folder) => - specsIn(join(SHARED, folder)).map((name) => join(SHARED, folder, name)), - ), - ]; -} - function folders(): string[] { return readdirSync(TESTS).filter( // Dot-directories are whatever a contributor's local tooling dropped here; they are @@ -169,67 +136,6 @@ describe("playwright layout", () => { ).toEqual([]); }); - it("a shared spec asserts nothing only one backend can answer", () => { - /* - * `shared/` runs in both suites, so a scenario query there is either ignored by a - * cluster or — worse — read as a claim the run cannot make. The narrowness is the - * folder's whole value: assertions true on both backends are the weakest ones, and - * a lifecycle drifting in here would quietly cost the mock suite its precision. - */ - for (const spec of sharedSpecs()) { - const source = readFileSync(spec, "utf8"); - expect( - /mock=|withScenario|scenario:|helpers\/mockCalls/.test(source), - `${spec} drives the mock backend, so it cannot run live`, - ).toBe(false); - - /* - * And the other half, which a scenario check misses entirely: a seeded row named - * by id. `instances.ready` is a UUID out of `src/mocks/fixtures.ts` — legal - * TypeScript, green in the mock lane, and a 404 against any cluster. The README - * has always said this folder may not name a fixture; until this ran, nothing - * checked the half that is not a query parameter. - */ - const imported = [...source.matchAll(/import\s*\{([^}]*)\}\s*from\s*"[^"]*helpers\/app"/g)] - .flatMap((match) => match[1].split(",")) - .map((binding) => binding.trim().split(/\s+as\s+/)[0].trim()); - const fixtures = imported.filter((binding) => FIXTURE_EXPORTS.includes(binding)); - expect( - fixtures, - `${spec} names seeded fixtures (${fixtures.join(", ")}), which no cluster has`, - ).toEqual([]); - } - }); - - it.each(sharedFolders())( - "shared/%s holds one spec, holding one test, titled for its folder", - (folder) => { - /* - * The same shape as a resource folder in `tests/`, and for the same reasons — one - * journey per resource, and a title that says where it lives so `--grep` can - * select an area. Not the empty-and-error rule, though: those need `?mock=`, which - * is the one thing a spec here may not touch. - */ - const specs = specsIn(join(SHARED, folder)); - expect(specs, `shared/${folder}/ should hold one spec`).toHaveLength(1); - - const source = readFileSync(join(SHARED, folder, specs[0]), "utf8"); - expect( - [...source.matchAll(/^test(\.skip)?\(/gm)], - `shared/${folder}/${specs[0]} should hold one test`, - ).toHaveLength(1); - - const expected = folder.replace(/-/g, " "); - for (const title of titles(join(SHARED, folder, specs[0]))) { - const prefix = title.split(":")[0]; - expect( - prefix === expected || prefix.startsWith(`${expected} `), - `shared/${folder}/${specs[0]}: "${title}" should begin with "${expected}"`, - ).toBe(true); - } - }, - ); - it("every spec outside a folder is about the application, not a resource", () => { // Top level means the shell, routing, the dashboard, theme contrast — things that // are about the app rather than about something it manages. A resource folder diff --git a/ui/playwright/globalSetup.ts b/ui/playwright/globalSetup.ts index 0ba0a79d1c..0d5b103e20 100644 --- a/ui/playwright/globalSetup.ts +++ b/ui/playwright/globalSetup.ts @@ -1,5 +1,5 @@ import { chromium, type FullConfig, type Page } from "@playwright/test"; -import { LIVE_IS_DEPLOYED, LIVE_PROJECT } from "../playwright.config"; +import { LIVE_PROJECT } from "../playwright.config"; /** * Waits until each server actually renders the app, not merely answers on its @@ -121,26 +121,4 @@ async function verifyLiveWiring(page: Page, baseUrl: string): Promise { `is a service worker, so a live run cannot be trusted while one is installed.`, ); } - - if (!LIVE_IS_DEPLOYED) return; - - /* - * The third way, and one only a deployment can be asked. The checks above lean on a - * build-time `VITE_API_MODE` pin, which the image has not got — what it has instead is - * no mock backend in it at all. So a 404 says both that fixtures cannot be served and - * that this is the built artifact rather than a dev server, which answers 200 and - * would pass every spec while testing none of the things the image is the point of. - */ - const worker = await page.request.get(`${baseUrl}/mockServiceWorker.js`, { - failOnStatusCode: false, - }); - if (worker.status() !== 404) { - throw new Error( - `${baseUrl}/mockServiceWorker.js answered ${worker.status()}, expected 404. ` + - `A built image does not contain that file and its nginx refuses the path, so ` + - `this address is serving something else — most likely a dev server, which ` + - `would pass these specs without testing nginx, the SPA fallback, or the ` + - `env-config.js rendered at pod start.`, - ); - } } diff --git a/ui/playwright/helpers/app.ts b/ui/playwright/helpers/app.ts index df424b0bd5..7f983645d8 100644 --- a/ui/playwright/helpers/app.ts +++ b/ui/playwright/helpers/app.ts @@ -6,21 +6,21 @@ * the app already ships for the purpose. */ -import { expect, test, type Locator, type Page } from "@playwright/test"; - -import { LIVE_PROJECT } from "../../playwright.config"; +import { expect, type Locator, type Page } from "@playwright/test"; /** Routes the suite drives. Mirrors `src/router/routes.ts`. */ export const routes = { dashboard: "/", login: "/login", agents: "/agents", + agentNew: "/agents/new", models: "/models", modelNew: "/models/new", mcpServers: "/mcp", mcpServerNew: "/mcp/new", prompts: "/prompts", promptNew: "/prompts/new", + snapshots: "/snapshots", substrate: "/substrate", /* The templates list is a tab of the agents page now. The old address still resolves — it redirects here — but a test should go where the reader goes. */ @@ -157,83 +157,9 @@ export function dataRows(page: Page): Locator { return page.locator("tbody tr.ant-table-row"); } -/** - * How long one read inside a journey may take, which is not how long the journey may. - * - * These specs asked for sixty seconds an assertion while the mock lane's whole - * `LIFECYCLE_TIMEOUT` is sixty — so an assertion could never exhaust its own budget, and - * a broken one was reported as "Test timeout of 60000ms exceeded" rather than by name. - * The numbers were sized for the live budget and inherited unchanged by the mock run. - * Same argument as `PRESS_TIMEOUT`: when this is what failed, this should be what says - * so. - */ -export const READ_TIMEOUT = process.env.UI_LOOP_LIVE === "true" ? 60_000 : 20_000; - - /** A navigation-sized budget, for the app booting rather than for what it rendered. */ const APP_BOOT_TIMEOUT = 15_000; -/** - * Whether this run is against a cluster rather than the fixtures. - * - * For the few places where the two backends differ in kind and not merely in speed — - * a reload restarts the in-browser fixture backend, and a controller fills a status - * only on a real one. A spec branching on this is saying so out loud, which is better - * than a claim that quietly means something different on each. - */ -export function isLiveRun(): boolean { - return test.info().project.name === LIVE_PROJECT; -} - -/** - * Navigates, for a spec in `shared/` that runs against either backend. - * - * `loadPage` cannot: it appends a `?mock=` scenario, which is meaningless to a cluster - * and misleading in a live trace. So the scenario is added only where there is a mock - * backend to read it, and the wait is on the shell rather than on a heading, a live - * page taking longer to have one. - */ -export async function loadApp(page: Page, path: string): Promise { - const live = isLiveRun(); - await page.goto(live ? path : withScenario(path, "ok"), { - waitUntil: "domcontentloaded", - }); - await expect(page.getByTestId("app-content")).toBeVisible({ - timeout: live ? 60_000 : APP_BOOT_TIMEOUT, - }); -} - -/** - * Fails when the page is reporting that it could not reach the backend. - * - * Worth calling before asserting on content: the alternative is a failure reading "the - * table is empty" when the truth is "the backend did not answer" — the same distinction - * the app itself is careful about. - * - * **Call it after something that proves the read landed**, never straight after a - * navigation. It is a count taken once, and `loadApp` returns on the shell: asked in - * that gap it passes on every page, including one whose read fails a moment later. A - * list's `expectListLoaded` or its own summary is the signal to put in front of it — - * `live/pages.spec.ts` keeps the table of what each page draws. - */ -export async function expectNoLoadFailure(page: Page): Promise { - /* - * Both families, because the app says it two ways. A page with room for an alert draws - * `-error`; one with only a line of copy where the data goes draws - * `-unavailable` — the dashboard's recent list, the schedules table, the tools - * chart, the schedule history, each rendered on `error` and nothing else. Matching the - * first alone, this passed on a dashboard that could not reach the controller. - */ - const alerts = page.locator( - '[data-testid$="-error"], [data-testid$="-unavailable"]', - ); - const count = await alerts.count(); - if (count === 0) return; - - const texts = await alerts.allInnerTexts(); - expect(count, `the page reported a failure to load: ${texts.join(" | ")}`).toBe(0); -} - /** * Resolves once the app is on screen and no loading indicator is left on it. * @@ -245,89 +171,3 @@ export async function expectSettled(page: Page): Promise { await expect(page.locator("#root")).not.toBeEmpty({ timeout: APP_BOOT_TIMEOUT }); await expect(page.locator(".ant-spin-spinning")).toHaveCount(0); } - -/** - * A name no human would choose, carrying the run that made it. - * - * A spec that creates on a real cluster deletes what it made, but a run killed between - * the two cannot — so the name has to be enough for a person to identify the litter - * without the harness. Unique per run on the fixtures too, where it costs nothing and - * keeps a shared spec reading the same on both backends. - */ -export const throwawayName = (label: string): string => - `e2e-live-${label}-${process.pid}-${Date.now().toString(36)}`; - -/** - * Narrows a list page to one name, using the search box the page already offers. - * - * Needed by any spec that creates a row and then reads it back on a list it did not - * seed. These tables page at 25, so on a cluster whose list already fills a page the - * new row is on page two — present, correct, and invisible to a locator. Searching for - * a name only this run could have made puts it on screen wherever it landed, and is - * what a reader looking for their own resource would do. - * - * Client-side, over every row fetched, so it is not a second read that could disagree - * with the first. - * - * @param list the page's test-id prefix — `models` for `models-filters-search`. - */ -export async function searchList(page: Page, list: string, term: string): Promise { - await page.getByTestId(`${list}-filters-search`).fill(term); -} - -/** - * Asserts how many rows the whole list holds — which is not how many are on screen. - * - * Read off the `-summary` line ("3 of 27 configurations"), whose second number is - * computed from every row fetched rather than from the page being shown. Counting - * `dataRows` instead answers a different question on any list longer than 25, and - * answers it wrongly while `searchList` is narrowing the table to one row. - * - * The summary renders only after a successful load, so waiting for it to say a number - * also distinguishes "the list holds that many" from "the read failed". - */ -export async function expectListTotal( - page: Page, - list: string, - total: number, - timeout = READ_TIMEOUT, -): Promise { - await expect(page.getByTestId(`${list}-summary`)).toContainText( - new RegExp(`\\bof ${total}\\b`), - { timeout }, - ); -} - -/** - * The list's total, off the summary that only a landed read draws. - * - * It read the summary twice and required the two to agree, because every list drew - * "0 of 0" for the 600ms before its read arrived. That is fixed where it belongs now — - * the summary waits for `data` — so this is one read again, and a return of the flash - * fails the journeys that count before and after a create. - */ -async function settledTotal(page: Page, list: string): Promise { - const summary = page.getByTestId(`${list}-summary`); - await expect(summary).toContainText(/\bof \d+\b/, { timeout: READ_TIMEOUT }); - - const text = (await summary.textContent()) ?? ""; - const read = /\bof (\d+)\b/.exec(text)?.[1]; - expect(read, `no total could be read from "${text}"`).toBeDefined(); - return Number(read); -} - -/** - * Resolves once the list has answered, which is what makes a row count mean anything. - * - * A list still fetching has no rows either, so a `count()` taken too early reads zero - * and every question asked of it gets the answer "not there" — including a cleanup - * asking whether there is anything left to delete. - */ -export async function expectListLoaded(page: Page, list: string): Promise { - await settledTotal(page, list); -} - -/** What `expectListTotal` would be reading now, for a count taken before a change. */ -export async function readListTotal(page: Page, list: string): Promise { - return settledTotal(page, list); -} diff --git a/ui/playwright/helpers/cleanup.ts b/ui/playwright/helpers/cleanup.ts deleted file mode 100644 index b5e5404ae5..0000000000 --- a/ui/playwright/helpers/cleanup.ts +++ /dev/null @@ -1,58 +0,0 @@ -import { expect, type Page } from "@playwright/test"; - -import { expectListLoaded, loadApp, rowNamed, searchList } from "./app"; -import { confirmDelete } from "./resource"; - -/** - * Removes a row this run made, from a `test.afterEach`, without ever throwing. - * - * Every move a cleanup makes can fail — the navigation, the list read, the delete — and - * each of them throws. Thrown from a cleanup, that replaces the failure the test was - * actually reporting, *and* skips the delete underneath it: the run reports a timeout in - * the cleanup while the resource stays on the cluster. - * - * A hook rather than a `finally`, which is the other half: a timed-out test has a closed - * page, so everything in its `finally` throws before it can delete anything. - * - * So it warns and returns. A cleanup that could not run says so in the output, and the - * test still reports what it found. - */ -export async function sweepUp( - page: Page, - list: string, - name: string, - /** Where that list lives, for the two that do not share their test-id prefix. */ - path = `/${list}`, -): Promise { - try { - await loadApp(page, path); - await searchList(page, list, name); - // Asked only once the list has answered: a read still in flight has no rows either, - // and taking that for "already gone" would leave it on the cluster. - await expectListLoaded(page, list); - if ((await rowNamed(page, name).count()) === 0) return; - - await confirmDelete(page, name); - await expect(rowNamed(page, name)).toHaveCount(0, { timeout: 60_000 }); - } catch (error) { - console.warn(`cleanup: ${name} may still be on the cluster — ${String(error)}`); - } -} - -/** - * The same promise for a cleanup that is not a row on a list: run it, never throw. - * - * For the journeys whose resource is reached by its own address — a schedule's detail - * page, a template's — where what has to happen is a navigation and a confirm rather - * than a search. - */ -export async function sweepQuietly( - what: string, - remove: () => Promise, -): Promise { - try { - await remove(); - } catch (error) { - console.warn(`cleanup: ${what} may still be on the cluster — ${String(error)}`); - } -} diff --git a/ui/playwright/helpers/resource.ts b/ui/playwright/helpers/resource.ts index f63a8c8b3b..35f3e2f261 100644 --- a/ui/playwright/helpers/resource.ts +++ b/ui/playwright/helpers/resource.ts @@ -13,7 +13,7 @@ import { expect, type Locator, type Page } from "@playwright/test"; -import { READ_TIMEOUT, withScenario } from "./app"; +import { withScenario } from "./app"; /** * How long one resource's whole lifecycle is allowed to take. @@ -33,28 +33,8 @@ import { READ_TIMEOUT, withScenario } from "./app"; * contended run, tight enough that a journey which doubles in cost is still a * failure. The slowest is prompts at about forty seconds under full parallel load, * its list fanning out one call per namespace. - * - * **Live is a different budget, not a slower version of the same one.** Against a - * cluster every step is a round trip to an API server and some of them wait on a - * controller to reconcile, which is work no mock does at all — the config already - * doubles the per-test default for that reason, and a `describe.configure` overrides - * it, so a lifecycle asking for sixty here would have been the tightest budget in the - * live run rather than the loosest. - */ -export const LIFECYCLE_TIMEOUT = - process.env.UI_LOOP_LIVE === "true" ? 180_000 : 60_000; - -/** - * How long `pressUntil` has to land a press, which has to outlast one attempt at it. - * - * `toPass` checks its deadline *between* attempts, so a budget shorter than one attempt - * buys exactly one press — the retry this helper exists for never happens. An attempt is - * a click plus the caller's `settled`, and `settled` is an assertion on the live - * project's own thirty-second `expect` timeout: mock, fifteen leaves room for two; - * live, fifteen was less than one, so a swallowed Delete was reported as "the page never - * navigated" and the resource stayed on the cluster. */ -const PRESS_TIMEOUT = process.env.UI_LOOP_LIVE === "true" ? 90_000 : 15_000; +export const LIFECYCLE_TIMEOUT = 60_000; /** * Presses a dialog's button, once the dialog has stopped arriving. @@ -117,7 +97,9 @@ export async function pressOnce(button: Locator): Promise { export async function pressUntil( button: Locator, settled: () => Promise, - timeout = PRESS_TIMEOUT, + // Half the thirty-second test budget, so that when this is what failed, this is + // what says so: at thirty the test expired first and reported its own timeout. + timeout = 15_000, ): Promise { await expect(async () => { // Bounded, because `toPass` checks its deadline between attempts and no @@ -128,36 +110,6 @@ export async function pressUntil( }).toPass({ timeout }); } -/** - * Whether something turned up, for a cleanup that has to tell "gone" from "not yet". - * - * A `count()` taken straight after a navigation asks the wrong question: `goto` and - * `loadApp` both return before the page's own read has landed, so nothing is on screen - * yet and every "is it still there?" is answered no — which is how a cleanup came to - * skip its delete and leave a real resource on the cluster. Waiting first is what makes - * an absence mean something. - * - * It resolves `false` rather than throwing, because this is called from a `finally`: - * an assertion failing there would replace the failure the test was actually reporting. - */ -export async function appeared(locator: Locator, timeout = READ_TIMEOUT): Promise { - try { - await locator.waitFor({ state: "visible", timeout }); - return true; - } catch (error) { - /* - * A timeout means it is not there. Anything else — a locator matching several, say — - * is a broken check reaching the caller as the same `false`, which a cleanup reads as - * "nothing to remove" while the resource stays on the cluster. Still `false`, since - * throwing from a `finally` would replace the test's own failure, but not silently. - */ - if ((error as Error | undefined)?.name !== "TimeoutError") { - console.warn(`appeared() could not check this locator: ${String(error)}`); - } - return false; - } -} - /** * Shows that a list says it is loading, and hands back a responsive backend. * @@ -209,15 +161,6 @@ export function optionNamed(page: Page, label?: string): Locator { ); } -/** - * The dropdown on screen, whichever select opened it. By Playwright's own visibility - * rather than antd's `ant-select-dropdown-hidden`, which a dismissed dropdown does not - * carry until its close animation ends — lagging at exactly the wrong moment. - */ -function openDropdown(page: Page): Locator { - return page.locator(".ant-select-dropdown").filter({ visible: true }); -} - /** Opens a Select by its test id and picks one option by the label a reader sees. */ export async function selectOption( page: Page, @@ -229,22 +172,12 @@ export async function selectOption( } /** - * The same, where any option will do — a required field the assertion does not care - * about, or a model on a cluster whose models the spec did not install. - * - * It cannot name what it wants, so it has to be sure *which* dropdown it reads: it - * waits out any still animating away, then scopes to the one on screen. A live run - * spent its whole budget clicking an `ate-system` option from a dismissed select. + * The same, where any option will do — a field the form requires but the assertion + * does not care about, like the namespace on a harness whose step is about the image. */ export async function selectFirstOption(page: Page, testId: string): Promise { - await expect(openDropdown(page)).toHaveCount(0); await page.getByTestId(testId).click(); - - const option = openDropdown(page).locator(".ant-select-item-option").first(); - await expect(option, `the select "${testId}" offered no options`).toBeVisible({ - timeout: 30_000, - }); - await option.click(); + await optionNamed(page).first().click(); } /** diff --git a/ui/playwright/live/agent-lifecycle.spec.ts b/ui/playwright/live/agent-lifecycle.spec.ts new file mode 100644 index 0000000000..45038048ab --- /dev/null +++ b/ui/playwright/live/agent-lifecycle.spec.ts @@ -0,0 +1,108 @@ +import { test, expect } from "@playwright/test"; +import { + dataRows, + expectNoLoadFailure, + liveRoutes, + loadLive, + rowNamed, + throwawayName, +} from "./helpers/live"; + +/** + * Creating and deleting an agent, on a real cluster, through the UI. + * + * This is the journey the mock suite cannot vouch for. Both halves of it were + * broken against a real controller while the mock suite was green: the form sent + * the model as a `namespace/name` ref where the controller wanted a bare name, so + * every create was rejected; and the page read the created agent out of a wrapper + * the create response does not have, so a create that *had* worked reported failure + * and stayed on the form. Neither could be seen without a cluster. + * + * The agent is deleted in teardown as well as in the spec body, because a run that + * dies midway would otherwise leave a real resource behind. + */ + +const AGENT = throwawayName("agent"); +const NAMESPACE = "kagent"; + +test.afterEach(async ({ request, baseURL }) => { + // Deleting through the API rather than the UI: teardown runs after a failure, when + // the page may be anywhere at all, and a cleanup that depends on the UI working is + // exactly the cleanup that fails when the UI does not. + const response = await request.delete( + `${baseURL}/api/agents/${NAMESPACE}/${AGENT}`, + { failOnStatusCode: false }, + ); + // 404 is the goal state: either the spec deleted it, or it was never created. + expect( + [200, 204, 404], + `teardown could not remove ${NAMESPACE}/${AGENT} (${response.status()})`, + ).toContain(response.status()); +}); + +test("live: an agent can be created and deleted through the UI", async ({ page }) => { + await test.step("1. the create form opens with the cluster's models offered", async () => { + await loadLive(page, liveRoutes.agentNew); + await expectNoLoadFailure(page); + + await page.getByTestId("agent-form-name").fill(AGENT); + await page.getByTestId("agent-form-namespace").fill(NAMESPACE); + await page + .getByTestId("agent-form-description") + .fill("Created by the live end-to-end suite; safe to delete."); + // Required, and the form says so rather than letting a create fail at the API — + // which is the client-side validation `playwright/DEFERRED.md` records as lost + // coverage. It is not lost; this spec relies on it. + await page + .getByTestId("agent-form-system-message") + .fill("You are a test agent created by an end-to-end run. Answer briefly."); + + // The options come from the cluster's own ModelConfigs, so an empty list here is + // a real failure rather than a slow render: the install ships one. + await page.getByTestId("agent-form-model").click(); + const option = page.locator(".ant-select-item-option").first(); + await expect(option, "the cluster offered no model configurations").toBeVisible({ + timeout: 30_000, + }); + await option.click(); + }); + + await test.step("2. submitting reaches the controller and reports success", async () => { + await page.getByTestId("agent-form-submit").click(); + + // Success is leaving the form. A create that failed keeps the user on it with an + // error, which is the shape the earlier defect produced for an agent that had in + // fact been created. + await expect(page).toHaveURL(/\/agents$/, { timeout: 60_000 }); + }); + + await test.step("3. the agent is listed, read back from the cluster", async () => { + await expectNoLoadFailure(page); + await expect(rowNamed(page, AGENT)).toHaveCount(1, { timeout: 60_000 }); + }); + + await test.step("4. deleting it asks first, and names what it will delete", async () => { + const before = await dataRows(page).count(); + + await page.getByTestId(`delete-${AGENT}`).click(); + const confirm = page.locator(".ant-popconfirm").filter({ hasText: AGENT }); + await expect(confirm, "the confirmation did not name the agent").toBeVisible(); + + await page.getByRole("button", { name: "Delete", exact: true }).last().click(); + + await test.step("and the row is gone from the list a reader looks at", async () => { + await expect(rowNamed(page, AGENT)).toHaveCount(0, { timeout: 60_000 }); + await expect(dataRows(page)).toHaveCount(before - 1); + }); + }); + + await test.step("5. the cluster agrees it is gone", async () => { + // The list could be stale; the controller cannot be. This is what makes the + // previous step evidence of a delete rather than of a re-render. + const response = await page.request.get( + `/api/agents/${NAMESPACE}/${AGENT}`, + { failOnStatusCode: false }, + ); + expect(response.status()).toBe(404); + }); +}); diff --git a/ui/playwright/live/helpers/live.ts b/ui/playwright/live/helpers/live.ts index b27d240f36..c7abe1c2e6 100644 --- a/ui/playwright/live/helpers/live.ts +++ b/ui/playwright/live/helpers/live.ts @@ -1,34 +1,70 @@ +import { expect, type Page } from "@playwright/test"; + /** * Helpers for the live suite. * - * Only what is genuinely live-specific. Anything that works on either backend lives in - * `helpers/app.ts` instead, so a spec in `shared/` can reach it — `loadApp`, - * `expectNoLoadFailure`, `rowNamed` and `dataRows` all moved there, and this file had - * its own slightly different copies of the last two for no reason anybody could name. + * The mock suite's helpers are built around `?mock=` scenarios, which is the one + * thing a real controller cannot be told to do — there is no way to ask a cluster + * for a 500. So the live specs assert what a cluster genuinely produces, and these + * are the parts of that worth sharing. */ -/** - * The pages the live sweep visits, which is not every route the app has. - * - * Its own table rather than `routes` from `helpers/app.ts`: that one is every address a - * spec might drive, including forms and detail pages that need an id. This is the set of - * *landing pages* worth loading against a cluster, and `pages.spec.ts` iterates it. - * - * Mirrors `src/router/routes.ts` rather than importing it, like the mock suite's table: - * a spec that reads the app's own constant follows a rename silently. The copy can rot - * instead — this one carried `/agents/new` for a form that had been deleted, and - * `agentDetail` (`/agents/:id`) swallowed the address so it was not even a 404. - */ +/** Where each page lives, so a renamed route breaks in one place. */ export const liveRoutes = { dashboard: "/", agents: "/agents", - /* A tab of the agents page. `/agent-templates` still redirects here, but a spec - should go where the reader goes. */ - agentTemplates: "/agents?tab=templates", - agentTemplateNew: "/agent-templates/new", + agentNew: "/agents/new", models: "/models", mcpServers: "/mcp", prompts: "/prompts", - schedules: "/schedules", substrate: "/substrate", } as const; + +/** + * Loads a page and waits for the app shell, not for the network to fall quiet. + * + * `networkidle` is the wrong signal against a real backend: a page that polls, or + * an agent whose status is still reconciling, may never produce a quiet network, + * and the wait would time out on a page that rendered correctly seconds earlier. + */ +export async function loadLive(page: Page, path: string): Promise { + await page.goto(path, { waitUntil: "domcontentloaded" }); + await page.waitForSelector('[data-testid="app-content"]', { timeout: 60_000 }); +} + +/** + * Fails when the page is reporting that it could not reach the backend. + * + * Called by every spec before asserting on content, because the alternative is a + * failure that reads as "the table is empty" when the truth is "the cluster did not + * answer" — the same distinction the app itself is careful about. + */ +export async function expectNoLoadFailure(page: Page): Promise { + const alerts = page.locator('[data-testid$="-error"]'); + const count = await alerts.count(); + if (count === 0) return; + + const texts = await alerts.allInnerTexts(); + expect( + count, + `the page reported a failure to load: ${texts.join(" | ")}`, + ).toBe(0); +} + +/** A table row containing `text`. */ +export const rowNamed = (page: Page, text: string) => + page.locator("tbody tr").filter({ hasText: text }); + +/** Rows that carry data, excluding the placeholder antd renders when there are none. */ +export const dataRows = (page: Page) => + page.locator("tbody tr").filter({ hasNot: page.locator(".ant-table-placeholder") }); + +/** + * A name no human would choose, carrying the run that made it. + * + * These specs create real resources on a real cluster. They delete what they create, + * but a run killed between the two cannot, so the name has to be enough for a person + * to identify the litter without the harness. + */ +export const throwawayName = (label: string): string => + `e2e-live-${label}-${process.pid}-${Date.now().toString(36)}`; diff --git a/ui/playwright/live/pages.spec.ts b/ui/playwright/live/pages.spec.ts index 6716687f58..51d951ee02 100644 --- a/ui/playwright/live/pages.spec.ts +++ b/ui/playwright/live/pages.spec.ts @@ -1,14 +1,10 @@ -import { type Locator, type Page } from "@playwright/test"; -import { test, expect } from "../fixtures/test"; +import { test, expect } from "@playwright/test"; import { - READ_TIMEOUT, dataRows, - expectListLoaded, expectNoLoadFailure, - loadApp, -} from "../helpers/app"; -import { LIFECYCLE_TIMEOUT } from "../helpers/resource"; -import { liveRoutes } from "./helpers/live"; + liveRoutes, + loadLive, +} from "./helpers/live"; /** * Every page, against a real controller. @@ -25,213 +21,75 @@ import { liveRoutes } from "./helpers/live"; * sent where a bare name belonged. None of them were visible to a green mock suite. */ -/** Any of these ids being on screen, whichever page they belong to. */ -const anyOf = (page: Page, ...testIds: string[]) => - page.locator(testIds.map((id) => `[data-testid="${id}"]`).join(", ")); - -/** A list's summary, which only a successful read draws, or the alert a failure does. */ -const listAnswered = (page: Page, list: string) => - anyOf(page, `${list}-summary`, `${list}-error`); - -/** - * What each page draws once it has an answer — success *or* failure, which `loadApp` - * does not wait for, it waiting for the shell. - * - * Without it the sweep asked its question before the page had asked the controller - * anything: nothing had rendered either way, so every page passed, and a read that - * failed a second later passed with it. Demonstrated by delaying one read and then - * failing it — the guard saw nothing and the alert arrived after the step had moved on. - * - * The failure states are in here beside the good ones deliberately. Gated on success - * alone this waits out sixty seconds and reports "no summary", where what happened is on - * screen and has a message on it: `expectNoLoadFailure` reads it out instead. - * - * Each list's summary is the signal rather than its rows, because it renders "N of M" - * for a read that succeeded — zero rows included — and nothing at all while one is in - * flight. - */ -const answered: Record< - Exclude, - (page: Page) => Locator -> = { - dashboard: (page) => - anyOf( - page, - "recent-agents", - "recent-agents-empty", - "recent-agents-unavailable", - "dashboard-error", - ), - agents: (page) => listAnswered(page, "agents"), - agentTemplates: (page) => listAnswered(page, "templates"), - models: (page) => listAnswered(page, "models"), - mcpServers: (page) => listAnswered(page, "mcp-servers"), - prompts: (page) => listAnswered(page, "prompts"), - // No summary on this one, so its table says it: rows, or "No schedules were found." - schedules: (page) => - anyOf(page, "schedules-empty", "schedules-error").or( - page.locator('[data-testid="schedules-table"] tbody tr.ant-table-row'), - ), - // The tiles draw an em-dash where the number goes until the read lands, so a digit is - // what says it has. `live/substrate.spec.ts` owns what the page then reports. - substrate: (page) => - page - .getByTestId("substrate-stat-actors-value") - .filter({ hasText: /\d/ }) - .or(page.getByTestId("substrate-inventory-error")), -}; - -/* - * Eight pages, each with a load and a read behind its own sixty seconds, in one test. - * On the live project's 120s default two slow ones exhaust it and the failure reads - * "Test timeout exceeded" rather than naming the page — the reporting loss that - * `READ_TIMEOUT` and `PRESS_TIMEOUT` exist to avoid. - */ -test.describe.configure({ timeout: LIFECYCLE_TIMEOUT }); - test("live: every page loads against the cluster and reports no failure", async ({ page, }) => { - /* - * What the controller refused, as it arrives. - * - * The rendering is a second-hand account of a read and it lags in both directions: a - * page that has just asked draws neither its summary nor its error, and the summary - * can be on screen for a frame before the answer lands. A check made in that gap - * passes on a read that fails a moment later — measured here by delaying one read to - * 1.5s and failing it, which the shell-only order and the summary-gated order both - * passed while the page ended up saying "Could not load agents · HTTP 500". - * - * A response has no such gap. Asserted per page for what has landed by then, and - * again at the end — which is the one that catches the case above: measured, the - * agents page's gate resolved with nothing refused yet, and the two 500s were in here - * three seconds later. Each is labelled with the page in hand when it landed, which - * for a late one is the page after the one that asked. - */ - const refused: string[] = []; - let current = ""; - page.on("response", (response) => { - if (!response.url().includes("/api/")) return; - // HTTP tells only half of it. The app speaks gRPC-Web, so a controller that refuses - // a read still answers 200 and puts the reason in a `grpc-status` trailer — measured - // against this cluster, a missing library came back `200 ok=true grpc-status=5`. - // Reading the status alone would have watched for a failure mode the controller does - // not have, leaving only nginx's own 502s. A refusal raised mid-response carries its - // status in the body instead, which this does not read. - // - // Every status counts, `NOT_FOUND` included: these are list pages, and none of them - // asks the controller for something it is allowed not to have. A page that did — - // a detail page reads one and renders a "no such library" state rather than an - // error — would need this to say which statuses it means. - const status = response.headers()["grpc-status"]; - if (response.ok() && (status === undefined || status === "0")) return; - const how = response.ok() ? `grpc-status ${status}` : `HTTP ${response.status()}`; - refused.push(`${current}: ${how} ${new URL(response.url()).pathname}`); - }); - for (const [name, path] of Object.entries(liveRoutes)) { - if (name === "agentTemplateNew") continue; // A form, covered by the lifecycle spec. + if (name === "agentNew") continue; // A form, covered by the lifecycle spec. await test.step(`${name} (${path})`, async () => { - current = name; - await loadApp(page, path); - // The page drew something, either way — see `answered`. A route added to - // `liveRoutes` without an entry there fails to compile rather than sweeping past. - await expect( - answered[name as keyof typeof answered](page).first(), - `${name} never came back, either way`, - ).toBeVisible({ timeout: 60_000 }); - // And what it drew was not a failure. The distinction worth keeping: a page that - // could not reach the controller must not be read as a page with nothing on it. + await loadLive(page, path); + // The distinction worth keeping: a page that could not reach the controller + // must not be read as a page with nothing on it. await expectNoLoadFailure(page); - expect(refused, "the controller refused a read").toEqual([]); + await expect(page.locator('[data-testid="app-content"]')).toBeVisible(); }); } - - // The reads that failed late, each named by the page that asked. - expect(refused, "a read failed after its page had been checked").toEqual([]); }); -test("live: the agents the cluster installed are listed, each on its harness", async ({ +test("live: the agents the cluster installed are listed with their model", async ({ page, }) => { - await loadApp(page, liveRoutes.agents); - await expectListLoaded(page, "agents"); + await loadLive(page, liveRoutes.agents); await expectNoLoadFailure(page); await test.step("the install's own agents are present", async () => { - // Off the summary: it counts every row fetched rather than the 25 on screen, and - // "some" rather than a number keeps this from breaking when the chart's default - // set changes — while still failing if the list came back empty. - await expect(page.getByTestId("agents-summary")).toContainText(/\bof [1-9]\d*\b/, { - timeout: 60_000, - }); + // kagent installs a set of agents; asserting on the count rather than on names + // keeps this from breaking when the chart's default set changes, while still + // failing if the list is empty because nothing was read. + await expect(dataRows(page).first()).toBeVisible({ timeout: 60_000 }); + const count = await dataRows(page).count(); + expect(count, "the cluster reported no agents at all").toBeGreaterThan(0); }); - await test.step("each row names the harness its agent runs on", async () => { - // An agent is a (template, harness) pair the controller materialises, so a row - // naming no harness is a pair that did not resolve. Read off the element holding - // the harness and nothing else: the row carries the namespace too, and on every - // cluster this runs against both are `kagent`. - const harnesses = page.locator('[data-testid^="agent-harness-"]'); - await expect(harnesses.first()).toBeVisible({ timeout: 60_000 }); - for (const harness of await harnesses.allTextContents()) { - expect(harness.trim(), "an agent row named no harness").not.toBe(""); - } + await test.step("each row resolves its model rather than showing a ref", async () => { + // The controller denormalises the referenced ModelConfig onto the row. A row + // showing a bare dash here means either the reference did not resolve or the UI + // read the wrong field — both of which a fixture would have hidden. + const first = dataRows(page).first(); + await expect(first).not.toContainText("__NS__"); + }); + + await test.step("the tool count is a number, not a crash", async () => { + // `tools` arrives as JSON null for an agent with none, because Go marshals a nil + // slice that way. Reading `.length` off it took this whole page down against a + // real cluster once. + await expect(page.getByTestId("agents-error")).toHaveCount(0); }); }); test("live: the models the cluster installed are listed with their provider", async ({ page, }) => { - await loadApp(page, liveRoutes.models); - await expectListLoaded(page, "models"); + await loadLive(page, liveRoutes.models); await expectNoLoadFailure(page); - await expect(page.getByTestId("models-summary")).toContainText(/\bof [1-9]\d*\b/, { - timeout: 60_000, - }); - - // The provider is the column most easily left blank: the row renders its tag whether - // or not `spec.provider` came back, so the claim is that the tag says something. - const provider = dataRows(page).first().locator(".ant-tag").first(); - await expect(provider, "the first model row named no provider").toHaveText(/\S/); + await expect(dataRows(page).first()).toBeVisible({ timeout: 60_000 }); + const count = await dataRows(page).count(); + expect(count, "the cluster reported no model configurations").toBeGreaterThan(0); }); -test("live: the tool server list counts what the controller gave it", async ({ - page, -}) => { - await loadApp(page, liveRoutes.mcpServers); - await expectListLoaded(page, "mcp-servers"); +test("live: tool servers report the tools they discovered", async ({ page }) => { + await loadLive(page, liveRoutes.mcpServers); await expectNoLoadFailure(page); await test.step("the summary counts servers and tools", async () => { - /* - * The numbers, because the words are static: "N of M servers · K tools" says - * "server" on a page that counted nothing, which is what this asserted before. - * - * Neither count may be required to be non-zero, and that is a fact about the - * clusters rather than a weakening. CI installs with `kagent-tools.enabled=false` - * and `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. Asking for one passed on a laptop and would have failed every CI run. - * Tool discovery is asynchronous besides (#2849), so a server that has just been - * registered honestly reports none for a while. - */ - const summary = page.getByTestId("mcp-servers-summary"); - await expect(summary).toContainText(/\b\d+ of \d+ servers?\b/, { - timeout: READ_TIMEOUT, + // Both counts are derived: the servers from the rows, the tools by summing what + // each discovered. A server that discovered none must still be counted, since a + // registered server reporting nothing is the one most likely to be misconfigured. + await expect(page.getByTestId("mcp-servers-summary")).toBeVisible({ + timeout: 60_000, }); - await expect(summary).toContainText(/·\s*\d+ tools?\b/); - }); - - await test.step("and each server it does have reports a tool count", async () => { - // Where the cluster has none this asserts nothing, which is why the summary above - // is the claim. `tools` arrives as JSON null for a server that discovered nothing, - // Go marshalling a nil slice that way, and reading `.length` off it took this page - // down against a real cluster once. - const rows = dataRows(page); - if ((await rows.count()) === 0) return; - await expect(rows.first()).toContainText(/\d/); + await expect(page.getByTestId("mcp-servers-summary")).toContainText("server"); }); }); diff --git a/ui/playwright/live/schedules.spec.ts b/ui/playwright/live/schedules.spec.ts index 6082c88b71..528065720f 100644 --- a/ui/playwright/live/schedules.spec.ts +++ b/ui/playwright/live/schedules.spec.ts @@ -1,152 +1,72 @@ import { test, expect } from "../fixtures/test"; -import { loadApp, throwawayName } from "../helpers/app"; +import { loadLive, throwawayName } from "./helpers/live"; import { tick } from "../helpers/controls"; -import { sweepQuietly } from "../helpers/cleanup"; -import { - LIFECYCLE_TIMEOUT, - appeared, - optionNamed, - pressUntil, -} from "../helpers/resource"; -import { liveRoutes } from "./helpers/live"; -/** - * A schedule survives a reload — which is the one claim the fixtures cannot answer. - * - * The journey itself is in `shared/schedules/schedules.spec.ts` and runs against both - * backends. What is left here is the part that has to be live: everything a create - * reports back could be the form showing itself its own draft, and only re-reading from - * a backend that actually stores can tell the two apart. The fixture backend keeps - * writes in the page's own memory, so a reload there starts a backend that has never - * heard of the schedule — the one move it cannot survive. - * - * The values are chosen to be the ones a controller drops or rounds: a fractional - * timeout stored as seconds plus nanos, a named IANA zone, and a weekday set. - * - * Kept paused: execution is covered by the Go scheduling E2Es with a controlled model. - */ - -const CREATED = throwawayName("schedule"); - -/* - * A journey's budget, not a page's. This creates on a cluster, reloads twice and deletes - * in a `finally` — the same shape as every `shared/` journey, which all take - * `LIFECYCLE_TIMEOUT`. Left on the global live budget it is the tightest one in the run, - * and a kill mid-cleanup leaves a real Schedule behind. - */ -test.describe.configure({ timeout: LIFECYCLE_TIMEOUT }); - -/** Where this run's schedule lives, so the hook below can remove it. */ -let detailURL: string | undefined; - -/* - * Cleanup in a hook, not in the body's `finally`. - * - * A timed-out test is the likeliest live failure — a controller that never reconciles — - * and it is exactly the one a `finally` cannot clean up after: Playwright has closed the - * page by then, so every call in it throws. Measured, with a four-second test: the - * `finally` was refused with "Target page, context or browser has been closed" while - * this hook still drove the page. Hooks get their own budget, which is the point. - */ -test.afterEach(async ({ page }) => { - if (detailURL === undefined) return; - const detail = detailURL; - detailURL = undefined; - await sweepQuietly(CREATED, async () => { - await page.goto(detail); - const remove = page - .getByTestId("schedule-danger") - .getByRole("button", { name: `Delete schedule ${CREATED}`, exact: true }); - // Waited for, not counted once: `goto` resolves on load and the detail read has not - // landed, so the danger zone is not drawn yet. See `appeared`. - if (await appeared(remove)) { - await remove.click(); - await pressUntil( - page - .getByRole("dialog", { name: `Delete schedule ${CREATED}?`, exact: true }) - .getByRole("button", { name: "Delete", exact: true }), - () => expect(page).toHaveURL(/\/schedules(\?|$)/), - ); - } - }); -}); - -test("live: a schedule's configuration survives a reload", async ({ page }) => { - await test.step("1. a schedule is created with the values most easily lost", async () => { - await loadApp(page, liveRoutes.schedules); - await page.getByTestId("schedules-new").click(); - - await page.getByTestId("schedule-agent").click(); - const agent = optionNamed(page).first(); - await expect(agent, "the cluster offered no agents to schedule").toBeVisible({ - timeout: 30_000, - }); - await agent.click(); - - await page.getByTestId("schedule-name").fill(CREATED); - await page.getByTestId("schedule-frequency").click(); - await pressUntil(optionNamed(page, "Weekly"), () => - expect(page.getByTestId("schedule-days")).toBeVisible(), - ); +// Requires one agent with a ready revision, and takes whichever that is. Keep the +// schedule paused: runtime execution is covered by the Go scheduling E2Es with a +// controlled model. +test("live: schedule configuration persists through the browser and controller", async ({ page }) => { + const name = throwawayName("schedule"); + let detailURL: string | undefined; + try { + await loadLive(page, "/schedules"); + await page.getByRole("button", { name: "New Schedule", exact: true }).click(); + await page.getByLabel("Agent", { exact: true }).click(); + // Whichever agent the fleet offers, rather than `kagent/smoke on kagent` by name: + // #2422 split an agent into a template and a harness, and the form appends a + // reason to the label of any whose revision is not ready, so an exact title cannot + // match. Disabled ones are excluded because antd renders its "no match" and loading + // rows as options too. + await page + .locator(".ant-select-dropdown:not(.ant-select-dropdown-hidden)") + .locator(".ant-select-item-option:not(.ant-select-item-option-disabled)") + .first() + .click(); + await page.getByLabel("Schedule Name", { exact: true }).fill(name); + await page.getByLabel("Prompt", { exact: true }).fill("Report cluster health."); + await page.getByLabel("Repeat", { exact: true }).click(); + await page.getByTitle("Weekly", { exact: true }).click(); for (const day of ["Tuesday", "Wednesday", "Thursday", "Friday"]) { + // `tick` rather than `check()`, which double-toggles when the card re-renders + // under it. await tick(page.getByLabel(day, { exact: true })); } - await page.getByTestId("schedule-time").fill("09:00"); - await page.getByTestId("schedule-timezone").locator("input").fill("America/New_York"); - await page.keyboard.press("Escape"); - await page.getByTestId("schedule-prompt").fill("Report cluster health."); - await page.getByTestId("schedule-timeout").fill("90.001"); - await page.getByTestId("schedule-enabled").uncheck(); - - await page.getByTestId("schedule-submit").click(); - /* - * The address first, then what is on it. Recorded after the heading, a create that - * reached the controller while the detail page was slow to draw left the hook with - * no URL to clean up — a real Schedule on the cluster, which is the one thing the - * hook exists to prevent. - */ - await page.waitForURL(/\/schedules\/[0-9a-f-]+(\?|$)/, { timeout: 60_000 }); + await page.getByLabel("At time", { exact: true }).fill("09:00"); + await page.getByLabel("Time zone", { exact: true }).fill("America/New_York"); + await page.getByLabel("Execution timeout (seconds)", { exact: true }).fill("90.001"); + await page.getByLabel("Enable Schedule", { exact: true }).uncheck(); + await page.getByRole("button", { name: "Create schedule", exact: true }).click(); + await expect(page.getByRole("heading", { name, exact: true })).toBeVisible(); detailURL = page.url(); - await expect(page.getByRole("heading", { name: CREATED, exact: true })).toBeVisible({ - timeout: 60_000, - }); - }); - - await test.step("2. a reload reads it back from the controller, unchanged", async () => { - // The whole spec. Everything above could be the page showing itself what it just - // sent; only a re-read says the controller stored it. await page.reload(); + await expect(page.getByRole("button", { name: "Resume", exact: true })).toBeEnabled(); + await expect(page.getByText("Weekdays at 09:00", { exact: true })).toBeVisible(); + await expect(page.getByText("90.001 seconds", { exact: true })).toBeVisible(); + await expect(page.getByText("America/New_York", { exact: true })).toBeVisible(); + await expect(page.getByText("No executions yet", { exact: true })).toBeVisible(); - await expect(page.getByTestId("schedule-pause")).toHaveText("Resume", { - timeout: 60_000, - }); - await expect(page.getByTestId("schedule-meta")).toContainText("Weekdays at 09:00"); - await expect(page.getByTestId("schedule-meta")).toContainText("America/New_York"); - // `90.001` is stored as seconds plus nanos, so a controller that kept only the - // seconds would read back "90 seconds" here. - await expect(page.getByTestId("schedule-detail")).toContainText("90.001 seconds"); - await expect(page.getByTestId("schedule-detail")).toContainText( - "Report cluster health.", - ); - }); - - await test.step("3. and so does an edit", async () => { - await page.getByTestId("schedule-edit").click(); - await expect(page.getByTestId("schedule-timeout")).toHaveValue("90.001", { - timeout: 60_000, - }); - await page.getByTestId("schedule-prompt").fill("Report unhealthy workloads only."); - await page.getByTestId("schedule-submit").click(); - await expect(page.getByRole("heading", { name: CREATED, exact: true })).toBeVisible({ - timeout: 60_000, - }); - + await page.getByRole("button", { name: "Edit", exact: true }).click(); + await expect(page.getByLabel("Enable Schedule", { exact: true })).not.toBeChecked(); + await expect(page.getByText("This schedule will not run automatically after it is saved.")).toBeVisible(); + await page.getByLabel("Time zone", { exact: true }).fill(""); + await expect(page.getByRole("status")).toHaveText("Weekdays at 09:00 (UTC)"); + await page.getByLabel("Time zone", { exact: true }).fill("America/New_York"); + await page.getByLabel("Prompt", { exact: true }).fill("Report unhealthy workloads only."); + await page.getByRole("button", { name: "Save changes", exact: true }).click(); + // The form is its own page, so a save leaves it rather than closing it. + await expect(page).toHaveURL(detailURL); await page.reload(); - const detail = page.getByTestId("schedule-detail"); - await expect(detail).toContainText("Report unhealthy workloads only.", { - timeout: 60_000, - }); - // The update did not quietly reset what it was not asked to change. - await expect(detail).toContainText("90.001 seconds"); - }); + await expect(page.getByText("Report unhealthy workloads only.", { exact: true })).toBeVisible(); + await expect(page.getByText("90.001 seconds", { exact: true })).toBeVisible(); + } finally { + if (detailURL) { + await page.goto(detailURL); + await page.getByRole("button", { name: `Delete schedule ${name}`, exact: true }).click(); + await page.getByRole("dialog").getByRole("button", { name: "Delete", exact: true }).click(); + await expect(page.getByText("This schedule was deleted. Its execution history is retained.")).toBeVisible(); + await expect(page.getByRole("button", { name: "Run", exact: true })).toBeDisabled(); + await page.getByRole("link", { name: "Back", exact: true }).click(); + await expect(page.getByRole("link", { name, exact: true })).toHaveCount(0); + } + } }); diff --git a/ui/playwright/live/substrate.spec.ts b/ui/playwright/live/substrate.spec.ts index 5cd822b3e1..9771eac132 100644 --- a/ui/playwright/live/substrate.spec.ts +++ b/ui/playwright/live/substrate.spec.ts @@ -1,6 +1,5 @@ -import { test, expect } from "../fixtures/test"; -import { expectNoLoadFailure, loadApp } from "../helpers/app"; -import { liveRoutes } from "./helpers/live"; +import { test, expect } from "@playwright/test"; +import { expectNoLoadFailure, liveRoutes, loadLive } from "./helpers/live"; /** * The substrate page, against what the controller actually sends. @@ -19,7 +18,8 @@ import { liveRoutes } from "./helpers/live"; * says whether the token it hands back means what the page thinks it means. */ test("live: the substrate page renders the cluster's own inventory", async ({ page }) => { - await loadApp(page, liveRoutes.substrate); + await loadLive(page, liveRoutes.substrate); + await expectNoLoadFailure(page); await test.step("1. the page is there rather than an error", async () => { await expect(page.getByTestId("substrate-actors-card")).toBeVisible({ @@ -29,16 +29,15 @@ test("live: the substrate page renders the cluster's own inventory", async ({ pa await test.step("2. the tiles report a real count, not zero", async () => { /* - * The assertion "no load failure" cannot make: a page that understood none of the - * answer draws the same tiles, with an em-dash where each number goes. Retrying, - * because that em-dash is also what shows while the read is in flight — read once, - * this failed reporting "Actors running—" against a cluster that said "0/4" a moment - * later. Against the mock that gap is a millisecond, so only a cluster showed it. + * The assertion "no load failure" cannot make. A page that reached the controller + * and understood none of the answer draws the same tiles with nothing in them, and + * this cluster is running a worker pool — so a zero here is a decode problem, not + * an empty cluster. */ - await expect( - page.getByTestId("substrate-stat-actors-value"), - "the actor tile should report a count", - ).toHaveText(/\d/, { timeout: 60_000 }); + const actors = page.getByTestId("substrate-stat-actors"); + await expect(actors).toBeVisible(); + const text = (await actors.textContent()) ?? ""; + expect(text, "the actor tile should report a count").toMatch(/\d/); }); await test.step("3. the worker table holds rows the cluster returned", async () => { @@ -50,13 +49,5 @@ test("live: the substrate page renders the cluster's own inventory", async ({ pa await expect(workers.locator(".ant-table-row").first()).toBeVisible({ timeout: 60_000, }); - - /* - * Last, and not after the card in step 1: the card is a shell the page draws - * before it has read anything, so an alert counted then is counted before any read - * could have failed — and `expectNoLoadFailure` reads once rather than retrying, so - * it would never look again. Here, three reads have demonstrably landed. - */ - await expectNoLoadFailure(page); }); }); diff --git a/ui/playwright/shared/agent-templates/agent-templates.spec.ts b/ui/playwright/shared/agent-templates/agent-templates.spec.ts deleted file mode 100644 index 900ba54a1e..0000000000 --- a/ui/playwright/shared/agent-templates/agent-templates.spec.ts +++ /dev/null @@ -1,321 +0,0 @@ -import { test, expect } from "../../fixtures/test"; -import { - LIFECYCLE_TIMEOUT, - appeared, - confirmation, - pressOnce, - selectFirstOption, - selectOption, -} from "../../helpers/resource"; -import { - READ_TIMEOUT, - expectListLoaded, - expectNoLoadFailure, - expectSettled, - isLiveRun, - loadApp, - rowNamed, - searchList, - throwawayName, -} from "../../helpers/app"; -import { sweepQuietly } from "../../helpers/cleanup"; - -/** - * Creating and deleting an agent template, through the UI, on either backend. - * - * The property it exists for: **admission is the controller's answer, not the form's.** - * `admittingHarnesses` is read from the template's *status* and cannot be computed in - * the browser, so a fixture can return any value it likes and the page will draw it — - * which is why the claim is worth making against a cluster as well as against fixtures. - * - * It replaces `live/agent-lifecycle.spec.ts`, which drove `/agents/new` — a page removed - * long before, for an agent nobody creates. Nothing ran the suite, so nothing said so. - * - * What stays in `tests/agent-templates/` is the seeded rows, the filter, the sorting, - * editing a template in place, and the empty and failure states. - */ - -/** The one this journey makes and removes. Carries the run, for litter left by a kill. */ -const TEMPLATE = throwawayName("template"); -const NAMESPACE = "kagent"; -/** What the edit moves the description to, read back off the page afterwards. */ -const DESCRIPTION = "Edited by the shared suite."; - -/* - * A lifecycle is longer than a journey, so it gets its own budget — see - * `LIFECYCLE_TIMEOUT`. It came free while this spec lived in `live/`, where the whole - * run is given two minutes for the cluster's sake; in `shared/` the mock projects apply - * the tight default, and a journey of this length is long rather than stuck. - */ -test.describe.configure({ timeout: LIFECYCLE_TIMEOUT }); - -/** Whether this run has a template on the cluster, read by the hook below. */ -let created = false; - -/* - * Cleanup in a hook, not in the body's `finally`. - * - * A timed-out test is the likeliest live failure — a controller that never reconciles — - * and it is exactly the one a `finally` cannot clean up after: Playwright has closed the - * page by then, so every call in it throws. Measured, with a four-second test: the - * `finally` was refused with "Target page, context or browser has been closed" while - * this hook still drove the page. Hooks get their own budget, which is the point. - */ -test.afterEach(async ({ page }) => { - if (!created) return; - created = false; - /* - * Through the UI because the app speaks gRPC-Web — there is no REST endpoint to call, - * though a previous version of this file believed there was. Guarded, because `created` - * says a create succeeded rather than that the template is still there: on the mock - * projects the navigation restarts the in-browser backend, so after a failure midway it - * is reliably not there, and an unguarded click would wait out the whole budget. - */ - await sweepQuietly(TEMPLATE, async () => { - await loadApp(page, `/agent-templates/${NAMESPACE}/${TEMPLATE}`); - /* - * Waited for, not counted once. `expectSettled` vouches for the shell and for antd - * spinners, and this page loads behind a `Skeleton` instead — so the count lands - * before the read does, reads zero, and leaves the template on the cluster. - */ - const remove = page.getByTestId(`delete-${TEMPLATE}`); - if (await appeared(remove)) { - await remove.click(); - await pressOnce(confirmation(page).getByRole("button", { name: "Delete" })); - await page.waitForURL(/\/agents\?.*tab=templates/, { timeout: READ_TIMEOUT }); - } - }); -}); - -test("agent templates: one is created, admitted, edited and deleted", async ({ - page, -}) => { - /** Which harness this install offered, read off the button in step 2. */ - let harness = ""; - - await test.step("1. the form offers the cluster's own model configurations", async () => { - await loadApp(page, "/agent-templates/new"); - - await selectOption(page, "template-form-namespace", NAMESPACE); - await page.getByTestId("template-form-name").fill(TEMPLATE); - - // `spec.modelConfig` is the one field the CRD requires, and the options are the - // cluster's own ModelConfigs. Whichever one this install ships, rather than a - // name — the assertion is that the cluster answered, not which model it named, - // and `selectFirstOption` fails with that message if the list is empty. - await selectFirstOption(page, "template-form-model"); - - // Asked here rather than after `loadApp`: the form's own reads had not gone out - // then, so there was nothing for it to find — see `expectNoLoadFailure`. - await expectNoLoadFailure(page); - }); - - await test.step("2. and the cluster's own harnesses, one of which makes it usable", async () => { - /* - * Two states, and which one appears is a fact about the cluster. With one harness - * the form applies its labels unasked, there being no decision to make; with - * several it warns until told. A `setup-cluster` cluster has one, CI's fixture - * five, so a spec that knew only the second would fail on every laptop. - */ - const admission = page.getByTestId("template-form-admission"); - const buttons = page.locator('[data-testid^="template-form-admit-"]'); - await expect(buttons.first(), "the cluster offered no harnesses").toBeVisible({ - timeout: 30_000, - }); - const offered = (await buttons.allTextContents()) - .map((name) => name.trim()) - .filter(Boolean); - - /* - * The one-harness case applies those labels in an effect, which lands *after* the - * buttons first render — so the single read below is a race against it, and losing - * it sends this step down the "nothing admits it" branch of a cluster where - * something does. The button is the signal to wait on: the form disables a harness - * that already admits the draft, and disables one whose selector is empty from the - * first render, so disabled is the settled state either way. With several - * harnesses nothing is applied unasked and a fresh template carries no labels, so - * the warning below is the only state there is. - */ - if (offered.length === 1) await expect(buttons.first()).toBeDisabled(); - - const admissionText = (await admission.textContent()) ?? ""; - harness = - offered.find((name) => admissionText.includes(`admitted by ${name}`)) ?? ""; - - if (harness === "") { - await expect(admission).toContainText("No harness will run this template"); - - // Enabled ones only: the form disables a harness whose selector is empty, since - // such a harness admits nothing and has no labels to copy. Clicking one would - // spend the step's budget on a button that was never going to answer. - const admit = page - .locator('[data-testid^="template-form-admit-"]:not([disabled])') - .first(); - await expect(admit, "no harness on the cluster admits anything").toBeVisible(); - harness = ((await admit.textContent()) ?? "").trim(); - // The button applies whatever labels that harness's selector matches on, which - // is the step a reader is most likely to miss and the one that makes the - // template mean anything. - await admit.click(); - } - - expect(harness, "no harness name could be read from the form").not.toBe(""); - await expect(admission).toContainText(`admitted by ${harness}`); - }); - - await test.step("3. submitting reaches the controller and lands on the list", async () => { - await expect(page.getByTestId("template-submit")).toBeEnabled(); - // Set before the submit, not after the redirect: a create the controller accepted - // but whose redirect was slow would otherwise fail the test with the flag still - // false, and the cleanup would skip a resource that really is on the cluster. The - // sweep looks for the row, so claiming one that was never made costs nothing. - created = true; - await page.getByTestId("template-submit").click(); - - // Success is leaving the form. A create the controller refused keeps the reader on - // it with `template-create-error` — which is the shape the defect this suite was - // written for produced for a template that had in fact been created. - await page.waitForURL(/\/agents\?.*tab=templates/, { timeout: READ_TIMEOUT }); - /* - * And the list comes back narrowed to the namespace that was being worked in. - * Nothing asserted this once, which is how two faults sat on the one line that - * asks for it: `/agent-templates` is a redirect carrying no query string, and the - * list narrows on `ns` while the caller was sending `namespace`. Either alone - * loses the filter, and the page looks reasonable both ways. - */ - await expect(page).toHaveURL(new RegExp(`[?&]ns=${NAMESPACE}(&|$)`)); - await expect( - page.getByTestId(`templates-filters-pill-ns-${NAMESPACE}`), - ).toBeVisible(); - }); - - await test.step("4. the row is read back from the cluster", async () => { - // Narrowed first. This table pages at 25 like the others, so on a namespace that - // already fills a page the new row is on page two — present, correct, and - // invisible to the locator below. `models` and `prompts` search for that reason - // and this journey was the one that did not. - await searchList(page, "templates", TEMPLATE); - await expectListLoaded(page, "templates"); - await expectNoLoadFailure(page); - await expect(rowNamed(page, TEMPLATE)).toHaveCount(1, { timeout: READ_TIMEOUT }); - /* - * Deliberately not asserting the harness here. The row carries the namespace too, - * and on every cluster this runs against both are `kagent` — so the assertion - * would pass on the namespace whatever the admission column said. It belongs on an - * element holding the harness and nothing else, which is the next step. - */ - }); - - await test.step("5. the controller agrees a harness admits it", async () => { - await page.getByTestId(`template-link-${TEMPLATE}`).click(); - await page.waitForURL( - new RegExp(`/agent-templates/${NAMESPACE}/${TEMPLATE}`), - { timeout: READ_TIMEOUT }, - ); - /* - * The claim this journey exists for, on an element that holds "Runs on" and the - * admitting harnesses and nothing else. `admittingHarnesses` comes from the - * template's *status*, so the harness appearing here means the controller observed - * the labels the form applied and agreed — not that the form echoed itself back. - */ - const status = page.getByTestId("template-admission-status"); - - if (isLiveRun()) { - /* - * Live, the page cannot find this out by waiting. A controller fills that status - * some time after the create returns, and this page reads it through SWR with no - * refresh interval and no revalidation on focus — one fetch, on mount. A longer - * assertion timeout would re-read a DOM that was never going to change, leaving - * the claim resting on whether the cluster reconciled in the seconds before the - * navigation. So the reload is the refetch, and the poll is how many times. - * - * Only live: the fixture backend keeps its writes in the page's own memory, so a - * reload there starts a backend that has never heard of this template — see - * `shared/schedules/schedules.spec.ts`, which avoids reloading for that reason. - */ - await expect(async () => { - /* - * `toPass`, not `expect.poll`. The poll calls its callback outside its own - * try/catch, so a throw ends it rather than failing one round — which two things - * in here do: `textContent` on a page still drawing its `Skeleton`, and - * `expectSettled` after the reload. This is the primitive for retrying a callback - * that throws, so the failure is the assertion's own rather than a string - * composed to keep the poll alive. - */ - if (!((await status.textContent()) ?? "").includes(harness)) { - // Re-read after the reload, not before it: the round that finally succeeds - // should be the one that says so. - await page.reload(); - await expectSettled(page); - } - await expect( - status, - `${TEMPLATE} was never admitted: the controller did not name ${harness} in its status`, - ).toContainText(harness); - }).toPass({ timeout: 90_000 }); - } else { - // The fixtures answer from the create itself, so one read settles it. - await expect(status).toContainText(harness, { timeout: 30_000 }); - } - - await expect(status).not.toContainText("No harness"); - }); - - await test.step("6. an edit in place is saved and read back", async () => { - /* - * The write half the create cannot show. A template's name and namespace are its - * ref and cannot change, so the description is what an edit has to move — and - * reading it back off the page after the save is what separates "the backend - * stored it" from "the draft is still on screen". - * - * Editing is a mode of the reading page rather than a separate address, so the - * submit appearing is also the assertion that the same component serves both. - */ - await expect(page.getByTestId("template-submit")).toHaveCount(0); - await page.getByTestId("template-edit").click(); - await expect(page.getByTestId("template-submit")).toBeVisible({ timeout: READ_TIMEOUT }); - - await page.getByTestId("template-form-description").fill(DESCRIPTION); - await page.getByTestId("template-submit").click(); - - // Back to reading, showing the saved value rather than the draft: a save that did - // not reach the backend would leave the old one here. - await expect(page.getByTestId("template-edit")).toBeVisible({ timeout: READ_TIMEOUT }); - await expect(page.getByTestId("template-form-description")).toHaveValue( - DESCRIPTION, - ); - }); - - await test.step("7. deleting says what it costs, against the backend's own count", async () => { - const remove = page.getByTestId(`delete-${TEMPLATE}`); - await expect(remove).toContainText("Delete template"); - await remove.click(); - - /* - * Either branch is legitimate: the count comes from `status.harnesses` while step 5 - * waited on `admittingHarnesses`, two fields filled by different work. The mock - * suite pins the wording; what a cluster shows is that the sentence is computed - * from real state at all, rather than coming back blank. - */ - await expect(page.getByTestId("template-delete-consequence")).toContainText( - /built from this template|no agent was ever built from it/, - { timeout: 30_000 }, - ); - }); - - await test.step("8. confirming removes it, and the re-read list agrees", async () => { - // Scoped to the visible popconfirm, and pressed once it has stopped arriving — - // see `helpers/resource` for what each of those is protecting against. - await pressOnce(confirmation(page).getByRole("button", { name: "Delete" })); - await page.waitForURL(/\/agents\?.*tab=templates/, { timeout: READ_TIMEOUT }); - created = false; - - // "Gone" has to name that one template rather than a read that returned nothing, - // and an empty table is exactly how a failed list would look. Said by the summary, - // which only a successful read draws: a row would do too, but only on a cluster - // that owns a template besides the one just deleted. - await expectListLoaded(page, "templates"); - await expectNoLoadFailure(page); - await expect(rowNamed(page, TEMPLATE)).toHaveCount(0, { timeout: READ_TIMEOUT }); - }); -}); diff --git a/ui/playwright/shared/dashboard.spec.ts b/ui/playwright/shared/dashboard.spec.ts deleted file mode 100644 index 84216de18a..0000000000 --- a/ui/playwright/shared/dashboard.spec.ts +++ /dev/null @@ -1,79 +0,0 @@ -import { test, expect } from "../fixtures/test"; -import { expectNoLoadFailure, loadApp } from "../helpers/app"; - -/** - * The dashboard's recent list, asserted the same way on either backend. - * - * The claim is a property rather than a value: `conversationTitle` answers with the - * name somebody gave a conversation, the title derived from its first message, or - * "Untitled" beside the short id — never the bare id alone. That holds whatever is in - * the list, so it is one of the few things worth asserting twice. - * - * What stays in `tests/dashboard.spec.ts` is what only fixtures can settle: that - * Refresh re-reads, and that a conversation somebody named shows that name. - */ -test("dashboard: the recent list names conversations rather than showing ids", async ({ - page, -}) => { - await loadApp(page, "/"); - - const card = page.getByTestId("dashboard-recent-card"); - await expect(card).toBeVisible({ timeout: 30_000 }); - await expect(card).toContainText("Recent agent conversations"); - - /* - * A cluster may genuinely have no conversations yet where the fixtures always do, so - * the count cannot be the claim. That one of the two good states is drawn can be: the - * card renders a list, an empty state, or the unavailable notice, and exactly one of - * the first two is what "the backend answered and was understood" looks like. - * - * Said this way rather than looping over whatever rows exist, because a shared spec - * that asserts nothing when the list is empty is the way this folder rots. - */ - await expect - .poll( - async () => { - if ((await page.getByTestId("recent-agents-unavailable").count()) > 0) - return "unavailable"; - if ((await page.getByTestId("recent-agents").count()) > 0) return "listed"; - if ((await page.getByTestId("recent-agents-empty").count()) > 0) return "empty"; - // The fourth state, and the reason this polls: while the read is in flight the - // card draws none of the three. Counted once instead, this failed on the mock - // backend and passed live, which is the wrong way round for a real defect. - return "loading"; - }, - { - message: "the recent card should settle on a list or an empty state", - timeout: 30_000, - }, - ) - .toMatch(/^(listed|empty)$/); - - // Asked now rather than after `loadApp`, which returns on the shell: the poll above is - // what makes an absence of alerts mean anything. - await expectNoLoadFailure(page); - - /* - * Read once, which the poll above has earned, and tied to which state it settled on. - * The loop below asserts nothing at all on an empty card — and a clean cluster, which - * is what CI runs this against, has no conversations — so without this the claim in - * the title goes unexercised in the one place the live lane runs. - */ - const listed = (await page.getByTestId("recent-agents").count()) > 0; - const labels = await page - .getByTestId("recent-agent") - .locator("a") - .evaluateAll((links) => links.map((link) => link.textContent?.trim() ?? "")); - if (listed) { - expect(labels.length, "the card drew a list and named nothing in it").toBeGreaterThan( - 0, - ); - } - - for (const label of labels) { - expect(label, "a conversation should be listed by name, not by its id").not.toMatch( - /^[0-9a-f]{8}$/, - ); - expect(label, "a conversation should carry some label").not.toBe(""); - } -}); diff --git a/ui/playwright/shared/harnesses/harnesses.spec.ts b/ui/playwright/shared/harnesses/harnesses.spec.ts deleted file mode 100644 index 87862ddd56..0000000000 --- a/ui/playwright/shared/harnesses/harnesses.spec.ts +++ /dev/null @@ -1,159 +0,0 @@ -import { test, expect } from "../../fixtures/test"; -import { - READ_TIMEOUT, - expectListTotal, - loadApp, - readListTotal, - throwawayName, -} from "../../helpers/app"; -import { sweepQuietly } from "../../helpers/cleanup"; -import { - LIFECYCLE_TIMEOUT, - appeared, - confirmDelete, - selectOption, -} from "../../helpers/resource"; - -/** - * A harness created, read back and deleted — on either backend. - * - * There is no update half: the tab offers create and delete and no edit. - * - * The claim worth running against a cluster is step 3. A newly created harness is - * "not ready yet" rather than broken — `ready: false` also covers one the controller - * has not observed — and that is a state only a real controller genuinely produces. - * A fixture answering "ready" would hide the one state a new harness is actually in. - */ - -const CREATED = throwawayName("harness"); - -/** A draft the cluster would accept, so that each refusal below is about one field. */ -const PINNED = `ghcr.io/example/runtime@sha256:${"a".repeat(64)}`; -const SNAPSHOT = "s3://ate-snapshots/kagent"; - -/** Its rows, which is the surface that can say whether any of this happened. */ -const table = "harnesses-table"; - -test.describe.configure({ timeout: LIFECYCLE_TIMEOUT }); - -/** Whether this run has a harness on the cluster, read by the hook below. */ -let created = false; - -/* - * Cleanup in a hook, not in the body's `finally`. - * - * A timed-out test is the likeliest live failure — a controller that never reconciles — - * and it is exactly the one a `finally` cannot clean up after: Playwright has closed the - * page by then, so every call in it throws. Measured, with a four-second test: the - * `finally` was refused with "Target page, context or browser has been closed" while - * this hook still drove the page. Hooks get their own budget, which is the point. - */ -test.afterEach(async ({ page }) => { - if (!created) return; - created = false; - await sweepQuietly(CREATED, async () => { - await loadApp(page, "/agents?tab=harnesses"); - // Waited for, not counted once: `loadApp` returns as soon as the shell is up, and a - // tab still fetching has no rows — which reads as "already gone" and leaves a real - // Harness on the cluster. See `appeared`. - if (await appeared(page.getByTestId(table).getByText(CREATED).first())) { - await confirmDelete(page, CREATED); - } - }); -}); - -test("harnesses: a harness is created, read and deleted", async ({ page }) => { - /** What the tab held before this journey, so the counts below can be relative. */ - let before = 0; - - await test.step("1. each field the cluster would refuse, refused on its own", async () => { - // Counted first: an absolute count is the fixtures' to make, but "one more, then - // one fewer" holds on any cluster. - await loadApp(page, "/agents?tab=harnesses"); - /* - * Off the summary, like every other list here. Counting rows instead needed a row to - * wait for, which demanded that the cluster already own a harness — and reading the - * count straight after the summary does not work either: the summary renders a frame - * before the rows do, so the count came back 0 against a tab holding four. - */ - before = await readListTotal(page, "harnesses"); - - await loadApp(page, "/harnesses/new"); - const create = page.getByTestId("harness-create"); - - await selectOption(page, "harness-namespace", "kagent"); - await page.getByTestId("harness-name").fill(CREATED); - await page.getByTestId("harness-worker-pool").fill("kagent-default"); - await page.getByTestId("harness-image").fill(PINNED); - await page.getByTestId("harness-snapshot").fill(SNAPSHOT); - - // A harness with no selector admits nothing, and the form says so before it is - // asked to create one — the one state that is a warning rather than a refusal. - await expect(page.getByTestId("harness-admits-nothing")).toBeVisible(); - await page.getByTestId("harness-selector-key").fill("runtime"); - await page.getByTestId("harness-selector-value").fill(CREATED); - await expect(page.getByTestId("harness-admits-nothing")).toHaveCount(0); - - /* - * Enabled with a complete draft, and each field below then broken on its own. - * Asserted the other way round — a bad image on a half-filled form — both of - * these passed on a form that had never looked at the field in question: the - * empty snapshot location was disabling the button by itself. - */ - await expect(create).toBeEnabled(); - - // The image, which the cluster refuses as a tag because a tag can move under a - // running agent. The form says which of the two it is unhappy about. - await page.getByTestId("harness-image").fill("ghcr.io/example/runtime:latest"); - await expect(create).toBeDisabled(); - await expect(page.getByText(/Pin the image by digest/)).toBeVisible(); - await page.getByTestId("harness-image").fill(PINNED); - await expect(create).toBeEnabled(); - - // And the snapshot location, which the CRD requires and which the controller - // would otherwise reject as "Invalid Harness", naming no field. - await page.getByTestId("harness-snapshot").fill(""); - await expect(create).toBeDisabled(); - await page.getByTestId("harness-snapshot").fill(SNAPSHOT); - }); - - await test.step("2. a complete draft is created", async () => { - await expect(page.getByTestId("harness-create")).toBeEnabled(); - // Set before the submit, not after the redirect: a create the controller accepted - // but whose redirect was slow would otherwise fail the test with the flag still - // false, and the cleanup would skip a resource that really is on the cluster. The - // sweep looks for the row, so claiming one that was never made costs nothing. - created = true; - await page.getByTestId("harness-create").click(); - - // Back to the tab it came from, with the new harness in the list. Read back off - // the table rather than from a toast: "the create returned" and "the thing - // exists" are different claims, and only the list checks the second. - await page.waitForURL(/tab=harnesses/, { timeout: READ_TIMEOUT }); - await expect(page.getByTestId(table)).toContainText(CREATED, { - timeout: READ_TIMEOUT, - }); - await expectListTotal(page, "harnesses", before + 1); - }); - - await test.step("3. and it is not ready yet, which is what a cluster reports", async () => { - const row = page.getByTestId(table).locator("tr", { hasText: CREATED }); - await expect(row.getByTestId("harness-ready")).toContainText("Not ready yet", { - timeout: READ_TIMEOUT, - }); - }); - - await test.step("4. it is removed from the same tab, and the rest stays", async () => { - await confirmDelete(page, CREATED); - - await expect(page.getByTestId(table)).not.toContainText(CREATED, { - timeout: READ_TIMEOUT, - }); - created = false; - // One row went, not the table: "gone" has to mean that harness rather than a read - // that failed and left an empty list behind it, and the summary is drawn only for a - // read that succeeded. - await expectListTotal(page, "harnesses", before); - await expect(page.getByTestId("harnesses-delete-error")).toHaveCount(0); - }); -}); diff --git a/ui/playwright/shared/models/models.spec.ts b/ui/playwright/shared/models/models.spec.ts deleted file mode 100644 index 2be4778d42..0000000000 --- a/ui/playwright/shared/models/models.spec.ts +++ /dev/null @@ -1,190 +0,0 @@ -import { test, expect } from "../../fixtures/test"; -import { - READ_TIMEOUT, - expectListLoaded, - expectListTotal, - expectNoLoadFailure, - loadApp, - readListTotal, - rowNamed, - searchList, - throwawayName, -} from "../../helpers/app"; -import { - LIFECYCLE_TIMEOUT, - confirmDelete, - confirmation, - selectOption, -} from "../../helpers/resource"; -import { sweepUp } from "../../helpers/cleanup"; - -/** - * A model configuration created, read back, changed and deleted — on either backend. - * - * The write path is 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. The fixtures answer whatever they were taught, so only a cluster can - * settle it — and only this spec asks both the same question. - * - * What stays in `tests/models/models.spec.ts` is what needs the fixtures: the exact - * seeded rows, the required-field marks, the refresh count, and the empty and failure - * states that no cluster can be asked for. - */ - -/** The one this journey makes, changes and removes. Unique, so a killed run leaves litter a person can spot. */ -const CREATED = throwawayName("model"); -const SECRET = "kagent-shared-e2e-secret"; - -test.describe.configure({ timeout: LIFECYCLE_TIMEOUT }); - -/** Whether this run has a configuration on the cluster, read by the hook below. */ -let created = false; - -/* - * Cleanup in a hook, not in the body's `finally`. - * - * A timed-out test is the likeliest live failure — a controller that never reconciles — - * and it is exactly the one a `finally` cannot clean up after: Playwright has closed the - * page by then, so every call in it throws. Measured, with a four-second test: the - * `finally` was refused with "Target page, context or browser has been closed" while - * this hook still drove the page. Hooks get their own budget, which is the point. - */ -test.afterEach(async ({ page }) => { - if (!created) return; - created = false; - await sweepUp(page, "models", CREATED); -}); - -test("models: a configuration is created, read, changed and deleted", async ({ - page, -}) => { - /** What the list held before this journey, so the counts below can be relative. */ - let before = 0; - - await test.step("1. a filled-in configuration is created and appears on the list", async () => { - /* - * Counted first, and relative from here on. The fixtures seed four and a cluster - * seeds whatever it was installed with, so an absolute count is the one thing this - * spec cannot assert — but "one more than before" is exactly as strong, and it is - * what catches a create that wrote two rows or a delete that took a neighbour. - * - * Off the summary rather than by counting rows: the table pages at 25, and a - * cluster is free to hold more than that — see `readListTotal`. - */ - await loadApp(page, "/models"); - // Off the summary alone: `readListTotal` waits for it, and it renders at "0 of 0" - // for a read that succeeded. Waiting for a row first demanded that the cluster - // already own a configuration, which is not something a shared spec may assume. - before = await readListTotal(page, "models"); - - await page.getByTestId("models-new").click(); - await page.waitForURL(/\/models\/new(\?|$)/, { timeout: READ_TIMEOUT }); - - // The provider list is the app's own enum rather than the backend's, so the name - // a reader sees is the same on either — `providerDisplayName` turns - // `AmazonBedrock` into "AWS Bedrock". - await selectOption(page, "model-provider", "Anthropic"); - - // An AutoComplete, not a Select: the id is on the wrapper and the caret goes in - // the input inside it. Typed rather than picked, the field existing to accept a - // model the catalogue has not heard of. - await page.getByTestId("model-model").locator("input").fill("claude-sonnet-4"); - - await page.getByTestId("model-name").fill(CREATED); - // The namespace is half the ref, so one created without it is addressed as - // `/name` and never appears on the list. `kagent` is where both backends put - // things. - await selectOption(page, "model-namespace", "kagent"); - await page.getByTestId("model-api-key").fill("sk-not-a-real-key"); - - // Set before the submit, not after the redirect: a create the controller accepted - // but whose redirect was slow would otherwise fail the test with the flag still - // false, and the cleanup would skip a resource that really is on the cluster. The - // sweep looks for the row, so claiming one that was never made costs nothing. - created = true; - await page.getByTestId("model-submit").click(); - await page.waitForURL(/\/models(\?|$)/, { timeout: READ_TIMEOUT }); - - // Read back off the list rather than from a toast or a closed form: those two - // only prove the app believes it worked. Narrowed to the one name this run - // invented, so the assertions below are about that row wherever the cluster's own - // configurations put it. - await searchList(page, "models", CREATED); - // The list has answered before its alerts are counted: `waitForURL` lands on a - // page that has not read anything yet, where there is nothing to count. - await expectListLoaded(page, "models"); - await expectNoLoadFailure(page); - const row = rowNamed(page, CREATED); - await expect(row).toHaveCount(1, { timeout: READ_TIMEOUT }); - await expect(row).toContainText("Anthropic"); - await expect(row).toContainText("claude-sonnet-4"); - await expectListTotal(page, "models", before + 1); - }); - - await test.step("2. the edit form opens on what was saved, not a blank draft", async () => { - await page.getByTestId(`edit-${CREATED}`).click(); - await page.waitForURL(new RegExp(`/models/kagent/${CREATED}/edit$`), { - timeout: READ_TIMEOUT, - }); - - await expect(page.getByTestId("model-name")).toHaveValue(CREATED, { - timeout: READ_TIMEOUT, - }); - /* - * The identity and the provider are the ref and what the ref means, so an edit - * changes neither. Asserted here because it is the boundary between "edit" and - * "make a new one", and a form that quietly allowed it would write a resource - * nothing else in the cluster points at. - */ - await expect(page.getByTestId("model-name")).toBeDisabled(); - await expect(page.getByTestId("model-model").locator("input")).toBeDisabled(); - }); - - await test.step("3. a change is saved, and the list shows it", async () => { - // The credential moves from one the backend minted to a secret it is told to - // read — the one change on this form the list has a column for, which is what - // makes the save checkable from outside the form. - await page - .getByTestId("model-auth-type") - .getByText("Existing secret", { exact: true }) - .click(); - await page.getByTestId("model-api-key-secret").fill(SECRET); - - await page.getByTestId("model-submit").click(); - await page.waitForURL(/\/models(\?|$)/, { timeout: READ_TIMEOUT }); - - // The search went with the form; the list is whole again on the way back. - await searchList(page, "models", CREATED); - const row = rowNamed(page, CREATED); - await expect(row).toContainText(SECRET, { timeout: READ_TIMEOUT }); - // Changed, not duplicated — which a create dressed as an update would be. - await expect(row).toHaveCount(1); - await expectListTotal(page, "models", before + 1); - }); - - await test.step("4. deleting asks first, and Keep leaves it alone", async () => { - await page.getByTestId(`delete-${CREATED}`).click(); - const prompt = confirmation(page); - // The confirmation names the row. "Delete this model configuration?" is no help - // in a table of five, and *which* is the one question the reader has. - await expect(prompt).toContainText(CREATED); - await prompt.getByRole("button", { name: "Keep" }).click(); - await expect(rowNamed(page, CREATED)).toHaveCount(1); - // Waited out rather than assumed gone: the dialog stays visible while it - // animates away, and the next step's click would land on it. - await expect(prompt).toHaveCount(0); - }); - - await test.step("5. confirming removes that row and leaves the rest", async () => { - await confirmDelete(page, CREATED); - await expect(rowNamed(page, CREATED)).toHaveCount(0, { timeout: READ_TIMEOUT }); - created = false; - - // One row went, not several, and not the read: a list that failed to reload is - // also a list the row is missing from, and the summary the total is read off - // renders only for a load that succeeded — so it is what is waited for, and the - // alert count after it is what names a failure. - await expectListTotal(page, "models", before); - await expectNoLoadFailure(page); - }); -}); diff --git a/ui/playwright/shared/prompts/prompts.spec.ts b/ui/playwright/shared/prompts/prompts.spec.ts deleted file mode 100644 index 09331ce78d..0000000000 --- a/ui/playwright/shared/prompts/prompts.spec.ts +++ /dev/null @@ -1,163 +0,0 @@ -import { type Page } from "@playwright/test"; -import { test, expect } from "../../fixtures/test"; -import { - READ_TIMEOUT, - expectListLoaded, - expectListTotal, - expectNoLoadFailure, - loadApp, - readListTotal, - rowNamed, - searchList, - throwawayName, -} from "../../helpers/app"; -import { - LIFECYCLE_TIMEOUT, - confirmDelete, -} from "../../helpers/resource"; -import { sweepUp } from "../../helpers/cleanup"; - -/** - * A prompt library created, read, changed and deleted — on either backend. - * - * A library is fragments keyed by name, and the write path is where a fixture and a - * controller most easily disagree about the shape of one. What stays in - * `tests/prompts/prompts.spec.ts` is the seeded libraries, the namespace filter, the - * discard prompts, and the empty and failure states. - */ - -const CREATED = throwawayName("library"); - -/** The nth fragment row's key and text boxes. */ -const fragmentKey = (page: Page, index: number) => - page.getByTestId("fragment-key").nth(index); -const fragmentValue = (page: Page, index: number) => - page.getByTestId("fragment-value").nth(index); - -test.describe.configure({ timeout: LIFECYCLE_TIMEOUT }); - -/** Whether this run has a library on the cluster, read by the hook below. */ -let created = false; - -/* - * Cleanup in a hook, not in the body's `finally`. - * - * A timed-out test is the likeliest live failure — a controller that never reconciles — - * and it is exactly the one a `finally` cannot clean up after: Playwright has closed the - * page by then, so every call in it throws. Measured, with a four-second test: the - * `finally` was refused with "Target page, context or browser has been closed" while - * this hook still drove the page. Hooks get their own budget, which is the point. - */ -test.afterEach(async ({ page }) => { - if (!created) return; - created = false; - await sweepUp(page, "prompts", CREATED); -}); - -test("prompts: a library is created, read, changed and deleted", async ({ page }) => { - /** What the list held before this journey, so the counts below can be relative. */ - let before = 0; - - await test.step("1. a library with one fragment is created and listed", async () => { - // Counted first, and relative from here on: the fixtures seed two and a cluster - // seeds whatever it was installed with, but "one more than before" is exactly as - // strong and catches a create that wrote twice. Off the summary rather than by - // counting rows, the table paging at 25 — see `readListTotal`. - await loadApp(page, "/prompts"); - // Off the summary alone — see `models`: waiting for a row demanded a library the - // cluster need not have. - before = await readListTotal(page, "prompts"); - - await page.getByTestId("prompts-new").click(); - await expect(page.getByTestId("prompt-submit")).toBeVisible({ timeout: READ_TIMEOUT }); - - await page.getByTestId("prompt-name").fill(CREATED); - await page.getByTestId("prompt-namespace").fill("kagent"); - await fragmentKey(page, 0).fill("changelog"); - await fragmentValue(page, 0).fill("Group by user impact."); - - // Set before the submit, not after the redirect: a create the controller accepted - // but whose redirect was slow would otherwise fail the test with the flag still - // false, and the cleanup would skip a resource that really is on the cluster. The - // sweep looks for the row, so claiming one that was never made costs nothing. - created = true; - await page.getByTestId("prompt-submit").click(); - await expect(page).toHaveURL(/\/prompts$/, { timeout: READ_TIMEOUT }); - - // Read back off the list rather than from a toast or a closed form: those two - // only prove the app believes it worked. Narrowed to the one name this run - // invented, so the assertions below are about that row wherever the cluster's own - // libraries put it. - await searchList(page, "prompts", CREATED); - // The list has answered before its alerts are counted — see `expectNoLoadFailure`. - await expectListLoaded(page, "prompts"); - await expectNoLoadFailure(page); - const row = rowNamed(page, CREATED); - await expect(row).toContainText("1 key", { timeout: READ_TIMEOUT }); - await expect(row).toContainText("changelog"); - await expectListTotal(page, "prompts", before + 1); - }); - - await test.step("2. opening it shows the fragment and how to include it", async () => { - await rowNamed(page, CREATED).getByRole("link").first().click(); - await page.waitForURL(new RegExp(`/prompts/kagent/${CREATED}$`), { - timeout: READ_TIMEOUT, - }); - - const fragments = page.getByTestId("prompt-fragments"); - await expect(fragments).toContainText("changelog", { timeout: READ_TIMEOUT }); - await expect(fragments).toContainText("Group by user impact."); - }); - - await test.step("3. a fragment is added, saved, and read back off the library", async () => { - await page.getByTestId("prompt-edit").click(); - await page.waitForURL(new RegExp(`/prompts/kagent/${CREATED}/edit$`), { - timeout: READ_TIMEOUT, - }); - // Seeded from the saved library, so the form and the page it came from agree. - await expect(fragmentKey(page, 0)).toHaveValue("changelog", { timeout: READ_TIMEOUT }); - - await page.getByTestId("fragment-add").click(); - await fragmentKey(page, 1).fill("handoff"); - await fragmentValue(page, 1).fill("Name the next owner explicitly."); - // The include tag is what a fragment is for, and it is offered before the save - // rather than only after it. - await expect(page.getByTestId("fragment-include-preview").last()).toContainText( - `{{include "${CREATED}/handoff"}}`, - ); - - await page.getByTestId("prompt-submit").click(); - await expect(page).toHaveURL(new RegExp(`/prompts/kagent/${CREATED}$`), { - timeout: READ_TIMEOUT, - }); - - // Read back from the re-read library rather than from the draft: a save that - // never reached the backend would leave the old text here. - const fragments = page.getByTestId("prompt-fragments"); - await expect(fragments).toContainText("Name the next owner explicitly.", { - timeout: READ_TIMEOUT, - }); - await expect(page.getByTestId("prompt-detail-meta")).toContainText("2 fragments"); - }); - - await test.step("4. the list behind it shows the change too", async () => { - await page.getByRole("link", { name: "Back to libraries" }).click(); - // The search went with the detail page; the list is whole again on the way back. - await searchList(page, "prompts", CREATED); - await expect(rowNamed(page, CREATED)).toContainText("2 keys", { timeout: READ_TIMEOUT }); - await expect(rowNamed(page, CREATED)).toContainText("handoff"); - }); - - await test.step("5. confirming a delete removes that row and leaves the rest", async () => { - await confirmDelete(page, CREATED); - await expect(rowNamed(page, CREATED)).toHaveCount(0, { timeout: READ_TIMEOUT }); - created = false; - - // One row went, not several, and not the read: a list that failed to reload is - // also a list the row is missing from, and the summary the total is read off - // renders only for a load that succeeded — so it is what is waited for, and the - // alert count after it is what names a failure. - await expectListTotal(page, "prompts", before); - await expectNoLoadFailure(page); - }); -}); diff --git a/ui/playwright/shared/routing.spec.ts b/ui/playwright/shared/routing.spec.ts deleted file mode 100644 index ca64a1e2bb..0000000000 --- a/ui/playwright/shared/routing.spec.ts +++ /dev/null @@ -1,74 +0,0 @@ -import { test, expect } from "../fixtures/test"; -import { expectPageTitle, loadApp, routes } from "../helpers/app"; -import { clickNav, expectShell } from "../helpers/nav"; - -/** - * Routing, which is the part of this app a *server* can still get wrong. - * - * Client-side routing means a deep link is not a file on disk, so something has to - * answer `/substrate` and `/no-such-page` with the app shell rather than a 404. In a - * cluster that something is nginx's `try_files $uri $uri/ /index.html`; under `yarn dev` - * it is Vite's own fallback, which is a different implementation of the same promise. - * Steps 4 and 5 are here rather than in `tests/` for exactly that reason: they are the - * one thing in this suite that can pass against a dev server and fail against the image. - * - * What stays in `tests/routing.spec.ts` is the two steps needing fixtures: a deep link - * carrying an instance id, and the standalone login route. - */ - -test("routing: in-app navigation, deep links and 404, on either backend", async ({ - page, -}) => { - await test.step("1. a sidebar click changes both the URL and the content", async () => { - await loadApp(page, routes.dashboard); - await expectPageTitle(page, "Dashboard"); - - await clickNav(page, "agents", /\/agents(\?|$)/); - await expectPageTitle(page, "Agents"); - - await clickNav(page, "prompts", /\/prompts(\?|$)/); - await expectPageTitle(page, "Prompts"); - }); - - await test.step("2. the sidebar marks the active destination", async () => { - await expect(page.getByTestId("nav-prompts")).toHaveClass(/ant-menu-item-selected/); - await expect(page.getByTestId("nav-agents")).not.toHaveClass( - /ant-menu-item-selected/, - ); - }); - - await test.step("3. browser history moves between routes", async () => { - await page.goBack(); - await page.waitForURL(/\/agents(\?|$)/); - await expectPageTitle(page, "Agents"); - - await page.goForward(); - await page.waitForURL(/\/prompts(\?|$)/); - await expectPageTitle(page, "Prompts"); - }); - - await test.step("4. a deep link renders that route on a cold load", async () => { - // A full page load, not a client-side transition: this is the link somebody pastes - // into chat, and the one a server that does not fall back to index.html would break. - await loadApp(page, routes.substrate); - await expectPageTitle(page, "Substrate"); - await expectShell(page); - await expect(page).toHaveURL(/\/substrate/); - }); - - await test.step("5. an unknown path renders 404 inside the shell", async () => { - await loadApp(page, "/no-such-page"); - // The address it tried, which is what a reader compares against the link they - // followed — "that page does not exist" told them nothing they could act on. - await expect(page.getByTestId("not-found-path")).toHaveText("/no-such-page"); - // And somewhere to go that is not just "back to the dashboard", which is the right - // destination only if that is where they were headed. - await expect(page.getByTestId("not-found-link-agents")).toBeVisible(); - // Still inside the app: a wrong URL should not strand the reader with no way back. - await expectShell(page); - - await page.getByTestId("not-found-dashboard").click(); - await page.waitForURL(/\/$/); - await expectPageTitle(page, "Dashboard"); - }); -}); diff --git a/ui/playwright/shared/schedules/schedules.spec.ts b/ui/playwright/shared/schedules/schedules.spec.ts deleted file mode 100644 index 0e0233661d..0000000000 --- a/ui/playwright/shared/schedules/schedules.spec.ts +++ /dev/null @@ -1,218 +0,0 @@ -import { test, expect } from "../../fixtures/test"; -import { READ_TIMEOUT, loadApp, throwawayName } from "../../helpers/app"; -import { tick } from "../../helpers/controls"; -import { sweepQuietly } from "../../helpers/cleanup"; -import { - LIFECYCLE_TIMEOUT, - appeared, - optionNamed, - pressUntil, -} from "../../helpers/resource"; - -/** - * A schedule created, read back, changed and deleted — on either backend. - * - * Kept paused throughout: execution is covered by the Go scheduling E2Es with a - * controlled model, and a suite that waited for a real agent to answer would be - * measuring the model rather than the schedule. - * - * **No reload anywhere in it, deliberately.** The fixture backend keeps writes in the - * page's own memory, so a reload starts a backend that has never heard of the schedule - * — which is why `live/schedules.spec.ts` exists alongside this and owns exactly that - * claim. Here every read is a click-through, which is what a reader does anyway. - */ - -const CREATED = throwawayName("schedule"); - -test.describe.configure({ timeout: LIFECYCLE_TIMEOUT }); - -/** Where this run's schedule lives, so the hook below can remove it. */ -let detailURL: string | undefined; - -/* - * Cleanup in a hook, not in the body's `finally`. - * - * A timed-out test is the likeliest live failure — a controller that never reconciles — - * and it is exactly the one a `finally` cannot clean up after: Playwright has closed the - * page by then, so every call in it throws. Measured, with a four-second test: the - * `finally` was refused with "Target page, context or browser has been closed" while - * this hook still drove the page. Hooks get their own budget, which is the point. - */ -test.afterEach(async ({ page }) => { - if (detailURL === undefined) return; - const detail = detailURL; - detailURL = undefined; - await sweepQuietly(CREATED, async () => { - await page.goto(detail); - const remove = page - .getByTestId("schedule-danger") - .getByRole("button", { name: `Delete schedule ${CREATED}`, exact: true }); - // Waited for, not counted once: `goto` resolves on load and the detail read has not - // landed, so the danger zone is not drawn yet. See `appeared`. - if (await appeared(remove)) { - await remove.click(); - await pressUntil( - page - .getByRole("dialog", { name: `Delete schedule ${CREATED}?`, exact: true }) - .getByRole("button", { name: "Delete", exact: true }), - () => expect(page).toHaveURL(/\/schedules(\?|$)/), - ); - } - }); -}); - -test("schedules: one is created, read, changed and deleted", async ({ page }) => { - await test.step("1. the form offers the backend's own agents", async () => { - await loadApp(page, "/schedules"); - await page.getByTestId("schedules-new").click(); - await expect(page).toHaveURL(/\/schedules\/new(\?|$)/); - - await page.getByTestId("schedule-agent").click(); - // Whichever agent this install has: which one has nothing to do with the claim. - const agent = optionNamed(page).first(); - await expect(agent, "no agents were offered to schedule").toBeVisible({ - timeout: 30_000, - }); - await agent.click(); - }); - - await test.step("2. a weekly, zoned, fractionally-timed schedule is described", async () => { - await page.getByTestId("schedule-name").fill(CREATED); - - await page.getByTestId("schedule-frequency").click(); - // Pressed until the cadence actually changes: the weekday checkboxes only exist - // once the frequency is weekly, so a dropdown click swallowed by the animation - // leaves the next line waiting for controls that are never coming. - await pressUntil(optionNamed(page, "Weekly"), () => - expect(page.getByTestId("schedule-days")).toBeVisible(), - ); - // Monday is already on, so these four make it the whole working week — which the - // app states back as "Weekdays", and which is the reading asserted below. - for (const day of ["Tuesday", "Wednesday", "Thursday", "Friday"]) { - await tick(page.getByLabel(day, { exact: true })); - } - - await page.getByTestId("schedule-time").fill("09:00"); - // The time zone is an AutoComplete, so its id is on the wrapper and the caret goes - // in the input inside it. Escape dismisses the zone list, which otherwise sits - // over the fields below. - const zone = page.getByTestId("schedule-timezone").locator("input"); - await zone.fill("America/New_York"); - await page.keyboard.press("Escape"); - - /* - * The cadence line says the zone back, and says "UTC" when the field is empty rather - * than leaving it blank — which is what the form will store for a blank one. A reader - * who cleared it is otherwise told nothing about what they have just chosen. - * - * One field at a time: cleared, then put back. - */ - const cadence = page.getByTestId("schedule-cadence"); - await expect(cadence).toContainText("(America/New_York)"); - await zone.fill(""); - await expect(cadence).toContainText("(UTC)"); - await zone.fill("America/New_York"); - await page.keyboard.press("Escape"); - await expect(cadence).toContainText("(America/New_York)"); - - await page.getByTestId("schedule-prompt").fill("Report cluster health."); - // A fractional timeout, because it is the value a backend most easily rounds off. - await page.getByTestId("schedule-timeout").fill("90.001"); - - await page.getByTestId("schedule-enabled").uncheck(); - await expect(page.getByTestId("schedule-enabled-note")).toContainText( - "will not run automatically after it is created", - ); - }); - - await test.step("3. creating it lands on its own page, showing what was asked for", async () => { - await page.getByTestId("schedule-submit").click(); - /* - * The address first, then what is on it. Recorded after the heading, a create that - * reached the controller while the detail page was slow to draw left the hook with - * no URL to clean up — a real Schedule on the cluster, which is the one thing the - * hook exists to prevent. - */ - await page.waitForURL(/\/schedules\/[0-9a-f-]+(\?|$)/, { timeout: READ_TIMEOUT }); - detailURL = page.url(); - await expect(page.getByRole("heading", { name: CREATED, exact: true })).toBeVisible({ - timeout: READ_TIMEOUT, - }); - await expect(page).toHaveURL(/\/schedules\/[0-9a-f-]+(\?|$)/); - - // Created paused, so the one control whose label flips offers to resume it. - await expect(page.getByTestId("schedule-pause")).toHaveText("Resume"); - // And it has never run. Its own state, not "no search matched" and not "the read - // failed" — three things the page keeps apart and a new schedule is the only one of - // them this journey can produce. - await expect(page.getByTestId("schedule-history-empty")).toBeVisible(); - await expect(page.getByTestId("schedule-meta")).toContainText("Weekdays at 09:00"); - await expect(page.getByTestId("schedule-meta")).toContainText("America/New_York"); - // The fractional second survived the round trip rather than being floored to 90. - await expect(page.getByTestId("schedule-detail")).toContainText("90.001 seconds"); - }); - - await test.step("4. the edit form opens on the stored values, not on defaults", async () => { - await page.getByTestId("schedule-edit").click(); - await expect(page.getByTestId("schedule-time")).toHaveValue("09:00", { - timeout: READ_TIMEOUT, - }); - await expect(page.getByTestId("schedule-timeout")).toHaveValue("90.001"); - await expect(page.getByTestId("schedule-timezone").locator("input")).toHaveValue( - "America/New_York", - ); - await expect(page.getByTestId("schedule-enabled")).not.toBeChecked(); - // Both ends of the weekday set, so a picker that kept only the last day chosen - // would not pass on one assertion. - await expect(page.getByLabel("Monday", { exact: true })).toBeChecked(); - await expect(page.getByLabel("Friday", { exact: true })).toBeChecked(); - }); - - await test.step("5. an edit is saved and read back", async () => { - await page.getByTestId("schedule-prompt").fill("Report unhealthy workloads only."); - await page.getByTestId("schedule-submit").click(); - await expect(page.getByRole("heading", { name: CREATED, exact: true })).toBeVisible({ - timeout: READ_TIMEOUT, - }); - - const detail = page.getByTestId("schedule-detail"); - await expect(detail).toContainText("Report unhealthy workloads only.", { - timeout: READ_TIMEOUT, - }); - // And the update did not quietly reset what it was not asked to change. - await expect(detail).toContainText("90.001 seconds"); - }); - - await test.step("6. deleting asks in a modal, and confirming leaves for the list", async () => { - const remove = page - .getByTestId("schedule-danger") - .getByRole("button", { name: `Delete schedule ${CREATED}`, exact: true }); - await remove.click(); - - // Pressed until it takes: a dropped Delete reports as "the page never navigated" - // rather than as a missed click. See `pressUntil`. - await pressUntil( - page - .getByRole("dialog", { name: `Delete schedule ${CREATED}?`, exact: true }) - .getByRole("button", { name: "Delete", exact: true }), - () => expect(page).toHaveURL(/\/schedules(\?|$)/), - ); - detailURL = undefined; - - /* - * Wait for the list to draw before asserting the row is gone. Zero rows is also - * what a list that has not rendered yet looks like, so without this a delete the - * controller refused still passes — either the empty state or a first row, then - * the absence. - */ - await expect( - page - .getByTestId("schedules-empty") - .or(page.locator('[data-testid="schedules-table"] tbody tr.ant-table-row')) - .first(), - ).toBeVisible({ timeout: READ_TIMEOUT }); - await expect(page.getByRole("link", { name: CREATED, exact: true })).toHaveCount(0, { - timeout: READ_TIMEOUT, - }); - }); -}); diff --git a/ui/playwright/tests/agent-templates/agent-templates.spec.ts b/ui/playwright/tests/agent-templates/agent-templates.spec.ts index 6dc3f417c7..4919eee196 100644 --- a/ui/playwright/tests/agent-templates/agent-templates.spec.ts +++ b/ui/playwright/tests/agent-templates/agent-templates.spec.ts @@ -2,41 +2,53 @@ import { test, expect } from "../../fixtures/test"; import { dataRows, expectSettled, loadPage, rowNamed, routes } from "../../helpers/app"; import { LIFECYCLE_TIMEOUT, + confirmation, expectRequired, pressOnce, selectOption, } from "../../helpers/resource"; /** - * Agent templates, on the fixtures. Creating, editing and removing one runs against both - * backends from `shared/agent-templates/`; what is left is the reading — the seeded rows, - * the narrowing, the sorting — the form's refusal of an unusable template, both branches - * of the delete warning, and the empty and failure states. The create below is setup - * rather than a claim; see the note on `CREATED`. + * Agent templates — the whole life of one, in a single journey. * - * **The property this spec exists for: a template no harness admits cannot be used, and - * nothing about it looks wrong.** A harness admits through a label selector, so a template - * whose labels match nothing reaches no prepared revision and every `CreateAgentInstance` - * naming it is refused — while still having a model, a prompt, and a row in this list. - * Confirmed against a cluster before any of this was built: an unlabelled template sat at - * `status: {observedGeneration: 1}` with no harnesses at all, and adding the one label its - * harness selects on took it to a ready golden snapshot in about ten seconds. So the "Runs - * on" column, the warning in the form and the button that applies a harness's labels are - * the feature rather than decoration. + * One test, because a video and a trace are recorded per *test* — see + * `playwright/README.md`. * - * **Reading a template is not the same act as changing one.** A row opens a details page - * with editing as a mode rather than a page of inputs with Save waiting, and both states - * are asserted on the same component, since a separate read-only view is what would drift. - */ - -/** - * The one this journey makes and edits — setup rather than the claim. + * ## The property this spec exists for + * + * **A template no harness admits cannot be used, and nothing about it looks wrong.** A + * `Harness` admits templates through a label selector, and the CRD is explicit that a + * harness with no selector admits none — so a template whose labels match nothing + * reaches no prepared revision and every `CreateAgentInstance` naming it is refused. It + * still has a model, a prompt, a row in this list. + * + * That was confirmed against a cluster before any of this was built: an unlabelled + * template sat at `status: {observedGeneration: 1}` with no harnesses at all, and adding + * the one label its harness selects on took it to *"ActorTemplate golden snapshot is + * ready"* in about ten seconds. + * + * So the "Runs on" column, the warning in the form and the button that applies a + * harness's labels are the feature, not decoration — and they are what steps 1, 4 and 5 + * cover. + * + * ## The second property * - * Creating and deleting a template is asserted against both backends from - * `shared/agent-templates/`, so the delete is gone from here. The create stays because - * steps 6 to 10 need a template of their own to read and edit: the only seeded one with - * a harness is `k8s-agent-7f3a91c`, and step 12 asserts its description unedited. + * **Reading a template is not the same act as changing one.** A row opens a details + * page with editing as a mode rather than a page of inputs with Save waiting, which is + * why steps 6 and + * 7 assert the *reading* state as well as the writing one — and why they assert both are + * the same component, since a separate read-only view is what would drift. + * + * ## One thing the ordering buys, and one it costs + * + * The mock backend keeps writes in the page's own memory, so a `page.goto` starts a + * backend that has never heard of the template just made. Everything from step 5 onwards + * therefore clicks through rather than navigating, and the created template survives all + * the way to the delete that removes it. The cost is that a failure stops the steps after + * it; the recording shows where. */ + +/** The one this journey makes, reads, edits and removes. */ const CREATED = "browser-made"; /* @@ -46,7 +58,7 @@ const CREATED = "browser-made"; */ test.describe.configure({ timeout: LIFECYCLE_TIMEOUT }); -test("agent templates: the list reads, and a template is read and edited", async ({ +test("agent templates: a template is created, read, edited and deleted", async ({ page, }) => { await test.step("1. the list says which templates anything will actually run", async () => { @@ -285,37 +297,58 @@ test("agent templates: the list reads, and a template is read and edited", async ); }); - await test.step("11. deleting counts what is built from the template, both ways", async () => { + await test.step("11. deleting says what it costs, in the confirmation", async () => { + // Before opening it: the consequence is nowhere on the page. That is the half of + // this property the confirmation itself cannot demonstrate — a warning a reader can + // walk past on the way to the button is a warning they will walk past. + await expect(page.locator("body")).not.toContainText("keep working"); + + // In the header, beside Edit and Back, rather than at the foot of the page. A + // destructive action a reader only reaches by scrolling past everything else reads + // as a footnote. + const deleteButton = page.getByTestId(`delete-${CREATED}`); + await expect(deleteButton).toContainText("Delete template"); + await deleteButton.click(); + /* - * Back to the list first. The delete that used to sit here navigated back as a side - * effect of removing the template, and it moved to `shared/agent-templates/` — so - * the return trip is now this step's own business rather than something it inherits. + * Measured against the controller, not read off the schema. A scratch template with + * a live pair was deleted over gRPC on a cluster: the call was accepted, the + * resource went, and the `agent_template_harness_pair` row survived in Postgres with + * `retired_at` set — retired, not removed. The revision collector skips any revision + * an `agent_instance.prepared_revision` points at before the `ON DELETE RESTRICT` on + * that column could fire, so an agent's revision is retained *for it*; and + * `GetLatestRuntimeRevisionForInstance` requires `retired_at IS NULL`, which is what + * stops anything new being cut from the template afterwards. * - * Both branches are asserted here because only the fixtures can hold both at once: - * the shared spec deletes a template it created seconds earlier, which can only ever - * be the "nothing is built from it" branch, and it says so by matching either - * sentence. The seeded wording is this suite's to pin. + * **What it must not say is that the agents keep running.** A (template, harness) + * pair *is* an agent here, and deleting the template retires the pair — that is + * exactly the mechanism that stops new work. What survives is the conversations + * already open, each holding a revision retained for it. */ - await page.getByRole("button", { name: "Back to templates" }).click(); - await page.waitForURL(/\/agents\?.*tab=templates/); - - // A template an agent *is* built from: the count, and the two things that follow - // from it — what survives the delete and what cannot be started after it. This is - // the branch a reader is most likely to be reading before they decide. - await page.getByTestId("templates-filters-search").fill("k8s-agent-7f3a91c"); - await page.getByTestId("template-link-k8s-agent-7f3a91c").click(); - await page.waitForURL(/\/agent-templates\/kagent\/k8s-agent-7f3a91c/); + const consequence = page.getByTestId("template-delete-consequence"); + await expect(consequence).toContainText("1 agent is built from this template"); + await expect(consequence).toContainText( + "Conversations already open with it keep working", + ); + await expect(consequence).toContainText("no new one can be started"); + }); - await page.getByTestId("delete-k8s-agent-7f3a91c").click(); - const populated = page.getByTestId("template-delete-consequence"); - await expect(populated).toContainText("1 agent is built from this template"); - await expect(populated).toContainText("keep working"); - await expect(populated).toContainText("no new one can be started"); + await test.step("12. confirming removes it, and the list that opens does not show it", async () => { + // Scoped to the visible popconfirm: every row's confirmation is in the DOM at once, + // so an unscoped Delete can answer a prompt nobody is looking at. + await confirmation(page).getByRole("button", { name: "Delete" }).click(); + await page.waitForURL(/\/agents\?.*tab=templates/, { timeout: 30_000 }); - await page.keyboard.press("Escape"); - await page.getByRole("button", { name: "Back to templates" }).click(); - await page.waitForURL(/\/agents\?.*tab=templates/); + // The claim worth making. The list is cached, so landing on it without re-reading + // shows the template that was just removed — which reads as a delete that silently + // failed, and is the reason the page invalidates before navigating. + await expect(rowNamed(page, CREATED)).toHaveCount(0, { timeout: 30_000 }); + // And the rest of the list is intact, so "gone" means that one rather than the read. + await expect(rowNamed(page, "k8s-agent-7f3a91c")).toBeVisible(); + await expect(page).toHaveURL(/[?&]ns=kagent(&|$)/); + }); + await test.step("13. a template nothing runs says that instead", async () => { // The other branch of the same sentence. Telling a reader that conversations will // keep working when no harness ever admitted the template would be noise dressed as // care. @@ -329,7 +362,7 @@ test("agent templates: the list reads, and a template is read and edited", async ); }); - await test.step("12. an agent in the Agents tab opens that agent", async () => { + await test.step("14. an agent in the Agents tab opens that agent", async () => { // The tab answers "what is built from this template", and each answer is a // (template, harness) pair — which is what an agent is. Leaving the rows as text // made it a dead end: it named the thing the reader wanted and gave them no way to @@ -370,17 +403,16 @@ test("agent templates: the list reads, and a template is read and edited", async await expect(page.getByTestId("chat-new-session")).toBeVisible({ timeout: 30_000 }); }); - await test.step("13. an empty result says so instead of showing a bare table", async () => { - // Last, and it has to be: reaching these needs the backend answering differently, - // and `?mock=` is per-navigation — so arriving here discards everything the steps - // above made. Nothing below wants it. (Step 11 opens the delete warning to read it - // and then keeps the template; the delete itself lives in `shared/agent-templates/`.) + await test.step("15. an empty result says so instead of showing a bare table", async () => { + // Last, after the delete, because reaching these needs the backend answering + // differently and `?mock=` is per-navigation — which discards what the journey made. + // By here there is nothing left to discard. await loadPage(page, routes.agentTemplates, { scenario: "empty", title: "Agents" }); await expect(page.getByText("No agent templates yet.")).toBeVisible(); await expect(dataRows(page)).toHaveCount(0); }); - await test.step("14. a failed load is reported, not disguised as an empty list", async () => { + await test.step("16. a failed load is reported, not disguised as an empty list", async () => { await loadPage(page, routes.agentTemplates, { scenario: "error", title: "Agents" }); const alert = page.getByTestId("templates-error"); diff --git a/ui/playwright/tests/agents/agent-details.spec.ts b/ui/playwright/tests/agents/agent-details.spec.ts deleted file mode 100644 index 99e0458a5d..0000000000 --- a/ui/playwright/tests/agents/agent-details.spec.ts +++ /dev/null @@ -1,106 +0,0 @@ -import { test, expect } from "../../fixtures/test"; -import { - agentDetail, - agentPage, - agents, - instances, - loadPage, -} from "../../helpers/app"; - -/** - * A conversation's own record — what the control plane knows about one `AgentInstance`. - * - * This is the page that replaced the old agent details page when agents became - * instances, and `DEFERRED.md` records why the replacement is not a reduction: an - * instance has no spec. What it has is a state, an operation, the pair it was cut from - * and a failure, and the configuration lives on the template and the harness it links - * to. Nothing had driven it in a browser until this spec. - */ - -test("agents: a conversation's record reports its state and links to what configures it", async ({ - page, -}) => { - await loadPage(page, agentDetail(instances.ready)); - - await test.step("1. the state, in words, and what the word means", async () => { - await expect(page.getByTestId("instance-status-card")).toBeVisible(); - // Read to the operator rather than passed through: `ACTOR_STATE_RUNNING` is the - // controller's vocabulary, and a reader should not have to learn it here. - const state = page.getByTestId("instance-state"); - await expect(state).toHaveText(/^[A-Z][a-z]/); - await expect(state).not.toContainText("_"); - // The sentence beside it, which is the part a state tag cannot carry: what being in - // this state means for what the reader can do next. - await expect(page.getByTestId("instance-state-meaning")).not.toBeEmpty(); - }); - - await test.step("2. it links to its agent and its template rather than restating them", async () => { - /* - * The claim the page exists to make. An instance carries no model, no prompt and no - * tools, so the record links to the two surfaces that do — and a link is checked by - * where it goes, a label being the easy half to get right. - */ - await expect(page.getByTestId("instance-agent-link")).toHaveAttribute( - "href", - agentPage(agents.k8s), - ); - await expect(page.getByTestId("instance-template-link")).toHaveAttribute( - "href", - /\/agent-templates\//, - ); - // And it says why the template link is the one that matters, since editing it - // changes every agent cut from it rather than this conversation alone. - await expect(page.getByTestId("instance-template-note")).toContainText( - "not only this one", - ); - }); - - await test.step("3. the record itself, which is what the controller stored", async () => { - await expect(page.getByTestId("instance-details")).toBeVisible(); - await expect(page.getByTestId("instance-details")).toContainText(instances.ready); - }); -}); - -test("agents: a record the reader cannot have says which of the two it is", async ({ - page, -}) => { - await test.step("1. a failed conversation reports the reason it failed", async () => { - /* - * The half of "an agent's readiness reason, end to end" that this page owns. A - * failure with no message renders a sentence saying the record held none — so an - * empty alert here would be a rendering fault rather than a quiet controller, and - * the fixture carries a real message to tell those apart. - */ - await loadPage(page, agentDetail(instances.failed)); - const failure = page.getByTestId("instance-failure"); - await expect(failure).toBeVisible(); - await expect(failure).toContainText("cannot be resumed"); - }); - - await test.step("2. somebody else's reads as not found, in the controller's own words", async () => { - /* - * Not a bug and not a euphemism: an instance is read as its creator, so the - * controller genuinely answers `NotFound` for one that is not yours. The page says - * both readings out loud rather than picking one, because a reader who knows the - * conversation exists would otherwise think the record was lost. - */ - await loadPage(page, agentDetail(instances.someoneElses)); - const notFound = page.getByTestId("instance-not-found"); - await expect(notFound).toBeVisible(); - await expect(notFound).toContainText("started by somebody else"); - // No half-drawn record behind the notice. - await expect(page.getByTestId("instance-details")).toHaveCount(0); - }); - - await test.step("3. a read that failed is not a record that is missing", async () => { - // The distinction this suite keeps everywhere: "we could not find out" must not - // render as "there is nothing here". Different id, different action — retry rather - // than a way back to the list. - await loadPage(page, agentDetail(instances.ready), { scenario: "error" }); - await expect(page.getByTestId("instance-error")).toBeVisible(); - await expect(page.getByTestId("instance-not-found")).toHaveCount(0); - await expect( - page.getByTestId("instance-error").getByRole("button", { name: "Try again" }), - ).toBeVisible(); - }); -}); diff --git a/ui/playwright/tests/agents/agent-page.spec.ts b/ui/playwright/tests/agents/agent-page.spec.ts index c8dfce2f7d..eb2a31ab6e 100644 --- a/ui/playwright/tests/agents/agent-page.spec.ts +++ b/ui/playwright/tests/agents/agent-page.spec.ts @@ -187,11 +187,8 @@ test("agents: somebody else's conversation is listed, and plainly cannot be open await expectSettled(page); await test.step("1. the toggle and its alert are gone", async () => { - // Asserted as "there is no such control", not as "two ids are absent": the ids this - // used to name have never existed in `src`, so those two assertions could not fail - // however the toggle came back. - await expect(page.getByRole("switch")).toHaveCount(0); - await expect(page.getByText(/created by (others|someone else)/i)).toHaveCount(0); + await expect(page.getByTestId("instances-all-creators")).toHaveCount(0); + await expect(page.getByTestId("instances-own-only")).toHaveCount(0); }); await test.step("2. everyone's conversations are listed", async () => { diff --git a/ui/playwright/tests/agents/agents.spec.ts b/ui/playwright/tests/agents/agents.spec.ts index 99db4ee820..ee8ea0c6b6 100644 --- a/ui/playwright/tests/agents/agents.spec.ts +++ b/ui/playwright/tests/agents/agents.spec.ts @@ -63,10 +63,7 @@ test("agents: the list is agents, and an agent is a template paired with a harne // below for why that row is there and when it is not. await expect(dataRows(page)).toHaveCount(6); await expect(page.getByTestId("agents-table")).toContainText("analytics"); - // "There is no such switch", rather than the absence of the id it used to carry — - // which has never existed in `src`, so that assertion could not fail whatever the - // page did. - await expect(page.getByRole("switch")).toHaveCount(0); + await expect(page.getByTestId("instances-all-namespaces")).toHaveCount(0); }); await test.step("2. a template two harnesses admit is two agents, told apart by the harness", async () => { diff --git a/ui/playwright/tests/app-shell.spec.ts b/ui/playwright/tests/app-shell.spec.ts index 092182103f..8c24e8f71d 100644 --- a/ui/playwright/tests/app-shell.spec.ts +++ b/ui/playwright/tests/app-shell.spec.ts @@ -57,19 +57,8 @@ test("app shell: chrome, navigation entries, and where creation lives", async ({ await expect(page.locator('[data-testid="app-header"][data-shell-probe="1"]')).toHaveCount(1); }); - await test.step("4. the chrome offers no create control of its own", async () => { - // Read off the chrome rather than aimed at one name: asserting that a - // `create-menu-trigger` is absent passes just as well on a header that grew a - // `header-new` instead, which is how this claim would actually be broken. Creating - // belongs to the list you are looking at — step 5. - const ids = await page - .locator('[data-testid="app-header"], [data-testid="app-sidebar"]') - .locator("[data-testid]") - .evaluateAll((nodes) => - nodes.map((node) => node.getAttribute("data-testid") ?? ""), - ); - expect(ids.length, "no controls were read off the chrome").toBeGreaterThan(0); - expect(ids.filter((id) => /(^|-)(new|create)(-|$)/.test(id))).toEqual([]); + await test.step("4. the chrome offers no create menu of its own", async () => { + await expect(page.getByTestId("create-menu-trigger")).toHaveCount(0); }); await test.step("5. every list that can create one says so, and reaches its form", async () => { @@ -110,40 +99,6 @@ test("app shell: chrome, navigation entries, and where creation lives", async ({ await expectShell(page); } }); - - await test.step("6. no page is printing its own source at the reader", async () => { - /* - * A `/* … *\/` written among a JSX element's children rather than inside `{…}` is a - * text node, not a comment, and it renders. Three of these pages shipped a paragraph - * about SWR into their filter bars that way, and nothing caught it: `tsc` and eslint - * both accept it, and no assertion in this suite read that region. So the pages are - * asked directly, tabs included — two of the three were tabs. - */ - const surfaces = [ - [routes.agents, "agents-table"], - [`${routes.agents}?tab=templates`, "templates-table"], - [`${routes.agents}?tab=harnesses`, "harnesses-table"], - [routes.models, "models-table"], - [routes.prompts, "prompts-table"], - [routes.mcpServers, "mcp-servers-table"], - ] as const; - - for (const [route, table] of surfaces) { - await loadPage(page, route); - /* - * The table first, and that is the whole assertion working. "There is no source on - * this page" is true of a page that has not drawn yet, so asked on arrival it - * passed over a genuinely broken harnesses tab — checked by breaking one on - * purpose. The absence only means something once there is something to be absent - * from. - */ - await expect(page.getByTestId(table)).toBeVisible(); - await expect( - page.locator("body"), - `${route} is printing source commentary at the reader`, - ).not.toContainText("*/"); - } - }); }); /** diff --git a/ui/playwright/tests/chat/approvals.spec.ts b/ui/playwright/tests/chat/approvals.spec.ts deleted file mode 100644 index f6e965d8d2..0000000000 --- a/ui/playwright/tests/chat/approvals.spec.ts +++ /dev/null @@ -1,126 +0,0 @@ -import { test, expect } from "../../fixtures/test"; -import { agentChat, instances } from "../../helpers/app"; - -/** - * A turn parked on something other than a question. - * - * `questions.spec.ts` covers `ask_user`, which is answerable. These are its two - * neighbours: a tool approval, which this build asks the reader to vouch for tool by - * tool, and a request it does not recognise, which it says plainly it cannot answer. - * - * Both were deferred as "blocked on a product decision" while the controls were being - * argued about. The decision landed in #2714 and the controls shipped with it — approve, - * reject, a reason per rejection — and nothing in the browser had driven them since. - */ - -const AGENT_CHAT = agentChat(instances.ready); - -test("chat: a tool approval is decided per tool, and the decisions are what go back", async ({ - page, -}) => { - await page.goto(`${AGENT_CHAT}?chat=approves`); - await page.getByTestId("chat-input").fill("Tidy the cluster up."); - await page.getByTestId("chat-send").click(); - - const prompt = page.getByTestId("chat-awaiting-reply"); - await expect(prompt).toBeVisible({ timeout: 30_000 }); - await expect(prompt).toHaveAttribute("data-kind", "tool_approval"); - - await test.step("1. it names what it is asking to run, and why", async () => { - // The tools by name, because "the agent wants to run some tools" is not something a - // reader can vouch for. The hint is the runtime's own sentence about the risk. - const tools = page.getByTestId("chat-approval-tool"); - await expect(tools).toHaveCount(2); - await expect(tools.first()).toHaveText("kubectl_apply"); - await expect(tools.last()).toHaveText("shell_exec"); - await expect(prompt).toContainText("Approve only what you recognise"); - }); - - await test.step("2. one allowed and one denied, in the same submission", async () => { - // Per row, not per form: the decision belongs to the tool, and a control wired to - // the form would send the same answer for both. - const rows = page.getByTestId("chat-approval-tool"); - await rows.first().locator("..").getByRole("button", { name: "Allow" }).click(); - await rows.last().locator("..").getByRole("button", { name: "Deny" }).click(); - - // The reason appears with the rejection and belongs to that tool alone — keyed on - // the tool's id, so a form that kept one reason for the page would fail here. - const reason = page.getByTestId("chat-approval-reason-call-2"); - await expect(reason).toBeVisible(); - await expect(page.getByTestId("chat-approval-reason-call-1")).toHaveCount(0); - await reason.fill("It deletes a cache I still need."); - }); - - await test.step("3. the agent is told which was which", async () => { - await page.getByTestId("chat-approval-submit").click(); - - /* - * Read back off the reply rather than off the form: the fixture reads the decisions - * out of the payload it received and says them, so this fails if the page sent both - * as approvals, paired the reason with the wrong tool, or dropped the reason. - */ - const transcript = page.getByTestId("chat-transcript"); - await expect(transcript).toContainText("call-1 approved", { timeout: 30_000 }); - await expect(transcript).toContainText("call-2 rejected"); - await expect(transcript).toContainText("It deletes a cache I still need."); - // And the turn is no longer parked, so the conversation is usable again. - await expect(prompt).toHaveCount(0); - }); -}); - -test("chat: one tool is approved or rejected on the prompt itself", async ({ page }) => { - /* - * One tool is a different prompt, not a shorter one: there is nothing to decide - * between, so the decision is the prompt's own Approve and Reject rather than a row's - * Allow and Deny behind a Submit. Both paths send the same payload, and only this one - * has ever been reachable by a reader with a single-tool agent. - */ - await page.goto(`${AGENT_CHAT}?chat=approves-one`); - await page.getByTestId("chat-input").fill("Clear the cache."); - await page.getByTestId("chat-send").click(); - - const prompt = page.getByTestId("chat-awaiting-reply"); - await expect(prompt).toBeVisible({ timeout: 30_000 }); - await expect(page.getByTestId("chat-approval-tool")).toHaveCount(1); - - // Rejecting asks why, with the caret already in the field — the one place it can be, - // there being no other tool to choose between. - await page.getByTestId("chat-approval-reject").click(); - const reason = page.getByTestId("chat-approval-reason-call-2"); - await expect(reason).toBeFocused(); - await page.keyboard.type("Not on a Friday."); - - await page.getByTestId("chat-approval-submit").click(); - const transcript = page.getByTestId("chat-transcript"); - await expect(transcript).toContainText("call-2 rejected", { timeout: 30_000 }); - await expect(transcript).toContainText("Not on a Friday."); - await expect(prompt).toHaveCount(0); -}); - -test("chat: a question this build cannot answer says so, rather than guessing", async ({ - page, -}) => { - /* - * A turn started without the HITL extension carries its question as prose and no - * correlation id, so there is nothing to answer against. It arises from a send this - * build did not make — a `kubectl`-driven one, an older client — and the only honest - * thing the page can do is say which of the two it is looking at. - */ - await page.goto(`${AGENT_CHAT}?chat=asks-unknown`); - await page.getByTestId("chat-input").fill("Do the thing."); - await page.getByTestId("chat-send").click(); - - const prompt = page.getByTestId("chat-awaiting-reply"); - await expect(prompt).toBeVisible({ timeout: 30_000 }); - await expect(prompt).toHaveAttribute("data-kind", "unknown"); - await expect(prompt).toContainText("without the extension that carries them"); - - // No controls invented for it: there is nothing to answer, so the only way out is to - // let the turn go. Asserted as an absence *after* the prompt is on screen, which is - // what makes the absence mean anything. - await expect(page.getByTestId("chat-approval-submit")).toHaveCount(0); - await expect(page.getByTestId("chat-answer-text-0")).toHaveCount(0); - - await page.getByTestId("chat-dismiss-question").click(); - await expect(prompt).toHaveCount(0); -}); diff --git a/ui/playwright/tests/chat/panels.spec.ts b/ui/playwright/tests/chat/panels.spec.ts index 4d47411844..389e00430a 100644 --- a/ui/playwright/tests/chat/panels.spec.ts +++ b/ui/playwright/tests/chat/panels.spec.ts @@ -39,15 +39,8 @@ test("chat: a conversation's record is read without leaving the conversation", a // There is no Edit anywhere on it: an instance has no spec to change. What the agent // *is* lives on its AgentTemplate and how it *runs* on its Harness, so a control here - // would offer something that does not exist. By what a reader would press rather than - // by a test id: a control added here would carry an id of its own, and a guard naming - // one would pass on any other. - // Found before it is asked anything: "no Edit inside the dialog" is also true of a - // dialog that is not there, which would be this assertion proving nothing at all. - const details = page.getByRole("dialog"); - await expect(details).toBeVisible(); - await expect(details.getByRole("button", { name: /edit/i })).toHaveCount(0); - await expect(details.getByRole("link", { name: /edit/i })).toHaveCount(0); + // would offer something that does not exist. + await expect(page.getByTestId("agent-details-edit")).toHaveCount(0); }); test("chat: the agent panel says what the conversation cannot", async ({ page }) => { diff --git a/ui/playwright/tests/dashboard.spec.ts b/ui/playwright/tests/dashboard.spec.ts index 5f7bc9d08f..4d477989a6 100644 --- a/ui/playwright/tests/dashboard.spec.ts +++ b/ui/playwright/tests/dashboard.spec.ts @@ -4,19 +4,15 @@ import { operationCalls, rpc } from "../helpers/mockCalls"; import { clickRefresh } from "../helpers/resource"; /** - * The dashboard's recent list, on the fixtures. + * The dashboard's recent list, which is conversations and now reads like it. * * The card was headed "Recently created agents" and listed `AgentInstance` rows, which * are conversations rather than agents — so the heading was wrong about what it held. * The rows were worse: each linked to a conversation under a bare eight-character id, - * on the reasoning that "an agent has no name". - * - * What is left here is what only a fixed backend can settle. That no row is a bare id - * is a property of any backend, so it runs against both from `shared/dashboard.spec.ts`. + * on the reasoning that "an agent has no name". A conversation does have one, and this + * is the third surface to show it after the rail and the agent's own table. */ -test("dashboard: Refresh re-reads, and a named conversation shows its name", async ({ - page, -}) => { +test("dashboard: recent conversations read as names, not as ids", async ({ page }) => { await loadPage(page, routes.dashboard); await expectSettled(page); @@ -36,10 +32,33 @@ test("dashboard: Refresh re-reads, and a named conversation shows its name", asy .toBeGreaterThan(before); }); - await test.step("2. and a conversation somebody named shows that name", async () => { - // The other half of the shared spec's claim: "not an id" would also be satisfied - // by every row reading "Untitled", which is true and useless. Only fixtures can - // guarantee a conversation whose name its reader chose. + await test.step("2. and no row is a bare id", async () => { + /* + * The property, rather than a fixture's particular name. + * + * `conversationTitle` answers with the name somebody gave it, the title derived + * from its first message, or "Untitled" beside the short id — never the short id + * alone. So a link whose whole text is eight hex characters is the old behaviour, + * whichever conversation happens to be recent enough to appear here. + */ + const rows = page.getByTestId("recent-agent"); + await expect(rows.first()).toBeVisible(); + const labels = await rows.locator("a").evaluateAll((links) => + links.map((link) => link.textContent?.trim() ?? ""), + ); + + expect(labels.length).toBeGreaterThan(0); + for (const label of labels) { + expect(label, "a conversation should be listed by name, not by its id").not.toMatch( + /^[0-9a-f]{8}$/, + ); + } + }); + + await test.step("3. and a conversation somebody named shows that name", async () => { + // The other half: "not an id" would also be satisfied by every row reading + // "Untitled", which is true and useless. At least one of the fixtures' recent + // conversations carries a name its reader chose. const labels = await page .getByTestId("recent-agent") .locator("a") diff --git a/ui/playwright/tests/harnesses/harnesses.spec.ts b/ui/playwright/tests/harnesses/harnesses.spec.ts index 2781dc8fd3..d19852572d 100644 --- a/ui/playwright/tests/harnesses/harnesses.spec.ts +++ b/ui/playwright/tests/harnesses/harnesses.spec.ts @@ -2,32 +2,44 @@ import { test, expect } from "../../fixtures/test"; import { loadPage, routes } from "../../helpers/app"; import { LIFECYCLE_TIMEOUT, + confirmDelete, expectRequired, selectFirstOption, } from "../../helpers/resource"; /** - * Harnesses, on the fixtures. The tab offers create and delete and no edit, and that - * journey runs against both backends from `shared/harnesses/`. What stays is the reading - * — the seeded rows, the selector on the page, the narrowing — and the two refusals - * below, which are the form enforcing the cluster's constraints rather than creating - * anything. + * Harnesses — the whole life of one, in a single journey. + * + * One test, because a video and a trace are recorded per *test* — see + * `playwright/README.md`. + * + * **There is no update half.** The tab offers create and delete and no edit, so the + * journey is create, read back, remove. That is narrower than `HarnessService`, which + * implements update too — this application has never called it. + * + * ## What the tab exists to say * * **The admission selector has to be visible.** A harness admits templates through a - * label selector, and that selector decides whether a template ever becomes an agent at - * all. One carrying no label it matches saves happily and then does nothing, with nothing - * on screen explaining why. + * label selector, and that selector is what decides whether a template ever becomes an + * agent at all. A template carrying no label it matches saves happily and then does + * nothing, with nothing on screen explaining why — so the selector is on the page rather + * than behind an expander. * * **A harness must not be called broken.** `ready: false` also covers one the controller - * has not observed yet, which is a different thing from one that failed — the `kagent` - * harness on a development cluster runs agents and carries `status: null`. + * has not observed yet, which is a different thing from one that failed — and the + * `kagent` harness on the development cluster is exactly that: it runs agents and carries + * `status: null`. Calling that "broken" sends somebody debugging a harness that works. * - * **The form is short because the CRD is strict**: exactly one runtime adapter, an image - * pinned by digest, and a worker pool to schedule onto. A form that accepted a tag would - * build a resource the cluster rejects, which is why the fixture refuses it too. + * ## Why the form is short + * + * The CRD is strict, and the constraints the form enforces are the cluster's rather than + * this page's: exactly one runtime adapter, an image pinned by digest, and a worker pool + * for the Substrate Actors to be scheduled onto. A form that accepted a tag would build a + * resource the cluster rejects — the failure that is invisible until somebody tries it + * for real, which is why the fixture refuses it too. */ -/** The name the validation steps type in. Nothing is created here — see the note above. */ +/** The one this journey makes, reads back and removes. */ const CREATED = "made-here"; /** Its rows, which is the surface that can say whether any of this happened. */ @@ -40,9 +52,7 @@ const table = "harnesses-table"; */ test.describe.configure({ timeout: LIFECYCLE_TIMEOUT }); -test("harnesses: the tab reads, and the form refuses what the CRD refuses", async ({ - page, -}) => { +test("harnesses: a harness is created, read and deleted", async ({ page }) => { await test.step("1. the harnesses are listed, with what admits a template on the page", async () => { await loadPage(page, routes.harnesses, { title: "Agents" }); await expect(page.getByTestId(table)).toBeVisible({ timeout: 30_000 }); @@ -135,17 +145,54 @@ test("harnesses: the tab reads, and the form refuses what the CRD refuses", asyn await expect(page.getByTestId("harness-create")).toBeDisabled(); }); - await test.step("7. an empty result leaves the tab standing, with no rows", async () => { - // Last, and it has to be: reaching these needs the backend answering differently, - // and `?mock=` is per-navigation — so arriving here discards everything the steps - // above made. (The delete this used to follow now lives in `shared/harnesses/`.) + await test.step("7. pinned by digest and told where snapshots go, it is created", async () => { + await page.getByTestId("harness-snapshot").fill("s3://ate-snapshots/kagent"); + await page.getByTestId("harness-selector-key").fill("runtime"); + await page.getByTestId("harness-selector-value").fill(CREATED); + await expect(page.getByTestId("harness-admits-nothing")).toHaveCount(0); + + await expect(page.getByTestId("harness-create")).toBeEnabled(); + await page.getByTestId("harness-create").click(); + + // Back to the tab it came from, with the new harness in the list. Read back off the + // table rather than from a toast or a closed form: "the create returned" and "the + // thing exists" are different claims, and only the list checks the second. + await page.waitForURL(/tab=harnesses/); + await expect(page.getByTestId(table)).toContainText(CREATED, { timeout: 30_000 }); + }); + + await test.step("8. and it is not ready yet, which is what a cluster reports", async () => { + // The controller has not observed it. A fixture that answered "ready" would hide the + // one state a newly created harness is actually in. + const row = page.getByTestId(table).locator("tr", { hasText: CREATED }); + await expect(row.getByTestId("harness-ready")).toContainText("Not ready yet"); + }); + + await test.step("9. it is removed from the same tab, and the rest stays", async () => { + const rows = page.getByTestId(table).locator("tbody tr"); + const before = await rows.count(); + + await confirmDelete(page, CREATED); + + await expect(page.getByTestId(table)).not.toContainText(CREATED, { + timeout: 30_000, + }); + // One row went, not the table: "gone" has to mean that harness rather than a read + // that failed and left an empty list behind it. + await expect.poll(() => rows.count()).toBe(before - 1); + await expect(page.getByTestId("harnesses-delete-error")).toHaveCount(0); + }); + + await test.step("10. an empty result leaves the tab standing, with no rows", async () => { + // Last, after the delete: reaching these needs the backend answering differently and + // `?mock=` is per-navigation, which discards what the journey made. await loadPage(page, routes.harnesses, { scenario: "empty", title: "Agents" }); await expect(page.getByTestId(table)).toBeVisible({ timeout: 30_000 }); await expect(page.getByTestId(table).locator("tbody tr.ant-table-row")).toHaveCount(0); await expect(page.getByTestId("harnesses-error")).toHaveCount(0); }); - await test.step("8. a failed load is reported, not disguised as an empty tab", async () => { + await test.step("11. a failed load is reported, not disguised as an empty tab", async () => { await loadPage(page, routes.harnesses, { scenario: "error", title: "Agents" }); const alert = page.getByTestId("harnesses-error"); diff --git a/ui/playwright/tests/models/models.spec.ts b/ui/playwright/tests/models/models.spec.ts index 9db93fdf0a..520999be23 100644 --- a/ui/playwright/tests/models/models.spec.ts +++ b/ui/playwright/tests/models/models.spec.ts @@ -11,18 +11,24 @@ import { expectLoading, chooseFilter, clickRefresh, + confirmDelete, + confirmation, expectRequired, + selectOption, } from "../../helpers/resource"; import { operationCalls, rpc } from "../../helpers/mockCalls"; /** - * Model configurations, on the fixtures. + * Model configurations — the whole life of one, in a single journey. * - * The write journey is not here: creating one, reading it back, changing its credential - * and deleting it runs against both backends from `shared/models/`. What is left is what - * only fixed data can settle — the seeded rows and their refs taken apart, the refresh - * confirmation, the filter in the address, the required-field marks, and the empty and - * failure states. + * One test, because a video and a trace are recorded per *test* — see + * `playwright/README.md` for the shape and the trade it makes. + * + * The fixture backend records writes (`src/mocks/state.ts`) so the reads afterwards + * can contradict the form, which is what steps 6, 8 and 10 rely on. Three claims that + * would otherwise each cost their own page load are steps here rather than files of + * their own: the required-field marks (5), the refresh confirmation (3) and the + * filter in the address (4). */ /** The four seeded configurations, which is what "nothing narrowed" has to mean. */ @@ -33,6 +39,9 @@ const SEEDED = [ "bedrock-haiku", ]; +/** The one this journey makes, reads, renames the model on, and removes. */ +const CREATED = "browser-made-model"; + /* * A lifecycle is longer than a journey, so it gets its own budget — see * `LIFECYCLE_TIMEOUT`. Set per file rather than across the suite, so the tight default @@ -40,7 +49,7 @@ const SEEDED = [ */ test.describe.configure({ timeout: LIFECYCLE_TIMEOUT }); -test("models: the list reads, narrows, and says what the form requires", async ({ +test("models: a configuration is created, read, changed and deleted", async ({ page, }) => { await test.step("1. a loading state precedes the data", async () => { @@ -157,47 +166,123 @@ test("models: the list reads, narrows, and says what the form requires", async ( }); }); - await test.step("6. editing does not ask for the API key again", async () => { + await test.step("6. a filled-in configuration is created and appears on the list", async () => { + // The provider is picked by the name a reader sees, which is not the enum the + // draft stores: `providerDisplayName` turns `AmazonBedrock` into "AWS Bedrock". + await selectOption(page, "model-provider", "Anthropic"); + + // The model is an AutoComplete, not a Select — the test id is on the wrapper and + // the caret goes in the input inside it. Typed rather than picked, because the + // field exists to accept a model the catalogue has not heard of. + const model = page.getByTestId("model-model").locator("input"); + await model.fill("claude-sonnet-4"); + + await page.getByTestId("model-name").fill(CREATED); + // The namespace is half the ref, so a configuration created without one is + // addressed as `/name` and never appears on the list — which is why the form + // marks it required and why this step chooses one rather than leaving the + // default. + await selectOption(page, "model-namespace", "kagent"); + await page.getByTestId("model-api-key").fill("sk-not-a-real-key"); + + await page.getByTestId("model-submit").click(); + await page.waitForURL(/\/models(\?|$)/, { timeout: 30_000 }); + + // Read back off the list rather than from a toast or a closed form: those two + // only prove the app believes it worked. + const row = rowNamed(page, CREATED); + await expect(row).toHaveCount(1, { timeout: 30_000 }); + await expect(row).toContainText("Anthropic"); + await expect(row).toContainText("claude-sonnet-4"); + await expect(dataRows(page)).toHaveCount(SEEDED.length + 1); + }); + + await test.step("7. the edit form opens on what was saved, not on a blank draft", async () => { + await page.getByTestId(`edit-${CREATED}`).click(); + await page.waitForURL(new RegExp(`/models/kagent/${CREATED}/edit$`)); + await expectSettled(page); + + await expect(page.getByTestId("model-name")).toHaveValue(CREATED); /* - * The other half of step 5, and the half a reader notices: a stored key is write-only - * — the controller never sends it back — so an edit form that marked the field - * required would demand the secret again to change a display name, and there would be - * nowhere to read it from. - * - * The label carries the promise ("leave blank to keep existing") and `expectRequired` - * checks that the mark agrees with it. Asserted as a pair for the same reason step 5 - * is: a form that marked nothing would pass a check that only looked at the unmarked - * list. + * The identity and the provider are the ref and what the ref means, so an edit + * changes neither: a configuration cannot be renamed, moved to another namespace, + * or repointed at a different provider's model. Asserted here because it is the + * boundary between "edit" and "make a new one", and a form that quietly allowed + * it would write a resource nothing else in the cluster is pointing at. */ - await loadPage(page, routes.models, { title: "Models" }); - await page.getByTestId("edit-default-model-config").click(); - await page.waitForURL(/\/models\/kagent\/default-model-config\/edit$/); - await expectSettled(page); + await expect(page.getByTestId("model-name")).toBeDisabled(); + await expect(page.getByTestId("model-model").locator("input")).toBeDisabled(); - // Every seeded configuration authenticates by secret reference, so the inline field - // has to be asked for. Switching to it is also the case that matters: the reader is - // replacing how this model authenticates, and still should not have to retype a key - // to do it. - // The label, not the input: antd's button-style radio hides the input itself, so a - // click on the role never lands. + /* + * And the key is not asked for again, because the cluster already holds it. The + * label says so in words; the mark has to agree, or the form is demanding a + * credential in order to change anything else. + * + * The radio is clicked first because the field is not on screen until it is: a + * key is write-only, so the configuration comes back carrying no credential to + * seed the draft from, and the form opens on the mode that matches what it was + * given rather than on the one it was created with. + */ await page .getByTestId("model-auth-type") .getByText("API key", { exact: true }) .click(); - await expectRequired(page, { - marked: ["Provider", "Model", "Name", "Namespace"], - unmarked: ["Authentication", "API key (leave blank to keep existing)"], + marked: ["Name", "Namespace"], + unmarked: ["API key (leave blank to keep existing)"], }); }); - await test.step("7. an empty result says so instead of showing a bare table", async () => { + await test.step("8. a change is saved, and the list shows it", async () => { + // The credential moves from one the controller minted to a Secret the cluster + // already holds — the one change on this form the list has a column for, which is + // what makes the save checkable from outside the form. + await page + .getByTestId("model-auth-type") + .getByText("Existing secret", { exact: true }) + .click(); + await page.getByTestId("model-api-key-secret").fill("kagent-anthropic"); + + await page.getByTestId("model-submit").click(); + await page.waitForURL(/\/models(\?|$)/, { timeout: 30_000 }); + + const row = rowNamed(page, CREATED); + await expect(row).toContainText("kagent-anthropic", { timeout: 30_000 }); + // Changed, not duplicated — which a create dressed as an update would be. + await expect(row).toHaveCount(1); + await expect(dataRows(page)).toHaveCount(SEEDED.length + 1); + }); + + await test.step("9. deleting asks first, and Keep leaves it alone", async () => { + await page.getByTestId(`delete-${CREATED}`).click(); + const prompt = confirmation(page); + // The confirmation names the row. "Delete this model configuration?" is no help + // in a table of five of them, and *which* is the one question the reader has. + await expect(prompt).toContainText(CREATED); + await prompt.getByRole("button", { name: "Keep" }).click(); + await expect(rowNamed(page, CREATED)).toHaveCount(1); + // Waited out rather than assumed gone: the dialog stays visible while it animates + // away, and the next step's click would land on it. + await expect(prompt).toHaveCount(0); + }); + + await test.step("10. confirming removes that row and leaves the rest", async () => { + await confirmDelete(page, CREATED); + + await expect(rowNamed(page, CREATED)).toHaveCount(0, { timeout: 30_000 }); + // "Gone" has to mean that one rather than the read: a list that failed to reload + // is also a list the row is missing from. + await expect(dataRows(page)).toHaveCount(SEEDED.length); + await expect(rowNamed(page, "default-model-config")).toHaveCount(1); + }); + + await test.step("11. an empty result says so instead of showing a bare table", async () => { await loadPage(page, routes.models, { scenario: "empty", title: "Models" }); await expect(page.getByText("No model configurations yet.")).toBeVisible(); await expect(dataRows(page)).toHaveCount(0); }); - await test.step("8. a failed load is reported, not disguised as an empty list", async () => { + await test.step("12. a failed load is reported, not disguised as an empty list", async () => { await loadPage(page, routes.models, { scenario: "error", title: "Models" }); const alert = page.getByTestId("models-error"); @@ -216,7 +301,7 @@ test("models: the list reads, narrows, and says what the form requires", async ( await expect(dataRows(page)).toHaveCount(0); }); - await test.step("9. retrying asks the backend again, and it recovers", async () => { + await test.step("13. retrying asks the backend again, and it recovers", async () => { const before = await operationCalls(page, rpc.listModelConfigs); await page.getByRole("button", { name: "Try again" }).click(); diff --git a/ui/playwright/tests/prompts/prompts.spec.ts b/ui/playwright/tests/prompts/prompts.spec.ts index 98d5aa54da..618e641709 100644 --- a/ui/playwright/tests/prompts/prompts.spec.ts +++ b/ui/playwright/tests/prompts/prompts.spec.ts @@ -13,27 +13,32 @@ import { anyDialog, chooseFilter, clickRefresh, + confirmDelete, expectRequired, pressUntil, } from "../../helpers/resource"; import { operationCallCounts, operationCalls, rpc } from "../../helpers/mockCalls"; /** - * Prompt libraries, on the fixtures. The write journey is in `shared/prompts/`; what is - * left is the seeded libraries, the namespace filter, the discard prompts, and the empty - * and failure states. + * Prompt libraries — the whole life of one, in a single journey. * - * Two things a fragment list has that other resources do not: + * One test, because a video and a trace are recorded per *test* — see + * `playwright/README.md`. + * + * ## The two things a fragment list has that other resources do not * * **A save replaces the library.** `UpdatePromptTemplate` assigns the ConfigMap's whole - * `data` map, so removing a row deletes a fragment and two rows sharing a key merge - * silently. The form says so and refuses both — step 8, before the save that would lose - * prose somebody wrote. + * `data` map, so removing a row deletes a fragment and two rows sharing a key silently + * merge into one. Nothing on screen would tell a reader that, so the form says it and + * refuses both — asserted in step 8, before the save that would otherwise lose prose + * somebody wrote. * - * **The reads are scoped per namespace.** `ListPromptTemplates` requires one and offers - * no wildcard, so `usePrompts` fans out a call per namespace rather than narrowing - * something already fetched. That makes step 5 a claim about what was *asked for*, and is - * why the timeouts here are longer than the default. + * **The reads are scoped per namespace.** `ListPromptTemplates` requires a namespace + * and offers no wildcard, so `usePrompts` fans out one call per namespace rather than + * narrowing something already fetched. That makes the filter in step 5 a claim about + * what was *asked for*, and it is also why the timeouts here are longer than the + * default: under the slow scenario the page waits on the namespace list and then on one + * call per namespace. */ /** The two seeded libraries. */ @@ -58,7 +63,7 @@ const fragmentValue = (page: Page, index: number) => */ test.describe.configure({ timeout: LIFECYCLE_TIMEOUT }); -test("prompts: libraries are read, edited, and their failures reported", async ({ +test("prompts: a library is created, read, changed and deleted", async ({ page, }) => { await test.step("1. a loading state precedes the data", async () => { @@ -178,22 +183,17 @@ test("prompts: libraries are read, edited, and their failures reported", async ( await expect(page).toHaveURL(/\/prompts\/new$/); await expect(page.getByTestId("prompt-name")).toHaveValue(CREATED); + await page.getByTestId("fragment-key").first().fill("changelog"); + await page.getByTestId("fragment-value").first().fill("Group by user impact."); + await page.getByTestId("prompt-submit").click(); - /* - * And that is where this step stops. Creating the library, reading it back, adding a - * fragment and deleting it runs against both backends from - * `playwright/shared/prompts/prompts.spec.ts`. Left here it was the same journey - * driven twice, and the count it used to assert is stronger there, where it is - * relative to whatever the list already held. - * - * Navigated rather than submitted, so the steps below start on the list. - * - * Not coverage of an abandoned draft, though it looks like it: `loadPage` is a - * `page.goto`, which restarts the module-level mock backend, so a draft that had - * wrongly saved itself would be wiped by the navigation and this would still pass. - */ - await loadPage(page, routes.prompts, { title: "Prompts" }); - await expect(dataRows(page)).toHaveCount(SEEDED.length); + await expect(page).toHaveURL(/\/prompts$/, { timeout: 30_000 }); + // Read back off the list rather than from a toast or a closed form: those two only + // prove the app believes it worked. + const row = rowNamed(page, CREATED); + await expect(row).toContainText("1 key", { timeout: 30_000 }); + await expect(row).toContainText("changelog"); + await expect(dataRows(page)).toHaveCount(SEEDED.length + 1); }); await test.step("7. the list's edit action opens the form, not the reading page", async () => { @@ -323,7 +323,7 @@ test("prompts: libraries are read, edited, and their failures reported", async ( // The keys column too, which is what the search on this page also covers. await expect(row).toContainText("handoff"); - await expect(dataRows(page)).toHaveCount(SEEDED.length); + await expect(dataRows(page)).toHaveCount(SEEDED.length + 1); await expect(rowNamed(page, "incident-playbooks")).toContainText("2 keys"); }); @@ -348,10 +348,20 @@ test("prompts: libraries are read, edited, and their failures reported", async ( await expect(page.getByTestId("prompt-discard-body")).toHaveCount(0); await page.getByRole("link", { name: "Back to libraries" }).click(); - await expect(rowNamed(page, "shared-fragments")).toHaveCount(1, { timeout: 30_000 }); + await expect(rowNamed(page, CREATED)).toHaveCount(1, { timeout: 30_000 }); + }); + + await test.step("13. confirming a delete removes that row and leaves the rest", async () => { + await confirmDelete(page, CREATED); + + await expect(rowNamed(page, CREATED)).toHaveCount(0, { timeout: 30_000 }); + // "Gone" has to mean that one rather than the read: a list that failed to reload is + // also a list the row is missing from. + await expect(dataRows(page)).toHaveCount(SEEDED.length); + await expect(rowNamed(page, "shared-fragments")).toHaveCount(1); }); - await test.step("13. a library that has gone says so instead of offering a form", async () => { + await test.step("14. a library that has gone says so instead of offering a form", async () => { // Deep-linked, the way a stale tab or a shared address arrives. An edit form over a // library the cluster does not have would take input for a save that cannot land. await page.goto("/prompts/kagent/not-a-library/edit?mock=ok"); @@ -368,13 +378,13 @@ test("prompts: libraries are read, edited, and their failures reported", async ( await expect(page.getByTestId("prompt-detail-error")).toHaveCount(0); }); - await test.step("14. an empty result says so instead of showing a bare table", async () => { + await test.step("15. an empty result says so instead of showing a bare table", async () => { await loadPage(page, routes.prompts, { scenario: "empty", title: "Prompts" }); await expect(page.getByText("No prompt libraries yet.")).toBeVisible(); await expect(dataRows(page)).toHaveCount(0); }); - await test.step("15. a failed load is reported, not disguised as an empty list", async () => { + await test.step("16. a failed load is reported, not disguised as an empty list", async () => { await loadPage(page, routes.prompts, { scenario: "error", title: "Prompts" }); const alert = page.getByTestId("prompts-error"); @@ -398,7 +408,7 @@ test("prompts: libraries are read, edited, and their failures reported", async ( await expect(page.getByTestId("prompt-fragments")).toHaveCount(0); }); - await test.step("16. retrying asks the backend again, and it recovers", async () => { + await test.step("17. retrying asks the backend again, and it recovers", async () => { await loadPage(page, routes.prompts, { scenario: "error", title: "Prompts" }); /* diff --git a/ui/playwright/tests/routing.spec.ts b/ui/playwright/tests/routing.spec.ts index 8166751215..6714b2adc1 100644 --- a/ui/playwright/tests/routing.spec.ts +++ b/ui/playwright/tests/routing.spec.ts @@ -1,21 +1,57 @@ import { test, expect } from "../fixtures/test"; import { agentChat, instances, loadPage, expectPageTitle, routes } from "../helpers/app"; -import { expectNoShell, expectShell } from "../helpers/nav"; +import { clickNav, expectNoShell, expectShell } from "../helpers/nav"; /** - * Routing, the two parts of it kept off the shared suite. + * Routing — new coverage for the thing this rewrite changed most. * - * A deep link carrying an `AgentInstance` id has to name a conversation that exists, - * which a clean cluster has not got. The login route is portable — it was tried against - * a deployment and passes — but `shared/routing.spec.ts` already proves the SPA fallback - * on `/substrate` and an unknown path, and `/login` is a third instance of that same - * claim, so it earns nothing by running twice. + * The old app routed on the server through Next's file-system router; this one + * is a single-page app with a client-side router, which puts four behaviours at + * risk that used to come for free: in-app navigation, deep linking straight to a + * route, an unknown path resolving to a 404 rather than a blank screen, and a + * standalone route rendering outside the shell. */ -test("routing: a deep link with params, and the standalone login route", async ({ +test("routing: in-app navigation, deep links, 404, and standalone routes", async ({ page, }) => { - await test.step("1. a deep link with route params renders too", async () => { + await test.step("1. a sidebar click changes both the URL and the content", async () => { + await loadPage(page, routes.dashboard, { title: "Dashboard" }); + + await clickNav(page, "agents", /\/agents(\?|$)/); + await expectPageTitle(page, "Agents"); + + await clickNav(page, "prompts", /\/prompts(\?|$)/); + await expectPageTitle(page, "Prompts"); + }); + + await test.step("2. the sidebar marks the active destination", async () => { + await expect(page.getByTestId("nav-prompts")).toHaveClass(/ant-menu-item-selected/); + await expect(page.getByTestId("nav-agents")).not.toHaveClass( + /ant-menu-item-selected/, + ); + }); + + await test.step("3. browser history moves between routes", async () => { + await page.goBack(); + await page.waitForURL(/\/agents(\?|$)/); + await expectPageTitle(page, "Agents"); + + await page.goForward(); + await page.waitForURL(/\/prompts(\?|$)/); + await expectPageTitle(page, "Prompts"); + }); + + await test.step("4. a deep link renders that route on a cold load", async () => { + // A full page load, not a client-side transition: this is the link someone + // pastes into chat, and the one a server that does not fall back to + // index.html would break. + await loadPage(page, routes.substrate, { title: "Substrate" }); + await expectShell(page); + await expect(page).toHaveURL(/\/substrate/); + }); + + await test.step("5. a deep link with route params renders too", async () => { // Two params: the namespace and the AgentInstance id, which is how every agent // surface is addressed now. await loadPage(page, agentChat(instances.ready)); @@ -29,7 +65,24 @@ test("routing: a deep link with params, and the standalone login route", async ( await expect(page.getByTestId("chat-panel")).toBeVisible(); }); - await test.step("2. login renders standalone, outside the shell", async () => { + await test.step("6. an unknown path renders 404 inside the shell", async () => { + await loadPage(page, "/no-such-page"); + // The address it tried, which is what a reader compares against the link they + // followed — "that page does not exist" told them nothing they could act on. + await expect(page.getByTestId("not-found-path")).toHaveText("/no-such-page"); + // And somewhere to go that is not just "back to the dashboard", which is the right + // destination only if that is where they were headed. + await expect(page.getByTestId("not-found-link-agents")).toBeVisible(); + // Still inside the app: a wrong URL should not strand the user with no + // way back. + await expectShell(page); + + await page.getByTestId("not-found-dashboard").click(); + await page.waitForURL(/\/$/); + await expectPageTitle(page, "Dashboard"); + }); + + await test.step("7. login renders standalone, outside the shell", async () => { await loadPage(page, routes.login); await expect(page.getByTestId("login-page")).toBeVisible(); await expectNoShell(page); diff --git a/ui/playwright/tests/schedules/schedules.spec.ts b/ui/playwright/tests/schedules/schedules.spec.ts index bfdf163fed..335bbbabad 100644 --- a/ui/playwright/tests/schedules/schedules.spec.ts +++ b/ui/playwright/tests/schedules/schedules.spec.ts @@ -1,33 +1,42 @@ import { test, expect } from "../../fixtures/test"; -import { - LIFECYCLE_TIMEOUT, - confirmation, - optionNamed, - pressOnce, -} from "../../helpers/resource"; +import { tick } from "../../helpers/controls"; +import { LIFECYCLE_TIMEOUT, optionNamed, pressUntil } from "../../helpers/resource"; /** - * Schedules — reading one, running it, and the states around that. + * Schedules — the whole life of one, in a single journey. * - * The write journey runs against both backends from `shared/schedules/`, and the claim - * that a schedule survives a reload — which the fixtures structurally cannot answer, - * keeping writes in the page's own memory — is `live/schedules.spec.ts`. That same - * memory is why the steps below click through rather than navigate wherever a write has - * to outlive the step that made it. + * One test, because a video and a trace are recorded per *test* — see + * `playwright/README.md`. * - * **A schedule is the only resource here that runs**, so pausing one, invoking it by hand - * while paused, and reading the execution it produced are covered here and nowhere else. + * ## What is distinctive about this resource, and therefore what is covered * - * **Its cadence has two representations.** An advanced cron expression the repeat picker - * cannot show has to survive an edit to some other field — the case that silently + * **A schedule is the only resource here that runs.** So the journey covers pausing one, + * invoking it by hand while paused, and reading the execution it produced — none of which + * any other resource has, and all of which is the reason a schedule exists. + * + * **Its cadence has two representations.** The form offers a repeat picker and an + * advanced cron expression, and an expression the picker cannot represent has to survive + * an edit to some other field. That is asserted because it is the one that silently * destroys a reader's work. * * **Its history outlives it.** A deleted schedule still opens by address and says what it - * is, so that address is neither a 404 nor a live schedule. + * is, because the executions are retained — so the address is not a 404 and must not be + * rendered as a live schedule either. + * + * ## What is still read as prose, deliberately + * + * Five selectors, and each is the right tool rather than a leftover. Two are fixture + * *data* in a cell — an execution's failure reason, its task id — which is the thing + * under test and has no id to give it. One is a form rule's message. The last two are + * `getByLabel("Monday")` on the weekday checkboxes, which are genuinely labelled + * controls: a label is what a reader clicks and what a screen reader announces, so + * reaching for one is not the same as matching copy. * - * Five selectors read prose deliberately: two are fixture data in a cell with no id to - * give it, one is a form rule's message, and two are `getByLabel` on the weekday - * checkboxes, which are genuinely labelled controls. + * ## Why it clicks through rather than navigating + * + * The mock backend keeps writes in the page's own memory, so a `page.goto` starts a + * backend that has never heard of the schedule just made. Everything from step 4 onwards + * therefore clicks, and the created schedule survives to the delete at the end. */ /** `Daily cluster report`, the seeded schedule the read half is asserted against. */ @@ -36,6 +45,10 @@ const SEEDED = "c686bd1d-9124-4e96-8df7-000000000001"; /** A schedule deleted before the fixtures were written, kept for its history. */ const RETIRED = "c686bd1d-9124-4e96-8df7-000000000004"; +/** The one this journey makes, reads, renames and removes. */ +const CREATED = "Probe alpha"; +const RENAMED = "Probe beta"; + /* * A lifecycle is longer than a journey, so it gets its own budget — see * `LIFECYCLE_TIMEOUT`. Set per file rather than across the suite, so the tight default @@ -43,7 +56,7 @@ const RETIRED = "c686bd1d-9124-4e96-8df7-000000000004"; */ test.describe.configure({ timeout: LIFECYCLE_TIMEOUT }); -test("schedules: a schedule is read, run, paused, and its failures reported", async ({ +test("schedules: a schedule is created, read, run, changed and deleted", async ({ page, }) => { const rows = page.getByRole("row"); @@ -57,16 +70,6 @@ test("schedules: a schedule is read, run, paused, and its failures reported", as // No pagination over a list this size: a control that pages nothing is a control // that implies there is more to see. await expect(page.getByTestId("schedules-pages")).toHaveCount(0); - - /* - * The row reads the cron rather than printing it. `0 9 * * *` is a field the - * controller stores and not something to put in front of a reader, and the column - * is the only place the derived reading is shown — `scheduleTiming.test.ts` covers - * every shape the reading takes, including the weekly ones no fixture here has. - */ - const row = rowNamed("Daily cluster report"); - await expect(row).toContainText("Every day at 09:00"); - await expect(row).not.toContainText("* * *"); }); await test.step("2. a row opens its schedule, and its buttons still do their own job", async () => { @@ -252,7 +255,184 @@ test("schedules: a schedule is read, run, paused, and its failures reported", as await expect(page.getByText("Choose an agent.", { exact: true })).toBeVisible(); }); - await test.step("11. a read failure is not an empty list", async () => { + await test.step("11. a filled-in schedule is created and lands on its own page", async () => { + await page.getByTestId("schedule-agent").click(); + await optionNamed(page, "kagent/k8s-agent-7f3a91c on k8s-agent").click(); + await page.getByTestId("schedule-name").fill(CREATED); + + // The picker, not the raw expression: a weekly cadence is stated back in words, so a + // reader can tell the schedule they described from the one they got. + await expect(page.getByTestId("schedule-cron")).toHaveCount(0); + await page.getByTestId("schedule-frequency").click(); + // Pressed until the cadence actually changes: the weekday checkboxes only exist + // once the frequency is weekly, so a dropdown click swallowed by the animation + // leaves the next line waiting for a control that is never coming. + await pressUntil(optionNamed(page, "Weekly"), () => + expect(page.getByTestId("schedule-days")).toBeVisible(), + ); + await page.getByTestId("schedule-time").fill("08:00"); + await tick(page.getByLabel("Wednesday", { exact: true })); + await expect(page.getByTestId("schedule-cadence")).toHaveText( + "Weekly on Monday, Wednesday at 08:00 (UTC)", + ); + + await page.getByTestId("schedule-prompt").fill("Check the probe."); + await page.getByTestId("schedule-timeout").fill("120"); + + // Enabled by default, and the sentence underneath changes with it — which is the + // only thing on screen that says whether creating this starts it running. + await expect(page.getByTestId("schedule-enabled")).toBeChecked(); + await expect(page.getByTestId("schedule-enabled-note")).toContainText( + "will run automatically after it is created", + ); + await page.getByTestId("schedule-enabled").uncheck(); + await expect(page.getByTestId("schedule-enabled-note")).toContainText( + "will not run automatically after it is created", + ); + + await page.getByTestId("schedule-submit").click(); + await expect( + page.getByRole("heading", { name: CREATED, exact: true }), + ).toBeVisible(); + await expect(page.getByTestId("schedule-meta")).toContainText( + "Weekly on Monday, Wednesday at 08:00", + ); + // The record below the header, not the pills beside the name: `schedule-meta` + // carries the cadence and the clock, and the timeout is one of its fields. + await expect(page.getByTestId("schedule-detail")).toContainText("120 seconds"); + }); + + await test.step("12. the list is the proof, with one more row", async () => { + // A closed form and a redirect only prove the app believes it worked. + await page.getByTestId("schedule-back").click(); + await expect(rowNamed(CREATED)).toHaveCount(1); + await expect(rowNamed(CREATED)).toContainText("Weekly on Monday, Wednesday"); + }); + + await test.step("13. an edit from the list renames it, rather than duplicating it", async () => { + const before = await rows.count(); + + await page.getByTestId(`edit-${CREATED}`).click(); + await expect( + page.getByRole("heading", { name: `Edit ${CREATED}`, exact: true }), + ).toBeVisible(); + // The draft opens on what was saved, including the switch that was turned off and + // both chosen days — a picker that kept only the last one would look right here with + // one assertion. + await expect(page.getByTestId("schedule-enabled")).not.toBeChecked(); + await expect(page.getByTestId("schedule-time")).toHaveValue("08:00"); + await expect(page.getByLabel("Monday", { exact: true })).toBeChecked(); + await expect(page.getByLabel("Wednesday", { exact: true })).toBeChecked(); + + // And the sentence says "saved" here where the create form said "created". Same + // switch, different consequence, and the wording is the only thing on screen that + // distinguishes them. + await expect(page.getByTestId("schedule-enabled-note")).toContainText( + "will not run automatically after it is saved", + ); + await tick(page.getByTestId("schedule-enabled")); + await expect(page.getByTestId("schedule-enabled-note")).toContainText( + "will run automatically after it is saved", + ); + + await page.getByTestId("schedule-name").fill(RENAMED); + // The time zone is an AutoComplete, so its id is on the wrapper and the caret goes + // in the input inside it. Every other field here carries its id on the control. + await page.getByTestId("schedule-timezone").locator("input").fill("Europe/Berlin"); + // The zone list is an autocomplete; dismiss it so it is not over the form. + await page.keyboard.press("Escape"); + await page.getByTestId("schedule-submit").click(); + await expect( + page.getByRole("heading", { name: RENAMED, exact: true }), + ).toBeVisible(); + + await page.getByTestId("schedule-back").click(); + await expect(rowNamed(RENAMED)).toHaveCount(1); + await expect(rowNamed(RENAMED)).toContainText("Europe/Berlin"); + // Renamed, not duplicated. + await expect(rowNamed(CREATED)).toHaveCount(0); + await expect(rows).toHaveCount(before); + }); + + await test.step("14. deleting asks first, navigates nowhere, and Keep leaves it", async () => { + await page + .getByRole("button", { name: `Delete schedule ${RENAMED}`, exact: true }) + .click(); + await expect(page.getByRole("button", { name: "Keep", exact: true })).toBeVisible(); + await expect(page).toHaveURL(/\/schedules(\?.*)?$/); + await pressUntil(page.getByRole("button", { name: "Keep", exact: true }), () => + expect(page.getByRole("button", { name: "Keep", exact: true })).toBeHidden(), + ); + await expect(rowNamed(RENAMED)).toHaveCount(1); + }); + + await test.step("15. and the delete on its own page asks in a modal, which Keep dismisses", async () => { + /* + * The other delete surface, and a different control: the list asks in a popconfirm + * beside the row, while the page about one schedule asks in a modal from its danger + * zone. Both are reached here rather than only the list one, because the two are + * separate call sites and it is the page-level one that carries the sentence saying + * what deleting costs. + */ + await page.getByTestId(`schedule-link-${RENAMED}`).click(); + await expect(page).toHaveURL(/\/schedules\/[0-9a-f-]+$/); + + await page + .getByTestId("schedule-danger") + .getByRole("button", { name: `Delete schedule ${RENAMED}`, exact: true }) + .click(); + // Titled with the schedule's name: "Delete this schedule?" is no help to somebody + // who arrived here from a list of four of them. + const confirmation = page.getByRole("dialog", { + name: `Delete schedule ${RENAMED}?`, + exact: true, + }); + await expect(confirmation).toContainText("Stops future executions."); + await pressUntil(confirmation.getByRole("button", { name: "Keep", exact: true }), () => + expect(confirmation).toBeHidden(), + ); + // Still usable afterwards, so a dismissed confirmation leaves no disabled page. + await expect(page.getByTestId("schedule-run")).toBeEnabled(); + }); + + await test.step("16. confirming removes it, leaves for the list, and the rest stays", async () => { + await page + .getByTestId("schedule-danger") + .getByRole("button", { name: `Delete schedule ${RENAMED}`, exact: true }) + .click(); + /* + * Pressed until it takes: a Delete click dropped on Firefox reports as "the page + * never navigated" rather than as a missed click. See `pressUntil`. + * + * Deleting leaves for the list, which is where the reader can act next — this page + * is now about a schedule that is gone — so the navigation is what proves the press + * landed. + */ + await pressUntil( + page + .getByRole("dialog", { name: `Delete schedule ${RENAMED}?`, exact: true }) + .getByRole("button", { name: "Delete", exact: true }), + () => expect(page).toHaveURL(/\/schedules(\?.*)?$/), + ); + await expect(rowNamed(RENAMED)).toHaveCount(0); + // One row went, not the table. + await expect( + page.getByTestId("schedule-link-Daily cluster report"), + ).toBeVisible(); + }); + + /* + * The states that need the backend answering differently, folded in here rather than + * kept as a second test. + * + * They were split out on the reasoning that a `page.goto` resets the fixture backend + * and would throw away the schedule the lifecycle is holding. True, and beside the + * point once they run *last*: by here the schedule has been deleted and there is + * nothing left to lose. `models`, `mcp-servers` and `prompts` all end the same way, + * and this file reading differently from them was the contradiction rather than the + * reset. + */ + await test.step("17. a read failure is not an empty list", async () => { await page.goto("/schedules?mock=error"); await expect(page.getByTestId("schedules-error")).toContainText( "Could not load schedules", @@ -262,7 +442,7 @@ test("schedules: a schedule is read, run, paused, and its failures reported", as await expect(page.getByTestId("schedules-empty")).toHaveCount(0); }); - await test.step("12. and an empty list says so plainly, with nothing to scroll", async () => { + await test.step("18. and an empty list says so plainly, with nothing to scroll", async () => { await page.goto("/schedules?mock=empty"); await expect(page.getByTestId("schedules-empty")).toBeVisible(); @@ -275,50 +455,7 @@ test("schedules: a schedule is read, run, paused, and its failures reported", as expect(overflows).toBe(false); }); - await test.step("13. delete asks twice over, and Keep leaves the row where it was", async () => { - await page.goto("/schedules?mock=ok"); - - /* - * The list confirms in a popconfirm and the detail page in a modal — two shapes, - * one copy, and the cancel on each reads "Keep" rather than "Cancel" because the - * reader is choosing between two outcomes rather than dismissing a dialog. - * - * Both paths matter: the sentence is what a reader decides on, and it is the part - * that would go stale silently if only one of the two were driven. - */ - await page.getByTestId("delete-Schedule 3").click(); - const popconfirm = confirmation(page); - await expect(popconfirm).toContainText("Stops future executions."); - await expect(popconfirm).toContainText("history and conversations are retained"); - await pressOnce(popconfirm.getByRole("button", { name: "Keep", exact: true })); - await expect(page.getByTestId("schedule-link-Schedule 3")).toBeVisible(); - - // And the same on the detail page's own delete, which is a modal. - await page.getByTestId("schedule-link-Daily cluster report").click(); - await page.waitForURL(new RegExp(`/schedules/${SEEDED}$`)); - await page.getByTestId("delete-Daily cluster report").click(); - const modal = page.getByRole("dialog"); - await expect(modal).toContainText("Stops future executions."); - await pressOnce(modal.getByRole("button", { name: "Keep", exact: true })); - await expect(page).toHaveURL(new RegExp(`/schedules/${SEEDED}$`)); - }); - - await test.step("14. a delete takes one row and leaves the others", async () => { - // The claim a delete test usually forgets: that it removed the row it was asked - // for and not the list. Only the seeded fixtures can say this, since a live journey - // deletes the one thing it made and has nothing else of its own to count. - await page.goto("/schedules?mock=ok"); - await page.getByTestId("delete-Schedule 3").click(); - await pressOnce( - confirmation(page).getByRole("button", { name: "Delete", exact: true }), - ); - - await expect(page.getByTestId("schedule-link-Schedule 3")).toHaveCount(0); - await expect(page.getByTestId("schedule-link-Daily cluster report")).toBeVisible(); - await expect(page.getByTestId("schedule-link-Schedule 2")).toBeVisible(); - }); - - await test.step("15. a link held from before a delete still opens, and says what it is", async () => { + await test.step("19. a link held from before a delete still opens, and says what it is", async () => { // The executions are retained, so the address is not a 404 — and must not render as // a live schedule either, or a reader will try to act on one that is gone. await page.goto(`/schedules/${RETIRED}?mock=ok`); @@ -332,7 +469,7 @@ test("schedules: a schedule is read, run, paused, and its failures reported", as await expect(page.getByTestId("schedule-meta")).toContainText("Deleted"); }); - await test.step("16. and it is not offered in the list it was removed from", async () => { + await test.step("20. and it is not offered in the list it was removed from", async () => { await page.goto("/schedules?mock=ok"); await expect( page.getByTestId("schedule-link-Retired sweep"), diff --git a/ui/playwright/tests/substrate/inventory.spec.ts b/ui/playwright/tests/substrate/inventory.spec.ts index c8e49554ae..be8210daba 100644 --- a/ui/playwright/tests/substrate/inventory.spec.ts +++ b/ui/playwright/tests/substrate/inventory.spec.ts @@ -252,24 +252,7 @@ test("substrate: an empty inventory is shown without errors", async ({ await loadPage(page, routes.substrate, { scenario: "empty", title: "Substrate" }); await expectSettled(page); - // Said by the two tables it applies to, not by a tile: a tile is for a number that - // moves, and an ate-api one read `connected` above that service's own timeout banner. - // The tiles are named rather than one absence asserted, an ate-api tile returning - // under any other id being the same regression. - const tiles = await page - .locator('[data-testid^="substrate-stat-"]') - .evaluateAll((nodes) => - nodes - .map((node) => node.getAttribute("data-testid") ?? "") - .filter((id) => !id.endsWith("-value")), - ); - expect(tiles.sort()).toEqual([ - "substrate-stat-actors", - "substrate-stat-pools", - "substrate-stat-scope", - "substrate-stat-templates", - "substrate-stat-workers", - ]); + await expect(page.getByTestId("substrate-stat-ateapi")).toHaveCount(0); await expect(page.getByTestId("substrate-inventory-error")).toHaveCount(0); await expect(page.getByTestId("substrate-partial")).toHaveCount(0); diff --git a/ui/src/api/chat/mockChatClient.ts b/ui/src/api/chat/mockChatClient.ts index 80f1c876ad..dbbd2c3f68 100644 --- a/ui/src/api/chat/mockChatClient.ts +++ b/ui/src/api/chat/mockChatClient.ts @@ -16,11 +16,7 @@ import { currentChatScenario } from "@/mocks/scenario"; import { allAgentInstances, instanceShareForToken } from "@/mocks/state"; import { ApiError } from "../ApiError"; import { agentInstanceShareToken } from "../shareToken"; -import { - HITL_EXTENSION_URI, - readToolApprovalResponse, - type PendingRequest, -} from "./hitl"; +import { HITL_EXTENSION_URI, type PendingRequest } from "./hitl"; import { conversationKey } from "./types"; import type { ChatClient, @@ -39,9 +35,6 @@ const TIMING = { error: { step: 300, word: 45 }, asks: { step: 300, word: 45 }, "asks-text": { step: 300, word: 45 }, - approves: { step: 300, word: 45 }, - "approves-one": { step: 300, word: 45 }, - "asks-unknown": { step: 300, word: 45 }, } as const; /** @@ -60,19 +53,6 @@ const NOTE_QUESTION = "What should I put on the order note?"; /** The correlation id, which a real answer echoes verbatim. */ const REQUEST_ID = "adk-mock-ask-1"; -/** - * The tools the scripted approval turn asks to run. - * - * Two, because the decision is per tool: one approved and one rejected in the same - * submission is the case a single-tool fixture cannot produce, and it is the one that - * says the controls are wired to their own row rather than to the form. - */ -const APPROVAL_TOOLS = [ - { id: "call-1", name: "kubectl_apply", args: { manifest: "deployment.yaml" } }, - { id: "call-2", name: "shell_exec", args: { command: "rm -rf /tmp//cache" } }, -]; -const APPROVAL_HINT = "These change the cluster. Approve only what you recognise."; - /** Where the scripted turn gives up when the scenario asks it to fail. */ const FAILURE_MESSAGE = "The agent stopped responding. The connection to the runtime was lost."; @@ -243,40 +223,18 @@ export class MockChatClient implements ChatClient { /* * What the agent understood, which is not the same as what it received. * - * The runtime reads a structured answer only from a message that both declares the - * extension and carries the payload under its URI; anything else reaches the agent - * as ordinary prose, the turn resumes, and the reply reads as though it worked. So - * the acknowledgement below says which happened, tool by tool for an approval — - * that silent failure is the reason this fixture bothers to check. - * - * Through the app's own parser rather than a second one: a copy here drifted - * permissive, taking an empty `approvals` array and an empty `rejection_reason` - * where the real reader rejects both, and a fixture that accepts more than the code - * it stands in for acknowledges payloads the app would refuse. The extension list - * is asserted rather than read — the port carries the payload directly, having no - * wire to declare it on. + * The runtime reads the structured answer only from a message that both + * declares the extension and carries the payload under its URI; anything else + * reaches the agent as ordinary prose, the turn resumes, and the reply reads + * as though it worked. So the acknowledgement here says which happened — that + * silent failure is the reason this fixture bothers to check. */ - const decisions = - parked.kind === "tool_approval" - ? readToolApprovalResponse(input.hitl, [HITL_EXTENSION_URI]) - : undefined; - const approvalReply = - decisions && - decisions - .map( - (decision) => - `${decision.id} ${decision.approved ? "approved" : `rejected (${decision.rejectionReason ?? "no reason given"})`}`, - ) - .join("; "); - const acknowledgement = message( `${parked.taskId}-ack`, "agent", structured && parked.kind === "ask_user" && structured.id === parked.requestId ? `Noted: **${structured.answers.map((a) => a.join(", ")).join("; ")}**.` - : approvalReply - ? `Noted: **${approvalReply}**.` - : `I did not catch a choice in that.`, + : `I did not catch a choice in that.`, parked.taskId, ); answered.push(acknowledgement); @@ -411,34 +369,6 @@ export class MockChatClient implements ChatClient { return; } - if (scenario === "approves" || scenario === "approves-one") { - // The same park, a different request: tools to vouch for rather than a question - // to answer. `hint` is the runtime's own sentence about why it is asking. - this.persist(sessionId); - const request: PendingRequest = { - kind: "tool_approval", - taskId, - tools: scenario === "approves-one" ? [APPROVAL_TOOLS[1]] : APPROVAL_TOOLS, - hint: APPROVAL_HINT, - }; - saveParked(sessionId, request); - yield { type: "status", state: "input_required", taskId, awaiting: request }; - return; - } - - if (scenario === "asks-unknown") { - /* - * Parked on something this build cannot render: a turn started without the - * extension carries its question as prose and no correlation id, so there is - * nothing to answer against. `unknown` is what `readRequest` returns for it. - */ - this.persist(sessionId); - const request: PendingRequest = { kind: "unknown", taskId }; - saveParked(sessionId, request); - yield { type: "status", state: "input_required", taskId, awaiting: request }; - return; - } - yield { type: "status", state: "completed", taskId }; } diff --git a/ui/src/api/hooks/useAgentBuildingBlocks.ts b/ui/src/api/hooks/useAgentBuildingBlocks.ts index a63c5f841d..f1157cf0a6 100644 --- a/ui/src/api/hooks/useAgentBuildingBlocks.ts +++ b/ui/src/api/hooks/useAgentBuildingBlocks.ts @@ -48,17 +48,9 @@ export function useAgentTemplates(namespace?: string): ApiResource { - /* - * Keyed on `namespaces` rather than on the joined string, because `[]` joins to - * `""` — and holding the read back for that conflates "we do not know the - * namespaces yet" with "we know, and there are none". The second is an answer: it - * resolves to an empty list, so a page can say "0 of 0" and mean it. Held back, the - * key stays null, `isLoading` is reported false (idle, not loading) and `data` never - * arrives, so a summary gated on `data` would never render at all. - */ const key = namespaces ? [...namespaces].sort().join(",") : undefined; - return useApiResource(namespaces ? ["harnesses.listAll", key] : null, async () => { + return useApiResource(key ? ["harnesses.listAll", key] : null, async () => { const names = key ? key.split(",").filter(Boolean) : []; const settled = await Promise.allSettled( names.map((namespace) => apiClient.agentBuildingBlocks.harnesses(namespace)), @@ -113,12 +105,10 @@ export function useAgentTemplatesAcrossNamespaces( ): ApiResource { // Sorted into the key, so the same set in a different order is the same read rather // than a cache miss that refetches everything. - // `namespaces`, not `key`: see `useHarnessesAcrossNamespaces` — `[]` joins to `""`, - // and a known-empty set is an answer rather than a reason to hold the read back. const key = namespaces ? [...namespaces].sort().join(",") : undefined; return useApiResource( - namespaces ? ["agentTemplates.listAll", key] : null, + key ? ["agentTemplates.listAll", key] : null, async () => { const names = key ? key.split(",").filter(Boolean) : []; const settled = await Promise.allSettled( diff --git a/ui/src/api/hooks/useApiResource.ts b/ui/src/api/hooks/useApiResource.ts index 4e1b26c4fc..d0aa2052bb 100644 --- a/ui/src/api/hooks/useApiResource.ts +++ b/ui/src/api/hooks/useApiResource.ts @@ -42,18 +42,8 @@ export function useApiResource( return { data, - /* - * A held-back request (`key === null`) is idle, not loading — otherwise a page - * waiting on a route param renders a spinner that never resolves. - * - * The consequence every caller has to handle: while a read is held back this - * reports `isLoading: false` with `data` undefined, so `!error && !isLoading` is - * true for a page that has asked nothing. A count rendered on that gate claims - * "0 of 0" about a backend it has not called. Gate a summary on `data !== undefined` - * as well — and if the key is held back on another read's result, make sure the - * empty case resolves rather than holding back forever, or the summary never - * renders at all. - */ + // A held-back request (`key === null`) is idle, not loading — otherwise a + // page waiting on a route param renders a spinner that never resolves. isLoading: key !== null && isLoading, isValidating, error: error ?? undefined, diff --git a/ui/src/mocks/scenario.ts b/ui/src/mocks/scenario.ts index bc12936faf..14d82de7b1 100644 --- a/ui/src/mocks/scenario.ts +++ b/ui/src/mocks/scenario.ts @@ -93,34 +93,7 @@ function isScenario(value: string | null): value is MockScenario { * `asks-text` offers one prose field — the only shape where the field can take the * caret on arrival and Enter can mean "send". */ -/** - * `approves` parks the turn on a tool approval request instead of a question: a - * different payload, and a different set of controls — approve or reject each tool, - * with a reason on a rejection. It is a third variation of the parked turn above rather - * than a new seam, because this client already yields the request at the `ChatClient` - * boundary; what it must not do is start emitting wire frames, which is a transport - * shape covered by unit tests over real bytes. - * - * `approves-one` is the same request with a single tool, which is a different set of - * controls rather than a shorter list of the same ones: Approve and Reject on the prompt - * itself, and a rejection that opens its reason with the caret already in it. A fixture - * with two tools cannot reach either. - * - * `asks-unknown` parks it on a request this build does not recognise, which is what a - * turn started without the HITL extension looks like: prose, no correlation id, nothing - * to answer against. The page says so and offers only the discard, and that sentence is - * the only thing standing between a reader and a question they cannot see is unanswerable. - */ -export const CHAT_SCENARIOS = [ - "ok", - "error", - "slow", - "asks", - "asks-text", - "approves", - "approves-one", - "asks-unknown", -] as const; +export const CHAT_SCENARIOS = ["ok", "error", "slow", "asks", "asks-text"] as const; export type ChatScenario = (typeof CHAT_SCENARIOS)[number]; diff --git a/ui/src/pages/AgentPage.tsx b/ui/src/pages/AgentPage.tsx index 50a58faf14..4cb1eecf83 100644 --- a/ui/src/pages/AgentPage.tsx +++ b/ui/src/pages/AgentPage.tsx @@ -68,10 +68,8 @@ const PAGE_SIZE = 25; * resolving through the revision rather than through labels selects conversations * stored before the fields existed, with no migration and no backfill. * - * Search and sort are still the browser's. That is honest only because this client - * follows every page token before rendering, so what it searches is every - * conversation with this agent rather than the first page; `playwright/DEFERRED.md` - * records what has to change if the page-following ever goes. + * Search and sort are still the browser's, over whatever pages have been read, and + * the note under the table says so rather than implying otherwise. * * ## Somebody else's conversation is listed and cannot be opened * @@ -571,10 +569,7 @@ export function AgentPage() { }, ]} trailing={ - /* `data !== undefined`: an idle read reports `isLoading: false` with nothing in it — see `useApiResource`. */ - !conversations.error && - !conversations.isLoading && - conversations.data !== undefined ? ( + !conversations.error && !conversations.isLoading ? ( {/* Only a successful load can be counted: "0 of 0" because a request failed would be a claim the page cannot support. */} - {/* `data !== undefined`: an idle read reports `isLoading: false` with nothing in it — see `useApiResource`. */} - {!loadFailure && !templates.isLoading && templates.data !== undefined ? ( + {!loadFailure && !templates.isLoading ? ( {filtered.length} of {rows.length}{" "} {rows.length === 1 ? "template" : "templates"} diff --git a/ui/src/pages/AgentsPage.tsx b/ui/src/pages/AgentsPage.tsx index 9f827569cb..62c80d020a 100644 --- a/ui/src/pages/AgentsPage.tsx +++ b/ui/src/pages/AgentsPage.tsx @@ -459,8 +459,7 @@ export function AgentsTab() { ]} trailing={ - {/* `data !== undefined`: an idle read reports `isLoading: false` with nothing in it — see `useApiResource`. */} - {!loadFailure && !templates.isLoading && templates.data !== undefined ? ( + {!loadFailure && !templates.isLoading ? ( {filtered.length} of {agents.length}{" "} {agents.length === 1 ? "agent" : "agents"} diff --git a/ui/src/pages/AppDetailPage.tsx b/ui/src/pages/AppDetailPage.tsx index d085a8a4e6..87dce43c56 100644 --- a/ui/src/pages/AppDetailPage.tsx +++ b/ui/src/pages/AppDetailPage.tsx @@ -120,15 +120,8 @@ export function AppDetailPage() { /> ) : null} - {/* Absence is only meaningful once the read finished and succeeded, and - `!isLoading` alone does not mean that: an idle read reports it false with - nothing in it, so this announced "No such app" about a list it had not - asked for. See `useApiResource`. */} - {appName && - !servers.error && - !servers.isLoading && - servers.data !== undefined && - matches.length === 0 ? ( + {/* Absence is only meaningful once the read finished and succeeded. */} + {appName && !servers.error && !servers.isLoading && matches.length === 0 ? ( {filtered.length} of {models.length}{" "} {models.length === 1 ? "configuration" : "configurations"} diff --git a/ui/src/pages/PromptsPage.tsx b/ui/src/pages/PromptsPage.tsx index be14963bc9..acf81e4e90 100644 --- a/ui/src/pages/PromptsPage.tsx +++ b/ui/src/pages/PromptsPage.tsx @@ -231,8 +231,7 @@ export function PromptsPage() { }, ]} trailing={ - /* `data !== undefined`: an idle read reports `isLoading: false` with nothing in it — see `useApiResource`. */ - !error && !isLoading && data !== undefined ? ( + !error && !isLoading ? ( {filtered.length} of {libraries.length}{" "} {libraries.length === 1 ? "library" : "libraries"} diff --git a/ui/src/pages/agents/HarnessesTab.tsx b/ui/src/pages/agents/HarnessesTab.tsx index a50501eb5b..03807f46c1 100644 --- a/ui/src/pages/agents/HarnessesTab.tsx +++ b/ui/src/pages/agents/HarnessesTab.tsx @@ -243,8 +243,7 @@ export function HarnessesTab() { ]} trailing={ - {/* `data !== undefined`: an idle read reports `isLoading: false` with nothing in it — see `useApiResource`. */} - {!harnesses.error && !harnesses.isLoading && harnesses.data !== undefined ? ( + {!harnesses.error && !harnesses.isLoading ? ( {filtered.length} of {rows.length}{" "} {rows.length === 1 ? "harness" : "harnesses"}