mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
11df817616
The slowest unit tests were waiting on real wall-clock timers, not doing
work. This makes them deterministic and fast with **no production
behavior change and no production test seams**.
## Speedup
**Suite-level**
| Scope | Before | After | Faster |
|---|---|---|---|
| tool-server — wall clock | 2.72s | ~1.3s | ~2× |
| tool-server — aggregate test time | 9.70s | 4.03s | 2.4× |
| argent-mcp — aggregate test time | 0.90s | 0.28s | 3.2× |
**Per file**
| File | Before | After | Faster |
|---|---|---|---|
| simulator-server-blueprint.test.ts | 2.55s | 0.05s | ~50× |
| boot-device-hardening.test.ts | 2.17s | 0.18s | 12× |
| boot-device-spawn-error.test.ts | 1.11s | 0.005s | ~200× |
| boot-device-hotboot.test.ts | 1.01s | 0.06s | 16× |
| fetch-timeout.test.ts | 0.81s | 0.18s | 4.5× |
| flow-tools.test.ts (delayMs test) | 0.31s | 0.04s | ~7× |
**Worst individual tests, before → after**
| Test | Before | After |
|---|---|---|
| rejects the boot promise…when spawn emits error | 1.06s | ~1ms |
| reports the emulator crash error… | 1.00s | ~2ms |
| falls back to cold boot when hot-boot child exits early | 1.00s | ~1ms
|
| reports a signal-terminated emulator (SIGSEGV)… | 1.00s | ~1ms |
| 5× simulator-server-blueprint tests | ~0.50s ea | ~0.001–0.02s ea |
| throws after retries when POST never responds | 0.60s | 0.14s |
| sleeps the step's delayMs | 0.31s | 0.04s |
No test exceeds ~0.7s anymore (the slowest,
`workspace-reader-integration`, is genuine subprocess I/O — `node
--version` etc. — left as-is on purpose; it is not a timeout).
## What changed
- **`boot-device.ts` (only production change):** the three hardcoded
poll cadences are extracted into a single immutable
`BOOT_POLL_INTERVALS_MS` constant (values unchanged: 1500/1000/500ms)
and referenced at the four `setTimeout` sites. Pure readability
extraction — **no mutable state, no exported test hooks, no runtime
behavior change.**
- **Boot-device tests:** the timing-sensitive tests now use vitest fake
timers (`vi.useFakeTimers()` + `vi.advanceTimersByTimeAsync`).
`boot-device-hardening` / `-spawn-error` add the same partial
`vi.mock("../src/utils/adb")` already used by
`boot-device-hotboot.test.ts`, so pre-flight does no real filesystem I/O
and the timers drive deterministically.
- **`simulator-server-blueprint.test.ts`:** `signalReady` now emits
`stream_ready` then `api_ready` (matching the real streaming server's
ordering) so the blueprint resolves immediately instead of waiting out
the 500ms `STREAM_GRACE_MS` fallback. Adds a dedicated fake-timer test
that explicitly covers the non-streaming grace path (coverage gained,
not lost).
- **`fetch-timeout.test.ts` / `flow-tools.test.ts`:** smaller timing
knobs; same behaviors asserted.
## No regressions
- 1011/1011 tests pass (tool-server 656, argent-mcp 53, argent 6,
registry 64, tools-client 1, installer 231); `tsc --build` and Prettier
clean. The converted boot-device, simulator-server-blueprint, and
flow-tools tests are deterministic under fake timers (verified across
stress runs).
- No production behavior change; `simulator-server.ts` is byte-identical
to `main`; net +1 test.
- Heads-up: `argent-mcp/test/fetch-timeout.test.ts` has a
**pre-existing** macOS flake (~10–20% rate; `connect EADDRNOTAVAIL
127.0.0.1:<port> - Local (0.0.0.0:0)` — kernel source-port exhaustion
under rapid-fire loopback fetches). Stress-testing showed it on `main`'s
pristine file too (2/10 failures vs 1/10 on this branch), so this PR
does not introduce or worsen it. Fixing it (e.g., EADDRNOTAVAIL retry,
or moving the test off real loopback fetch) is worth a follow-up.
205 lines
8.3 KiB
TypeScript
205 lines
8.3 KiB
TypeScript
/**
|
|
* Regression test: `attemptBoot` in `boot-device.ts` must register an `error`
|
|
* event handler on the spawned `emulator` ChildProcess. Node's spawn emits an
|
|
* `error` event when the binary cannot be exec'd (e.g. ENOENT/EACCES — emulator
|
|
* binary removed or permission flipped mid-flight, transient FS hiccup).
|
|
* EventEmitter convention is that an unhandled `error` event escapes as an
|
|
* uncaught exception that crashes the host process; without a listener, the
|
|
* tool-server would crash instead of returning a clean error to the caller.
|
|
*
|
|
* The boot promise must also reject (not hang) and the in-flight Map entry
|
|
* must be cleared so a retry doesn't coalesce into the dead promise.
|
|
*/
|
|
|
|
import { describe, it, expect, vi, beforeEach } from "vitest";
|
|
|
|
const execFileMock = vi.fn();
|
|
const spawnMock = vi.fn();
|
|
|
|
vi.mock("node:child_process", async () => {
|
|
const actual = await vi.importActual<typeof import("node:child_process")>("node:child_process");
|
|
return {
|
|
...actual,
|
|
execFile: (
|
|
cmd: string,
|
|
args: readonly string[],
|
|
opts: unknown,
|
|
cb?: (err: Error | null, out: { stdout: string; stderr: string }) => void
|
|
) => {
|
|
const callback = typeof opts === "function" ? opts : cb!;
|
|
const options = typeof opts === "function" ? undefined : opts;
|
|
const result = execFileMock(cmd, args, options);
|
|
if (result instanceof Error) {
|
|
const e = result as Error & { stderr?: string; stdout?: string };
|
|
callback(e, { stdout: e.stdout ?? "", stderr: e.stderr ?? "" });
|
|
} else callback(null, result ?? { stdout: "", stderr: "" });
|
|
},
|
|
spawn: (cmd: string, args: string[], opts: unknown) => spawnMock(cmd, args, opts),
|
|
};
|
|
});
|
|
|
|
// `boot-device` now goes through `resolveAndroidBinary` for both ensureDep
|
|
// and the spawn path. Stub the resolver to return the bare name so existing
|
|
// `cmd === "adb" / "emulator"` and `spawnMock("emulator", ...)` matchers fire
|
|
// regardless of host SDK install state.
|
|
vi.mock("../src/utils/android-binary", () => ({
|
|
resolveAndroidBinary: vi.fn(async (name: "adb" | "emulator") => name),
|
|
__resetAndroidBinaryCacheForTesting: () => {},
|
|
}));
|
|
|
|
// Stub the snapshot probes so pre-flight does no real ~/.android filesystem
|
|
// I/O. That keeps the whole boot path on microtasks + faked timers, which
|
|
// vitest fake timers drive deterministically. Returning `false` forces the
|
|
// cold path — behaviour-identical to the real probe for a fake AVD that has
|
|
// no default_boot snapshot on disk. (Same partial-mock shape used in
|
|
// boot-device-hotboot.test.ts.)
|
|
vi.mock("../src/utils/adb", async () => {
|
|
const actual = await vi.importActual<typeof import("../src/utils/adb")>("../src/utils/adb");
|
|
return {
|
|
...actual,
|
|
hasDefaultBootSnapshot: async () => false,
|
|
checkSnapshotLoadable: async () => ({ loadable: false, reason: "test" }),
|
|
};
|
|
});
|
|
|
|
import { __primeDepCacheForTests, __resetDepCacheForTests } from "../src/utils/check-deps";
|
|
import {
|
|
__resetInFlightBootsForTesting,
|
|
createBootDeviceTool,
|
|
} from "../src/tools/devices/boot-device";
|
|
import type { Registry } from "@argent/registry";
|
|
import { EventEmitter } from "node:events";
|
|
|
|
beforeEach(() => {
|
|
execFileMock.mockReset();
|
|
spawnMock.mockReset();
|
|
__resetDepCacheForTests();
|
|
__primeDepCacheForTests(["adb"]);
|
|
__resetInFlightBootsForTesting();
|
|
});
|
|
|
|
describe("boot-device — spawn error handling", () => {
|
|
it("registers an `error` listener on the spawned emulator child", async () => {
|
|
vi.useFakeTimers();
|
|
try {
|
|
const proc = new EventEmitter() as EventEmitter & {
|
|
unref: () => void;
|
|
kill: (sig?: string) => boolean;
|
|
exitCode: number | null;
|
|
signalCode: NodeJS.Signals | null;
|
|
};
|
|
proc.unref = () => {};
|
|
proc.kill = () => true;
|
|
proc.exitCode = null;
|
|
proc.signalCode = null;
|
|
spawnMock.mockReturnValue(proc);
|
|
|
|
execFileMock.mockImplementation((cmd: string, args: string[]) => {
|
|
if (cmd === "emulator" && args[0] === "-list-avds") {
|
|
return { stdout: "Pixel_7_API_34\n", stderr: "" };
|
|
}
|
|
if (cmd === "adb" && args[0] === "version") {
|
|
return { stdout: "Android Debug Bridge\n", stderr: "" };
|
|
}
|
|
if (cmd === "adb" && args[0] === "start-server") return { stdout: "", stderr: "" };
|
|
if (cmd === "adb" && args[0] === "devices") {
|
|
return { stdout: "List of devices attached\n", stderr: "" };
|
|
}
|
|
return { stdout: "", stderr: "" };
|
|
});
|
|
|
|
const registry: Registry = { resolveService: async () => ({}) } as unknown as Registry;
|
|
const tool = createBootDeviceTool(registry);
|
|
const promise = tool.execute!({}, { avdName: "Pixel_7_API_34", bootTimeoutMs: 30_000 });
|
|
promise.catch(() => {}); // detach so the test doesn't hang after assertion
|
|
|
|
// Flush pre-flight microtasks so attemptBoot spawns the child and
|
|
// synchronously attaches its listeners (adb is mocked → no real I/O).
|
|
await vi.advanceTimersByTimeAsync(10);
|
|
|
|
// Without an `error` listener, an emitted error escapes as an uncaught
|
|
// exception and crashes the tool-server.
|
|
expect(proc.listenerCount("error")).toBeGreaterThan(0);
|
|
} finally {
|
|
vi.useRealTimers();
|
|
}
|
|
}, 5_000);
|
|
|
|
it("rejects the boot promise and clears the in-flight entry when spawn emits `error`", async () => {
|
|
vi.useFakeTimers();
|
|
try {
|
|
const proc = new EventEmitter() as EventEmitter & {
|
|
unref: () => void;
|
|
kill: (sig?: string) => boolean;
|
|
exitCode: number | null;
|
|
signalCode: NodeJS.Signals | null;
|
|
};
|
|
proc.unref = () => {};
|
|
proc.kill = () => true;
|
|
proc.exitCode = null;
|
|
proc.signalCode = null;
|
|
spawnMock.mockReturnValue(proc);
|
|
|
|
execFileMock.mockImplementation((cmd: string, args: string[]) => {
|
|
if (cmd === "emulator" && args[0] === "-list-avds") {
|
|
return { stdout: "Pixel_7_API_34\n", stderr: "" };
|
|
}
|
|
if (cmd === "adb" && args[0] === "version") {
|
|
return { stdout: "Android Debug Bridge\n", stderr: "" };
|
|
}
|
|
if (cmd === "adb" && args[0] === "start-server") return { stdout: "", stderr: "" };
|
|
if (cmd === "adb" && args[0] === "devices") {
|
|
return { stdout: "List of devices attached\n", stderr: "" };
|
|
}
|
|
return { stdout: "", stderr: "" };
|
|
});
|
|
|
|
const registry: Registry = { resolveService: async () => ({}) } as unknown as Registry;
|
|
const tool = createBootDeviceTool(registry);
|
|
const promise = tool.execute!({}, { avdName: "Pixel_7_API_34", bootTimeoutMs: 30_000 });
|
|
promise.catch(() => {}); // detach so the rejection can't escape between advances
|
|
|
|
// Reach spawn (the impl attaches the `error` listener synchronously
|
|
// right after spawn(); adb is mocked so pre-flight is microtasks only).
|
|
await vi.advanceTimersByTimeAsync(10);
|
|
|
|
// Simulate ENOENT — emulator binary missing/exec failure.
|
|
const spawnErr = new Error("spawn ENOENT") as NodeJS.ErrnoException;
|
|
spawnErr.code = "ENOENT";
|
|
proc.emit("error", spawnErr);
|
|
|
|
// Stage-2 is parked on the adb-register poll; advancing past one poll
|
|
// (>1s, the production cadence) wakes it so it throws the earlyExitError
|
|
// instead of hanging. The boot promise must reject, not hang.
|
|
await vi.advanceTimersByTimeAsync(2_000);
|
|
await expect(promise).rejects.toThrow(/ENOENT|spawn|emulator/i);
|
|
|
|
// The in-flight Map entry must be cleared so a retry doesn't coalesce
|
|
// into the dead promise. We assert this indirectly: a second call for the
|
|
// same AVD must invoke spawn again rather than awaiting the prior promise.
|
|
spawnMock.mockClear();
|
|
|
|
const secondProc = new EventEmitter() as EventEmitter & {
|
|
unref: () => void;
|
|
kill: (sig?: string) => boolean;
|
|
exitCode: number | null;
|
|
signalCode: NodeJS.Signals | null;
|
|
};
|
|
secondProc.unref = () => {};
|
|
secondProc.kill = () => true;
|
|
secondProc.exitCode = null;
|
|
secondProc.signalCode = null;
|
|
spawnMock.mockReturnValue(secondProc);
|
|
|
|
const second = tool.execute!({}, { avdName: "Pixel_7_API_34", bootTimeoutMs: 30_000 });
|
|
second.catch(() => {}); // detach
|
|
|
|
// Give the second call enough fake time to reach spawn.
|
|
await vi.advanceTimersByTimeAsync(10);
|
|
expect(spawnMock).toHaveBeenCalled();
|
|
} finally {
|
|
vi.useRealTimers();
|
|
}
|
|
}, 10_000);
|
|
});
|