Skip to content

Commit 4a1b546

Browse files
anandgupta42claude
andcommitted
test: harden flaky tests against parallel-suite contention
Four file changes that fix every flake reported in the last CI run plus the cascade ones the audit subagent traced back to root causes. Local full suite: 7965 pass / 0 fail / 181s (was 7849 pass / 7 fail / 9 errors before). All four prior failure classes addressed; no test was simply disabled. 1. test/cli/tui/thread.test.ts — root cause of multi-suite cascade The original test used `mock.module()` to stub @/util/log, @/project/instance, @/util/rpc, @/cli/cmd/tui/app, @/cli/network. Per the project's saved memory rule (no mock.module for shared modules in Bun's multi-file runner), those mocks survived to subsequent test files and turned Log.Default.* into no-ops + fake Instance.provide. Downstream tests that depended on real logger / Instance behaviour failed cascade-style under parallel load — including the 8 issue #704 theme fallback tests that have nothing to do with logging or instance state but couldn't complete cleanly when the cascade ran. Fix: extracted the directory-resolution logic into a pure helper `resolveProjectDirectory(project, pwd, cwd)` in src/cli/cmd/tui/thread.ts. Tests now call the helper directly with controlled inputs — no mocks, no leaks. The original test exercised only the resolution logic anyway; the mocks were just intercepting downstream calls to verify what `directory` got passed to Instance/tui. Local sanity: bun test test/cli/tui/thread.test.ts → 4/4 pass. 2. test/session/prompt.test.ts — wall-clock starvation + timer leak "does not loop empty assistant turns" + "records aborted errors when prompt is cancelled mid-stream" both timed out at 5s under suite load (the package.json --timeout 30000 only applies to bare `bun test`; `bun test <paths>` reverts to default 5s). The cancelled-stream test also leaked a 10s setTimeout in the `hanging()` helper that was only cleared via stream.cancel — server.stop didn't propagate cancel. Fix: added explicit `, 30_000)` to both tests; refactored hanging() to expose `dispose()` so the test's finally block can clear the timer. 3. test/session/llm.test.ts — FIFO queue cross-contamination "sends responses API payload for OpenAI" timed out at 5s; its in-flight request landed in the next test's deferred via the shared FIFO `state.queue`, making "sends Google API payload for Gemini" see "/v1/responses" instead of "/v1beta/...:streamGenerateContent". Fix: replaced the FIFO queue with a path-keyed Map. The server matches incoming requests by path suffix; out-of-order requests can no longer resolve another test's deferred. beforeEach now also rejects (not just clears) leftover deferreds so tests fail fast rather than hang. Both tests get explicit 30_000 timeouts for the slow-SDK case. 4. test/cli/tui/thread.test.ts (covered above) Verified - bun test test/cli/tui/thread.test.ts → 4/4 pass - bun test test/session/prompt.test.ts → 9/9 pass - bun test test/session/llm.test.ts → 8/8 pass + 2 skip - bun test test/cli/tui/theme-light-mode-704.test.ts → 26/26 pass - bun test (full suite, --timeout 30000) → 7965 pass / 503 skip / 0 fail / 181s Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 69ebaae commit 4a1b546

4 files changed

Lines changed: 129 additions & 173 deletions

File tree

packages/opencode/src/cli/cmd/tui/thread.ts

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,22 @@ async function input(value?: string) {
6262
return piped + "\n" + value
6363
}
6464

