From 0b0f24201932dfec2253de1bef370020158319d2 Mon Sep 17 00:00:00 2001 From: Janghoon Lee <44862514+savagemanage@users.noreply.github.com> Date: Mon, 21 Sep 2026 11:20:39 +0000 Subject: [PATCH] fix(test): apply the slow-platform allowance where the bounds are actually written The first version of this fix scaled only the DEFAULT child timeout, and the very test that was flaking passes its own `timeoutMs: 30_000`. So it stayed pinned to thirty seconds and failed again on the next Windows run, at 30872ms -- which is how I know the allowance was not reaching it. Two gaps, both now closed by construction rather than by remembering: - `cliIt.concurrent` treats a caller's test timeout as a FLOOR, not a ceiling. Thirty-seven call sites pass `60_000`, comfortably above the old fixed 30s child bound and BELOW the Windows one, so honouring them literally reintroduced exactly the drift the derived constants exist to prevent. One clamp fixes all of them; editing thirty-seven call sites would not have kept the thirty-eighth honest. - `slowPlatform(ms)` is exported for the call sites that genuinely need their own bound, so the intent stays readable at the call site while the correction is applied for them. The duration assertion beside one of those bounds is scaled WITH it. Its claim is that the CLI exits promptly rather than being killed by the timeout, and a duration pinned to a base figure while the bound moves tests something else. --- .../redrob/test/cli/run/run-process.test.ts | 10 +++++--- packages/redrob/test/lib/cli-process.ts | 25 ++++++++++++++++--- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/packages/redrob/test/cli/run/run-process.test.ts b/packages/redrob/test/cli/run/run-process.test.ts index cb318b5fea..f72728da03 100644 --- a/packages/redrob/test/cli/run/run-process.test.ts +++ b/packages/redrob/test/cli/run/run-process.test.ts @@ -6,7 +6,7 @@ import { describe, expect } from "bun:test" import { Effect } from "effect" import { reply } from "../../lib/llm-server" -import { cliIt } from "../../lib/cli-process" +import { cliIt, slowPlatform } from "../../lib/cli-process" describe("redrob run (non-interactive subprocess)", () => { // Happy path: prompt completes, output reaches stdout, process exits 0. @@ -73,10 +73,12 @@ describe("redrob run (non-interactive subprocess)", () => { Effect.gen(function* () { const result = yield* redrob.run("say hi", { model: "test/nonexistent-model", - timeoutMs: 15_000, + timeoutMs: slowPlatform(15_000), }) expect(result.exitCode).not.toBe(0) - expect(result.durationMs).toBeLessThan(15_000) + /* Scaled WITH the bound: the claim is that it exits promptly rather than being killed, and a + duration assertion pinned to a base figure while the bound moves tests the wrong thing. */ + expect(result.durationMs).toBeLessThan(slowPlatform(15_000)) }), 60_000, ) @@ -96,7 +98,7 @@ describe("redrob run (non-interactive subprocess)", () => { ) yield* llm.fail("upstream provider exploded mid-stream") yield* llm.text("recovered") - const result = yield* redrob.run("trigger midstream error", { timeoutMs: 30_000 }) + const result = yield* redrob.run("trigger midstream error", { timeoutMs: slowPlatform(30_000) }) expect(result.exitCode).toBe(0) expect(result.stdout).toBe("partial response\nrecovered\n") expect(result.stderr).not.toContain("upstream provider exploded mid-stream") diff --git a/packages/redrob/test/lib/cli-process.ts b/packages/redrob/test/lib/cli-process.ts index e9d141e2d7..80537c6d0e 100644 --- a/packages/redrob/test/lib/cli-process.ts +++ b/packages/redrob/test/lib/cli-process.ts @@ -63,6 +63,16 @@ const CHILD_TIMEOUT_MS = 30_000 * SLOW_PLATFORM_FACTOR const READY_TIMEOUT_MS = 15_000 * SLOW_PLATFORM_FACTOR const TEST_TIMEOUT_MS = CHILD_TIMEOUT_MS * 2 +/** + * A test's own bound, scaled for the slow platform. + * + * For the call sites that pass an explicit `timeoutMs` rather than taking the default -- those bypass the + * allowance entirely, which is how the first version of this fix left the very test that was flaking still + * pinned to thirty seconds. Write `slowPlatform(30_000)` and the intent stays readable while the platform + * correction is applied for you. + */ +export const slowPlatform = (ms: number) => ms * SLOW_PLATFORM_FACTOR + export const testModelID = "test/test-model" // Wrap a Bun subprocess pipe (or any ReadableStream) as a Stream. @@ -572,9 +582,16 @@ export const cliIt = { (process.platform === "win32" ? test : test.concurrent)( name, () => Effect.runPromise(Effect.scoped(withCliFixture(body))), - // Bun's timeout includes spawn-gate wait, so it must stay above the child timeout or a queued - // concurrent CLI test expires while holding no permit. Derived from it rather than restated, which - // is what let the two drift apart. - opts ?? TEST_TIMEOUT_MS, + /* + A caller's own number is a FLOOR, not a ceiling. + + Bun's timeout includes spawn-gate wait, so it must stay above the child timeout or a queued test + expires while holding no permit -- and that failure reads as a slow command rather than as a + mis-tuned harness. Dozens of call sites pass `60_000`, which was comfortably above the old fixed + 30s child bound and is BELOW the Windows one, so honouring them literally would have reintroduced + exactly the drift these constants exist to prevent. Clamped here rather than edited at every call + site: the invariant then holds by construction instead of by everyone remembering it. + */ + typeof opts === "number" ? Math.max(opts, TEST_TIMEOUT_MS) : (opts ?? TEST_TIMEOUT_MS), ), }