From e099b51bca3609dfd0605c5e997cae334d708b35 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 20:52:39 +0100 Subject: [PATCH 1/4] test(e2e): state per-row response defences and the real token surface Follow-ups 1, 2 and the inventory half of 4 from the #2679 review. The walk's header says which defence covers each `response` row is "stated per row in the inventory". Four rows had no such note, so add them: `home.quick-capture-submit`, `inbox.archive` and `review.approve` name the POST that no mount can raise (each anchored to the view's own `onMounted`), and `calendar.next-month` gains the spent-mount-read half its comment was missing beside its exact-string post-condition. The token docblock claimed substitution happens in `url` and `response` pattern strings and that `{openingMonthLabel}` is among them. Neither is the whole truth: `fillTokens` is applied at five consequence sites plus a `role` selector name, and `{openingMonthLabel}` appears in no pattern at all. State the real surface, and say what is used verbatim so a token written into an `attribute` selector or a test id is understood to fail silently. `entryPath` is documented as the read-nothing field it is. Record on the `textChangedFrom` kind that its negated matcher is not vacuous on a missing node: Playwright fails a zero-element `not.toHaveText` rather than passing it. Refs #2682 --- .../e2e/support/routeAffordanceInventory.ts | 73 +++++++++++++++++-- 1 file changed, 68 insertions(+), 5 deletions(-) diff --git a/frontend/taskdeck-web/tests/e2e/support/routeAffordanceInventory.ts b/frontend/taskdeck-web/tests/e2e/support/routeAffordanceInventory.ts index 22462e1ab..4d176091b 100644 --- a/frontend/taskdeck-web/tests/e2e/support/routeAffordanceInventory.ts +++ b/frontend/taskdeck-web/tests/e2e/support/routeAffordanceInventory.ts @@ -70,9 +70,27 @@ export type AffordanceSelector = /** * What must become observably true after the affordance is activated. `url` and - * `response` pattern strings are regular-expression SOURCES; the `{boardId}`, - * `{metricsBoardId}` and `{openingMonthLabel}` tokens in any of these strings - * are substituted from the walk's runtime context before use. + * `response` pattern strings are regular-expression SOURCES. + * + * WHERE TOKENS ARE SUBSTITUTED, EXACTLY. `fillTokens` in + * `tests/e2e/route-affordances.spec.ts` recognises `{boardId}`, `{boardName}`, + * `{metricsBoardId}` and `{openingMonthLabel}`, and the walk calls it at five + * places only: `url.pathPattern`, `response.urlPattern`, `value.value`, + * `text.text` and `textChangedFrom.from` — plus the `name` of a `role` selector + * (see `AffordanceSelector` above). Everywhere else the string is used + * VERBATIM: `attribute`'s `selector`, `attribute` and `value`, the `enabled` + * selector, the `node` and `focus` test ids, and every `css` and `testId` + * selector value. A token written into one of those is matched literally, so + * the row quietly fails to find its node rather than reporting a bad token. + * + * Which token appears where today: `{boardId}` in three `url.pathPattern`s and + * nowhere else; `{metricsBoardId}` in three `response.urlPattern`s and in the + * `value.value` of the metrics board-select post-condition; `{boardName}` in + * one `role` selector name; and `{openingMonthLabel}` in exactly two places — + * the `textChangedFrom.from` of `calendar.previous-month` and the `text.text` + * of `calendar.next-month`. `{openingMonthLabel}` appears in no `url` or + * `response` pattern at all, which is what the sentence this replaced got + * wrong. * * A `response` consequence proves an endpoint answered, which on its own cannot * tell the click's own request apart from one the view already had in flight. @@ -109,6 +127,21 @@ export type AffordanceConsequence = * whose new text depends on the runtime clock (the calendar month label), * this is the strongest claim the inventory can make without re-implementing * the view's own date formatting inside the test. + * + * THE NEGATION IS NOT VACUOUS ON A MISSING NODE. The obvious worry about a + * negated matcher is that it passes when the selector matches nothing, which + * would make this kind assert nothing at all. Playwright does not behave that + * way: `expect(locator).not.toHaveText(x)` FAILS on a zero-element locator. + * Only `to.be.visible`, `to.be.hidden`, `to.be.attached`, `to.be.detached`, + * `to.be.in.viewport` and the `.array`/`to.have.count` expressions are + * special-cased for a missing node; every other expression falls through to + * `matches = options.isNot`, which the retry loop reads as "not satisfied + * yet", so the assertion polls to its timeout and reports `element(s) not + * found`. Measured on Playwright 1.62.1 — `playwright-core/lib/coreBundle.js` + * (`Frame._expectInternal` and `Frame.expect`) for the code, and a chromium + * probe for the behaviour. So no `toHaveCount` companion is needed, and + * `calendar.previous-month` stands on its own rather than leaning on + * `calendar.next-month`'s positive check one row later. */ | { kind: 'textChangedFrom'; selector: string; from: string } @@ -159,7 +192,13 @@ export interface RouteAffordance { export interface RouteEntry { /** Must equal a `name` in the real `router.getRoutes()` table. */ routeName: string - /** Concrete path the walk navigates to; `{boardId}` is substituted. */ + /** + * The route's entry path, written the way the patterns write theirs so the + * two read alike. DOCUMENTATION ONLY: nothing reads this field — the walk + * navigates with its own literal `page.goto` calls and the coverage guard + * compares route NAMES — so `{boardId}` here is never substituted by + * anything. Changing it changes no assertion. + */ entryPath: string /** Two to five affordances. Fewer says nothing; more is a second slice. */ affordances: RouteAffordance[] @@ -182,6 +221,12 @@ export const ROUTE_AFFORDANCE_INVENTORY: RouteEntry[] = [ selector: { kind: 'testId', value: 'paper-home-capture-input' }, source: 'src/views/paper/PaperHomeView.vue:609', precondition: 'session', + // NO MOUNT READ CAN SETTLE THIS. The defence here is the method, not a + // post-condition: PaperHomeView's `onMounted` fetches the home summary + // and nothing else (PaperHomeView.vue:381-384), so a POST to + // /api/capture/items can only have been raised by this submit. The walk + // consumes that summary GET before it starts, but for first paint, not + // to isolate this row. consequence: { kind: 'response', method: 'POST', urlPattern: '/api/capture/items$' }, status: { activate: true }, }, @@ -374,6 +419,11 @@ export const ROUTE_AFFORDANCE_INVENTORY: RouteEntry[] = [ selector: { kind: 'css', value: '[data-action="reject"]' }, source: 'src/views/paper/inbox/PaperTriageTable.vue:838', precondition: 'seeded-capture', + // NO MOUNT READ CAN SETTLE THIS. Same defence as the Home quick + // capture, doubly so: PaperInboxView's `onMounted` + // (PaperInboxView.vue:504-508) issues no request at all, and the + // pattern is anchored to a POST on /archive, which nothing fetches. + // Only this click can raise it. consequence: { kind: 'response', method: 'POST', urlPattern: '/api/capture/items/[^/]+/archive$' }, status: { activate: true }, }, @@ -398,6 +448,11 @@ export const ROUTE_AFFORDANCE_INVENTORY: RouteEntry[] = [ selector: { kind: 'css', value: '[data-testid="decision-apply"][data-apply-phase="approve"]' }, source: 'src/views/paper/review/ReviewDecisionRail.vue:252', precondition: 'seeded-proposal', + // NO MOUNT READ CAN SETTLE THIS. PaperReviewView's `onMounted` + // (PaperReviewView.vue:2546-2558) loads the board options and the + // proposals and starts a queue poll, all GETs; nothing POSTs on mount + // or on the poll. The pattern is anchored to /approve, phase 1 of the + // ADR-0003 gate, which this control alone reaches. consequence: { kind: 'response', method: 'POST', urlPattern: '/automation/proposals/[^/]+/approve$' }, status: { activate: true }, }, @@ -511,10 +566,18 @@ export const ROUTE_AFFORDANCE_INVENTORY: RouteEntry[] = [ selector: { kind: 'role', role: 'button', name: 'Next month' }, source: 'src/views/CalendarView.vue:227', precondition: 'session', + // THE MOUNT READ IS ALREADY SPENT. Like the row above, this waits on a + // GET CalendarView also issues in `onMounted` (CalendarView.vue:178-179) + // — but the walk consumed that read before `calendar.previous-month`, + // and that row's own wait consumed the next one, so by the time this + // row arms only a third request can settle it. + // // Walked immediately after `calendar.previous-month`, which is why the // post-condition can name an exact string: stepping forward from the // previous month lands back on the label the route opened with, with no - // date formatting re-implemented in the test. + // date formatting re-implemented in the test. The label is rendered from + // `viewDate` (CalendarView.vue:36-39), so that string is the independent + // half here just as the negated one is on the previous row. consequence: { kind: 'response', method: 'GET', urlPattern: '/api/workspace/calendar' }, postCondition: { kind: 'text', From c5bbfd5b25e5443814bf859fca829949637013ad Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 20:52:47 +0100 Subject: [PATCH 2/4] test(guards): record why the assertEnabled branch is dead, not a choice Follow-up 3 from the #2679 review, guard half. `AffordanceStatus`'s guarded variant declares `assertEnabled: true` as a required literal, so the checker's `!== true` throw cannot fire for a real inventory row and assertion 8's docblock was describing a per-row decision the type forecloses. Keep the read as belt-and-braces and say so at the site: the type is the enforcement, and widening the field to `boolean` is what would make the check live and owe it a canary. That also answers why this branch has no canary while the consequence branch does. A `response` consequence on a guarded row type-checks -- assertion 9 builds exactly that row with no cast -- so the shape it rejects is writable. The `assertEnabled` shape is not reachable without a cast, so a canary would prove the checker against something the type already makes unconstructible. Also note that offering `textChangedFrom` to guarded rows is safe: Playwright fails a zero-element `not.toHaveText` with `element(s) not found`, so every kind classified `element` asserts something about a node that exists. Refs #2682 --- .../guards/routeAffordanceCoverage.spec.ts | 38 +++++++++++++++++-- 1 file changed, 35 insertions(+), 3 deletions(-) diff --git a/frontend/taskdeck-web/src/tests/guards/routeAffordanceCoverage.spec.ts b/frontend/taskdeck-web/src/tests/guards/routeAffordanceCoverage.spec.ts index c16b45fb6..458ba5fb6 100644 --- a/frontend/taskdeck-web/src/tests/guards/routeAffordanceCoverage.spec.ts +++ b/frontend/taskdeck-web/src/tests/guards/routeAffordanceCoverage.spec.ts @@ -132,6 +132,17 @@ function assertGuardedRowsAreAssertableWithoutActivation(inventory: RouteEntry[] ) } + // DEAD AT RUNTIME, AND THE TYPE IS WHY. `AffordanceStatus`'s + // `guarded-not-activated` variant declares `assertEnabled: true` as a + // REQUIRED LITERAL, so a row that reaches this line has already been + // proved to satisfy it by `npm run typecheck`; this `!== true` can never + // be true for a real inventory row. That is the difference from the + // consequence check above, whose bad shape the type DOES permit (a + // `response` consequence on a guarded row type-checks — assertion 9 + // builds exactly that row with no cast) and which therefore earns a + // canary. This branch has none because there is no cast-free way to + // reach it. It is kept as belt-and-braces: it is the check that starts + // doing work the moment the field is widened to `boolean`. if (status.assertEnabled !== true) { throw new Error( `guarded row '${affordance.id}' (${entry.routeName}) must set assertEnabled: true. ` @@ -274,9 +285,30 @@ describe('route affordance inventory coverage (GH-1949 AC4)', () => { * The walk asserts `guarded-not-activated` rows and never clicks them, so a * `response` consequence copied onto one from an activated sibling would * assert NOTHING and still count as walked — the row would be reported as - * covered while proving only that the control rendered. `assertEnabled` is - * likewise load-bearing: the walk reads it to decide whether the declined - * control has to be a live choice. + * covered while proving only that the control rendered. + * + * THE TWO HALVES ARE NOT ENFORCED THE SAME WAY. The consequence half is a + * live check: a `response` consequence on a guarded row type-checks, so the + * shape it rejects is one someone can actually write, which is why assertion + * 9 below carries a canary for it. The `assertEnabled` half is NOT: the + * guarded status variant declares `assertEnabled: true` as a required + * literal, so `npm run typecheck` has already rejected every row this check + * could catch, and its `!== true` throw cannot fire for a real row. Both the + * checker here and the walk's `if (affordance.status.assertEnabled)` in + * `tests/e2e/route-affordances.spec.ts` are kept as belt-and-braces, and + * each says so at its own site. Neither describes a per-row choice — the + * type forecloses the choice. Widening the field to `boolean` is what would + * make them live, and would then owe this assertion a canary of its own. + * + * ELEMENT CONSEQUENCES ARE SAFE TO OFFER A GUARDED ROW, INCLUDING THE + * NEGATED ONE. The permitted list below includes `textChangedFrom`, whose + * assertion is `not.toHaveText`. A negated matcher would be worthless here + * if it passed on a selector that matched nothing, but Playwright fails a + * zero-element `not.toHaveText` with `element(s) not found` rather than + * passing it — measured on 1.62.1 and recorded in full on the + * `textChangedFrom` kind in `tests/e2e/support/routeAffordanceInventory.ts`. + * So every kind classified `element` really does assert something about a + * node that exists. */ it('gives every guarded-not-activated row an element consequence and assertEnabled', () => { const guardedRows = ROUTE_AFFORDANCE_INVENTORY.flatMap((entry) => From b458e1cb07c87f4252744865cafc66597e55c1b5 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 20:52:56 +0100 Subject: [PATCH 3/4] test(e2e): park the four mount-read outcomes before goto Follow-up 5 from the #2679 review, plus the walk-side halves of 3 and 4. The calendar, metrics, notifications and home mount reads all armed a `page.waitForResponse` before `page.goto` and awaited it only on the next line. If the `goto` threw, nothing awaited the wait and its 30 s rejection landed on the worker with no test to own it. Extract `readOnMount`, which parks the outcome the moment the wait is armed -- the pattern `activate` already uses for row waits -- and route all four through it. The success path is byte-for-byte the same semantics: armed before, awaited after, asserted 2xx. Measured before the change, against a `goto` to a refusing port: the shipped shape leaked exactly one unhandled rejection ("page.waitForResponse: Timeout exceeded"), the parked shape leaked none, and both reported the same navigation error. Also record at their own sites that `if (affordance.status.assertEnabled)` is dead by construction, and that `textChangedFrom`'s `not.toHaveText` fails rather than passes on a zero-element locator -- so it needs no `toHaveCount` companion and `calendar.previous-month` proves its own move. Refs #2682 --- .../tests/e2e/route-affordances.spec.ts | 93 ++++++++++++++++--- 1 file changed, 80 insertions(+), 13 deletions(-) diff --git a/frontend/taskdeck-web/tests/e2e/route-affordances.spec.ts b/frontend/taskdeck-web/tests/e2e/route-affordances.spec.ts index 9f76cd6d6..aa97a18bf 100644 --- a/frontend/taskdeck-web/tests/e2e/route-affordances.spec.ts +++ b/frontend/taskdeck-web/tests/e2e/route-affordances.spec.ts @@ -53,6 +53,10 @@ * A `response` row's wait is armed BEFORE the act and its rejection is parked * immediately (see `activate`), so a control that never becomes actionable * reports its own actionability failure rather than an unhandled rejection. + * The mount reads those routes consume first are armed before `page.goto` for + * the same reason and parked the same way (see `readOnMount`), so a `goto` that + * throws reports its own failure instead of trailing a stray 30 s + * `waitForResponse` rejection that lands on the worker with no test to own it. * * COMPLETENESS. Every block ends with `assertBlockCompleted`, which proves the * declared `WALK_PLAN` still names exactly the inventory's `activate: true` and @@ -69,7 +73,7 @@ * run summary for four passing blocks is what closes that. */ -import { expect, test, type Locator, type Page } from '@playwright/test' +import { expect, test, type Locator, type Page, type Response } from '@playwright/test' import { registerAndAttachSession, type AuthResult } from './support/authSession' import { createBoardWithColumn } from './support/boardHelpers' import { @@ -345,6 +349,18 @@ async function expectConsequence( return case 'textChangedFrom': + // THE ONLY NEGATED MATCHER IN THIS SWITCH, AND IT IS NOT VACUOUS ON A + // MISSING NODE. `expect(locator).not.toHaveText(x)` FAILS when the + // locator resolves to zero elements — it does not quietly pass the way + // `not.toBeVisible()` does. Playwright special-cases only + // visible/hidden/attached/detached/in-viewport and the array expressions + // for a missing node; `to.have.text` falls through to "not satisfied + // yet", polls to the timeout and reports `element(s) not found`. + // Confirmed against Playwright 1.62.1; the code references and the + // chromium probe are recorded on the `textChangedFrom` kind in + // `support/routeAffordanceInventory.ts`. So this needs no `toHaveCount` + // companion, and `calendar.previous-month` proves its own move rather + // than leaning on `calendar.next-month` one row later. await expect( scope.locator(consequence.selector).first(), `${id} must move ${consequence.selector} off '${consequence.from}'`, @@ -460,6 +476,16 @@ async function assertReachableButNotActivated( const target = locate(page, affordance.selector, context) await expect(target, `${id} (${affordance.source}) must be present`).toBeVisible() await expectConsequence(page, page, id, affordance.consequence, context) + // THE TYPE IS THE ENFORCEMENT; THIS READ CANNOT TAKE ITS FALSE BRANCH. + // `AffordanceStatus`'s guarded variant declares `assertEnabled: true` as a + // REQUIRED LITERAL, so no row reaching here can have it false or absent and + // the `else` is unreachable at runtime. It is kept as belt-and-braces: it + // documents at the point of use that the enabled check is conditional on the + // flag, and it is what starts working if the field is ever widened to + // `boolean`. Read it as "the type already guaranteed this", not as a choice + // rows make. The guard's matching read (assertion 8 in + // `src/tests/guards/routeAffordanceCoverage.spec.ts`) is dead for the same + // reason and says so there. if (affordance.status.assertEnabled) { await expect( target, @@ -469,6 +495,43 @@ async function assertReachableButNotActivated( guardedIds.add(id) } +/** + * Navigate to a route and consume the read it issues on MOUNT. + * + * WHY THE MOUNT READ IS CONSUMED AT ALL. Four routes here (calendar, metrics, + * notifications, home) fetch on mount, and three of them own `response` rows + * waiting on the SAME endpoint. A wait armed after navigation could be settled + * by the mount's own request, so a dead control would still pass. Awaiting the + * mount read here takes it off the wire before any row arms its own wait. + * + * WHY THE OUTCOME IS PARKED. The wait must be armed BEFORE `page.goto` — that + * is the whole point, the response can arrive during the navigation — but a + * `goto` that throws would then leave the `waitForResponse` promise with + * nothing awaiting it, and its 30 s default rejection would surface later as an + * unhandled rejection attributed to the worker rather than as the navigation + * failure that actually happened. Parking the outcome the moment it is armed, + * exactly as `activate` does for row waits, keeps the navigation's own error + * first while still failing on a mount read that never arrives. The success + * path is unchanged: armed before, awaited after, asserted 2xx. + */ +async function readOnMount( + page: Page, + label: string, + matches: (response: Response) => boolean, + navigate: () => Promise, +): Promise { + const settledRead = page.waitForResponse(matches).then( + (response) => ({ settled: 'fulfilled' as const, response }), + (reason: unknown) => ({ settled: 'rejected' as const, reason }), + ) + await navigate() + const outcome = await settledRead + if (outcome.settled === 'rejected') { + throw outcome.reason instanceof Error ? outcome.reason : new Error(String(outcome.reason)) + } + await assertOk(outcome.response, label) +} + function uniqueSeed(): string { return `${Date.now()}-${Math.floor(Math.random() * 1_000_000)}` } @@ -539,12 +602,13 @@ test('walks the board-seeded route affordances', async ({ page, request }) => { // treatment as metrics and notifications: consume the mount read first. The // month label is then the independent half, declared as a postCondition on // both rows rather than asserted ad hoc here. - const mountCalendarRead = page.waitForResponse( + await readOnMount( + page, + 'calendar read issued on mount', (response) => response.request().method() === 'GET' && /\/api\/workspace\/calendar/.test(response.url()), + () => page.goto('/workspace/calendar'), ) - await page.goto('/workspace/calendar') - await assertOk(await mountCalendarRead, 'calendar read issued on mount') const monthLabel = page.locator('.paper-calendar__month-label') await expect(monthLabel).toBeVisible() @@ -563,13 +627,14 @@ test('walks the board-seeded route affordances', async ({ page, request }) => { // mount read and a dead select would still pass. Two defences: consume the // mount read here, and pin the rows to the OTHER of the two boards this block // seeded, so the mount's URL can never match them. - const mountMetricsRead = page.waitForResponse( + await readOnMount( + page, + 'metrics read issued on mount', (response) => response.request().method() === 'GET' && /\/api\/metrics\/boards\/[a-f0-9-]+\?from=/.test(response.url()), + () => page.goto('/workspace/metrics'), ) - await page.goto('/workspace/metrics') - await assertOk(await mountMetricsRead, 'metrics read issued on mount') const boardSelect = page.locator('#board-select') await expect(boardSelect, 'the board select must settle on its auto-selection first') @@ -606,12 +671,13 @@ test('walks the board-seeded route affordances', async ({ page, request }) => { // it first: only a genuinely new request can settle the rows below. Neither // row can carry a post-condition, because the filtered and unfiltered empty // states render identical copy for a user with no notifications. - const mountNotificationsRead = page.waitForResponse( + await readOnMount( + page, + 'notifications read issued on mount', (response) => response.request().method() === 'GET' && /\/api\/notifications(\?|$)/.test(response.url()), + () => page.goto('/workspace/notifications'), ) - await page.goto('/workspace/notifications') - await assertOk(await mountNotificationsRead, 'notifications read issued on mount') await expect(page.getByRole('button', { name: 'Refresh' })).toBeVisible() await activate(page, 'notifications.refresh', context) @@ -636,12 +702,13 @@ test('walks the Home quick capture and the Inbox triage affordances', async ({ p const captureText = `Route walk boardless capture ${seed}` // ── /workspace/home — await the summary read rather than sleeping ──────── - const homeSummary = page.waitForResponse( + await readOnMount( + page, + 'Home summary read on first paint', (response) => response.request().method() === 'GET' && /\/api\/workspace\/home$/.test(response.url()), + () => page.goto('/workspace/home'), ) - await page.goto('/workspace/home') - await assertOk(await homeSummary, 'Home summary read on first paint') await expect(page.getByTestId('paper-home')).toBeVisible() // The Home quick capture posts `boardId: null`, so it seeds the boardless From 4b1a20003c536e5103b3812daf0a6348dfe87563 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 21:08:15 +0100 Subject: [PATCH 4/4] test(e2e): say what actually bounds the mount reads, and complete the token census Round 2 wording-only fixes on the #2682 follow-ups. No executable change; no bound added. 1. The header and `readOnMount` both claimed an unparked mount read would reject after "30 s". False under this config: the calls pass no `timeout` and nothing supplies a default, so the wait has no event timeout at all. Chain read in the installed tree -- `playwright/lib/index.js:259` defaults the `actionTimeout` fixture to 0 and `playwright.config.ts` never sets it (only `timeout: 45_000` and `expect.timeout: 8_000`, and nothing under `tests/` calls `setDefaultTimeout`); `:349` assigns `_defaultContextTimeout = actionTimeout || 0`; `coreBundle.js:57160-57169` returns that 0 rather than falling through to the library default; and `:58412` arms a timer only `if (timeout)`, which 0 is not. So such a read HANGS to the test timeout (90 s board-seeded, 45 s Home/Inbox) and reports "Test timeout exceeded". Parking still earns its place: an unawaited wait is rejected at page close with a `TargetClosedError` (`coreBundle.js:61278-61279`, `:61265-61267`), with no test left to own it. Also record that `activate` caps its row waits at 15 s while these do not, and that bounding them is tracked on #2682. 2. "`{boardId}` ... nowhere else" was false: it also appears in the `workspace-board` entry's `entryPath`, which this same PR documents as never substituted. Name that occurrence. 3. The verbatim-usage list omitted `value.selector`, `text.selector` and `textChangedFrom.selector`. Added, so the list is complete rather than illustrative. Refs #2682 --- .../tests/e2e/route-affordances.spec.ts | 40 +++++++++++++++---- .../e2e/support/routeAffordanceInventory.ts | 21 ++++++---- 2 files changed, 45 insertions(+), 16 deletions(-) diff --git a/frontend/taskdeck-web/tests/e2e/route-affordances.spec.ts b/frontend/taskdeck-web/tests/e2e/route-affordances.spec.ts index aa97a18bf..2451ea4e4 100644 --- a/frontend/taskdeck-web/tests/e2e/route-affordances.spec.ts +++ b/frontend/taskdeck-web/tests/e2e/route-affordances.spec.ts @@ -55,8 +55,11 @@ * reports its own actionability failure rather than an unhandled rejection. * The mount reads those routes consume first are armed before `page.goto` for * the same reason and parked the same way (see `readOnMount`), so a `goto` that - * throws reports its own failure instead of trailing a stray 30 s - * `waitForResponse` rejection that lands on the worker with no test to own it. + * throws reports its own failure instead of trailing a `waitForResponse` + * settlement with no test left to own it. Note the asymmetry in what BOUNDS the + * two: `activate` passes `{ timeout: 15_000 }`, while the mount reads pass no + * timeout and this config gives them none, so they are bounded only by the test + * timeout. `readOnMount` explains that; bounding them is tracked on #2682. * * COMPLETENESS. Every block ends with `assertBlockCompleted`, which proves the * declared `WALK_PLAN` still names exactly the inventory's `activate: true` and @@ -507,12 +510,33 @@ async function assertReachableButNotActivated( * WHY THE OUTCOME IS PARKED. The wait must be armed BEFORE `page.goto` — that * is the whole point, the response can arrive during the navigation — but a * `goto` that throws would then leave the `waitForResponse` promise with - * nothing awaiting it, and its 30 s default rejection would surface later as an - * unhandled rejection attributed to the worker rather than as the navigation - * failure that actually happened. Parking the outcome the moment it is armed, - * exactly as `activate` does for row waits, keeps the navigation's own error - * first while still failing on a mount read that never arrives. The success - * path is unchanged: armed before, awaited after, asserted 2xx. + * nothing awaiting it. Parking the outcome the moment it is armed, exactly as + * `activate` does for row waits, keeps the navigation's own error first while + * still failing on a mount read that never arrives. The success path is + * unchanged: armed before, awaited after, asserted 2xx. + * + * WHAT AN UNPARKED WAIT WOULD ACTUALLY DO HERE — NOT TIME OUT. These calls pass + * no `timeout`, and under this repo's config nothing supplies a default one, so + * the wait has NO event timeout of its own. The chain, read in the installed + * tree: `playwright/lib/index.js:259` defaults the `actionTimeout` fixture to + * `0` and `playwright.config.ts` never sets it (it sets only `timeout: 45_000` + * and `expect.timeout: 8_000`, and nothing under `tests/` calls + * `setDefaultTimeout`); `playwright/lib/index.js:349` then assigns + * `_defaultContextTimeout = actionTimeout || 0`; `coreBundle.js:57160-57169` + * returns that `0` rather than falling through to Playwright's own default; + * and `coreBundle.js:58412` arms a timer only `if (timeout)`, which `0` is not. + * So a mount read whose response never arrives does not reject after 30 s — it + * HANGS until the test timeout (90 s in the board-seeded walk, 45 s in the + * Home/Inbox block) and reports "Test timeout exceeded". Parking still earns + * its place: an unawaited wait is rejected at page close with a + * `TargetClosedError` (`coreBundle.js:61278-61279` and `:61265-61267`), and by + * then the test that would have owned it is over. + * + * NOT BOUNDED LIKE `activate`. `activate` caps its row waits at + * `{ timeout: 15_000 }` precisely so a dead control does not burn the shared + * E2E Smoke budget; these mount reads take no such cap and are left to the test + * timeout. That asymmetry is deliberate for now, not an oversight — giving them + * their own bound is a behavioural change and is tracked on #2682. */ async function readOnMount( page: Page, diff --git a/frontend/taskdeck-web/tests/e2e/support/routeAffordanceInventory.ts b/frontend/taskdeck-web/tests/e2e/support/routeAffordanceInventory.ts index 4d176091b..d89ccda56 100644 --- a/frontend/taskdeck-web/tests/e2e/support/routeAffordanceInventory.ts +++ b/frontend/taskdeck-web/tests/e2e/support/routeAffordanceInventory.ts @@ -78,15 +78,20 @@ export type AffordanceSelector = * places only: `url.pathPattern`, `response.urlPattern`, `value.value`, * `text.text` and `textChangedFrom.from` — plus the `name` of a `role` selector * (see `AffordanceSelector` above). Everywhere else the string is used - * VERBATIM: `attribute`'s `selector`, `attribute` and `value`, the `enabled` - * selector, the `node` and `focus` test ids, and every `css` and `testId` - * selector value. A token written into one of those is matched literally, so - * the row quietly fails to find its node rather than reporting a bad token. + * VERBATIM — the complete list: `attribute`'s `selector`, `attribute` and + * `value`; the `enabled` selector; the `selector` of `value`, `text` and + * `textChangedFrom` (only their `value`/`text`/`from` payloads are filled, not + * the node they are read from); the `node` and `focus` test ids; and every + * `css` and `testId` selector value. A token written into one of those is + * matched literally, so the row quietly fails to find its node rather than + * reporting a bad token. * - * Which token appears where today: `{boardId}` in three `url.pathPattern`s and - * nowhere else; `{metricsBoardId}` in three `response.urlPattern`s and in the - * `value.value` of the metrics board-select post-condition; `{boardName}` in - * one `role` selector name; and `{openingMonthLabel}` in exactly two places — + * Which token appears where today: `{boardId}` in three `url.pathPattern`s, and + * once more in the `workspace-board` entry's `entryPath`, where nothing + * substitutes it (see `RouteEntry.entryPath` below); `{metricsBoardId}` in + * three `response.urlPattern`s and in the `value.value` of the metrics + * board-select post-condition; `{boardName}` in one `role` selector name; and + * `{openingMonthLabel}` in exactly two places — * the `textChangedFrom.from` of `calendar.previous-month` and the `text.text` * of `calendar.next-month`. `{openingMonthLabel}` appears in no `url` or * `response` pattern at all, which is what the sentence this replaced got