diff --git a/packages/agents/run-bun-tests.ts b/packages/agents/run-bun-tests.ts index f9252224e2..a64fb806f8 100644 --- a/packages/agents/run-bun-tests.ts +++ b/packages/agents/run-bun-tests.ts @@ -39,7 +39,13 @@ import { writeFileSync, } from 'node:fs'; import { join, relative } from 'node:path'; -import { availableParallelism, tmpdir } from 'node:os'; +import { tmpdir } from 'node:os'; +import { + DEFAULT_PER_FILE_TIMEOUT_MS, + DEFAULT_PER_TEST_TIMEOUT_MS, + MAX_TEST_CONCURRENCY, + resolveTestConcurrency, +} from '../../scripts/lib/bun-test-policy.js'; /** * Every path this runner touches — discovery, the child's working directory @@ -51,46 +57,24 @@ const JUNIT_PATH = join(WORKSPACE_ROOT, 'junit.xml'); const TEST_ROOTS = ['src'] as const; -/** Upper bound on file concurrency, regardless of how many cores are present. */ -const MAX_CONCURRENCY = 4; - -/** Lower bound: small CI runners need one free core for each Bun child. */ -const MIN_CONCURRENCY = 1; +/** + * Upper bound on file concurrency, regardless of how many cores are present. + * + * Kept as a named constant because the JUnit summary reports it. + */ +const MAX_CONCURRENCY = MAX_TEST_CONCURRENCY; /** * Number of test files executed at once. * - * Deliberately half the core count, clamped to [1, 4]. Each file is a fresh - * `bun test` process that re-executes the whole agents module graph, and many - * suites under `src/api/__tests__/` additionally build a real Agent (tool - * registry, provider bootstrap, settings) per test. Saturating every core with - * that work starves individual tests past the 30s budget, which surfaces as a - * different file failing on each run rather than as an honestly slow run. - * Leaving headroom matters most on small CI runners, where the core count is - * roughly the concurrency an unclamped default would pick. macOS CI runs one - * file at a time because its virtual cores repeatedly starve one process past - * the test budget even at half-concurrency. - * - * `LLXPRT_AGENTS_TEST_CONCURRENCY` overrides this. + * The half-the-cores policy this workspace arrived at by measurement now lives + * in scripts/lib/bun-test-policy.ts, so the other runners inherit it instead of + * rediscovering it (issue #3139). `LLXPRT_AGENTS_TEST_CONCURRENCY` overrides. */ -function resolveConcurrency(): number { - const override = process.env.LLXPRT_AGENTS_TEST_CONCURRENCY; - if (override !== undefined) { - if (!/^[1-9][0-9]*$/.test(override.trim())) { - throw new Error( - `LLXPRT_AGENTS_TEST_CONCURRENCY must be a positive integer, got: ${override}`, - ); - } - return Number.parseInt(override.trim(), 10); - } - if (process.platform === 'darwin' && process.env.CI === 'true') { - return MIN_CONCURRENCY; - } - const half = Math.floor(availableParallelism() / 2); - return Math.min(MAX_CONCURRENCY, Math.max(MIN_CONCURRENCY, half)); -} - -const CONCURRENCY = resolveConcurrency(); +const CONCURRENCY = resolveTestConcurrency({ + envVar: 'LLXPRT_AGENTS_TEST_CONCURRENCY', + maxConcurrency: MAX_CONCURRENCY, +}); /** * Per-test timeout, mirroring the `testTimeout: 30000` this workspace ran under @@ -112,7 +96,7 @@ const CONCURRENCY = resolveConcurrency(); * by PER_FILE_TIMEOUT_MS below, which is what should happen - a raised * per-test bound must not turn a hang into a longer hang. */ -const PER_TEST_TIMEOUT_MS = 180_000; +const PER_TEST_TIMEOUT_MS = DEFAULT_PER_TEST_TIMEOUT_MS; /** * Per-file wall-clock budget. The slowest agents files (streaming chat-session @@ -123,7 +107,7 @@ const PER_TEST_TIMEOUT_MS = 180_000; // subagentOrchestrator-loadBalancer was measured at 99.4s under load, leaving // almost no headroom under the previous 120s cap. This remains the backstop // for a suite that genuinely hangs. -const PER_FILE_TIMEOUT_MS = 300_000; +const PER_FILE_TIMEOUT_MS = DEFAULT_PER_FILE_TIMEOUT_MS; /** * Directories that are pruned during discovery. diff --git a/packages/auth/run-bun-tests.ts b/packages/auth/run-bun-tests.ts index 3eee67dc94..d802edd5ec 100644 --- a/packages/auth/run-bun-tests.ts +++ b/packages/auth/run-bun-tests.ts @@ -14,7 +14,11 @@ import { spawn } from 'node:child_process'; import { readdirSync, statSync, writeFileSync } from 'node:fs'; import { join, relative } from 'node:path'; -import { availableParallelism } from 'node:os'; +import { + DEFAULT_PER_FILE_TIMEOUT_MS, + DEFAULT_PER_TEST_TIMEOUT_MS, + resolveTestConcurrency, +} from '../../scripts/lib/bun-test-policy.js'; /** * Every path this runner touches — discovery, the child's working directory, @@ -25,8 +29,10 @@ import { availableParallelism } from 'node:os'; const WORKSPACE_ROOT = import.meta.dir; const PRELOAD = join(WORKSPACE_ROOT, 'bun-preload.ts'); const JUNIT_PATH = join(WORKSPACE_ROOT, 'junit.xml'); -const CONCURRENCY = Math.min(8, availableParallelism()); -const PER_FILE_TIMEOUT_MS = 60_000; +const CONCURRENCY = resolveTestConcurrency({ + envVar: 'LLXPRT_AUTH_TEST_CONCURRENCY', +}); +const PER_FILE_TIMEOUT_MS = DEFAULT_PER_FILE_TIMEOUT_MS; const TEST_ROOTS = ['src'] as const; @@ -88,7 +94,16 @@ export function runTestFile(file: string): Promise { let resolved = false; const child = spawn( process.execPath, - ['test', '--preload', PRELOAD, file], + [ + 'test', + // Bun 1.3.14 ignores a `[test] timeout` key in bunfig.toml and falls + // back to 5s, so this flag is the only thing that sets the budget. + '--timeout', + String(DEFAULT_PER_TEST_TIMEOUT_MS), + '--preload', + PRELOAD, + file, + ], { cwd: WORKSPACE_ROOT, stdio: 'inherit', @@ -96,17 +111,18 @@ export function runTestFile(file: string): Promise { }, ); + // Set by the timer so the exit handler can report the real reason. The + // result is only produced once the process has actually been reaped: + // `kill()` only sends a signal and returns, so resolving from the timer + // would free the worker slot while the killed process was still running, + // letting the pool exceed its concurrency cap exactly when the machine is + // already struggling. This mattered less under the old fixed batches; with + // a worker pool the freed slot is filled immediately. + let killedByTimeout = false; + const timer = setTimeout(() => { - if (resolved) return; - resolved = true; + killedByTimeout = true; child.kill('SIGKILL'); - resolve({ - file, - passed: false, - exitCode: null, - timedOut: true, - signal: null, - }); }, PER_FILE_TIMEOUT_MS); child.on('exit', (code, signal) => { @@ -115,10 +131,10 @@ export function runTestFile(file: string): Promise { clearTimeout(timer); resolve({ file, - passed: code === 0, - exitCode: code, - timedOut: false, - signal: signal ?? null, + passed: !killedByTimeout && code === 0, + exitCode: killedByTimeout ? null : code, + timedOut: killedByTimeout, + signal: killedByTimeout ? null : (signal ?? null), }); }); @@ -198,14 +214,23 @@ async function main(): Promise { `Running ${testFiles.length} test files with concurrency ${CONCURRENCY}`, ); + // A worker pool rather than fixed batches: a batch only advances when its + // slowest file finishes, so one slow file leaves CONCURRENCY - 1 slots idle + // exactly when the machine has work queued behind it. const results: TestResult[] = []; + let nextIndex = 0; - for (let i = 0; i < testFiles.length; i += CONCURRENCY) { - const batch = testFiles.slice(i, i + CONCURRENCY); - const batchResults = await Promise.all(batch.map(runTestFile)); - results.push(...batchResults); + async function worker(): Promise { + while (nextIndex < testFiles.length) { + const file = testFiles[nextIndex++]; + results.push(await runTestFile(file)); + } } + await Promise.all( + Array.from({ length: Math.min(CONCURRENCY, testFiles.length) }, worker), + ); + const passed = results.filter((r) => r.passed).length; const failed = results.filter((r) => !r.passed); diff --git a/packages/auth/src/__tests__/run-bun-tests.behavior.test.ts b/packages/auth/src/__tests__/run-bun-tests.behavior.test.ts index fc2e516735..35cf8c8a5a 100644 --- a/packages/auth/src/__tests__/run-bun-tests.behavior.test.ts +++ b/packages/auth/src/__tests__/run-bun-tests.behavior.test.ts @@ -12,6 +12,14 @@ import { runTestFile, type TestResult, } from '../../run-bun-tests.js'; +import { DEFAULT_PER_FILE_TIMEOUT_MS } from '../../../../scripts/lib/bun-test-policy.js'; + +/** + * Derived from the shared policy rather than hardcoded: the assertion is that + * the reason names the budget that was actually applied, not that the budget + * has one particular value (issue #3139). + */ +const EXPECTED_TIMEOUT_REASON = `Timed out after ${DEFAULT_PER_FILE_TIMEOUT_MS / 1000}s`; describe('auth run-bun-tests JUnit failure reporting', () => { const baseResult: TestResult = { @@ -38,7 +46,7 @@ describe('auth run-bun-tests JUnit failure reporting', () => { it('reports a timeout', () => { const xml = generateJUnit([{ ...baseResult, timedOut: true }], 1, 1); - expect(xml).toContain('Timed out after 60s'); + expect(xml).toContain(EXPECTED_TIMEOUT_REASON); }); it('falls back to an exit code when neither signal nor timeout is present', () => { @@ -67,7 +75,7 @@ describe('auth run-bun-tests failure reason formatting', () => { signal: 'SIGTERM', exitCode: 1, }); - expect(reason).toBe('Timed out after 60s'); + expect(reason).toBe(EXPECTED_TIMEOUT_REASON); }); it('reports a signal before an exit code', () => { diff --git a/packages/cli/run-bun-tests.ts b/packages/cli/run-bun-tests.ts index 0ee70327e0..d10dd04fab 100644 --- a/packages/cli/run-bun-tests.ts +++ b/packages/cli/run-bun-tests.ts @@ -24,25 +24,38 @@ import { spawn } from 'node:child_process'; import { readdirSync, realpathSync, statSync, writeFileSync } from 'node:fs'; import { join, relative } from 'node:path'; -import { availableParallelism } from 'node:os'; +import { + DEFAULT_PER_FILE_TIMEOUT_MS, + DEFAULT_PER_TEST_TIMEOUT_MS, + resolveTestConcurrency, +} from '../../scripts/lib/bun-test-policy.js'; -const PER_FILE_TIMEOUT_MS = 120_000; +const PER_FILE_TIMEOUT_MS = DEFAULT_PER_FILE_TIMEOUT_MS; const PER_INTEGRATION_FILE_TIMEOUT_MS = 900_000; /** - * Per-test timeout, matching the testTimeout the removed vitest.config.ts set. - * Bun defaults to 5s, which the tests that spawn the real CLI exceed once the - * suite runs with concurrency. Passed as a flag because the bunfig.toml key is - * not picked up for a single-file invocation. + * Per-test timeout. Bun defaults to 5s, which the tests that spawn the real + * CLI exceed once the suite runs with concurrency. Passed as a flag because + * the bunfig.toml key is not picked up for a single-file invocation. + * + * Raised from 30s to the shared budget for issue #3139: this workspace was the + * worst-failing CI shard (5/15 first attempts) because it combined the tight + * bound with a pool that saturated every core. */ -const PER_TEST_TIMEOUT_MS = 30_000; +const PER_TEST_TIMEOUT_MS = DEFAULT_PER_TEST_TIMEOUT_MS; /** * Integration tests spawn the built CLI, which cold-starts from TypeScript * source and is far slower than an in-process test — especially on a loaded CI * runner. They get a larger per-test budget so a slow boot is not reported as a * failure. + * + * Expressed as a multiple of the shared budget rather than a fixed 120s: once + * the unit budget rose to 180s for issue #3139 a fixed value silently became + * the *smaller* of the two, which would have given the slowest tests in the + * workspace the tightest bound. It stays well inside + * PER_INTEGRATION_FILE_TIMEOUT_MS, which remains the hang backstop. */ -const PER_INTEGRATION_TEST_TIMEOUT_MS = 120_000; +const PER_INTEGRATION_TEST_TIMEOUT_MS = DEFAULT_PER_TEST_TIMEOUT_MS * 2; const SKIPPED_DIRECTORIES = new Set([ 'node_modules', @@ -104,7 +117,7 @@ function parseConcurrency(): number { return parsed; } } - return Math.max(1, Math.min(8, availableParallelism())); + return resolveTestConcurrency({ envVar: 'LLXPRT_CLI_TEST_CONCURRENCY' }); } export function isTestFile(fileName: string): boolean { @@ -186,11 +199,17 @@ function runTestFile(file: string): Promise { output += chunk.toString(); }); + // Set by the timer so the exit handler can report the real reason. The + // result is only produced once the process has actually exited: killing a + // tree only signals it, so resolving from the timer would free this worker + // slot while the tree was still winding down. The pool would then exceed + // its concurrency cap exactly when the machine is already struggling — + // which is how a timeout on one file turns into timeouts on others. + let killedByTimeout = false; + const timer = setTimeout(() => { - if (settled) return; - settled = true; + killedByTimeout = true; killProcessTree(child); - resolve({ file, passed: false, exitCode: null, timedOut: true, output }); }, fileTimeoutForFile(file)); child.on('exit', (code) => { @@ -199,9 +218,9 @@ function runTestFile(file: string): Promise { clearTimeout(timer); resolve({ file, - passed: code === 0, - exitCode: code, - timedOut: false, + passed: !killedByTimeout && code === 0, + exitCode: killedByTimeout ? null : code, + timedOut: killedByTimeout, output, }); }); diff --git a/packages/core/run-bun-tests.ts b/packages/core/run-bun-tests.ts index b08c95d8fd..91c4141866 100644 --- a/packages/core/run-bun-tests.ts +++ b/packages/core/run-bun-tests.ts @@ -13,9 +13,12 @@ * file as a separate `bun test ` invocation avoids the multi-file * process management that triggers the hang. * - * Each child process has a per-file timeout (60s on POSIX, 180s on Windows). - * If a file takes longer, the process is killed to prevent a single - * slow/hanging file from blocking the entire suite. + * Concurrency and both timeout budgets come from + * scripts/lib/bun-test-policy.ts, shared with the other runners (issue #3139). + * This workspace keeps its own lower concurrency cap because its files are + * unusually heavy; the budgets are the shared ones. If a file exceeds the + * per-file budget the process is killed, so a single hanging file cannot block + * the suite. * * Exit code is 0 if all files pass, 1 if any file fails. */ @@ -23,7 +26,11 @@ import { spawn, type ChildProcess } from 'node:child_process'; import { readdirSync, statSync, writeFileSync } from 'node:fs'; import { join, relative } from 'node:path'; -import { availableParallelism } from 'node:os'; +import { + DEFAULT_PER_FILE_TIMEOUT_MS, + DEFAULT_PER_TEST_TIMEOUT_MS, + resolveTestConcurrency, +} from '../../scripts/lib/bun-test-policy.js'; /** * Every path this runner touches — discovery, the child's working directory, @@ -38,9 +45,12 @@ const JUNIT_PATH = join(WORKSPACE_ROOT, 'junit.xml'); // children overlap. POSIX retains bounded parallelism without saturating shared // CI runners, where event-loop starvation can trip otherwise healthy test files. const MAX_CONCURRENCY = process.platform === 'win32' ? 1 : 2; -const CONCURRENCY = Math.min(MAX_CONCURRENCY, availableParallelism()); -const PER_TEST_TIMEOUT_MS = 30_000; -const PER_FILE_TIMEOUT_MS = process.platform === 'win32' ? 180_000 : 60_000; +const CONCURRENCY = resolveTestConcurrency({ + envVar: 'LLXPRT_CORE_TEST_CONCURRENCY', + maxConcurrency: MAX_CONCURRENCY, +}); +const PER_TEST_TIMEOUT_MS = DEFAULT_PER_TEST_TIMEOUT_MS; +const PER_FILE_TIMEOUT_MS = DEFAULT_PER_FILE_TIMEOUT_MS; const TEST_ROOTS = ['src', 'test'] as const; diff --git a/packages/core/tsconfig.runner.json b/packages/core/tsconfig.runner.json index c1c338dff1..4d420f9786 100644 --- a/packages/core/tsconfig.runner.json +++ b/packages/core/tsconfig.runner.json @@ -9,5 +9,9 @@ "@vybestack/llxprt-code-core/*": ["./src/*"] } }, - "include": ["run-bun-tests.ts", "test/run-bun-tests.test.ts"] + "include": [ + "run-bun-tests.ts", + "test/run-bun-tests.test.ts", + "../../scripts/lib/bun-test-policy.ts" + ] } diff --git a/scripts/lib/bun-test-policy.ts b/scripts/lib/bun-test-policy.ts new file mode 100644 index 0000000000..54fdf6da07 --- /dev/null +++ b/scripts/lib/bun-test-policy.ts @@ -0,0 +1,158 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Single source of truth for how the Bun test runners schedule work + * (issue #3139). + * + * Each workspace owns a `run-bun-tests.ts` that spawns one `bun test` process + * per file. Those runners grew independently and ended up disagreeing about + * both how many files to run at once and how long a test may take. The + * `agents` runner was tuned against measurement in #3084 and the others were + * not, so the workspaces still carrying the untuned policy are the ones whose + * CI shards fail: `cli` saturated every core while keeping a 30s per-test + * budget, and `auth` passed no `--timeout` at all and therefore ran on Bun's + * 5s default. + * + * The failures that policy produces are indistinguishable from flakiness: a + * different file times out on each run and every one of them passes in + * isolation. The work completes; the budget simply cut it off. + * + * Justified per-runner differences are preserved through `maxConcurrency` and + * explicit timeout arguments. What is centralised is the shape of the policy, + * so a future runner cannot quietly reintroduce the divergence. + */ + +import { availableParallelism } from 'node:os'; + +/** + * Per-test budget. + * + * Measured on the worst offender: `subagentOrchestrator-loadBalancer` takes + * ~430ms per launch in isolation but was timed at 78.6s with the pool + * saturated, while consuming 0.8s of user CPU — it waits rather than computes. + * Its failure rate across repeated runs was 4/24 at 30s and 0/24 at 60s. + */ +export const DEFAULT_PER_TEST_TIMEOUT_MS = 180_000; + +/** + * Whole-file budget, and the backstop that keeps a raised per-test bound from + * turning a genuine hang into a longer hang. Sized to admit a slow but + * progressing file rather than to bound total runtime. + * + * A runner enforcing this must not report the file's result from inside the + * timeout callback. Killing a process — or a process tree — only signals it; + * resolving straight away frees the worker slot while the child is still + * winding down, so the pool exceeds its concurrency cap exactly when the + * machine is already struggling, and a timeout on one file becomes timeouts on + * others. Wait for the child to be reaped first, either by settling from the + * `exit` handler (see the agents, cli and auth runners) or by awaiting the kill + * (see the core runner). + */ +export const DEFAULT_PER_FILE_TIMEOUT_MS = 300_000; + +/** + * Ceiling on files run at once, regardless of core count. + * + * Each file is a fresh `bun test` process that re-executes the whole workspace + * module graph, so throughput stops improving well before the core count and + * the extra processes only steal time from the tests already running. + */ +export const MAX_TEST_CONCURRENCY = 4; + +/** A pool of zero would never run anything. */ +export const MIN_TEST_CONCURRENCY = 1; + +export interface TestConcurrencyOptions { + /** + * Environment variable that overrides the computed value, e.g. + * `LLXPRT_AGENTS_TEST_CONCURRENCY`. Must parse as a positive integer. + */ + readonly envVar?: string; + /** + * Ceiling for this runner, when it is lower than {@link MAX_TEST_CONCURRENCY} + * for a reason of its own. `core` caps at 2 because its files are unusually + * heavy. + */ + readonly maxConcurrency?: number; + /** Injected for testing; defaults to the real environment. */ + readonly env?: NodeJS.ProcessEnv; + /** Injected for testing; defaults to the real platform. */ + readonly platform?: NodeJS.Platform; + /** Injected for testing; defaults to `availableParallelism()`. */ + readonly cores?: number; +} + +/** + * Files to run at once: half the available cores, clamped. + * + * Half rather than all, because a `bun test` child is not single-threaded — it + * transpiles and collects garbage on its own threads — so one process per core + * oversubscribes the machine. Throughput does not degrade gracefully there; it + * collapses. The 680-file `cli` suite, measured on a 16-core machine: + * + * | processes per core | wall clock | + * | ------------------ | ------------------------------ | + * | 0.25 | 184s | + * | 0.50 | 124s | + * | 1.00 | >9min, not one file completed | + * + * Half the cores is both the fastest setting measured and the one that keeps + * the machine out of the collapse. It is also the setting under which a test + * finishes inside its budget: at one process per core the same tests are still + * running when the budget expires, which is why a different file failed on + * each CI run while every one of them passed in isolation. + * + * macOS CI runs one file at a time: its virtual cores repeatedly starved a + * process past the budget even at half. + */ +export function resolveTestConcurrency( + options: TestConcurrencyOptions = {}, +): number { + const env = options.env ?? process.env; + const platform = options.platform ?? process.platform; + const ceiling = options.maxConcurrency ?? MAX_TEST_CONCURRENCY; + + if (!Number.isInteger(ceiling) || ceiling < MIN_TEST_CONCURRENCY) { + throw new Error( + `maxConcurrency must be an integer >= ${MIN_TEST_CONCURRENCY}, got: ${ceiling}`, + ); + } + + if (options.envVar !== undefined) { + const override = env[options.envVar]; + if (override !== undefined && override.trim() !== '') { + const parsed = Number.parseInt(override.trim(), 10); + // The digit-shape check alone would accept an arbitrarily long run of + // digits, which parseInt rounds to an imprecise Number or to Infinity; + // that would silently become the size of the worker pool. + if ( + !/^[1-9][0-9]*$/.test(override.trim()) || + !Number.isSafeInteger(parsed) + ) { + throw new Error( + `${options.envVar} must be a positive integer, got: ${override}`, + ); + } + // Deliberately not clamped: an override exists so a human can exceed the + // default, most often to pin a run to 1 while chasing a flake. + return parsed; + } + } + + if (platform === 'darwin' && env['CI'] === 'true') { + return MIN_TEST_CONCURRENCY; + } + + // availableParallelism() reports the machine's cores, not this process's + // share of them. On a CI runner — which is dedicated — those are the same + // thing. On a development machine running several checkouts at once they are + // not, and half of the cores is still more than the runner actually has; the + // env override above is the escape hatch for that case. + const cores = options.cores ?? availableParallelism(); + const half = Math.floor(cores / 2); + return Math.min(ceiling, Math.max(MIN_TEST_CONCURRENCY, half)); +} diff --git a/scripts/run_bun_tests.ts b/scripts/run_bun_tests.ts index 4cff9dd584..0ed2436fa8 100644 --- a/scripts/run_bun_tests.ts +++ b/scripts/run_bun_tests.ts @@ -41,6 +41,7 @@ import { import { dirname, resolve, join } from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { resolveBunTestFiles, type BunTestFile } from './bun-test-roots.js'; +import { DEFAULT_PER_TEST_TIMEOUT_MS } from './lib/bun-test-policy.js'; import { buildVitestJsonReport, parseJUnitXml, @@ -328,7 +329,10 @@ function parseArgs(argv: string[]): CliOptions { const options: CliOptions = { workspace: null, tsconfig: null, - timeout: 30_000, + // Shared with the workspace runners (issue #3139): the two paths both run + // in CI, and a shard that used a tighter bound than the workspace runner + // failed work the workspace runner would have let finish. + timeout: DEFAULT_PER_TEST_TIMEOUT_MS, dryRun: false, junit: null, jsonReport: null, diff --git a/scripts/tests/bun-test-policy.bun.test.ts b/scripts/tests/bun-test-policy.bun.test.ts new file mode 100644 index 0000000000..2595c8b433 --- /dev/null +++ b/scripts/tests/bun-test-policy.bun.test.ts @@ -0,0 +1,225 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Behavioral tests for the shared Bun test scheduling policy (issue #3139). + * + * The policy exists because four runners disagreeing about concurrency and + * timeouts made CI green on only 45% of first attempts. These tests pin the + * behaviour that fixes that, and the last suite pins the invariant that let + * the divergence happen in the first place: a runner may not spawn `bun test` + * without an explicit `--timeout`. + */ + +import { describe, it, expect } from 'bun:test'; +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { + DEFAULT_PER_FILE_TIMEOUT_MS, + DEFAULT_PER_TEST_TIMEOUT_MS, + MAX_TEST_CONCURRENCY, + MIN_TEST_CONCURRENCY, + resolveTestConcurrency, +} from '../lib/bun-test-policy.js'; + +const REPO_ROOT = join(import.meta.dir, '..', '..'); + +/** + * Every runner that spawns `bun test` children — the workspace runners and the + * shard runner CI invokes alongside them. Both paths run in CI, and it was the + * disagreement between them that gave the same test two different budgets. + */ +const RUNNERS: string[] = [ + 'packages/agents/run-bun-tests.ts', + 'packages/cli/run-bun-tests.ts', + 'packages/core/run-bun-tests.ts', + 'packages/auth/run-bun-tests.ts', + 'scripts/run_bun_tests.ts', +]; + +function readRunner(runner: string): string { + return readFileSync(join(REPO_ROOT, runner), 'utf8'); +} + +describe('resolveTestConcurrency (issue #3139)', () => { + const linux: NodeJS.Platform = 'linux'; + + it('uses half the cores so a bun child is not fighting another for a core', () => { + expect(resolveTestConcurrency({ cores: 8, platform: linux, env: {} })).toBe( + 4, + ); + expect(resolveTestConcurrency({ cores: 4, platform: linux, env: {} })).toBe( + 2, + ); + }); + + it('clamps to the shared ceiling on a large machine', () => { + expect( + resolveTestConcurrency({ cores: 64, platform: linux, env: {} }), + ).toBe(MAX_TEST_CONCURRENCY); + }); + + it('never returns zero on a single-core machine', () => { + expect(resolveTestConcurrency({ cores: 1, platform: linux, env: {} })).toBe( + MIN_TEST_CONCURRENCY, + ); + }); + + it('honours a lower per-runner ceiling', () => { + expect( + resolveTestConcurrency({ + cores: 16, + platform: linux, + env: {}, + maxConcurrency: 2, + }), + ).toBe(2); + }); + + it('runs one file at a time on macOS CI', () => { + expect( + resolveTestConcurrency({ + cores: 12, + platform: 'darwin', + env: { CI: 'true' }, + }), + ).toBe(MIN_TEST_CONCURRENCY); + }); + + it('does not serialize macOS outside CI', () => { + expect( + resolveTestConcurrency({ cores: 12, platform: 'darwin', env: {} }), + ).toBe(MAX_TEST_CONCURRENCY); + }); + + it('lets an env override exceed the default, for pinning a run while chasing a flake', () => { + expect( + resolveTestConcurrency({ + cores: 4, + platform: linux, + env: { LLXPRT_TEST_CONCURRENCY: '9' }, + envVar: 'LLXPRT_TEST_CONCURRENCY', + }), + ).toBe(9); + }); + + it('lets an env override pin a run to one file', () => { + expect( + resolveTestConcurrency({ + cores: 16, + platform: linux, + env: { LLXPRT_TEST_CONCURRENCY: '1' }, + envVar: 'LLXPRT_TEST_CONCURRENCY', + }), + ).toBe(1); + }); + + it('ignores an unset or blank override rather than treating it as zero', () => { + expect( + resolveTestConcurrency({ + cores: 8, + platform: linux, + env: { LLXPRT_TEST_CONCURRENCY: ' ' }, + envVar: 'LLXPRT_TEST_CONCURRENCY', + }), + ).toBe(4); + }); + + it('rejects a non-numeric override instead of silently falling back', () => { + expect(() => + resolveTestConcurrency({ + cores: 8, + platform: linux, + env: { LLXPRT_TEST_CONCURRENCY: 'lots' }, + envVar: 'LLXPRT_TEST_CONCURRENCY', + }), + ).toThrow('must be a positive integer'); + }); + + it('rejects an override too large to be an exact integer', () => { + // All digits, so the shape check alone accepts it, but parseInt rounds it + // to an imprecise Number — which would then size the worker pool. + expect(() => + resolveTestConcurrency({ + cores: 8, + platform: linux, + env: { LLXPRT_TEST_CONCURRENCY: '99999999999999999999999' }, + envVar: 'LLXPRT_TEST_CONCURRENCY', + }), + ).toThrow('must be a positive integer'); + }); + + it('accepts the largest exactly representable override', () => { + expect( + resolveTestConcurrency({ + cores: 8, + platform: linux, + env: { LLXPRT_TEST_CONCURRENCY: String(Number.MAX_SAFE_INTEGER) }, + envVar: 'LLXPRT_TEST_CONCURRENCY', + }), + ).toBe(Number.MAX_SAFE_INTEGER); + }); + + it('rejects a zero override, which would run nothing', () => { + expect(() => + resolveTestConcurrency({ + cores: 8, + platform: linux, + env: { LLXPRT_TEST_CONCURRENCY: '0' }, + envVar: 'LLXPRT_TEST_CONCURRENCY', + }), + ).toThrow('must be a positive integer'); + }); + + it('rejects a nonsensical per-runner ceiling', () => { + expect(() => + resolveTestConcurrency({ + cores: 8, + platform: linux, + env: {}, + maxConcurrency: 0, + }), + ).toThrow('maxConcurrency'); + }); +}); + +describe('shared timeout budgets (issue #3139)', () => { + it('gives a test more than the 30s bound that was measured to cut work off', () => { + expect(DEFAULT_PER_TEST_TIMEOUT_MS).toBeGreaterThan(60_000); + }); + + it('keeps the whole-file backstop above the per-test budget so a hang is still caught', () => { + expect(DEFAULT_PER_FILE_TIMEOUT_MS).toBeGreaterThan( + DEFAULT_PER_TEST_TIMEOUT_MS, + ); + }); +}); + +describe('runner invariants (issue #3139)', () => { + it('every runner passes an explicit --timeout to bun test', () => { + // Bun 1.3.14 ignores a `[test] timeout` key in bunfig.toml and falls back + // to 5s, so the flag is the only thing that actually sets the budget. The + // auth runner shipped without it and therefore ran on 5s. + const missing = RUNNERS.filter( + (runner) => !readRunner(runner).includes("'--timeout'"), + ); + expect(missing).toStrictEqual([]); + }); + + it('every runner derives its policy from the shared module', () => { + const missing = RUNNERS.filter( + (runner) => !readRunner(runner).includes('bun-test-policy.js'), + ); + expect(missing).toStrictEqual([]); + }); + + it('no runner recomputes concurrency from availableParallelism itself', () => { + const offenders = RUNNERS.filter((runner) => + readRunner(runner).includes('availableParallelism()'), + ); + expect(offenders).toStrictEqual([]); + }); +}); diff --git a/tsconfig.scripts.json b/tsconfig.scripts.json index a72f3f0308..228de8ca59 100644 --- a/tsconfig.scripts.json +++ b/tsconfig.scripts.json @@ -76,6 +76,7 @@ "scripts/bun-test-roots.ts", "scripts/run_bun_tests.ts", "scripts/bun-junit-to-json-report.ts", + "scripts/tests/bun-test-policy.bun.test.ts", "scripts/tests/bun-test-roots.bun.test.ts", "scripts/tests/bun-test-root-ownership.bun.test.ts", "scripts/tests/test-file-coverage.bun.test.ts",