Add an end-to-end engine that drives the app against throwaway state

[juanmaguitar]

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 The checkout is a standard Git repository; the app is a porcelain over it #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.

---

Design decisions and alternatives considered

@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.

Review outcome (required — see AGENTS.md)

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.

Implementation notes

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.