e2e: packaged-app smoke test on macOS and Windows - #70
Open
juanmaguitar wants to merge 6 commits into
Open
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>
juanmaguitar
force-pushed
the
juanmaguitar/e2e-packaged-smoke-test
branch
from
July 30, 2026 14:42
e39670b to
841acf2
Compare
juanmaguitar
marked this pull request as ready for review
July 30, 2026 14:45
`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>
3 tasks
`--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>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| "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().
This was referenced Aug 10, 2026
juanmaguitar
added a commit
that referenced
this pull request
Aug 10, 2026
) ## 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>
juanmaguitar
added a commit
that referenced
this pull request
Aug 10, 2026
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`.
juanmaguitar
added a commit
that referenced
this pull request
Aug 10, 2026
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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