Skip to content

Commit a18acd6

Browse files
committed
test: stop two suites from waiting on the Windows runner
Three CI runs failed only on windows-latest, each on a different test, all passing locally. Two of them share a cause: the test waits on something the runner controls, and Bun's 5s budget expires first. Sidebar routes spawned the user's real gh. 0af17fb already fixed the Windows .cmd shim resolution, and the tests still timed out after it, because resolving the binary correctly does not make an external process fast on a loaded runner. star-state.ts already had a StarDeps seam with an injectable runGh; setStarDepsForTests() selects it. The route tests now assert what they actually claim — reachability, shape, and that no gh output, token, or account identifier is ever serialized — against a deterministic fake, in 0.25ms instead of 5s. Production keeps the real runner. Server auth built four proxy/upstream harnesses inside one test. On Windows the startup cost alone exhausted the budget, and the last request was still in flight when the next test began, racing it through global fetch. One harness now serves all four malformed-detail cases; each still proves its exact original 400 and a single acct-pool-a dispatch. No timeout was raised, no assertion deleted, no test skipped. Those would have traded a reliability signal for silence. The tray failure is NOT fixed. It launches PowerShell, then Bun, then rebinds the same port, and proving the child does not inherit the listen socket is the whole point of the test. Making it deterministic needs a process-launch seam in src/tray/windows.ts, and faking it only in the test would delete the proof. Left alone deliberately; PR #801 targets a neighbouring Windows enumeration flake.
1 parent 679eda0 commit a18acd6

3 files changed

Lines changed: 81 additions & 31 deletions

File tree

src/github/star-state.ts

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,8 @@ async function spawnGh(args: string[], timeoutMs: number): Promise<{ status: num
7979
}
8080
}
8181

82-
const defaultDeps: StarDeps = { runGh: spawnGh, nowMs: () => Date.now() };
82+
const productionDeps: StarDeps = { runGh: spawnGh, nowMs: () => Date.now() };
83+
let defaultDeps = productionDeps;
8384

8485
let cached: { timestamp: number; state: StarState } | null = null;
8586
/** Coalesces concurrent probes so parallel sidebar polls share one `gh` run. */
@@ -148,6 +149,17 @@ export function invalidateStarStatusCache(): void {
148149
inflight = null;
149150
}
150151

