HOMELAB-1885: fix(cockpit-e2e): wait for the SPA to mount before measuring the honest row #601

Merged

aaron merged 1 commit from plane/HOMELAB-1885-cockpit-e2e into live 2026-10-03 18:05:33 +00:00

Conversation 1 Commits 1 Files changed 1 +12

aaron commented 2026-10-03 17:42:39 +00:00

Owner

Copy link

Root cause

e2e/telemetry.spec.ts:275 ("the honest row reads as a sentence at 390px, outside any scroller") is the only measurement in the suite taken straight off page.goto('/') — every other DOM-touching assertion awaits a locator first (telemetry.spec.ts:58,136,263), and responsive.spec.ts inserts waitForTimeout(300).

page.goto resolves on load, but the cockpit mounts after it. The build is adapter-static with ssr = false: build/index.html is an empty body plus the SvelteKit bootstrap script, whose entry (entry/start.*.js) is a dynamic import. So at load the body holds nothing but the inline __sveltekit_* script — document.querySelector('[data-testid="wave-row-note"]') (and [data-pane="fleet"]) return null, getBoundingClientRect() never runs, and line 295 receives null.

Not a product regression: the component renders the note (line 248 of wave-view.svelte), and it renders at 390px inside the pane. The spec was green when LD3 merged — PR #589 cockpit-e2e success 3m0s — and red on a slower runner: run #1839 (PR #593, a longhorn retention change that touches no cockpit code) cockpit-e2e failure 5m4s, failing on attempt #1 and retry #1. The window is the runner's mount latency, so any unrelated PR inherits it.

What changed and why

lab-director/cockpit/web/e2e/telemetry.spec.ts — two awaited locators added between goto and the measurement:

await expect(page.locator('[data-pane="fleet"]')).toBeVisible();
await expect(page.getByTestId('wave-row-note')).toBeVisible();

The assertions below are unchanged and not weakened: the note must still exist and its own measured box must sit inside the pane's box at 390px, outside any scroller. The waits only make the measurement mean "the rendered row" instead of "whatever the DOM held at load" — the same discipline the sibling test in that describe already uses.

Evidence (local, live @ b3108c2 + this patch)

Reproduced the CI window headless by throttling the CPU 20x via CDP, then running both legs of the sequence:

PROBE unwaited={"note":null,"pane":null} fixed={"noteLeft":108,"noteRight":357,"paneLeft":16,"paneRight":374}

The un-waited read is exactly the CI symptom (both boxes null); the fixed read measures the note inside the pane at 390px. Then, with the probe removed:

• npx playwright test e2e/telemetry.spec.ts → 12 passed
• npx playwright test (full suite, CI=true) → 76 passed (1.1m) — including telemetry.spec.ts:275

Blocker note

This blocks the infra-core merge queue: cockpit-e2e runs only on PRs (not on live pushes), so the failure is charged to whichever unrelated PR is next. Branch is off current origin/live only, one file, no rebase of other branches.

## Root cause `e2e/telemetry.spec.ts:275` ("the honest row reads as a sentence at 390px, outside any scroller") is the **only** measurement in the suite taken straight off `page.goto('/')` — every other DOM-touching assertion awaits a locator first (`telemetry.spec.ts:58,136,263`), and `responsive.spec.ts` inserts `waitForTimeout(300)`. `page.goto` resolves on `load`, but the cockpit mounts **after** it. The build is `adapter-static` with `ssr = false`: `build/index.html` is an empty body plus the SvelteKit bootstrap script, whose entry (`entry/start.*.js`) is a **dynamic import**. So at `load` the body holds nothing but the inline `__sveltekit_*` script — `document.querySelector('[data-testid="wave-row-note"]')` (and `[data-pane="fleet"]`) return `null`, `getBoundingClientRect()` never runs, and line 295 receives `null`. Not a product regression: the component renders the note (line 248 of `wave-view.svelte`), and it renders at 390px inside the pane. The spec was **green when LD3 merged** — PR #589 cockpit-e2e `success 3m0s` — and red on a *slower* runner: run #1839 (PR #593, a longhorn retention change that touches no cockpit code) cockpit-e2e `failure 5m4s`, failing on attempt #1 and retry #1. The window is the runner's mount latency, so any unrelated PR inherits it. ## What changed and why `lab-director/cockpit/web/e2e/telemetry.spec.ts` — two awaited locators added between `goto` and the measurement: ```ts await expect(page.locator('[data-pane="fleet"]')).toBeVisible(); await expect(page.getByTestId('wave-row-note')).toBeVisible(); ``` The assertions below are **unchanged and not weakened**: the note must still exist and its own measured box must sit inside the pane's box at 390px, outside any scroller. The waits only make the measurement mean "the rendered row" instead of "whatever the DOM held at `load`" — the same discipline the sibling test in that describe already uses. ## Evidence (local, live @ b3108c2 + this patch) Reproduced the CI window headless by throttling the CPU 20x via CDP, then running both legs of the sequence: ``` PROBE unwaited={"note":null,"pane":null} fixed={"noteLeft":108,"noteRight":357,"paneLeft":16,"paneRight":374} ``` The un-waited read is exactly the CI symptom (both boxes null); the fixed read measures the note inside the pane at 390px. Then, with the probe removed: - `npx playwright test e2e/telemetry.spec.ts` → **12 passed** - `npx playwright test` (full suite, `CI=true`) → **76 passed (1.1m)** — including `telemetry.spec.ts:275` ## Blocker note This blocks the infra-core merge queue: cockpit-e2e runs only on PRs (not on `live` pushes), so the failure is charged to whichever unrelated PR is next. Branch is off current `origin/live` only, one file, no rebase of other branches.

