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
This commit is contained in:
100
apps/desktop/e2e/electron-binary.ts
Normal file
100
apps/desktop/e2e/electron-binary.ts
Normal file
@@ -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.',
|
||||
)
|
||||
}
|
||||
54
apps/desktop/e2e/electron-binary.unit.test.ts
Normal file
54
apps/desktop/e2e/electron-binary.unit.test.ts
Normal file
@@ -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')
|
||||
})
|
||||
})
|
||||
@@ -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])
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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']
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user