From 011cd88f8c382dd9bc4b65ae2b468834f1424b2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacy=20=C5=81=C4=85tka?= Date: Mon, 3 Aug 2026 16:07:15 +0200 Subject: [PATCH] test(chromium): stop electron boot tests depending on host CDP ports boot-electron-spawn-error.test.ts drove bootElectronApp against hardcoded CDP ports 19222-19226 and relied on nothing answering there. Any Chromium or Electron endpoint on one of those ports lets waitForCdpReady win bootElectronApp's readiness race, so the boot resolves and detaches the child's boot listeners before the test emits its synthetic spawn error; the emit then throws on a listener-less EventEmitter instead of rejecting. 19222 is argent's own example port and appears in nine sibling test files, so a live endpoint there is a realistic host condition rather than a hypothetical one. Route those boots through a single UNREACHABLE_CDP_PORT constant pinned to the privileged port 1, which no unprivileged process can bind. Apply the same fix to the one sibling sharing the shape: chromium-discovery's dead-port pruning test asserted an unreachable port in the ephemeral range, which a listen(0) fake CDP server in another test file can be handed. Also redirect ARGENT_CHROMIUM_PORTS_FILE to a throwaway path. The file's successful-boot test ran trackChromiumPort against the real ~/.argent/chromium-cdp-ports.json and appended an ephemeral port on every run; both sibling files already guard against that. --- .../test/boot-electron-spawn-error.test.ts | 57 +++++++++++++++---- .../test/chromium-discovery.test.ts | 21 ++++--- 2 files changed, 58 insertions(+), 20 deletions(-) diff --git a/packages/tool-server/test/boot-electron-spawn-error.test.ts b/packages/tool-server/test/boot-electron-spawn-error.test.ts index 415dd76a6..4644c52dd 100644 --- a/packages/tool-server/test/boot-electron-spawn-error.test.ts +++ b/packages/tool-server/test/boot-electron-spawn-error.test.ts @@ -17,6 +17,24 @@ import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; +/** + * CDP port for the boots below that must never reach a live endpoint. + * + * Every test that drives a spawn-error / early-exit / no-pid path needs + * `waitForCdpReady` to keep failing, so the readiness probe cannot win + * `bootElectronApp`'s `Promise.race`. When it does win, the boot resolves and + * detaches the child's boot listeners — the synthetic `'error'` emit that + * follows then hits an EventEmitter with no `'error'` listener and throws + * instead of rejecting, failing the test. + * + * Port 1 is privileged, so no unprivileged process on the host can bind it. + * A port in the unprivileged range is bindable by anything: argent's own + * examples and sibling tests use `chromium-cdp-19222`, so a real Chromium / + * Electron endpoint on 19222-19226 is exactly the kind of neighbour that turns + * these tests into host-dependent coin flips. + */ +const UNREACHABLE_CDP_PORT = 1; + const spawnMock = vi.fn(); vi.mock("node:child_process", async () => { @@ -50,7 +68,19 @@ function makeFakeChild(opts: { pid?: number | undefined } = {}): FakeChild { } let appDir: string; +let prevPortsFile: string | undefined; +const TEST_PORTS_FILE = path.join( + os.tmpdir(), + `argent-boot-electron-spawn-ports-${process.pid}.json` +); beforeAll(() => { + // The successful boot below calls trackChromiumPort, which persists the + // booted port to ~/.argent/chromium-cdp-ports.json. Redirect that to a + // throwaway file so tests never touch the real state on a developer machine + // or CI runner — the port they would leave behind is an ephemeral one that + // list-devices then probes on every call. + prevPortsFile = process.env.ARGENT_CHROMIUM_PORTS_FILE; + process.env.ARGENT_CHROMIUM_PORTS_FILE = TEST_PORTS_FILE; // resolveLauncher() fs-checks the app path before spawn, so the test needs // a real directory on disk. The spawn itself is mocked, so the contents // don't matter — only the path's existence. @@ -62,6 +92,9 @@ beforeAll(() => { fs.writeFileSync(path.join(appDir, "main.js"), "// fake\n"); }); afterAll(() => { + if (prevPortsFile === undefined) delete process.env.ARGENT_CHROMIUM_PORTS_FILE; + else process.env.ARGENT_CHROMIUM_PORTS_FILE = prevPortsFile; + fs.rmSync(TEST_PORTS_FILE, { force: true }); if (appDir) fs.rmSync(appDir, { recursive: true, force: true }); }); @@ -76,7 +109,7 @@ describe("bootElectronApp — spawn error handling", () => { const promise = bootElectronApp({ appPath: appDir, - port: 19222, + port: UNREACHABLE_CDP_PORT, readyTimeoutMs: 100, }); promise.catch(() => {}); // detach so the test doesn't hang after assertion @@ -107,7 +140,7 @@ describe("bootElectronApp — spawn error handling", () => { appPath: appDir, // Unreachable port → the readiness race rejects fast; we only care about // the spawn env, so detach and swallow the rejection. - port: 1, + port: UNREACHABLE_CDP_PORT, readyTimeoutMs: 50, }); promise.catch(() => {}); @@ -134,7 +167,7 @@ describe("bootElectronApp — spawn error handling", () => { const promise = bootElectronApp({ appPath: appDir, - port: 1, + port: UNREACHABLE_CDP_PORT, readyTimeoutMs: 50, extraArgs: ["--user-flag"], }); @@ -147,7 +180,7 @@ describe("bootElectronApp — spawn error handling", () => { expect(args).toContain("--disable-renderer-backgrounding"); // User extras survive alongside the defaults. expect(args).toContain("--user-flag"); - expect(args).toContain("--remote-debugging-port=1"); + expect(args).toContain(`--remote-debugging-port=${UNREACHABLE_CDP_PORT}`); await promise.catch(() => {}); }); @@ -158,7 +191,7 @@ describe("bootElectronApp — spawn error handling", () => { const promise = bootElectronApp({ appPath: appDir, - port: 19223, + port: UNREACHABLE_CDP_PORT, readyTimeoutMs: 30_000, }); @@ -180,7 +213,7 @@ describe("bootElectronApp — spawn error handling", () => { const promise = bootElectronApp({ appPath: appDir, - port: 19224, + port: UNREACHABLE_CDP_PORT, readyTimeoutMs: 30_000, }); await new Promise((r) => setTimeout(r, 10)); @@ -200,7 +233,7 @@ describe("bootElectronApp — spawn error handling", () => { await expect( bootElectronApp({ appPath: appDir, - port: 19225, + port: UNREACHABLE_CDP_PORT, readyTimeoutMs: 100, }) ).rejects.toThrow(/spawn returned without a pid/); @@ -313,10 +346,10 @@ describe("bootElectronApp — spawn error handling", () => { await expect( bootElectronApp({ appPath: appDir, - // Pick an unbound port; ensureCdpReachable will fail repeatedly - // until readyTimeoutMs elapses. Don't pick 0 — we want a real - // unreachable port, not OS-assigned ephemeral. - port: 1, + // ensureCdpReachable fails repeatedly until readyTimeoutMs elapses. + // Don't pick 0 — we want a real unreachable port, not an OS-assigned + // ephemeral one. + port: UNREACHABLE_CDP_PORT, readyTimeoutMs: 100, }) ).rejects.toBeInstanceOf(Error); @@ -358,7 +391,7 @@ describe("bootElectronApp — spawn error handling", () => { await expect( bootElectronApp({ appPath: appDir, - port: 19226, + port: UNREACHABLE_CDP_PORT, readyTimeoutMs: 100, }) ).rejects.toThrow(/spawn returned without a pid/); diff --git a/packages/tool-server/test/chromium-discovery.test.ts b/packages/tool-server/test/chromium-discovery.test.ts index f25bc54e1..50ee47eb3 100644 --- a/packages/tool-server/test/chromium-discovery.test.ts +++ b/packages/tool-server/test/chromium-discovery.test.ts @@ -186,14 +186,19 @@ describe("port persistence across tool-server restarts", () => { }); it("a dead persisted port is pruned from the file after a failed probe", async () => { - trackChromiumPort(43211); - portsToCleanup.push(43211); - expect(JSON.parse(fs.readFileSync(TEST_PORTS_FILE, "utf8"))).toContain(43211); - - // Nothing listens on 43211 — the probe fails and prunes it everywhere. - await discoverChromiumDevices({ timeoutMs: 300, ports: [43211] }); - expect(JSON.parse(fs.readFileSync(TEST_PORTS_FILE, "utf8"))).not.toContain(43211); - expect(getCandidateChromiumPorts()).not.toContain(43211); + // Port 1 is privileged, so no unprivileged process on the host can bind it + // and the probe below is guaranteed to fail. A port in the unprivileged + // range would be bindable by anything — including a `listen(0)` fake CDP + // server in a sibling test file, which serves the very /json responses that + // would make this probe succeed and leave the port tracked. + const deadPort = 1; + trackChromiumPort(deadPort); + portsToCleanup.push(deadPort); + expect(JSON.parse(fs.readFileSync(TEST_PORTS_FILE, "utf8"))).toContain(deadPort); + + await discoverChromiumDevices({ timeoutMs: 300, ports: [deadPort] }); + expect(JSON.parse(fs.readFileSync(TEST_PORTS_FILE, "utf8"))).not.toContain(deadPort); + expect(getCandidateChromiumPorts()).not.toContain(deadPort); }); it("untrackChromiumPort removes the port from the persisted file", () => {