aaron added 1 commit 2026-10-03 17:42:39 +00:00

HOMELAB-1885: fix(cockpit-e2e): wait for the SPA to mount before measuring the honest row a72435a874

aaron commented 2026-10-03 18:02:28 +00:00

Author

Owner

Copy link

Review verdict — HOMELAB-1885 (PR #601)

Head sha reviewed: a72435a87453bfe851ea3eb007c543cd32ec0a18 (base live)
Changed: lab-director/cockpit/web/e2e/telemetry.spec.ts only, +12/-0.
Verdict: APPROVED — no blocker findings. Harmless class (test-only, no runtime/manifest change).

Reviewed in a fresh context against the ticket's acceptance criteria, from primary sources only (PR diff, ticket, live contents of e2e/telemetry.spec.ts, svelte.config.js, src/routes/+layout.ts, src/lib/components/dashboard/wave-view.svelte, playwright.config.ts, Forgejo status API).

Correctness vs acceptance criteria

• Scope 1 — product bug or test bug? Test-side. adapter-static + fallback: index.html and export const ssr = false mean build/index.html is an empty body plus the inline SvelteKit bootstrap, whose app entry is a dynamic import. page.goto resolves on load, so document.querySelector('[data-testid="wave-row-note"]') / [data-pane="fleet"] return null, getBoundingClientRect() never runs, and line 295 gets null — exactly the CI symptom. The component itself renders the row (wave-view.svelte:248), so this is not a product regression.
• Scope 2 — fixed in the correct layer. Change is confined to the spec; no product, config, or manifest edit. Branch is plane/HOMELAB-1885-cockpit-e2e → live.
• Scope 3 — CI green on the fix PR. Verified independently on head a72435a8: runs #1859/#1860/#1861, 12/12 jobs success, incl. cockpit-e2e success 5m50s (slower than the 5m4s run that failed — the fix does not depend on runner speed), cockpit-test 2m46s, dispatcher-test 2m48s, build-and-push 3m49s, terraform-validate 4m49s.
• Ticket Verify row ("passes on subsequent PRs branched from live") is inherently post-merge and is what landing this PR unblocks; it cannot be evidenced by the diff itself.

Fix correctness

Two awaited locators are inserted between goto and the measurement:

await expect(page.locator('[data-pane="fleet"]')).toBeVisible();
await expect(page.getByTestId('wave-row-note')).toBeVisible();

This matches the discipline the sibling test in the same describe already uses (telemetry.spec.ts:263 on live), and the diff is additions only — no assertion line is touched, removed, or relaxed.

• Not weakened or vacuous: a permanently-absent note now fails at the new expect rather than later, so a real regression surfaces earlier, not later. A clipped/off-screen note still passes toBeVisible (Playwright ignores scroll clipping) and must then satisfy the unchanged out-of-scroller box assertion, so clipping is not masked either.
• Waits are auto-retrying with the config's 10s expect timeout inside a 45s test timeout against a locally served static asset — ample margin.

Findings

No blocker findings were raised: the review produced one concern and two suggestions, none of which block.

• Concern (minor): getByTestId('wave-row-note') is strict-mode; if the DOM ever renders more than one such node the wait would throw rather than pick the first. CI green implies a single node today.
• Suggestion: if this measurement ever flakes again, await layout/font stability (poll the measured box or document.fonts.ready) rather than adding another visibility wait.
• Suggestion: selector-style drift — getByTestId here vs [data-testid=...] in the page.evaluate below; cosmetic only.

Concern/suggestion findings do not block. Proceeding to merge (repo convention: merge commit).

## Review verdict — HOMELAB-1885 (PR #601) **Head sha reviewed:** `a72435a87453bfe851ea3eb007c543cd32ec0a18` (base `live`) **Changed:** `lab-director/cockpit/web/e2e/telemetry.spec.ts` only, +12/-0. **Verdict: APPROVED — no blocker findings.** Harmless class (test-only, no runtime/manifest change). Reviewed in a fresh context against the ticket's acceptance criteria, from primary sources only (PR diff, ticket, `live` contents of `e2e/telemetry.spec.ts`, `svelte.config.js`, `src/routes/+layout.ts`, `src/lib/components/dashboard/wave-view.svelte`, `playwright.config.ts`, Forgejo status API). ### Correctness vs acceptance criteria - **Scope 1 — product bug or test bug?** Test-side. `adapter-static` + `fallback: index.html` and `export const ssr = false` mean `build/index.html` is an empty body plus the inline SvelteKit bootstrap, whose app entry is a dynamic import. `page.goto` resolves on `load`, so `document.querySelector('[data-testid="wave-row-note"]')` / `[data-pane="fleet"]` return `null`, `getBoundingClientRect()` never runs, and line 295 gets `null` — exactly the CI symptom. The component itself renders the row (`wave-view.svelte:248`), so this is not a product regression. - **Scope 2 — fixed in the correct layer.** Change is confined to the spec; no product, config, or manifest edit. Branch is `plane/HOMELAB-1885-cockpit-e2e` → `live`. - **Scope 3 — CI green on the fix PR.** Verified independently on head `a72435a8`: runs #1859/#1860/#1861, 12/12 jobs success, incl. `cockpit-e2e success 5m50s` (slower than the 5m4s run that failed — the fix does not depend on runner speed), `cockpit-test 2m46s`, `dispatcher-test 2m48s`, `build-and-push 3m49s`, `terraform-validate 4m49s`. - **Ticket Verify row** ("passes on subsequent PRs branched from `live`") is inherently post-merge and is what landing this PR unblocks; it cannot be evidenced by the diff itself. ### Fix correctness Two awaited locators are inserted between `goto` and the measurement: ```ts await expect(page.locator('[data-pane="fleet"]')).toBeVisible(); await expect(page.getByTestId('wave-row-note')).toBeVisible(); ``` This matches the discipline the sibling test in the same `describe` already uses (`telemetry.spec.ts:263` on `live`), and the diff is additions only — no assertion line is touched, removed, or relaxed. - Not weakened or vacuous: a permanently-absent note now fails at the new `expect` rather than later, so a real regression surfaces earlier, not later. A clipped/off-screen note still passes `toBeVisible` (Playwright ignores scroll clipping) and must then satisfy the unchanged out-of-scroller box assertion, so clipping is not masked either. - Waits are auto-retrying with the config's 10s `expect` timeout inside a 45s test timeout against a locally served static asset — ample margin. ### Findings No blocker findings were raised: the review produced one concern and two suggestions, none of which block. - Concern (minor): `getByTestId('wave-row-note')` is strict-mode; if the DOM ever renders more than one such node the wait would throw rather than pick the first. CI green implies a single node today. - Suggestion: if this measurement ever flakes again, await layout/font stability (poll the measured box or `document.fonts.ready`) rather than adding another visibility wait. - Suggestion: selector-style drift — `getByTestId` here vs `[data-testid=...]` in the `page.evaluate` below; cosmetic only. Concern/suggestion findings do not block. Proceeding to merge (repo convention: merge commit).

aaron merged commit a33035367c into live 2026-10-03 18:05:33 +00:00

aaron referenced this pull request from a commit 2026-10-03 18:05:35 +00:00

Merge pull request 'HOMELAB-1885: fix(cockpit-e2e): wait for the SPA to mount before measuring the honest row' (#601) from plane/HOMELAB-1885-cockpit-e2e into live

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

operator-review

Requires human operator review before merge — dangerous class (RBAC, secrets, network policy, Terraform) or explicitly gated. Never auto-merge.

No labels

operator-review

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Homelab/infra-core!601

No description provided.