fix(extension): repair rotted e2e sidepanel selectors and run them in CI - #690
Conversation
All 4 e2e tests failed because their selectors described an older UI. The
sidepanel itself renders correctly.
- `getByText("Pilo Settings")` never matched: the header renders "Tabstack
Pilo" (h1) and the view renders "Settings" (h2) as separate nodes, so no
single node contains that string.
- `getByText("Back to Chat")` never matched: the button is labelled "Back to
chat", and getByText is case-sensitive.
- `getByTestId("settings-button")` never matched: no such testid exists. The
button is an icon button identified by its aria-label, "Open settings".
Switched to getByRole throughout, which is both accurate and less brittle
than matching raw copy.
Also prettier-ignore the playwright artifact directories. They are already
gitignored, but prettier still walks them, so running the e2e suite locally
left `format:check` failing on playwright-report/index.html and
test-results/.last-run.json.
Verified: 4 passed on three consecutive runs (14-24s each). Refs #689.
The extension job ran `test:e2e:headless:chrome`, which is not a script in
packages/extension/package.json. `pnpm --filter <pkg> run <missing>` exits 0
with only a warning, so the step has been reporting green while running
nothing:
$ pnpm --filter pilo-extension run test:e2e:headless:chrome
None of the selected packages has a "test:e2e:headless:chrome" script
$ echo $?
0
Sequenced after the spec repair on purpose: correcting this first would have
turned the extension job red. Refs #689.
There was a problem hiding this comment.
🔵 Needs a closer look
The E2E suite is newly exercised in CI, with Linux behavior not yet verified.
Pull request overview
Repairs stale extension sidepanel E2E selectors, excludes generated artifacts from formatting, and enables the suite in CI.
Changes:
- Updated selectors to use accessible roles.
- Added Playwright artifacts to
.prettierignore. - Corrected the CI E2E command.
File summaries
| File | Description |
|---|---|
packages/extension/e2e/sidepanel.spec.ts |
Repairs sidepanel selectors. |
.prettierignore |
Ignores generated Playwright artifacts. |
.github/workflows/build-test.yml |
Runs the valid E2E script in CI. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Following up on the review's one flag — "the E2E suite is newly exercised in CI, with Linux behavior not yet verified" — that's now verified, and it passed. From the That's on Worth noting the ordering, since the review reads as an open question: the review was submitted at 23:01:58Z and the e2e step ran 23:02:22Z → 23:02:30Z, so the concern was raised ~24s before the step it was about had started. It was flagging the caveat from the PR description, not an independent finding in the diff — the review itself reports 0 comments generated at "Lite" effort. So: nothing outstanding from that review. All 14 checks green. Still needs a human approving review to satisfy the |
Fixes #689 (partially — see below).
What was wrong
Two problems that hid each other: CI never ran the e2e suite, and the suite had rotted.
The CI step was a silent no-op. The extension job called
test:e2e:headless:chrome, which isn't a script inpackages/extension/package.json.pnpm --filter <pkg> run <missing>exits 0 with just a warning, so the step reported green while running nothing.All 4 tests failed on stale selectors. The sidepanel renders correctly — the selectors described an older UI:
getByText("Pilo Settings")getByText("Back to Chat")getByTextis case-sensitivegetByTestId("settings-button")aria-label="Open settings"Switched to
getByRolethroughout — accurate, and less brittle than matching raw copy.Also prettier-ignored the playwright artifact dirs. They're gitignored, but prettier still walks them, so running the suite locally left
format:checkfailing onplaywright-report/index.htmlandtest-results/.last-run.json.Correction to the issue
#689 claimed a third cause — that the
extensionIdfixture couldn't survive sequentiallaunchPersistentContextcalls. That was wrong. I probed it directly (3 sequential launches, logging each step) and the fixture works fine every time. The "Test timeout of 30000ms exceeded while setting up extensionId" messages were the 30s test budget being consumed while the machine was loaded by the earlier failing runs — a symptom of the selector rot, not an independent bug. I've corrected the issue.One real but harmless thing the probe did surface, left alone here:
serviceWorker.evaluate(() => sw.registration.active)in the fixture returns a non-serializableServiceWorker(comes back as{}ornull), so the comment calling it "critical for MV3 in headless mode" is misleading — it isn't actually waiting for anything. I didn't touch it since I can't reproduce a failure it would fix, and the specs pass without changing it.Verification
Locally (macOS arm64, node 22.23.2): 4 passed on three consecutive runs, 14–24s each. Also clean:
format:check, extensiontypecheck, and the 273 unit tests.Note the second commit is deliberately sequenced after the spec repair — correcting the workflow first would have turned the extension job red. This PR is the first time CI has actually run these tests, so the extension job here is the real test of whether they pass on Linux; I could only verify macOS. If it's red on the runner, the sensible move is to keep the spec fix and revert the workflow commit while we sort out the platform difference.