test(desktop): finish moving non-Electron coverage out of the E2E tier - #4877
Conversation
#4523 bought E2E time with four Xvfb displays and four Playwright workers. That parallelism is what invalidated the premises the tests were written against, and #4761 removed the need for it: the tier is 35 tests, all of them here because they need a native window, and `workers: 1` has been the config's answer since. The CI step still built four displays for a single worker to use one of. The step is now the same one-line `xvfb-run` every other Electron job in the workflow uses. `MAKA_E2E_X_DISPLAY_BASE` and the worker fixture that read it go with it, and the config comment stops describing a warm prompt-rail scenario that #4825 deleted. Refs #4761
1b5a21a to
275cce3
Compare
275cce3 to
6e25b0a
Compare
Four geometry contracts were still costing an Electron window each, and none of them read anything Electron owns. They are layout under a column width: the browser address field tracking the workbar column, the tool picker's shortcuts staying inside the panel at the 320px floor, Side Chat's composer keeping the conversation's radius token and its send button inside the card, and WorkHub's submitted-work button keeping the target's project name inside its own box. Each is now a story whose `play` measures the same boxes in real Chromium. WorkHub gets its first story group: `WorkHubSurface` already takes its `controller` as a prop, which is the seam production fills, so a story serves one pinned projection instead of driving a live Coordination Session. Every assertion was mutation-checked: deleting the rule it pins turns the story red, with the failure naming that rule's own measurement. Refs #4761 Generated-by: Claude Code
`fixture-thread-search.spec.ts` launched a window, seeded a transcript and asserted that a content hit carries its turn id and position. The main-process test for the same handler already asserts exactly that — same query, same `用户消息` summary, same four-field target — plus the title-hit and transcript-failure cases the E2E never covered. Nothing here needed a renderer: the spec's own body only ever called `window.maka.search.thread`. The window fixture it was the sole user of goes with it. Refs #4761 Generated-by: Claude Code
6e25b0a to
0de0318
Compare
The E2E test spent 75 lines of Electron on three claims that are all CSS and renderer state: the tab strip and the collapse toggle report one centre line, the bar gives back the width the platform reports in `--maka-titlebar-overlay-right-width`, and collapsing parks the same toggle in the titlebar at the same x/y before restoring the face that was open rather than the picker. `app-shell.stories.tsx` already mounts the real titlebar and the real shell grid, so the story hosts both bands the toggle moves between — which is the one reason this could not live with the workbar's own stories. The collapse state runs through `reduceWorkbarLayout`, the same reducer the app dispatches into, so "restore brings back the face" is the production rule firing rather than a story-local `useState`. Mutation-checked, each against the built story: - dropping the caption term from the bar's `padding-inline` → the toggle does not move (80px off); - `align-items: flex-start` on the tab strip → centre line off by 2px; - `collapse` clearing `panels` in the reducer → the picker comes back; - `[+]` dispatching `open-launcher` instead of opening the face menu → no `role=menu`. Tier-1 is now 30 tests in 16 files. Generated-by: Claude Code
The tier just went from 110 tests to 30 by asking one question of each test: does this need a real window? Nothing kept the answer. The PR body said "what stays needs a native window, and was re-checked one by one" — prose, in a PR that will scroll away. `apps/desktop/e2e-budget.json` is that sentence made checkable. It records each spec's test count and the Electron-owned mechanism it needs, and the admission rule sits at the top of the same file, printed back on failure. Counts regenerate with `--write`; the reason cannot, so a new spec has to be justified in the diff or the guard stays red. Not extracted into a shared ratchet with `check-renderer-architecture`: the 3.5k lines there are Babel analysis and `--base` baseline rematerialization, and the part these two share — read a ledger, diff, exit 1 — is under twenty lines each. The subjects also differ (monotonic debt vs an exact inventory), so a common interface would be guessed from two instances rather than read off three. It runs in the planning lane, not the e2e job: that job is conditional, and a guard that only checks the tier when the tier already ran is a guard that grows back. Pure Node, no build — it reads the spec sources, and refuses any top-level `test.*` form whose count it cannot see (a `test.describe` would hide its tests from the scanner rather than be undercounted). Refs #4761 Generated-by: Claude Code
Ran the removal pass over the two commits before this one. Six things came out; nothing else could be removed without losing a proven check. Two story assertions could never fail: - `expect(frame).toBeVisible()` with the face menu open. `frame` is the workbar card, and the launcher lives inside it — the very regression the line was aimed at leaves it visible. `pickerIsShowing()` is the check that actually bites, and it is already there. - comparing the restored tab's label to the one recorded before collapse. The story opens one face, so there is no other label the tab could carry. Four pieces of the budget guard were built for cases the repo does not have: - `--write`, and the two package scripts for it. The ledger is one integer and a hand-written sentence per spec; the failure message already names the number to type. A generator that can only regenerate the half you could read off the error is not worth its own mode. - the `test.only` / `test.skip` / `test.fixme` counting table and the seven-entry `test.setTimeout` / `test.use` / hook allowlist. Zero top-level `test.*` forms exist in the tier. An unrecognised form now throws with the message that tells you to teach the script, which is the same one-line edit adding to a table would have been. - `"version": 1` in the ledger, which nothing read. The assertions that stayed are the ones a mutation turned red, including two checked for this pass: `WorkbarTitlebarActions` rendering while the column is open, and a 4px shift of the titlebar's right cluster. Refs #4761 Generated-by: Claude Code
Second removal pass, over the tests rather than the code. `BrowserAtColumnFloor` and `BrowserAt400` were pixel stories with no play and no state of their own. `BrowserAddressFieldTracksColumnWidth` uses the same fixture and ends its play at 320px, so the floor was already on screen under a name that says what it proves; 400px sits between two widths with no rule between them, so it showed nothing 320 and 480 do not. Four theme renders in the smoke for a duplicate. The budget guard had two tests over `compare`, one per violation kind. They are one obligation -- the ledger and the tier disagree -- so they are one test with all four disagreements in it. Two claims in comments were false rather than redundant: `workbar/side-chat.css` still sent the reader to `session-workbar.spec.ts` for the `overflow: visible` assertion that now lives in the story, and the budget's reason for `settings.spec.ts` justified two of its four tests. The workbar-chrome one rides along on a window the other tests already need; it is recorded that way instead of being described as something it is not. Refs #4761 Generated-by: Claude Code
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed current head 57b1dae273438220df279fcf6e2075a2130435b4 (OPEN). NO-GO — one P2 test-coverage gap below; the integration gate is also still open (test/package pending). No new P0/P1 production correctness, permission, or concurrency issues found.
P2 — the deleted Electron search test's Host-session key projection is uncovered
The deleted fixture-thread-search.spec.ts:21-55 verified through the real Electron/preload/Host fixture chain that window.maka.search.thread returns desktopSessionKey, turnId=turn-prompt-rail-3, and sequence=4. The replacements do not cover preload.ts:3003-3018's Host-session key projection: runtime-host-search-ipc-main.test.ts:28-90 calls the main handler directly with a raw session id and single-message sequence=0; runtime-host-guest-ipc-preload.test.ts:65-89's search stub returns an empty array. If that mapping regresses, search-result navigation fails before parsing at preload.ts:419-438. Fix: add a non-empty search-hit preload bridge test asserting JSON.stringify([hostId, sessionId]), turnId, and nonzero sequence — no need to restore the E2E as-is.
Scope and limits
check:e2e-budget (30 tests/16 files), node --check, git diff --check pass; merge-tree against latest origin/main=d2d7efe6 clean. Exact-head hosted audit, package-linux, and both Runtime Host owner checks green; test/package still pending. Local Storybook/typecheck/E2E could not run independently (missing tsc and Playwright deps).
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
评审结论来自自动化审查流程;发布者没有读这份 diff,核的是当前 head 有没有漂移、以及 exact-head 的门禁状态。当前 head 是 57b1dae,未关闭。一条 P2:删掉的 Electron 搜索用例覆盖的 key 投影丢了,替代测试没覆盖到。修好再合。
…it cost Adversarial review of the branch. Three findings were real defects in this change; the rest were narrower than reported. The budget guard did not bound the tier it claimed to bound. `playwright.config.ts` sets `testDir: '.'` with no `testMatch`, so the tier is Playwright's default pattern — recursive, and `.test.ts` as well as `.spec.ts`. The guard read one flat directory for one suffix, and counted only `test(` at column 0. Three ways in, all silent: a subdirectory, the other suffix, and a loop or helper generating tests. Root cause is one seam, not three holes: the guard and the ledger were both written from a single reading of today's 16 files and validated against that same reading, so neither could catch the other. Discovery now mirrors Playwright's own pattern, and a `test(` this scanner cannot attribute to a line is refused the way `test.describe` already was, rather than counted as zero. Deleting `fixture-thread-search.spec.ts` cost two assertions the main-process test did not carry. Its fake catalog returned a bare Runtime Host id where the real one returns a composed Desktop key, and its single-message transcript made `sequence: 0` pass for any projection. Restoring `sequence: messageIndex` as the only value that passes: `packages/core/src/thread-search.ts` forced to `sequence: 0` now turns the unit test red. Without this, a search hit could open the wrong message, or the wrong Session on a second Host. `WorkbarCollapseKeepsOneToggleInPlace` asserted the workbar frame came back but not the face inside it. The face's content is a sibling panel with its own `hidden` (workbar-surface.tsx), so a frame-visible check passes while the face stays hidden — the exact regression the migrated E2E covered. Smaller corrections: `expect.soft` from `storybook/test` is not soft (it builds the assertion before setting the flag, and needs a vitest test context the play function has not got), so it is plain `expect`; the Side Chat story waits for its companion instead of reading it synchronously; the budget's reason for `session-workbar.spec.ts` now covers all five of its tests including the renderer-only one riding along; and three notes this branch made stale — a four-worker CI aside, a story pointing at the retired E2E assertion, and `fullyParallel` under `workers: 1` — are corrected or removed. Left alone: the Side Chat radius story now pins the side composer against `--radius-chat` rather than against the main composer, so a main-composer drift is unguarded. Adding that probe belongs to a main-composer story, not this one; recorded on #2188. Refs #4761 Generated-by: Claude Code
jackwener
left a comment
There was a problem hiding this comment.
Approving at exact head e8fa69a1. Checked the counts, the five destinations and the one outright deletion; all hold. No P0–P2.
- The tier numbers are exact. 16
e2e/*.spec.ts, 30test(calls, and 16 entries ine2e-budget.json— the budget describes the tier that is actually there, not the one being described. - The five geometry contracts land where the table says:
BrowserAddressFieldTracksColumnWidth,ToolPickerAtColumnFloorandSideChatAtColumnFloorinsession-workbar.stories.tsx,WorkbarCollapseKeepsOneToggleInPlaceinapp-shell.stories.tsx,SubmittedWorkKeepsTargetMetadataInsideinworkhub.stories.tsx. - The titlebar case is the one worth spot-checking, and it survives the check. Its play distinguishes the workbar frame from the face's sibling overlay panel — a visible frame is not a visible face — and states "not the picker" as a reachability claim via
queryByRolerather than a presence check, since the launcher stays mounted behind the face. That is a story reasoning about the real composition, not a re-skin of the assertion. - The deletion is justified, which is the claim that usually is not.
fixture-thread-search.spec.tsis removed with no replacement becauseruntime-host-search-ipc-main.test.tsalready asserts the same thing: a content hit whose summary is用户消息and whose target passes a four-fielddeepEqual, plus the title-hit case the E2E never had. The destination is stronger than what left. - The new guard is actually wired:
check:e2e-budgetis a root script,check-e2e-budget.mjsships with its owncheck-e2e-budget.test.mjs, and it runs fromci.yml. Putting it in the planning lane rather than the conditional e2e job is the right call for the reason given — a guard that only checks the tier when the tier already ran is not a guard.
The reasoning for retiring the four-display harness is sound on its own terms: the parallelism invalidated the premises these tests were written against, and the config has answered workers: 1 since anyway.
简体中文
在 exact head e8fa69a1 上批准。数目、五处去处、以及那一处彻底删除都核过,全部成立。 无 P0–P2。
- 层级数字是精确的:16 个
e2e/*.spec.ts、30 个test(、e2e-budget.json16 条 —— 预算描述的是真实存在的那个层,不是被描述的那个。 - 五个几何契约都落在表格所说的位置(workbar 三个、app-shell 一个、workhub 一个)。
- titlebar 那条最值得抽查,而它经得起抽查:它的 play 区分了 workbar frame 与 face 的兄弟 overlay 面板(frame 可见不等于 face 可见),并把「不是 picker」表述为可达性主张、用
queryByRole判断,因为 launcher 仍挂在 face 背后。这是一条在对真实组合做推理的 story,不是把断言换层皮。 - 那处删除是有据的,而这恰恰是通常不成立的那类主张。
fixture-thread-search.spec.ts被无替代删除,因为runtime-host-search-ipc-main.test.ts已经断言同样的事:摘要为用户消息的内容命中,且 target 通过四字段deepEqual,外加 E2E 从未有过的标题命中。去处比离开的更强。 - 新守卫是真的接上了:
check:e2e-budget是根脚本,check-e2e-budget.mjs自带check-e2e-budget.test.mjs,并由ci.yml触发。放在 planning lane 而不是条件式 e2e job 里是对的,理由如正文所说 —— 只有在该层已经跑过时才检查该层的守卫,不是守卫。
退役四显示器并行装置的理由本身也成立:那种并行恰恰使这些测试所依据的前提失效,而且配置此后一直是 workers: 1。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed current head e8fa69a1b2fdd10bf958a7d34e3f9b8f103845be (OPEN, MERGEABLE/BLOCKED awaiting human review). NO-GO — one P2 test-coverage gap below, still open. Prior conclusions were re-verified against this head, not carried over: the new commit strengthens main-process search tests (composed Desktop session key, nonzero sequence) and Workbar hide/restore assertions, but the gap below survives. Checks: audit, package, package-linux, both owner checks green; test still pending.
P2 — the empty search stub still leaves the key projection uncovered
runtime-host-guest-ipc-preload.test.ts:58-89's search:thread stub still returns an empty array — no coverage of preload.ts:3003-3025's Host-session key projection for a non-empty search hit. A regression there still breaks search-result navigation before parsing at preload.ts:419-438. Fix: one non-empty preload bridge test asserting JSON.stringify([hostId, sessionId]), turnId, and nonzero sequence.
Scope and limits
Merge-tree against latest origin/main=d2d7efe6 clean. Local full verification tooling gaps as before; no GitHub review published from the lane for this head yet (this synthesis is the published record).
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
评审结论来自自动化审查流程;发布者没有读这份 diff,核的是当前 head 有没有漂移、以及 exact-head 的门禁状态。当前 head 是 e8fa69a,未关闭。新提交补强了主进程测试,但空 stub 那条覆盖缺口还在。修好再合。
#4694 moved the right workbar's collapse bit into a per-Session map after #4877 wrote the shell story against the old `rightCollapsed` field, and it added a sixth Electron test to `session-workbar.spec.ts` without recording it in `e2e-budget.json`. Both guards fail on `main` since; this branch carries them so its CI can be read. Generated-by: Claude Code
#4694 replaced WorkbarLayoutState.rightCollapsed with activeSessionId + collapsedBySession; #4877 landed a story that builds the old shape. Both were green on their own bases, so main's typecheck fails on stories/app-shell.stories.tsx. Build the story on the new state and read the collapse bit through isSessionWorkbarCollapsed, as the controller does. Generated-by: Claude Code
…4895) `check:e2e-budget` and the Storybook typecheck both failed on `main` after #4694 merged: that PR added a sixth test to `session-workbar.spec.ts` without updating `e2e-budget.json`, and replaced `WorkbarLayoutState.rightCollapsed` with `activeSessionId` + `collapsedBySession` while #4877 had just landed `stories/app-shell.stories.tsx` building the old shape. Its CI ran on a base that predated both, so neither showed until merge (reported on #4894). The budget entry now records 6 tests and names the Electron-owned mechanism (per-Session collapse persisted across a renderer reload). The story builds the new layout state and reads the collapse bit through `isSessionWorkbarCollapsed`, as the controller does. No behavior change; no migration. Refs #4694, #4761, #4877, #4894 Generated-by: Claude Code
e2e-budget.json keeps layout geometry and pure CSS in a Storybook `play`, not the Electron tier, and apache#4877 already moved the sibling Side Chat / WorkHub geometry there while deleting the old E2E radius checks. Revert the E2E assertion added in the previous commit and put the contract in the existing SubmittedWorkKeepsTargetMetadataInside play, comparing the real user bubble's computed radius against the real composer plate — so the bubble and the dock on one surface must round together or this fails. Generated-by: Claude Code
Summary
The end of #4761: nothing is left in the Electron tier that does not need a native window, and the parallel harness that tier was carrying is gone. The tier goes from 35 tests / 17 files to 30 tests / 16 files, ~2.5 minutes on one worker.
The last geometry cases become stories. Five contracts were still costing an Electron window each, and none read anything Electron owns — they are layout and renderer state, measured in Chromium either way:
Product/Session Workbar/Browser Address Field Tracks Column WidthProduct/Session Workbar/Tool Picker At Column Floor--radius-chatand keeps its send button inside the card when the model chip runs longProduct/Session Workbar/Side Chat At Column FloorProduct/Shell Official AppShell/Workbar Collapse Keeps One Toggle In PlaceProduct/WorkHub/Submitted Work Keeps Target Metadata InsideThe titlebar case is the one that looked Electron-shaped, because the toggle moves between the workbar's own bar and the titlebar's right cluster and
workbar/shell.csspads the bar with the titlebar strip's gutter so it lands on the same x in both.app-shell.stories.tsxalready mounts the real titlebar over the real shell grid, so it hosts both bands; the story's collapse runs throughreduceWorkbarLayout, the reducer the app dispatches into, so "restore brings back the face" is the production rule firing rather than a story-localuseState.ComposedShellgains two optional props to let a story drive the titlebar's restore affordance it was previously rendering as a staticcollapsed={false}.WorkHub gets its first story group.
WorkHubSurfacealready takes itscontrolleras a prop — the seam production fills withcreateWorkHubController— so a story serves one pinned projection instead of driving a live Coordination Session.One E2E spec was pure duplication.
fixture-thread-search.spec.tslaunched a window to callwindow.maka.search.threadand assert a content hit carries its turn id.runtime-host-search-ipc-main.test.tsalready asserts exactly that — same query, same用户消息summary, same four-field target — plus the title-hit and transcript-failure cases the E2E never covered. Deleted with no replacement, along with the window fixture it was the sole user of.The parallel display harness retires. #4523 bought E2E wall-clock with four hand-built Xvfb displays and
--workers=4. That parallelism is what invalidated the premises these tests were written against — a fresh renderer per test, exclusive OS focus, unthrottled frames — which is the flake class #4761 exists to remove. The config has answeredworkers: 1since; CI was building four displays for one worker to use one of. The step is now the same one-linexvfb-runevery other Electron job uses, andMAKA_E2E_X_DISPLAY_BASEplus its worker fixture go with it.What stays is now written down, not asserted.
apps/desktop/e2e-budget.jsonrecords each of the 16 remaining specs, its test count, and the Electron-owned mechanism it needs — the OS folder chooser,/compact's Host round trip, a pointer capture released outside the window, renderer-reload persistence, WorkHub's delegation lifecycle, revision-draft branching, the CDP wheel input the transcript perf budget measures.npm run check:e2e-budgetfails on a spec that is not in the budget, a count that drifted, or an empty reason; the admission rule sits at the top of the same file and is printed back on failure. There is no regenerate mode — the failure names the number to type, and the reason has to be written by hand, so a new spec is justified in the diff or the guard stays red.It runs in the planning lane rather than the e2e job, because that job is conditional and a guard that only checks the tier when the tier already ran is a guard that grows back. Pure Node, no build. Its discovery mirrors Playwright's own default
testMatch— recursive,.spec.tsand.test.ts— becauseplaywright.config.tssetstestDir: '.'and overrides neither; atest(it cannot attribute to a line of its own (a loop, a helper) is refused rather than counted as zero.Not folded into a shared ratchet with
check-renderer-architecture: the 3.5k lines there are Babel analysis and--basebaseline rematerialization, and what the two share — read a ledger, diff, exit 1 — is under twenty lines each, over different subjects (monotonic debt vs an exact inventory). Worth revisiting at a third real instance.Refs #4761
Verification
.maka-browser-address-field'sflex/min-width→expected 154 to be greater than 250min-width: 0→expected 1304 to be <= 1280--_chat-composer-radius→'12px' to be '28px'min-width: 0→ the send button 40px past the card's right edge.workhub-submitted > button'sheight: auto→ the project name 17px below the buttonpadding-inline→expected 1224 to be close to 1144, received difference is 80align-items: flex-starton the tab strip → centre line off by 2pxreduceWorkbarLayout'scollapseclearingpanels→ the picker comes back instead of the face[+]dispatchingopen-launcherinstead of opening the face menu →Unable to find role="menu"npm run smoke:storybook -w @maka/desktop— 310 stories, 337 theme renders, all pass.npx playwright test --config e2e/playwright.config.ts --list— 30 tests in 16 files;e2e/session-workbar.spec.tsre-run on macOS, 5 passed..test.ts, generated tests) — root cause was that the guard and the ledger were written from one reading of today's tier and validated against that same reading; deleting the thread-search E2E cost two assertions the main-process test did not carry (a composed Desktop session key where the fake returned a bare Host id, and asequencethat any projection satisfied at 0) —thread-search.tsforced tosequence: 0now turns the unit test red; and the migrated titlebar story checked the workbar frame rather than the face inside it, which carries its ownhidden.toBeVisibleon the workbar card while the face menu is open; comparing the restored tab's label in a story with one face) and four unused pieces of the guard (--write, thetest.only/test.skip/test.fixmeand hook-form tables, the unreadversionfield). Two more mutations were run to confirm what stayed does bite:WorkbarTitlebarActionsrendering while the column is open, and a 4px shift of the titlebar's right cluster.npm run check:e2e-budget— 30 tests in 16 files; mutation-checked by adding an unjustified spec (not in the budget) and an extra test in an existing one (records 4 test(s), the file has 5).node scripts/audit-alignment.mjs— all fixtures clean.npm run format/npm run lint— clean.npm run typecheck -w @maka/desktop— the three pre-existing Astryx surface errors (ChatLayoutProps.autoScroll,MarkdownProps.settledText,SideNavItemProps.trailingAction) reproduce onmainand are untouched; nothing else reports.AI use
Select exactly one:
Tool(s) and scope: Claude Code moved the tests, removed the harness and wrote this PR.
Checklist
Does this PR entail a change in behavior?