65+
/**
66+
* Resolves the project directory from --project arg, PWD, and cwd.
67+
* Pulls realpath via Filesystem.resolve so the caller can safely chdir to
68+
* the result and share the directory key with workers / Instance.provide.
69+
*
70+
* Pure function — extracted from TuiThreadCommand.handler so it can be
71+
* unit-tested without mock.module() (which leaks across test files in
72+
* Bun's multi-file runner; see the related thread.test.ts comment).
73+
*/
74+
export function resolveProjectDirectory(project: string | undefined, pwdOrCwd: string, cwd: string): string {
75+
const root = Filesystem.resolve(pwdOrCwd)
76+
return project
77+
? Filesystem.resolve(path.isAbsolute(project) ? project : path.join(root, project))
78+
: Filesystem.resolve(cwd)
79+
}
80+
6581
export const TuiThreadCommand = cmd({
6682
command: "$0 [project]",
6783
// altimate_change start — branding (describe text was "opencode" upstream)
@@ -119,10 +135,7 @@ export const TuiThreadCommand = cmd({
119135

120136
// Resolve relative --project paths from PWD, then use the real cwd after
121137
// chdir so the thread and worker share the same directory key.
122-
const root = Filesystem.resolve(process.env.PWD ?? process.cwd())
123-
const next = args.project
124-
? Filesystem.resolve(path.isAbsolute(args.project) ? args.project : path.join(root, args.project))
125-
: Filesystem.resolve(process.cwd())
138+
const next = resolveProjectDirectory(args.project, process.env.PWD ?? process.cwd(), process.cwd())
126139
const file = await target()
127140
try {
128141
process.chdir(next)
Lines changed: 46 additions & 140 deletions
Original file line numberDiff line numberDiff line change
@@ -1,157 +1,63 @@
1-
import { describe, expect, mock, test } from "bun:test"
1+
// altimate_change start — rewritten to drop mock.module() (which leaks across
2+
// test files in Bun's multi-file runner — was the root cause of issue #704
3+
// theme tests + several other suites failing under parallel load).
4+
//
5+
// The original test mocked @/util/log, @/project/instance, @/util/rpc and
6+
// @/cli/cmd/tui/app via mock.module() to intercept what `directory` got
7+
// passed downstream. Those mocks survived to subsequent test files and
8+
// turned Log.Default.* into no-ops, breaking any later test that depended
9+
// on real logger behaviour.
10+
//
11+
// Fix: src/cli/cmd/tui/thread.ts now exposes the directory-resolution logic
12+
// as a pure helper `resolveProjectDirectory(project, pwd, cwd)`. Tests call
13+
// it directly with controlled inputs — no mocks, no leakage.
14+
// altimate_change end
15+
import { describe, expect, test } from "bun:test"
216
import fs from "fs/promises"
317
import path from "path"
418
import { tmpdir } from "../../fixture/fixture"
19+
import { resolveProjectDirectory } from "../../../src/cli/cmd/tui/thread"
520

6-
const stop = new Error("stop")
7-
const seen = {
8-
tui: [] as string[],
9-
inst: [] as string[],
10-
}
11-
12-
mock.module("../../../src/cli/cmd/tui/app", () => ({
13-
tui: async (input: { directory: string }) => {
14-
seen.tui.push(input.directory)
15-
throw stop
16-
},
17-
}))
18-
19-
mock.module("@/util/rpc", () => ({
20-
Rpc: {
21-
client: () => ({
22-
call: async () => ({ url: "http://127.0.0.1" }),
23-
on: () => {},
24-
}),
25-
},
26-
}))
27-
28-
mock.module("@/cli/ui", () => ({
29-
UI: {
30-
error: () => {},
31-
},
32-
}))
33-
34-
mock.module("@/util/log", () => ({
35-
Log: {
36-
init: async () => {},
37-
create: () => ({
38-
error: () => {},
39-
info: () => {},
40-
warn: () => {},
41-
debug: () => {},
42-
time: () => ({ stop: () => {} }),
43-
}),
44-
Default: {
45-
error: () => {},
46-
info: () => {},
47-
warn: () => {},
48-
debug: () => {},
49-
},
50-
},
51-
}))
52-
53-
mock.module("@/util/timeout", () => ({
54-
withTimeout: <T>(input: Promise<T>) => input,
55-
}))
56-
57-
mock.module("@/cli/network", () => ({
58-
withNetworkOptions: <T>(input: T) => input,
59-
resolveNetworkOptions: async () => ({
60-
mdns: false,
61-
port: 0,
62-
hostname: "127.0.0.1",
63-
}),
64-
}))
65-
66-
mock.module("../../../src/cli/cmd/tui/win32", () => ({
67-
win32DisableProcessedInput: () => {},
68-
win32InstallCtrlCGuard: () => undefined,
69-
}))
70-
71-
mock.module("@/config/tui", () => ({
72-
TuiConfig: {
73-
get: () => ({}),
74-
},
75-
}))
76-
77-
mock.module("@/project/instance", () => ({
78-
Instance: {
79-
provide: async (input: { directory: string; fn: () => Promise<unknown> | unknown }) => {
80-
seen.inst.push(input.directory)
81-
return input.fn()
82-
},
83-
},
84-
}))
85-
86-
describe("tui thread", () => {
87-
async function call(project?: string) {
88-
const { TuiThreadCommand } = await import("../../../src/cli/cmd/tui/thread")
89-
const args: Parameters<NonNullable<typeof TuiThreadCommand.handler>>[0] = {
90-
_: [],
91-
$0: "opencode",
92-
project,
93-
prompt: "hi",
94-
model: undefined,
95-
agent: undefined,
96-
session: undefined,
97-
continue: false,
98-
fork: false,
99-
port: 0,
100-
hostname: "127.0.0.1",
101-
mdns: false,
102-
"mdns-domain": "opencode.local",
103-
mdnsDomain: "opencode.local",
104-
cors: [],
21+
describe("tui thread > resolveProjectDirectory", () => {
22+
test("uses the real cwd when PWD points at a symlink", async () => {
23+
await using tmp = await tmpdir({ git: true })
24+
const link = path.join(path.dirname(tmp.path), path.basename(tmp.path) + "-link")
25+
const linkType = process.platform === "win32" ? "junction" : "dir"
26+
await fs.symlink(tmp.path, link, linkType)
27+
try {
28+
// PWD = symlink, cwd = real path. Without the implicit project arg
29+
// the resolver should return realpath(cwd) = tmp.path.
30+
const resolved = resolveProjectDirectory(undefined, link, tmp.path)
31+
expect(resolved).toBe(tmp.path)
32+
} finally {
33+
await fs.rm(link, { recursive: true, force: true }).catch(() => undefined)
10534
}
106-
return TuiThreadCommand.handler(args)
107-
}
35+
})
10836

109-
async function check(project?: string) {
37+
test("uses the real cwd after resolving a relative project from PWD", async () => {
11038
await using tmp = await tmpdir({ git: true })
111-
const cwd = process.cwd()
112-
const pwd = process.env.PWD
113-
const worker = globalThis.Worker
114-
const tty = Object.getOwnPropertyDescriptor(process.stdin, "isTTY")
11539
const link = path.join(path.dirname(tmp.path), path.basename(tmp.path) + "-link")
116-
const type = process.platform === "win32" ? "junction" : "dir"
117-
seen.tui.length = 0
118-
seen.inst.length = 0
119-
await fs.symlink(tmp.path, link, type)
120-
121-
Object.defineProperty(process.stdin, "isTTY", {
122-
configurable: true,
123-
value: true,
124-
})
125-
globalThis.Worker = class extends EventTarget {
126-
onerror = null
127-
onmessage = null
128-
onmessageerror = null
129-
postMessage() {}
130-
terminate() {}
131-
} as unknown as typeof Worker
132-
40+
const linkType = process.platform === "win32" ? "junction" : "dir"
41+
await fs.symlink(tmp.path, link, linkType)
13342
try {
134-
process.chdir(tmp.path)
135-
process.env.PWD = link
136-
await expect(call(project)).rejects.toBe(stop)
137-
expect(seen.inst[0]).toBe(tmp.path)
138-
expect(seen.tui[0]).toBe(tmp.path)
43+
// project = ".", PWD = symlink. The resolver should join PWD + "."
44+
// and then realpath that, returning tmp.path.
45+
const resolved = resolveProjectDirectory(".", link, tmp.path)
46+
expect(resolved).toBe(tmp.path)
13947
} finally {
140-
process.chdir(cwd)
141-
if (pwd === undefined) delete process.env.PWD
142-
else process.env.PWD = pwd
143-
if (tty) Object.defineProperty(process.stdin, "isTTY", tty)
144-
else delete (process.stdin as { isTTY?: boolean }).isTTY
145-
globalThis.Worker = worker
14648
await fs.rm(link, { recursive: true, force: true }).catch(() => undefined)
14749
}
148-
}
50+
})
14951

150-
test("uses the real cwd when PWD points at a symlink", async () => {
151-
await check()
52+
test("absolute --project bypasses PWD entirely", async () => {
53+
await using tmp = await tmpdir({ git: true })
54+
const resolved = resolveProjectDirectory(tmp.path, "/some/unrelated/pwd", "/another/cwd")
55+
expect(resolved).toBe(tmp.path)
15256
})
15357

154-
test("uses the real cwd after resolving a relative project from PWD", async () => {
155-
await check(".")
58+
test("falls back to cwd when both PWD and project are missing", async () => {
59+
await using tmp = await tmpdir({ git: true })
60+
const resolved = resolveProjectDirectory(undefined, tmp.path, tmp.path)
61+
expect(resolved).toBe(tmp.path)
15662
})
15763
})

packages/opencode/test/session/llm.test.ts

Lines changed: 32 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -89,49 +89,65 @@ type Capture = {
8989
body: Record<string, unknown>
9090
}
9191

92+
type Pending = { path: string; response: Response; resolve: (value: Capture) => void; reject: (e: unknown) => void }
93+
9294
const state = {
9395
server: null as ReturnType<typeof Bun.serve> | null,
94-
queue: [] as Array<{ path: string; response: Response; resolve: (value: Capture) => void }>,
96+
// Map<pathSuffix, Pending>. Path-keyed (was a FIFO queue) so a slow OpenAI
97+
// request can't resolve a faster Gemini deferred — that cascade was the
98+
// failure mode under heavy parallel-suite load (test 3 timed out at 5s,
99+
// its in-flight request landed in test 4's queue, test 4 saw "/v1/responses"
100+
// instead of "/v1beta/...:streamGenerateContent").
101+
pending: new Map<string, Pending>(),
95102
}
96103

97104
function deferred<T>() {
98-
const result = {} as { promise: Promise<T>; resolve: (value: T) => void }
99-
result.promise = new Promise((resolve) => {
105+
const result = {} as { promise: Promise<T>; resolve: (value: T) => void; reject: (e: unknown) => void }
106+
result.promise = new Promise((resolve, reject) => {
100107
result.resolve = resolve
108+
result.reject = reject
101109
})
102110
return result
103111
}
104112

105113
function waitRequest(pathname: string, response: Response) {
106114
const pending = deferred<Capture>()
107-
state.queue.push({ path: pathname, response, resolve: pending.resolve })
115+
state.pending.set(pathname, { path: pathname, response, resolve: pending.resolve, reject: pending.reject })
108116
return pending.promise
109117
}
110118

111119
beforeAll(() => {
112120
state.server = Bun.serve({
113121
port: 0,
114122
async fetch(req) {
115-
const next = state.queue.shift()
116-
if (!next) {
117-
return new Response("unexpected request", { status: 500 })
123+
const url = new URL(req.url)
124+
// Find the pending entry whose registered suffix matches this request's path.
125+
// Last-suffix-wins for unlikely overlapping registrations (none today).
126+
let matchedKey: string | undefined
127+
for (const key of state.pending.keys()) {
128+
if (url.pathname.endsWith(key)) matchedKey = key
129+
}
130+
if (!matchedKey) {
131+
return new Response(`unexpected request: ${url.pathname}`, { status: 500 })
118132
}
133+
const next = state.pending.get(matchedKey)!
134+
state.pending.delete(matchedKey)
119135

120-
const url = new URL(req.url)
121136
const body = (await req.json()) as Record<string, unknown>
122137
next.resolve({ url, headers: req.headers, body })
123-
124-
if (!url.pathname.endsWith(next.path)) {
125-
return new Response("not found", { status: 404 })
126-
}
127-
128138
return next.response
129139
},
130140
})
131141
})
132142

133143
beforeEach(() => {
134-
state.queue.length = 0
144+
// Reject any leftover deferreds before clearing — otherwise an awaiting test
145+
// would hang for the full timeout. Always-resolved capture means the next
146+
// test sees real path mismatch errors rather than a corrupted Capture.
147+
for (const pending of state.pending.values()) {
148+
pending.reject(new Error("test cleanup: pending request never received"))
149+
}
150+
state.pending.clear()
135151
})
136152

137153
afterAll(() => {
@@ -430,7 +446,7 @@ describe("session.llm.stream", () => {
430446
// altimate_change end
431447
},
432448
})
433-
})
449+
}, 30_000)
434450

435451
test.skip("sends messages API payload for Anthropic models", async () => {
436452
const server = state.server
@@ -652,5 +668,5 @@ describe("session.llm.stream", () => {
652668
expect(config?.maxOutputTokens).toBe(ProviderTransform.maxOutputTokens(resolved))
653669
},
654670
})
655-
})
671+
}, 30_000)
656672
})

0 commit comments

Comments
 (0)