From 7a1fca667612177909fa673f9726f0c3bf5944d2 Mon Sep 17 00:00:00 2001 From: Jack Lau <72348727+jackulau@users.noreply.github.com> Date: Sun, 16 Aug 2026 22:13:28 -0500 Subject: [PATCH] fix(desktop): resolve the e2e Electron binary per platform and layout findElectron() probed exactly one path, and got three things wrong at once for anyone not on a hoisted POSIX install: * It looked only under the REPO ROOT. This is an npm workspaces repo and npm hoists a dependency only when nothing conflicts, so `electron` installing into apps/desktop/node_modules is an ordinary outcome, not a broken tree. * It joined a bare `electron`. On Windows the dist file is `electron.exe`, so the probe could never match there. * Its PATH fallback spawned `which`, which is not a command on Windows, so the fallback failed for a reason unrelated to whether electron is on PATH. The three combine into a misleading error: the suite refuses to start with 'Run "npm install" from the repo root' on a tree that has electron installed. Reproduced on Windows 11 against this repo, where apps/desktop/node_modules/electron/dist/electron.exe exists and the old body throws that message; the reporter on #88036 hit the same thing on Linux and had to hand-symlink the package before the suite would run. Resolution now asks the installed `electron` package for its own path first (its main export IS the absolute executable, resolved from path.txt and honouring ELECTRON_OVERRIDE_DIST_PATH), then falls back to explicit dist probes for each root, then to PATH with the platform's lookup command. The error message lists what was searched. The rules live in e2e/electron-binary.ts so they can be unit-tested without importing the Playwright runner, with the platform passed in rather than read from process.platform: reading it would leave every Windows rule untested on the Linux CI runner. Wiring: the vitest `electron` project picks up e2e/**/*.unit.test.ts and Playwright ignores the same pattern, so helper unit tests run in exactly one runner and the specs are untouched. Verified: 5 unit tests pass; mutation-checked one rule at a time (hardcoding the binary name fails 2, reversing the probe order fails 1, hardcoding `which` fails 1). tsc -p . and tsc -p tsconfig.e2e.json clean. This is the environment blocker called out in #88036, not its rendering bug, so it is deliberately a subset. Refs #88036 --- apps/desktop/e2e/electron-binary.ts | 100 ++++++++++++++++++ apps/desktop/e2e/electron-binary.unit.test.ts | 54 ++++++++++ apps/desktop/e2e/fixtures.ts | 28 ++--- apps/desktop/playwright.config.ts | 4 + apps/desktop/vitest.config.ts | 5 +- 5 files changed, 170 insertions(+), 21 deletions(-) create mode 100644 apps/desktop/e2e/electron-binary.ts create mode 100644 apps/desktop/e2e/electron-binary.unit.test.ts diff --git a/apps/desktop/e2e/electron-binary.ts b/apps/desktop/e2e/electron-binary.ts new file mode 100644 index 0000000000..c272c00be1 --- /dev/null +++ b/apps/desktop/e2e/electron-binary.ts @@ -0,0 +1,100 @@ +/** + * Locating the dev Electron binary for the e2e fixtures. + * + * Kept in its own module so the resolution rules can be unit-tested without + * importing the Playwright runner (fixtures.ts pulls in `_electron`, the mock + * server and the error-banner guard). + * + * Three rules the previous single-path probe got wrong: + * + * 1. The binary is not always under the REPO ROOT. This is an npm workspaces + * repo, and npm only hoists a dependency to the root when nothing conflicts + * — otherwise `electron` installs into `apps/desktop/node_modules`. Both + * layouts are normal, so both have to be searched, nearest package first. + * 2. The binary is `electron.exe` on Windows. A bare `electron` never exists + * there, so the probe could only ever miss. + * 3. `which` is not a command on Windows. The PATH fallback spawned it + * unconditionally, so on Windows the fallback failed for the wrong reason + * and the error message blamed a missing `npm install`. + */ + +import { spawnSync } from 'node:child_process' +import * as fs from 'node:fs' +import { createRequire } from 'node:module' +import * as path from 'node:path' + +/** The dist file name: `electron.exe` on Windows, `electron` elsewhere. */ +export function electronBinaryName(platform: NodeJS.Platform = process.platform): string { + return platform === 'win32' ? 'electron.exe' : 'electron' +} + +/** + * Where an npm install can leave the binary, in probe order: nearest package + * first, so a workspace-local install wins over a stale hoisted one. + */ +export function electronDistCandidates(roots: string[], platform: NodeJS.Platform = process.platform): string[] { + return roots.map((root) => path.join(root, 'node_modules', 'electron', 'dist', electronBinaryName(platform))) +} + +/** The PATH-lookup command for this platform. Windows has `where`, not `which`. */ +export function pathLookupCommand(platform: NodeJS.Platform = process.platform): string { + return platform === 'win32' ? 'where' : 'which' +} + +/** + * Ask the installed `electron` package where its own binary is. + * + * Its main export IS the absolute executable path, resolved from `path.txt` + * and honouring `ELECTRON_OVERRIDE_DIST_PATH`, so this covers layouts and + * overrides a hand-built path cannot know about. Returns null when the package + * is not resolvable from `from`, or when it does not hand back a path (the + * export is the Electron API object, not a path, when required from inside + * Electron itself). + */ +export function electronPackagePath(from: string): null | string { + try { + const resolved = createRequire(path.join(from, 'package.json'))('electron') as unknown + + return typeof resolved === 'string' && resolved ? resolved : null + } catch { + return null + } +} + +/** + * Resolve the Electron binary, or throw with the layouts that were searched. + * + * `roots` are searched in order; pass the desktop package before the repo root. + */ +export function resolveElectronBinary(roots: string[]): string { + for (const root of roots) { + const declared = electronPackagePath(root) + + if (declared && fs.existsSync(declared)) { + return declared + } + } + + for (const candidate of electronDistCandidates(roots)) { + if (fs.existsSync(candidate)) { + return candidate + } + } + + // Nix devshells put `electron` on PATH with no node_modules copy at all. + const lookup = spawnSync(pathLookupCommand(), ['electron'], { encoding: 'utf8' }) + + if (lookup.status === 0 && lookup.stdout.trim()) { + // `where` reports every match, one per line; take the first. + const first = lookup.stdout.trim().split(/\r?\n/)[0].trim() + + if (first) { + return first + } + } + + throw new Error( + `Electron binary not found. Searched ${electronDistCandidates(roots).join(', ')} and PATH. ` + + 'Run "npm install" from the repo root to install devDependencies.', + ) +} diff --git a/apps/desktop/e2e/electron-binary.unit.test.ts b/apps/desktop/e2e/electron-binary.unit.test.ts new file mode 100644 index 0000000000..c33d8dd49d --- /dev/null +++ b/apps/desktop/e2e/electron-binary.unit.test.ts @@ -0,0 +1,54 @@ +import * as path from 'node:path' + +import { describe, expect, it } from 'vitest' + +import { electronBinaryName, electronDistCandidates, pathLookupCommand } from './electron-binary' + +// Platform is a parameter everywhere below rather than read from +// process.platform, so the Windows rules are pinned on the Linux CI runner too. +// Reading the real platform would leave every Windows-only rule untested. + +describe('electronBinaryName', () => { + it('asks for electron.exe on Windows', () => { + expect(electronBinaryName('win32')).toBe('electron.exe') + }) + + it('asks for a bare electron everywhere else', () => { + expect(electronBinaryName('linux')).toBe('electron') + expect(electronBinaryName('darwin')).toBe('electron') + }) +}) + +describe('electronDistCandidates', () => { + const desktop = path.join('repo', 'apps', 'desktop') + const repo = 'repo' + + it('probes the workspace-local install before the hoisted one', () => { + // npm only hoists `electron` to the repo root when nothing conflicts, so + // apps/desktop/node_modules is an ordinary outcome of `npm install`, not a + // broken tree. Probing only the repo root is what makes the suite refuse to + // start with "run npm install" on a tree that has electron installed. + expect(electronDistCandidates([desktop, repo], 'linux')).toEqual([ + path.join(desktop, 'node_modules', 'electron', 'dist', 'electron'), + path.join(repo, 'node_modules', 'electron', 'dist', 'electron'), + ]) + }) + + it('carries the platform binary name into every candidate', () => { + // A bare `electron` file never exists in a Windows dist, so a probe built + // from a hardcoded name cannot match there no matter which root it walks. + for (const candidate of electronDistCandidates([desktop, repo], 'win32')) { + expect(path.basename(candidate)).toBe('electron.exe') + } + }) +}) + +describe('pathLookupCommand', () => { + it('uses where on Windows and which elsewhere', () => { + // `which` is not a command on Windows; spawning it unconditionally made the + // PATH fallback fail for a reason unrelated to whether electron is on PATH. + expect(pathLookupCommand('win32')).toBe('where') + expect(pathLookupCommand('linux')).toBe('which') + expect(pathLookupCommand('darwin')).toBe('which') + }) +}) diff --git a/apps/desktop/e2e/fixtures.ts b/apps/desktop/e2e/fixtures.ts index 787be42188..5e2774b2c0 100644 --- a/apps/desktop/e2e/fixtures.ts +++ b/apps/desktop/e2e/fixtures.ts @@ -20,13 +20,13 @@ * Prerequisite: `npm run build` must have been run so that `dist/` exists. */ -import { spawnSync } from 'node:child_process' import * as fs from 'node:fs' import * as os from 'node:os' import * as path from 'node:path' import { _electron, type ElectronApplication, type Page } from '@playwright/test' +import { resolveElectronBinary } from './electron-binary' import { startMockServer, type MockServerOptions } from './mock-server' import { installErrorBannerGuard } from './test' @@ -281,30 +281,18 @@ function assertDistBuilt(): void { /** * Find the Electron binary. In the nix devshell, `electron` is on PATH. - * As a fallback, use the node_modules/.bin/electron from the desktop package. + * As a fallback, use the node_modules/electron install from either package. */ export function findElectron(): string { // In dev mode, we use the `electron` binary directly (not the packaged app). // The dev:electron script in package.json does exactly this: `electron .` // after building. We replicate that here. - const localElectron = path.join(REPO_ROOT, 'node_modules', 'electron', 'dist', 'electron') - - if (fs.existsSync(localElectron)) { - return localElectron - } - - // Fall back to PATH - const result = spawnSync('which', ['electron'], { - encoding: 'utf8', - }) - - if (result.status === 0 && result.stdout.trim()) { - return result.stdout.trim() - } - - throw new Error( - 'Electron binary not found. Run "npm install" from the repo root to install devDependencies.', - ) + // + // The desktop package is searched first: npm workspaces only hoist + // `electron` to the repo root when nothing conflicts, so a workspace-local + // install is just as ordinary an outcome as a hoisted one. The rules live in + // ./electron-binary so they can be unit-tested per platform. + return resolveElectronBinary([DESKTOP_ROOT, REPO_ROOT]) } /** diff --git a/apps/desktop/playwright.config.ts b/apps/desktop/playwright.config.ts index dc06c59fe7..100c002252 100644 --- a/apps/desktop/playwright.config.ts +++ b/apps/desktop/playwright.config.ts @@ -28,6 +28,10 @@ export default defineConfig({ /* Test files live under e2e/ so they never collide with the vitest suite * under src/ or the node:test files under electron/. */ testDir: './e2e', + /* ...except `*.unit.test.ts`, which covers the e2e HELPERS (no Electron, no + * app) and is owned by the vitest `electron` project. Without this the + * default testMatch would claim those files too and run them twice. */ + testIgnore: '**/*.unit.test.ts', /* The desktop app can take a while to bootstrap on cold CI runners — 90 s * per test gives us headroom without masking real hangs. */ timeout: 90_000, diff --git a/apps/desktop/vitest.config.ts b/apps/desktop/vitest.config.ts index 70835d91f0..619683e82e 100644 --- a/apps/desktop/vitest.config.ts +++ b/apps/desktop/vitest.config.ts @@ -20,7 +20,10 @@ const electronNative: TestProjectConfiguration = { test: { name: 'electron', environment: 'node', - include: ['electron/**/*.test.ts', 'scripts/**.test.{ts,mjs}'], + // `e2e/**/*.unit.test.ts` is the e2e HELPERS, not the specs: plain node + // modules that should be provable without booting Electron. Playwright + // ignores the same pattern so they run in exactly one runner. + include: ['electron/**/*.test.ts', 'scripts/**.test.{ts,mjs}', 'e2e/**/*.unit.test.ts'], exclude: ['scripts/run-short-session-hang-repro.test.mjs'] } }