e2e: packaged-app smoke test on macOS and Windows - #70
Conversation
Nothing verified that the packaged app launches. Packaging failures — asar layout, native module rebuilds, bundled CLI resolution — were only discoverable by downloading an artifact and installing it in a VM. One Playwright `_electron` test runs against an unsigned `electron-builder --dir` build and asserts three things: 1. It boots — the first window appears and the renderer paints. 2. The preload bridge is whole — every `contextBridge` key matches a checked-in list. There is no typecheck here, so nothing else catches a handler added in the main process and never exposed to the renderer. 3. `require.resolve()` from inside the packaged app for the Playground CLI and `fs-ext`. That one catches a silent failure: `fs-ext` is an optional native dependency, so a failed rebuild ships broken file locking with no error anywhere in the install or build logs. Each assertion was verified to fail when the thing it covers is broken. `CSC_IDENTITY_AUTO_DISCOVERY=false` is mandatory on macOS — electron-builder signs during `--dir` otherwise. Buildkite is untouched and still produces the signed artifacts. Closes #67 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
e39670b to
841acf2
Compare
`pull_request` defaults to opened/synchronize/reopened. Without `ready_for_review`, a PR opened as a draft and later marked ready never triggers this workflow again — the draft guard skips the only run it ever gets, and the smoke test silently never runs. Caught on this PR's own first ready transition. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The smoke test found a real bug on its first Windows run: fs-ext compiles from source, the compile fails on Windows, and npm drops the optional dependency silently. Every Windows artifact ships without file locking. Tracked in #71. Marking it with `test.fail` rather than skipping it: the suite goes red if the module ever resolves on Windows, so the assertion turns itself back on when #71 is fixed instead of waiting for someone to remember. Also splits the resolution test per module, so the report names which one broke. Includes a temporary `--foreground-scripts` on the install step to surface the node-gyp output npm swallows. Reverted in the next commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`--foreground-scripts` did its job: the Windows failure in #71 is node-gyp being unable to recognise the Visual Studio on the runner, not a missing compiler. Recorded in the issue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…here
The earlier `test.fail` marker was based on a wrong premise. Upstream disables
fs-ext on Windows outright:
os.platform() === 'win32'
? undefined // @todo: Enable fs-ext here when it works with Windows.
: await import('fs-ext').then((m) => m.flockSync)
So Windows packages are not missing file locking because of our build; the module
is simply unused there. Asserting it tested something the app does not do, and
the marker would have gone red if the module ever *did* resolve on Windows — an
outcome that is harmless and perfectly possible with a different toolchain.
Keeps the assertion on macOS and Linux, where a silently dropped optional native
dependency does degrade locking. The upstream TODO is recorded next to the module
list: if that guard goes away, Windows belongs back in it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both sides added devDependencies and scripts — @playwright/test and test:e2e here, the ESLint toolchain on trunk (#112). The resolution keeps both and regenerates the lockfile from trunk's with npm install --package-lock-only. Brings in the automated review so it runs on this PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| "xterm": "^5.3.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@playwright/test": "^1.61.1", |
There was a problem hiding this comment.
🟡 architecture · [fix here]
@playwright/test pulls in playwright → fsevents (macOS-optional), and the lockfile shows it with hasInstallScript: true (package-lock.json, new node_modules/fsevents entry). It has no corresponding entry in allowScripts below (line 93), unlike the other lifecycle-script deps already listed there (electron, electron-winstaller, esbuild, fs-ext-extra-prebuilt).
Per this repo's convention, a lifecycle-script dependency without an allowScripts entry is a finding regardless of how low-impact this particular one is (dev-only, optional, darwin-only). Add "fsevents@2.3.2": true alongside the existing entries.
| ]); | ||
|
|
||
| if (proc && proc.exitCode === null) { | ||
| proc.kill(); |
There was a problem hiding this comment.
🔵 cross-platform · [fix here]
This fallback signals only the top-level Electron process, not its tree — the same pattern src/kill-tree.js exists specifically to avoid (see its header comment and issue #83: "a plain child.kill() signals only the first link, so quitting the app left watchers and servers running"). The comment two lines up even notes "the app keeps child processes alive."
Right now the three tests in this file never start a real npm-install/server child, so the blast radius is just Electron's own helper processes and it only matters in the already-degraded case where close() hangs. But this fixture is the natural place future e2e tests will exercise startServer/runNpmInstall, at which point a bare proc.kill() here will leak orphaned processes on whichever platform's close() misbehaves. Consider reusing killChildTree (exported from src/kill-tree.js) instead of proc.kill().
) ## Why The app's user documentation is the README, and it has outgrown it: install steps, a ten-step walkthrough and the trunk-update rules all compete for the same page, and there is nowhere to put a screenshot. This PR gives the guide a home and a deploy pipeline. It carries no guide content beyond the landing page — that arrives in #232, and the harness that photographs the app in #231. ## What changes - `docs/` becomes a VitePress site with **its own npm package**, so a docs-only CI job never runs the root `postinstall` (electron-builder + esbuild) and the app's dependency tree stays free of a static-site generator. - `.github/workflows/docs.yml` builds and deploys to GitHub Pages on pushes to `trunk` that touch `docs/`. Pull requests get a **build-only** job: a dead link fails before merge, and the job holding the OIDC token never runs on PR code. - The site's `base` is derived from the repository name at run time. A project Pages site is served under `/<repo-name>/`, and this repo has already been renamed once — deriving it means another rename cannot break every asset URL. - `srcExclude` keeps `docs/testing.md` (from #70) out of the published site: it documents how to run the suites, which is contributor material, and this site is for users of the app. ## How to test this **Starting state:** this branch checked out, `npm ci` done. 1. `npm run docs:build` — expect `build complete`. Now add a link to a page that does not exist in `docs/index.md` and run it again: it must **fail** with `dead link(s) found`. Undo. 2. `npm run docs:preview`, open the printed URL — the home page renders with the WordPress Contributor Toolkit hero and the sidebar. Clean URLs work; this is exactly what Pages serves. 3. `npm run docs:dev` — pages are served at their `.html` paths in dev (`/index.html`). With the dev server open, run `npm run docs:build` in another terminal: the dev server must **not** spew reload lines (it ignores its own output directory). 4. `npm run lint` and `npm test` — both green, unchanged from `trunk`. **Must not have happened:** no `deploy` job may run on this pull request — check the Actions tab and confirm only `build site` ran. **Not testable by hand here:** the deploy itself. It needs the workflow on `trunk`, and Pages is already set to "GitHub Actions" as its source. ## Risks Merging this alone deploys a site whose sidebar names the full guide, so **those links 404 until the content PR lands**. Merge the stack in order rather than leaving this on `trunk` by itself. The deploy job holds `pages: write` and `id-token: write`. Its actions are pinned to commit SHAs rather than tags, following the reasoning already written down in `download-stats.yml`. ## Related Stack: this → #231 (screenshot harness) → #232 (guide content). Touches #70 only through `srcExclude`; that PR needs no change. --- <details> <summary><b>Self-review</b> — 5 findings, all fixed</summary> Ran `.github/instructions/code-review.instructions.md` with the judgement pass in a fresh context. Deterministic layer was clean (ESLint and the unit suite on macOS and Windows). **5 [fix here] · 1 [follow-up]. All five fixed in this branch:** | Dimension | Was | Now | |---|---|---| | security 🟡 | `checkout` kept the job's `GITHUB_TOKEN` in `.git/config` while the job runs PR-authored code (`npm ci` with lifecycle scripts, and a VitePress build that evaluates `config.mjs`) | `persist-credentials: false`, matching `lint.yml` | | security 🔵 | `if: github.event_name != 'pull_request'` let a `workflow_dispatch` on any ref publish to the live site, which is not what the comment claimed | `if: github.ref == 'refs/heads/trunk'` | | cross-platform 🔵 | `node-version: 22` hardcoded while `.nvmrc` says 24.18.0 — reintroducing the Node drift of #37/#46 for the one command CONTRIBUTING calls "what CI runs" | `node-version-file: .nvmrc` | | architecture 🔵 | one concurrency group spanning build and deploy, with `cancel-in-progress: true`, so a second push could cancel an in-flight `deploy-pages` | split: builds cancel, deploys do not | | architecture 🔵 | CONTRIBUTING documented `docs:*` without saying the nested package needs its own install, so the commands fail on a clean clone | `npm ci --prefix docs` documented | **Deferred [follow-up]:** `electron-builder` has no `files` filter, so tracked `docs/` sources ship inside `app.asar` — and #232 adds screenshots on top. Real, pre-existing, and overlaps #23; an `!docs{,/**/*}` entry closes it. Not done here because it changes what every release artifact contains, which deserves its own PR and its own testing. **Checked and clean:** all four action pins resolve to the tags their comments claim (verified against the GitHub API); `pull_request_target` is correctly not used and no fork PR can reach `pages: write` / `id-token: write`; the lockfile is 175 packages, all from registry.npmjs.org, with install scripts only on esbuild and fsevents; the `import/no-unresolved` exemption is a genuine false positive and hides nothing; no `src/` code, IPC surface or spawn path is touched. </details>
The guide needs pictures of every screen, and they have to be retakeable by whoever changes the UI. `npm run shots` launches the repo's own Electron binary through playwright-core's Electron driver, drives each documented screen by the words on it, and writes a fixed-size PNG into docs/public/screenshots/. It never touches the contributor's real sites. A new TOOLKIT_USER_DATA_DIR hook in src/main.js points userData at a throwaway directory holding a seeded settings.json. The hook is guarded to dev runs — a packaged app ignores the variable, so no installed build can be redirected to an attacker-chosen store path through the environment — and test/user-data-override.test.cjs pins both sides of that guard. Fixture site paths are deliberately username-free (/tmp/wpct-docs-fixture/...): safe-log.js redacts logs, but nothing redacts a screenshot, and every path the app renders ends up in a published image. Screens a seeded registry cannot reach — a running dev server, a real diff, the GitHub device-code screen — are declared as a live tier instead of faked: `npm run shots -- --tier=live` prompts for each one and waits. playwright-core rather than playwright: same Electron driver, no postinstall script, no browser download. If #70 lands @playwright/test, this direct dependency can go and the harness can require the driver from there. The harness lives under scripts/ rather than e2e/ because it is a CLI, not a test: #70 makes e2e/ the Playwright testDir, and nothing here is run by `npm run test:e2e`.
…231) ## Why A user guide needs pictures of the screens it describes, and they have to be **retakeable** by whoever changes the UI — a screenshot nobody can regenerate is a screenshot that goes stale and starts lying. This adds the harness. The images themselves land with the guide in #232. Stacked on #230; review that one first. ## What changes - `npm run shots` launches the repo's own Electron binary through **playwright-core**'s Electron driver, drives each documented screen by the words on it, and writes a fixed-size PNG into `docs/public/screenshots/`. Eleven screens are declared today. - A new **`TOOLKIT_USER_DATA_DIR`** hook in `src/main.js` (10 lines) points `userData` at a throwaway directory holding a seeded `settings.json`, so the harness never reads or writes the contributor's real site registry. - `test/user-data-override.test.cjs` pins **both sides** of that hook's guard. - Screens a seeded registry cannot reach — a running dev server, a real diff, the GitHub device-code screen — are declared as a **live tier** rather than faked: `npm run shots -- --tier=live` prompts for each and waits. <details> <summary>Why these specific choices</summary> **`playwright-core`, not `playwright`** — same Electron driver, no postinstall script, no browser download, so no new entry in `allowScripts`. If #70 lands `@playwright/test`, this direct dependency can go and the harness can require the driver from there; the note is in the file. **Under `scripts/`, not `e2e/`** — it is a CLI, not a test. #70 makes `e2e/` the Playwright `testDir`, and nothing here is run by `npm run test:e2e`. It also means no new ESLint exemption: `scripts/**/*.cjs` already allows `console`. **Username-free fixture paths** (`/tmp/wpct-docs-fixture/...`) — `safe-log.js` redacts logs, but nothing redacts a screenshot, and every path the app renders ends up in a published image. **Captured locally, not in CI** — capturing needs the app running against a seeded site, which is minutes of flaky work on a runner for images that change rarely. </details> ## How to test this **Starting state:** a machine that has used the app before, so you have **real sites registered**. `npm ci` done on this branch. 1. Launch the app normally (`npm start`) and note your real sites in the sidebar. Quit. 2. `npm run shots` — the app opens and closes a few times; expect eleven `✓ <name>.png` lines and the files under `docs/public/screenshots/`. 3. Open each PNG. Every path visible must start `/tmp/wpct-docs-fixture/` — **your home directory must appear nowhere**. 4. `npm start` again — your real sites are still there, exactly as in step 1. 5. `npm run shots -- --only=terminal` captures just that one; `--only=nope` fails and lists the valid slugs. 6. `npm test` — green, including the three new guard tests. **Must not have happened:** your real `settings.json` must be untouched (step 4 is the check), and no packaged build may honour the variable — the third guard test is what pins that. **Platforms:** driven on macOS. The fixture root falls back to `os.tmpdir()` on Windows, which is the part most worth a second pair of eyes; the harness is a developer tool, so a Windows failure costs a retake, not a shipped bug. ## Risks `src/main.js` gains an environment-driven `app.setPath` call. It is guarded by `!app.isPackaged`, so an installed build ignores it — that guard is the whole security story here and the reason the test asserts the packaged case explicitly. Nothing automated catches a **stale** screenshot. CONTRIBUTING now says a UI-changing PR re-runs `npm run shots`; that is a convention, not a gate. ## Related Stack: #230 → this → #232 (guide content). Related to #70: it defines `e2e/` as the Playwright test directory, which is why this lives in `scripts/`. No change needed there. --- <details> <summary><b>Self-review</b> — 2 findings fixed, 1 follow-up taken anyway, security guard verified</summary> Ran `.github/instructions/code-review.instructions.md` with the judgement pass in a fresh context. Deterministic layer was clean. **2 [fix here] · 1 [follow-up]. All three fixed:** | Dimension | Was | Now | |---|---|---| | architecture 🔵 | `app.setPath` throws when the directory does not exist (Electron typings, `electron.d.ts:1781`). It sat at module scope, before `initLogging()` and before any window, so a stale `TOOLKIT_USER_DATA_DIR` in a shell profile — or a temp dir the OS reaped — killed `npm start` with a stack on stdout and nothing in the log file | wrapped; a bad value is reported on stderr and the real userData is kept. A fourth test covers it, and it **fails without the fix** (verified by reverting the guard) | | cross-platform 🔵 | `spawnSync('npm', …, { shell: true on Windows })` — a bare host `npm` and a Windows shell, both shapes the standard calls out | deleted. `"shots": "npm run build:once && node …"` does the same job with no spawn, no shell, no `existsSync` | | architecture 🔵 | the live tier called `launchApp({})`, and `launchApp` spreads `process.env` — so an exported `TOOLKIT_USER_DATA_DIR` (easiest to acquire while debugging the fixture tier) would silently point the "your real sites" tier at fixture state, producing committed screenshots that are wrong in a way only a careful look at the sidebar reveals | passes `TOOLKIT_USER_DATA_DIR: undefined` explicitly. Taken despite being `[follow-up]`: one line, and the failure mode is a wrong published image | **The security guard was checked hard and stands.** Nothing required above the hook resolves `userData` at require time — `settings-store.js` defers its `import('electron-store')` to the first `getStore()`, and `logging.js` only reads `app.getPath('logs')` inside `initLogging()`. `sessionData` follows the override too, since it resolves lazily from `userData` pre-`ready`. A packaged build is genuinely dead to the variable. **And the test was checked against the "green while proving nothing" list.** It requires the real `src/main.js` under a stubbed `electron` and asserts on calls the real guard makes; deleting either condition fails a case. `Module._load`, `process.env` and the require-cache entry all restore in a `finally`, and `node --test` gives each file its own process, so there is no cross-file leak. **Also verified:** `playwright-core` has no `scripts` field and no `hasInstallScript` in the lockfile, so it owes no `allowScripts` entry and downloads no browsers. `cleanFixtureSites` can only remove the one module-level fixture root; userData dirs come from `mkdtempSync`; an unexpected `variant` still writes only under those roots. </details>
|
Closing this in favour of #360, which builds the same capability with a wider remit — the plumbing here has to serve journey tests that write real state, not just a smoke test that writes none. Two things had gone stale enough that this was a rewrite rather than a rebase. The list of bridged keys checks roughly half the entries the app exposes today, and the native module it probes is no longer optional and no longer absent on Windows, so the deliberate Windows exclusion documented in the file now skips the one failure it was written to catch. What carries over, and it is most of the file:
#67 stays open. It gets its test back on top of #360, with the key list regenerated and the native module asserted on both platforms. |
…362) ## Why Nothing in the suite has ever launched the app. The Git modules are covered against real repositories and the IPC handlers against a loaded main process, but the flows a contributor actually performs — linking a ticket, applying a patch, moving between branches — are verified only by a person following a **How to test this** section by hand. That matters now because of #350. The git-native refactor moves ticket branches, applied patches, conflicts and trunk updates all at once, and a characterisation suite written afterwards can only record the refactor. Written first, it says which of the old guarantees were quietly dropped — which is the failure mode here, since nearly every regression in this area is silent: work parked and never restored, a patch quietly missing a file, a ticket that forgets its base. This PR is the engine. The journeys themselves are #361. ## What changes Two suites, because there are two different apps under test and they answer different questions. **`npm run test:e2e` — the journeys.** Drives the app built from the source tree and writes real state. Everything a journey needs before it can assert anything lives in one session helper, so no spec has to remember it: - **It cannot reach your own sites.** Every launch goes through the development-only data-directory redirect, and then reads back the path the app actually chose and refuses to continue if it is any other one. The env var is set last and is not overridable by the ambient environment. - **Teardown always terminates.** `close()` is raced against a timeout and the process killed after it — closing is known to hang on Windows in apps that keep child processes alive, which this one does constantly. - **A failure leaves evidence.** The trace is Playwright's; the screen and the state the app had persisted are attached here, because the interesting half of a failure in this app is on disk rather than on screen. - **It can relaunch against the same profile.** This is not a convenience. It is the only way to tell "the app persisted this" from "the app still had it in memory", and it is exactly what a change to the storage layer breaks without any other test noticing. **`npm run test:e2e:packaged` — the packaged smoke test.** Launches an unsigned `--dir` build and asks only whether packaging worked. This is the test from the now-closed #70, **rebuilt rather than rebased** — see below. Both run on macOS and Windows for every non-draft pull request, as two separate jobs so a five-second suite is not waiting behind a fifteen-minute one. Neither downloads a browser: the only thing launched is the Electron already in the tree, so there is no `playwright install` step anywhere. **Deliberately not in this PR:** any journey. `e2e/journeys/engine.spec.js` asserts nothing about ticket branches or patches — it asserts that the four things every journey will depend on actually work, so that when a journey fails it is about the flow and not about the harness. ## How to test this Platforms: **any** for the journeys. The packaged half is macOS or Windows only, and the point of the CI matrix is that Windows is the half nobody can check locally. **Starting state:** a clean checkout of this branch, `npm ci` done. Note the modification time of your real settings file before you start — on macOS, `~/Library/Application Support/electron-setup-wordpress-core/settings.json`. 1. `npm run build:once && npm run test:e2e` - Expected: 4 passed, in a few seconds. A real Electron window opens and closes four times. 2. `npm test` - Expected: **1027 passed** — the same count as on `trunk`. The unit runner only collects `test/`, so it must not pick up anything added here. 3. `npm run lint` - Expected: clean. It is repo-wide. 4. `CSC_IDENTITY_AUTO_DISCOVERY=false npm run pack:dir && npm run test:e2e:packaged` - Expected: 4 passed. Skip this if you would rather read the CI result — it takes several minutes. 5. Break the guard on purpose: in `e2e/helpers/app.cjs`, change `TOOLKIT_USER_DATA_DIR: this.userDataDir` to `TOOLKIT_USER_DATA_DIR: undefined`, then `npm run test:e2e`. - Expected: all four tests fail, each saying the app is using your real profile and that it refuses to run. Put the line back. 6. Break an assertion on purpose: in `e2e/packaged/smoke.spec.js`, delete any one entry from `EXPECTED_API_KEYS`, then `npm run test:e2e:packaged`. - Expected: the bridge test fails and names the missing key. Put it back. **What must not have happened:** - **Your real settings file must not have been touched.** Compare its modification time with what you noted in the starting state. This is the whole reason step 5 exists: the guard is the only thing standing between a future change to the redirect hook and a suite that silently edits the site list you actually work on. - **`npm test` must not have grown.** If the count moved off 1027, the unit runner started collecting end-to-end specs, and `npm test` stops being something you run without thinking. - **No Electron processes left running** after any of the above. `ps aux | grep -i electron` on macOS, Task Manager on Windows. ## Risks and limitations - **Windows is unverified by hand.** Everything above was run on macOS. Two of the four fixes in this branch are specifically about Windows behaviour I could not reproduce locally — path shapes and file handles — so the CI run on this PR is the first real evidence either way. That is the honest state of it. - **CI time.** The packaged job packages on both platforms for every non-draft PR: roughly ten to fifteen minutes per platform. The journeys job takes seconds. If the feedback loop gets annoying, the packaging job is what to trim, not the journeys. - **The trunk update cannot be driven from a journey at all**, on any platform. It reaches for a hardcoded clone URL rather than the checkout's own remote, so exercising it means cloning from the network — unacceptable in CI. Its two interesting cases stay in the integration suite, against a local server. Making the update read the remote is worth doing on its own merits and belongs in #350; I left a note there. - The teardown's last-resort kill is a bare `kill()` rather than `src/kill-tree.js`. It only runs after `close()` has already had its timeout, and it is test code, but if orphaned Electron processes ever show up on the runners, that is where to look. ## Related Part of #359. Closes #360. **Closes #67** — its three assertions ship here as the `packaged` project, on both platforms, documented in `CONTRIBUTING.md`, with Buildkite untouched. Replaces #70, which is closed. Unblocks #361, and #350 after it. --- <details> <summary>Design decisions and alternatives considered</summary> **`@playwright/test` rather than `node --test` with `playwright-core`.** The first draft went the other way, to keep one runner in the repo and add no dependency. Requiring the suite to run in CI on two platforms for every pull request inverted that: retries, per-test timeouts, traces, the HTML report and the reporting artifacts are all things I would otherwise have written and then maintained, badly. Two runners is not a defect here — `npm test` stays the fast offline suite and `npm run test:e2e` is the one that launches an app, and they are meant to be told apart. The dependency cost turned out to be smaller than assumed: `@playwright/test` at this version has no install script and downloads no browser. I checked the published tarball rather than trusting the recollection — an older version of Playwright did download browsers on install, which is why `scripts/screenshots/capture.cjs` depends on `playwright-core` directly and says so in its header. **A session object rather than a `page` fixture.** A journey has to build its Git fixture *before* the app opens, because the site list has to name directories that already exist, and it needs the profile path to write that list. A fixture that launches the app for you takes that away. So the profile directory exists from construction and the spec decides when to start. **Rebuilt #70 rather than rebasing it.** Two of its three assertions had gone stale in ways that would fail on first run: the list of bridged keys had roughly half the entries the app exposes today, and the native module it probes is no longer optional and no longer absent on Windows — it is a hard dependency of the bundled PHP runtime with a Windows-specific file-lock path — so its documented Windows exclusion had turned from a decision into a hole. Its structure, its comments and its reasoning are otherwise carried over almost verbatim; the closing comment on #70 lists what came across. **Not adding `data-testid` anywhere.** The selectors use the text and roles already on screen, which also pins the visible copy as a contract. Where a name appears twice — the sidebar entry and the heading of the open site — roles tell them apart, and the assertion says which half of the app it is about. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> `2 [fix here] · 2 [follow-up]` — both `[fix here]` fixed before this was pushed. **🔴 cross-platform · `[fix here]` — fixed.** The launch guard compared `path.resolve(a) !== path.resolve(b)`. That would have aborted every test on Windows, and possibly on macOS, for reasons unrelated to what it guards: macOS returns `/var/folders/…` from `os.tmpdir()` and `/private/var/…` from the app, since the first is a symlink to the second, and Windows returns short `RUNNER~1` names in some environments and long ones in others, on a case-insensitive filesystem. Replaced with a `samePath()` that resolves through `realpath` and lowercases off POSIX. **🔴 cross-platform · `[fix here]` — fixed.** The fake site directories were removed in an `afterEach`, which Playwright runs *before* fixture teardown — with the app still alive and holding handles on them. On Windows that is `EPERM`, and a failure that is not the test's. Cleanup moved onto the session, which removes them after `close()`. **🟡 tests · `[follow-up]`.** The teardown's last-resort `kill()` is not `src/kill-tree.js`. Noted in **Risks** above. **🔵 performance · `[follow-up]`.** The packaging job's cost per pull request. Noted in **Risks** above. No findings in architecture (nothing under `src/` changes), security (the file-dialog stub is installed at runtime from the test, so nothing test-shaped ships in the app), or the new-dependency rule (devDependency, no install script — verified against the published tarball — no native compilation, so no `allowScripts` entry is needed and the zero-prerequisite promise is untouched). **One caveat.** The standard asks for this pass to run in fresh context rather than in the session that wrote the code. It did not: the session that wrote this reviewed it. Worth a second pass by someone who has not already decided it is correct. </details> <details> <summary>Implementation notes</summary> Two things surfaced only by running this against the current tree, neither of which #70 could have seen: **`process.mainModule.filename` is not stable in Electron.** The module-resolution assertions anchored a `createRequire` there. Partway through a run it reads back as the bare string `'electron'`, so the first module checked failed and the same test passed when run alone — a packaging test going red for a reason that has nothing to do with packaging. Now anchored at `join(app.getAppPath(), 'package.json')`, which is deterministic and is inside the asar either way. **Two bridged keys cannot be found by reading `src/preload.js`.** `signInToGithub` and `cancelGithubSignIn` are spread in from an immediately invoked function that closes over a listener, so they exist only once the file has run. Any attempt to derive the expected-keys list by grepping the object literal misses them — which is the same class of gap the assertion itself exists to close, one level up. There is a comment in the file saying so, because the next person to regenerate that list will otherwise reintroduce the same two-key error. The `react-hooks/rules-of-hooks` override for `e2e/` is not a suppression of a real problem: Playwright's fixture API is `async ({ deps }, use) => {}`, and the rule reads a bare `use(...)` as React's `use` hook outside a component. There is no React in that directory; the app under test is a separate process. </details> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Closes #67.
Why
Nothing verifies that the packaged app launches. Packaging failures — asar layout, native module rebuilds, bundled CLI resolution — were only discoverable by downloading an artifact and installing it in a VM.
What
One Playwright
_electrontest against an unsignedelectron-builder --dirbuild, onmacos-latestandwindows-latest, for non-draft PRs and pushes totrunk.contextBridgekey is compared against a checked-in list. There is no typecheck in this repo, so nothing else catches anipcMain.handleadded insrc/main.jsand never exposed insrc/preload.js.@wp-playground/cli, andfs-exton macOS and Linux.fs-extis an optional native dependency, so a failed rebuild degrades file locking with no error anywhere in the install or build logs; this is the only thing that catches it. Windows is excluded on purpose — the Playground CLI does not load the module there (os.platform() === 'win32' ? undefined : await import('fs-ext'), with an upstream@TODO: Enable fs-ext here when it works with Windows). If that guard ever goes away, Windows belongs back in the list; the note lives next to the module list in the spec.Deliberately minimal: one spec, no session helper, no page objects, no temp state directories.
Verified
Ran locally on macOS arm64, then broke each thing on purpose to confirm the test can actually go red:
loadFile→ a missing filegetSitesfromsrc/preload.jsrm -rf node_modules/fs-extUnit suite unaffected — 28 tests green on both the system Node and Electron's bundled Node;
node --testdoes not pick upe2e/*.spec.js.Notes
trunkafter Run the unit suite on macOS and Windows, on both Node runtimes #68/Fix the Electron test pass silently breaking on Node 22 and newer #69 landed; the workflow follows the conventions inunit-tests.yml.ready_for_reviewhad to be added to the trigger list. It is not in thepull_requestdefault set, so a PR opened as a draft and later marked ready would have skipped its only run and never tested anything. Caught on this PR's own first ready transition.CSC_IDENTITY_AUTO_DISCOVERY=falseis mandatory on macOS — electron-builder signs during--dirotherwise. Windows signing already no-ops without the Azure env vars.electronApp.close()is raced against a timeout and a hard kill, since it hangs on Windows for apps that keep child processes alive.pageerrorlistener attaches too late to be reliable.🤖 Generated with Claude Code