152+
/**
153+
* Route tests must not launch the user's `gh` executable: its installation,
154+
* authentication helper, and Windows shim are all outside the route contract.
155+
* Production keeps the real runner; tests install an explicit deterministic
156+
* dependency and must reset it afterwards.
157+
*/
158+
export function setStarDepsForTests(deps: StarDeps | null): void {
159+
defaultDeps = deps ?? productionDeps;
160+
invalidateStarStatusCache();
161+
}
162+
151163
/**
152164
* Star the repository through the user's `gh` login. Returns the resulting
153165
* state so the caller does not need a second round trip; an unauthenticated

tests/server-auth.test.ts

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1955,14 +1955,18 @@ describe("server local API auth", () => {
19551955
});
19561956

19571957
test("missing or non-string detail never authorizes a pool retry", async () => {
1958-
for (const body of ["{}", '{"detail":null}', '{"detail":400}', '{"detail":{"message":"unsupported"}}']) {
1959-
const harness = await startPoolRetryHarness(() => rejectionResponse(body));
1960-
try {
1958+
const bodies = ["{}", '{"detail":null}', '{"detail":400}', '{"detail":{"message":"unsupported"}}'];
1959+
let nextBody = 0;
1960+
const harness = await startPoolRetryHarness(() => rejectionResponse(bodies[nextBody++]!));
1961+
try {
1962+
for (const body of bodies) {
1963+
const priorDispatches = harness.dispatches.length;
19611964
await expectOriginal400(await harness.request(), body);
1962-
expect(harness.dispatches).toEqual(["acct-pool-a"]);
1963-
} finally {
1964-
await stopPoolRetryHarness(harness);
1965+
expect(harness.dispatches.slice(priorDispatches)).toEqual(["acct-pool-a"]);
19651966
}
1967+
expect(nextBody).toBe(bodies.length);
1968+
} finally {
1969+
await stopPoolRetryHarness(harness);
19661970
}
19671971
});
19681972

tests/sidebar-routes.test.ts

Lines changed: 58 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { describe, expect, test } from "bun:test";
22
import { handleManagementAPI } from "../src/server/management-api";
3-
import { invalidateStarStatusCache } from "../src/github/star-state";
3+
import { invalidateStarStatusCache, setStarDepsForTests, type StarDeps } from "../src/github/star-state";
44
import type { OcxConfig } from "../src/types";
55

66
/**
@@ -31,6 +31,27 @@ async function call(
3131
return { status: res.status, body: raw ? JSON.parse(raw) : null, raw, routed: true };
3232
}
3333

34+
async function withStarDeps<T>(deps: StarDeps, run: () => Promise<T>): Promise<T> {
35+
setStarDepsForTests(deps);
36+
try {
37+
return await run();
38+
} finally {
39+
setStarDepsForTests(null);
40+
}
41+
}
42+
43+
// Why there is no per-test timeout here any more.
44+
//
45+
// 600ef52f2 raised these two tests to a 20s budget, and the diagnosis behind it was
46+
// right: star-state's 5s AUTH kill raced Bun's 5s default, so a slow Windows
47+
// credential helper failed the test even when spawnGh resolved `gh` correctly.
48+
//
49+
// The budget is moot once the route stops spawning `gh` at all. What these tests
50+
// actually claim is that the route is reachable, answers in the documented shape, and
51+
// never serializes gh output — none of which needs a real process. Injecting the
52+
// existing StarDeps seam makes them deterministic in microseconds instead of buying
53+
// headroom against an external binary's worst case.
54+
3455
describe("GET /api/update/badge", () => {
3556
test("is routed and returns the badge shape", async () => {
3657
const { status, body } = await call("GET", "/api/update/badge");
@@ -51,32 +72,45 @@ describe("GET /api/update/badge", () => {
5172
});
5273

5374
describe("GET /api/github/star", () => {
54-
// These hit a real `gh` spawn (auth then api). AUTH_TIMEOUT_MS is 5s and API_TIMEOUT_MS
55-
// is 10s in star-state.ts — Bun's default 5s test budget races the auth kill on a slow
56-
// Windows credential helper and flakes as a timeout even when spawnGh is correct.
57-
const STAR_ROUTE_TIMEOUT_MS = 20_000;
58-
5975
test("is routed and reports one of the three known states", async () => {
60-
invalidateStarStatusCache();
61-
const { status, body } = await call("GET", "/api/github/star");
62-
expect(status).toBe(200);
63-
const star = body as Record<string, unknown>;
64-
expect(["starred", "not-starred", "unauthenticated"]).toContain(star.state);
65-
expect(star.repo).toBe("lidge-jun/opencodex");
66-
expect(star.url).toBe("https://github.com/lidge-jun/opencodex");
67-
}, { timeout: STAR_ROUTE_TIMEOUT_MS });
76+
const calls: string[][] = [];
77+
await withStarDeps({
78+
nowMs: () => 0,
79+
// An absent CLI is the deterministic equivalent of a runner where `gh`
80+
// cannot start. The route must still answer without spawning anything.
81+
async runGh(args) { calls.push(args); return null; },
82+
}, async () => {
83+
invalidateStarStatusCache();
84+
const { status, body } = await call("GET", "/api/github/star");
85+
expect(status).toBe(200);
86+
const star = body as Record<string, unknown>;
87+
expect(["starred", "not-starred", "unauthenticated"]).toContain(star.state);
88+
expect(star.repo).toBe("lidge-jun/opencodex");
89+
expect(star.url).toBe("https://github.com/lidge-jun/opencodex");
90+
});
91+
expect(calls).toEqual([["auth", "status", "--hostname", "github.com"]]);
92+
});
6893

6994
test("never serializes gh output, tokens, or account identifiers", async () => {
70-
invalidateStarStatusCache();
71-
const { raw } = await call("GET", "/api/github/star");
72-
// `gh auth status` prints "Logged in to github.com account <name>" and the token
73-
// scopes; none of that may cross this boundary.
74-
expect(raw.toLowerCase()).not.toContain("logged in");
75-
expect(raw.toLowerCase()).not.toContain("token");
76-
expect(raw.toLowerCase()).not.toContain("scope");
77-
expect(raw).not.toContain("gho_");
78-
expect(raw).not.toContain("ghp_");
79-
}, { timeout: STAR_ROUTE_TIMEOUT_MS });
95+
const calls: string[][] = [];
96+
await withStarDeps({
97+
nowMs: () => 0,
98+
// A non-zero status models a CLI whose credential helper has stalled or
99+
// failed, without executing that external helper in this route test.
100+
async runGh(args) { calls.push(args); return { status: 1 }; },
101+
}, async () => {
102+
invalidateStarStatusCache();
103+
const { raw } = await call("GET", "/api/github/star");
104+
// `gh auth status` prints "Logged in to github.com account <name>" and the token
105+
// scopes; none of that may cross this boundary.
106+
expect(raw.toLowerCase()).not.toContain("logged in");
107+
expect(raw.toLowerCase()).not.toContain("token");
108+
expect(raw.toLowerCase()).not.toContain("scope");
109+
expect(raw).not.toContain("gho_");
110+
expect(raw).not.toContain("ghp_");
111+
});
112+
expect(calls).toEqual([["auth", "status", "--hostname", "github.com"]]);
113+
});
80114
});
81115

82116
describe("route surface", () => {

0 commit comments

Comments
 (0)