mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
13f43bd0cd
Follow-up to #709, which merged while this was still under review. #709 landed the port fix; these are the two gaps a review pass over it turned up, both in the same two files. ## The ports-file guard was pinned by nothing #714 added `vi.mock("../src/utils/chromium-discovery", …)` to `boot-electron-spawn-error.test.ts` so a completed boot stops persisting its port into the developer's real `~/.argent/chromium-cdp-ports.json`. Nothing asserted it still worked. Measured, on the pre-change file: deleting the mock leaves the suite **green at 9/9** while `$HOME/.argent/chromium-cdp-ports.json` is created containing a live ephemeral port. Replacing it with a call-through variant — the shape `list-devices.test.ts:76-81` already uses on this same module — does the same: ```ts vi.mock("../src/utils/chromium-discovery", async () => { const actual = await vi.importActual<...>("../src/utils/chromium-discovery"); return { ...actual, trackChromiumPort: vi.fn(actual.trackChromiumPort), untrackChromiumPort: vi.fn(actual.untrackChromiumPort) }; }); ``` → `Tests 9 passed (9)`, and the ports file contains `[55770]`. That is how the file accumulated entries before #714, so the guard rotting back is not hypothetical. Each file now scopes `HOME` with #714's own `scopeTempHome()` helper and asserts, after a completed boot, that no ports file exists. Both mutations now fail. **All three files that carry this mock are fixed, not just the one.** `boot-electron-kill.test.ts` and `boot-electron-ready-race.test.ts` hold the byte-identical mock — under a comment naming each other — and each completes a real boot. Measured before this change: deleting the mock left `kill` green at **15/15** while eleven live ports, one per boot, were written; the call-through variant did the same to `ready-race` **while still satisfying its own `expect(trackChromiumPort).toHaveBeenCalledWith(port)` at line 175**, which is precisely why the absence assertion is the right shape. Asserting the absence rather than `expect(trackChromiumPort).toHaveBeenCalledWith(port)` is deliberate: the call is already pinned by `boot-electron-ready-race.test.ts:175`, and a spy assertion only proves the export *is* a spy — the call-through mutation satisfies it while still writing. ## The readiness timeout was pinned by its wording, not its code The catch-path test matched `/CDP never became reachable/`. `FAILURE_CODES.CHROMIUM_ELECTRON_CDP_TIMEOUT` had **no assertion anywhere in the repo** — `grep -rn CHROMIUM_ELECTRON_CDP_TIMEOUT packages/ --include=*.ts` outside `dist/` hits only the throw site and the enum. Swapping the throw site to `CHROMIUM_ELECTRON_SPAWN_FAILED` left `boot-electron-spawn-error` + `-kill` + `-ready-race` + `failure-classification` + `chromium-discovery` at **60/60 passed**. So the half agents and telemetry bucket on was free to change, while a harmless reword would have broken the test. Now pinned by code via `getFailureSignal`, matching how `boot-electron-ready-race.test.ts:137` pins its own failure path. It still discriminates the vacuity the message match was added for: a nonexistent `appPath` yields `CHROMIUM_ELECTRON_APP_PATH_INVALID` and fails. `failure_stage` is pinned alongside it, as `ready-race` already does for its own path — `electron_cdp_ready` likewise had no assertion anywhere. ## Two rationales corrected - The catch-path comment blamed "no HTTP server on the port" for the probe failing. `fetch` declines port 1 as a WHATWG bad port *before it dials* — verified with `net.connect` hooked: **0 sockets opened** for port 1, 2 sockets for ports 2, 1023 and 19222. No listener is involved either way, and the file's own constant JSDoc says so. - The `chromium-discovery.test.ts` constant pointed at "a `listen(0)` fake CDP server **in a sibling test file**", when the server that makes the point is `startFakeCdpServer` in this same file, above the constant. - It also credited "anything in the unprivileged range" with being bindable by `listen(0)`. `listen(0)` draws from the kernel's *ephemeral* range — `net.inet.ip.portrange` 49152-65535 on macOS, `ip_local_port_range` 32768-60999 on the Ubuntu runner. That narrower range is what put the old `43211` at risk; calling it "unprivileged" also revives the privilege reasoning the sibling constant explicitly disowns. ## `chromium-discovery.test.ts` gets a second layer too It is the one file here that drives the **real** `persistPorts`, and its only guard was a single `beforeAll` line setting `ARGENT_CHROMIUM_PORTS_FILE`. That line is pinned — deleting it fails 3 tests — but the write lands before the detection, and `persistPorts` merges rather than replaces, so on a real machine the test ports would be appended for `list-devices` to probe afterwards. It now scopes `HOME` as well, so a lapsed redirect falls back somewhere disposable. Measured with a pre-seeded `$HOME/.argent/chromium-cdp-ports.json` = `[9222]`: deleting the redirect still fails 3 tests, and the seeded file comes back **unchanged**. ## Verification | mutation | before | after | |---|---|---| | delete the module mock | 9/9 pass, ports file written | fails | | call-through `vi.fn(actual.trackChromiumPort)` | 9/9 pass, ports file written | fails | | swap `CHROMIUM_ELECTRON_CDP_TIMEOUT` at the throw site | 60/60 pass | fails | | nonexistent `appPath` in the catch-path test | — | fails (`APP_PATH_INVALID`) | | swap `failure_stage` at the throw site | — | fails | | delete / call-through mock in `boot-electron-kill` | 15/15 pass, 11 ports written | 11 failed \| 4 passed | | call-through mock in `boot-electron-ready-race` | 2/2 pass | 1 failed \| 1 passed | Full `packages/tool-server` in 6 shards: 4303 passed, 1 skipped, every shard exit 0, and no `.argent` directory created under a redirected `HOME`. `eslint` 0, `prettier --check` 0, `tsc --build` 0, `tsc -p tsconfig.test.json` 0, `knip` 0 — exit codes read directly.
229 lines
7.8 KiB
TypeScript
229 lines
7.8 KiB
TypeScript
import { describe, it, expect, afterEach, beforeAll, afterAll, vi } from "vitest";
|
|
import * as fs from "node:fs";
|
|
import * as http from "node:http";
|
|
import * as os from "node:os";
|
|
import * as path from "node:path";
|
|
import { AddressInfo } from "node:net";
|
|
import { scopeTempHome } from "./helpers/temp-home";
|
|
import {
|
|
discoverChromiumDevices,
|
|
getCandidateChromiumPorts,
|
|
trackChromiumPort,
|
|
untrackChromiumPort,
|
|
} from "../src/utils/chromium-discovery";
|
|
|
|
interface FakeCdpServer {
|
|
port: number;
|
|
close: () => Promise<void>;
|
|
}
|
|
|
|
async function startFakeCdpServer(options?: {
|
|
responses?: {
|
|
version?: number | object;
|
|
list?: number | object;
|
|
};
|
|
}): Promise<FakeCdpServer> {
|
|
const server = http.createServer((req, res) => {
|
|
if (req.url === "/json/version") {
|
|
const r = options?.responses?.version ?? {
|
|
Browser: "Chrome/148.0.7778.97",
|
|
webSocketDebuggerUrl: `ws://127.0.0.1:0/devtools/browser/x`,
|
|
};
|
|
if (typeof r === "number") {
|
|
res.statusCode = r;
|
|
res.end();
|
|
return;
|
|
}
|
|
res.setHeader("content-type", "application/json");
|
|
res.end(JSON.stringify(r));
|
|
return;
|
|
}
|
|
if (req.url === "/json/list") {
|
|
const r = options?.responses?.list ?? [
|
|
{
|
|
id: "abc",
|
|
type: "page",
|
|
title: "Test Page",
|
|
url: "file:///tmp/index.html",
|
|
webSocketDebuggerUrl: `ws://127.0.0.1:0/devtools/page/abc`,
|
|
},
|
|
];
|
|
if (typeof r === "number") {
|
|
res.statusCode = r;
|
|
res.end();
|
|
return;
|
|
}
|
|
res.setHeader("content-type", "application/json");
|
|
res.end(JSON.stringify(r));
|
|
return;
|
|
}
|
|
res.statusCode = 404;
|
|
res.end();
|
|
});
|
|
|
|
await new Promise<void>((resolve) => server.listen(0, "127.0.0.1", resolve));
|
|
const port = (server.address() as AddressInfo).port;
|
|
return {
|
|
port,
|
|
close: () =>
|
|
new Promise<void>((resolve) => {
|
|
server.close(() => resolve());
|
|
}),
|
|
};
|
|
}
|
|
|
|
/**
|
|
* A port whose probe can never be answered: Node's fetch refuses port 1 as a
|
|
* WHATWG "bad port" before it opens a socket, so no listener can satisfy it.
|
|
* A port from the kernel's ephemeral range can be: that is what `listen(0)`
|
|
* hands out, and the fake CDP server above answers the very /json requests a
|
|
* probe makes.
|
|
*/
|
|
const UNREACHABLE_PORT = 1;
|
|
|
|
const portsToCleanup: number[] = [];
|
|
const serversToCleanup: FakeCdpServer[] = [];
|
|
|
|
// Redirect port persistence to a throwaway file so tests never touch the
|
|
// real ~/.argent/chromium-cdp-ports.json on a developer machine or CI runner.
|
|
// This file drives the real persistPorts, which merges rather than replaces, so
|
|
// a lapsed redirect would leave test ports in that file for list-devices to
|
|
// probe. Scope HOME too: the fallback path then lands somewhere disposable.
|
|
scopeTempHome("argent-chromium-discovery-home-");
|
|
|
|
const TEST_PORTS_FILE = path.join(os.tmpdir(), `argent-test-chromium-ports-${process.pid}.json`);
|
|
|
|
beforeAll(() => {
|
|
process.env.ARGENT_CHROMIUM_PORTS_FILE = TEST_PORTS_FILE;
|
|
});
|
|
|
|
afterAll(() => {
|
|
delete process.env.ARGENT_CHROMIUM_PORTS_FILE;
|
|
try {
|
|
fs.unlinkSync(TEST_PORTS_FILE);
|
|
} catch {
|
|
// never created — fine
|
|
}
|
|
});
|
|
|
|
afterEach(async () => {
|
|
for (const p of portsToCleanup.splice(0)) untrackChromiumPort(p);
|
|
for (const s of serversToCleanup.splice(0)) await s.close();
|
|
});
|
|
|
|
describe("getCandidateChromiumPorts", () => {
|
|
it("always includes 9222", () => {
|
|
const ports = getCandidateChromiumPorts();
|
|
expect(ports).toContain(9222);
|
|
});
|
|
|
|
it("includes tracked ports added via trackChromiumPort", () => {
|
|
trackChromiumPort(54321);
|
|
portsToCleanup.push(54321);
|
|
expect(getCandidateChromiumPorts()).toContain(54321);
|
|
});
|
|
|
|
it("removes tracked ports via untrackChromiumPort", () => {
|
|
trackChromiumPort(54322);
|
|
expect(getCandidateChromiumPorts()).toContain(54322);
|
|
untrackChromiumPort(54322);
|
|
expect(getCandidateChromiumPorts()).not.toContain(54322);
|
|
});
|
|
});
|
|
|
|
describe("discoverChromiumDevices", () => {
|
|
it("finds a fake CDP endpoint when its port is tracked", async () => {
|
|
const server = await startFakeCdpServer();
|
|
serversToCleanup.push(server);
|
|
trackChromiumPort(server.port);
|
|
portsToCleanup.push(server.port);
|
|
|
|
const devices = await discoverChromiumDevices({ timeoutMs: 1500 });
|
|
const ours = devices.find((d) => d.port === server.port);
|
|
expect(ours).toBeDefined();
|
|
expect(ours?.platform).toBe("chromium");
|
|
expect(ours?.id).toBe(`chromium-cdp-${server.port}`);
|
|
expect(ours?.title).toBe("Test Page");
|
|
expect(ours?.url).toBe("file:///tmp/index.html");
|
|
expect(ours?.browser).toMatch(/Chrome/);
|
|
expect(ours?.state).toBe("Running");
|
|
});
|
|
|
|
it("returns no entry for a non-responsive port", async () => {
|
|
trackChromiumPort(UNREACHABLE_PORT);
|
|
portsToCleanup.push(UNREACHABLE_PORT);
|
|
const devices = await discoverChromiumDevices({ timeoutMs: 300, ports: [UNREACHABLE_PORT] });
|
|
expect(devices).toEqual([]);
|
|
});
|
|
|
|
it("filters out ports with no page targets", async () => {
|
|
const server = await startFakeCdpServer({
|
|
responses: {
|
|
list: [
|
|
{ id: "x", type: "service_worker", title: "", url: "", webSocketDebuggerUrl: "ws://x" },
|
|
],
|
|
},
|
|
});
|
|
serversToCleanup.push(server);
|
|
const devices = await discoverChromiumDevices({ timeoutMs: 1500, ports: [server.port] });
|
|
expect(devices).toEqual([]);
|
|
});
|
|
|
|
it("untracks a port after it stops responding", async () => {
|
|
const server = await startFakeCdpServer();
|
|
trackChromiumPort(server.port);
|
|
portsToCleanup.push(server.port);
|
|
|
|
// First probe succeeds — port stays tracked.
|
|
let devices = await discoverChromiumDevices({ timeoutMs: 1500 });
|
|
expect(devices.some((d) => d.port === server.port)).toBe(true);
|
|
|
|
// Close the server, probe again — port should be untracked.
|
|
await server.close();
|
|
devices = await discoverChromiumDevices({ timeoutMs: 300 });
|
|
expect(devices.some((d) => d.port === server.port)).toBe(false);
|
|
expect(getCandidateChromiumPorts()).not.toContain(server.port);
|
|
});
|
|
});
|
|
|
|
describe("port persistence across tool-server restarts", () => {
|
|
// Booted Chromium apps are detached and outlive the tool-server (which
|
|
// auto-exits on idle). A fresh module instance — same as a fresh process —
|
|
// must rediscover them from the persisted file.
|
|
it("a fresh module instance sees ports tracked by a previous one", async () => {
|
|
trackChromiumPort(43210);
|
|
portsToCleanup.push(43210);
|
|
|
|
vi.resetModules();
|
|
const fresh = await import("../src/utils/chromium-discovery");
|
|
expect(fresh.getCandidateChromiumPorts()).toContain(43210);
|
|
});
|
|
|
|
it("a dead persisted port is pruned from the file after a failed probe", async () => {
|
|
trackChromiumPort(UNREACHABLE_PORT);
|
|
portsToCleanup.push(UNREACHABLE_PORT);
|
|
expect(JSON.parse(fs.readFileSync(TEST_PORTS_FILE, "utf8"))).toContain(UNREACHABLE_PORT);
|
|
|
|
await discoverChromiumDevices({ timeoutMs: 300, ports: [UNREACHABLE_PORT] });
|
|
expect(JSON.parse(fs.readFileSync(TEST_PORTS_FILE, "utf8"))).not.toContain(UNREACHABLE_PORT);
|
|
expect(getCandidateChromiumPorts()).not.toContain(UNREACHABLE_PORT);
|
|
});
|
|
|
|
it("untrackChromiumPort removes the port from the persisted file", () => {
|
|
trackChromiumPort(43212);
|
|
expect(JSON.parse(fs.readFileSync(TEST_PORTS_FILE, "utf8"))).toContain(43212);
|
|
untrackChromiumPort(43212);
|
|
expect(JSON.parse(fs.readFileSync(TEST_PORTS_FILE, "utf8"))).not.toContain(43212);
|
|
});
|
|
|
|
it("ignores a corrupt persistence file", () => {
|
|
fs.writeFileSync(TEST_PORTS_FILE, "not json{{{");
|
|
expect(() => getCandidateChromiumPorts()).not.toThrow();
|
|
expect(getCandidateChromiumPorts()).toContain(9222);
|
|
// Tracking after corruption rewrites the file cleanly.
|
|
trackChromiumPort(43213);
|
|
portsToCleanup.push(43213);
|
|
expect(JSON.parse(fs.readFileSync(TEST_PORTS_FILE, "utf8"))).toContain(43213);
|
|
});
|
|
});
|