Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions packages/redrob/test/cli/run/run-process.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
)
Expand All @@ -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")
Expand Down
25 changes: 21 additions & 4 deletions packages/redrob/test/lib/cli-process.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<Uint8Array>) as a Stream.
Expand Down Expand Up @@ -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),
),
}
Loading