From 48ff146d295ab739c9efac1cabd066f9ca01fb84 Mon Sep 17 00:00:00 2001 From: acoliver Date: Thu, 6 Aug 2026 13:34:44 -0300 Subject: [PATCH 1/3] Improve Bun test timing and runner diagnostics (Fixes #2904) --- packages/auth/run-bun-tests.ts | 51 +++- ...th-errors.spec.ts => oauth-errors.test.ts} | 264 ++++++++++++------ .../__tests__/run-bun-tests.behavior.test.ts | 132 +++++++++ packages/core/src/config/config.d.test.ts | 4 +- .../config.folderTrustMcpWiring.test.ts | 16 +- ... => provider-settings-integration.test.ts} | 19 +- .../src/recording/sessionManagement.test.ts | 38 +-- .../test/utils/ripgrepPathResolver.test.ts | 82 ++---- project-plans/issue2904/plan.md | 137 +++++++++ 9 files changed, 533 insertions(+), 210 deletions(-) rename packages/auth/src/__tests__/{oauth-errors.spec.ts => oauth-errors.test.ts} (76%) create mode 100644 packages/auth/src/__tests__/run-bun-tests.behavior.test.ts rename packages/core/src/integration-tests/{provider-settings-integration.spec.ts => provider-settings-integration.test.ts} (92%) create mode 100644 project-plans/issue2904/plan.md diff --git a/packages/auth/run-bun-tests.ts b/packages/auth/run-bun-tests.ts index b2e2902c2b..3eee67dc94 100644 --- a/packages/auth/run-bun-tests.ts +++ b/packages/auth/run-bun-tests.ts @@ -75,14 +75,15 @@ export function discoverTestFiles(root: string): string[] { return results; } -interface TestResult { +export interface TestResult { file: string; passed: boolean; exitCode: number | null; timedOut: boolean; + signal: NodeJS.Signals | null; } -function runTestFile(file: string): Promise { +export function runTestFile(file: string): Promise { return new Promise((resolve) => { let resolved = false; const child = spawn( @@ -99,14 +100,26 @@ function runTestFile(file: string): Promise { if (resolved) return; resolved = true; child.kill('SIGKILL'); - resolve({ file, passed: false, exitCode: null, timedOut: true }); + resolve({ + file, + passed: false, + exitCode: null, + timedOut: true, + signal: null, + }); }, PER_FILE_TIMEOUT_MS); - child.on('exit', (code) => { + child.on('exit', (code, signal) => { if (resolved) return; resolved = true; clearTimeout(timer); - resolve({ file, passed: code === 0, exitCode: code, timedOut: false }); + resolve({ + file, + passed: code === 0, + exitCode: code, + timedOut: false, + signal: signal ?? null, + }); }); child.on('error', (err: Error) => { @@ -114,7 +127,13 @@ function runTestFile(file: string): Promise { resolved = true; clearTimeout(timer); console.error(`Error spawning test for ${file}: ${err.message}`); - resolve({ file, passed: false, exitCode: -1, timedOut: false }); + resolve({ + file, + passed: false, + exitCode: -1, + timedOut: false, + signal: null, + }); }); }); } @@ -127,7 +146,17 @@ function escapeXml(value: string): string { .replace(/"/g, '"'); } -function generateJUnit( +export function formatFailureReason(result: TestResult): string { + if (result.timedOut) { + return `Timed out after ${PER_FILE_TIMEOUT_MS / 1000}s`; + } + if (result.signal !== null) { + return `Killed by signal ${result.signal}`; + } + return `Exit code ${result.exitCode ?? -1}`; +} + +export function generateJUnit( results: TestResult[], totalFiles: number, failedCount: number, @@ -138,10 +167,9 @@ function generateJUnit( const className = escapeXml( r.file.replace(/^src\//, '').replace(/\.(test|spec)\.tsx?$/, ''), ); - const exitCode = r.exitCode ?? -1; const failureXml = r.passed ? '' - : `FAILED`; + : `FAILED`; const timeAttr = r.passed ? '' : ' time="0"'; return ` ${failureXml}`; }) @@ -182,10 +210,7 @@ async function main(): Promise { const failed = results.filter((r) => !r.passed); for (const result of failed) { - const reason = result.timedOut - ? `TIMEOUT after ${PER_FILE_TIMEOUT_MS}ms` - : `exit code ${result.exitCode ?? -1}`; - console.error(`FAILED: ${result.file} (${reason})`); + console.error(`FAILED: ${result.file} (${formatFailureReason(result)})`); } console.log( diff --git a/packages/auth/src/__tests__/oauth-errors.spec.ts b/packages/auth/src/__tests__/oauth-errors.test.ts similarity index 76% rename from packages/auth/src/__tests__/oauth-errors.spec.ts rename to packages/auth/src/__tests__/oauth-errors.test.ts index 9a78fc31de..3d09f4d9c9 100644 --- a/packages/auth/src/__tests__/oauth-errors.spec.ts +++ b/packages/auth/src/__tests__/oauth-errors.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { describe, it, expect, beforeEach, vi } from 'bun:test'; import { OAuthError, OAuthErrorFactory, @@ -12,8 +12,16 @@ import { OAuthErrorCategory, RetryHandler, GracefulErrorHandler, - DEFAULT_RETRY_CONFIG, } from '../oauth-errors.js'; +import type { OAuthLogger } from '../oauth-errors.js'; + +function serviceUnavailable(): OAuthError { + return new OAuthError( + OAuthErrorType.SERVICE_UNAVAILABLE, + 'test-provider', + 'temporarily unavailable', + ); +} describe('OAuthError', () => { it('should create error with proper classification', () => { @@ -308,16 +316,24 @@ describe('OAuthErrorFactory', () => { }); describe('RetryHandler', () => { + const silentLogger: OAuthLogger = { + debug() {}, + error() {}, + }; + let retryHandler: RetryHandler; beforeEach(() => { - retryHandler = new RetryHandler({ - maxAttempts: 3, - baseDelayMs: 0, - backoffMultiplier: 1, - maxDelayMs: 0, - jitter: false, // Disable jitter for predictable tests - }); + retryHandler = new RetryHandler( + { + maxAttempts: 5, + baseDelayMs: 1000, + backoffMultiplier: 2, + maxDelayMs: 5000, + jitter: false, + }, + silentLogger, + ); }); it('should succeed on first attempt', async () => { @@ -332,32 +348,150 @@ describe('RetryHandler', () => { expect(operation).toHaveBeenCalledTimes(1); }); - it('should retry transient errors with exponential backoff', async () => { - // Use a retry handler with no delay to avoid timing issues - const testRetryHandler = new RetryHandler({ - maxAttempts: 3, - baseDelayMs: 0, - backoffMultiplier: 1, - maxDelayMs: 0, - jitter: false, - }); + it('retries transient errors at exact exponential-backoff boundaries', async () => { + const operation = vi + .fn() + .mockRejectedValueOnce(serviceUnavailable()) + .mockRejectedValueOnce(serviceUnavailable()) + .mockResolvedValueOnce('success'); - let attempts = 0; - const operation = vi.fn().mockImplementation(async () => { - attempts++; - if (attempts < 3) { - throw OAuthErrorFactory.networkError('test-provider'); - } - return 'success'; - }); + vi.useFakeTimers(); + try { + const result = retryHandler.executeWithRetry(operation, 'test-provider'); + result.catch(() => {}); - const result = await testRetryHandler.executeWithRetry( - operation, - 'test-provider', + await vi.advanceTimersByTimeAsync(0); + expect(operation).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(999); + expect(operation).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(1); + expect(operation).toHaveBeenCalledTimes(2); + + await vi.advanceTimersByTimeAsync(1999); + expect(operation).toHaveBeenCalledTimes(2); + + await vi.advanceTimersByTimeAsync(1); + expect(operation).toHaveBeenCalledTimes(3); + + await expect(result).resolves.toBe('success'); + } finally { + vi.useRealTimers(); + } + }); + + it('caps later retry delays at maxDelayMs and proves every exact delay boundary', async () => { + const operation = vi.fn().mockRejectedValue(serviceUnavailable()); + + vi.useFakeTimers(); + try { + const result = retryHandler.executeWithRetry(operation, 'test-provider'); + result.catch(() => {}); + + await vi.advanceTimersByTimeAsync(0); + expect(operation).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(999); + expect(operation).toHaveBeenCalledTimes(1); + await vi.advanceTimersByTimeAsync(1); + expect(operation).toHaveBeenCalledTimes(2); + + await vi.advanceTimersByTimeAsync(1999); + expect(operation).toHaveBeenCalledTimes(2); + await vi.advanceTimersByTimeAsync(1); + expect(operation).toHaveBeenCalledTimes(3); + + await vi.advanceTimersByTimeAsync(3999); + expect(operation).toHaveBeenCalledTimes(3); + await vi.advanceTimersByTimeAsync(1); + expect(operation).toHaveBeenCalledTimes(4); + + await vi.advanceTimersByTimeAsync(4999); + expect(operation).toHaveBeenCalledTimes(4); + await vi.advanceTimersByTimeAsync(1); + expect(operation).toHaveBeenCalledTimes(5); + + await expect(result).rejects.toBeInstanceOf(OAuthError); + } finally { + vi.useRealTimers(); + } + }); + + it('retries at the explicit retryAfterMs boundary', async () => { + const operation = vi + .fn() + .mockRejectedValueOnce(OAuthErrorFactory.rateLimited('test-provider', 2)) + .mockResolvedValueOnce('success'); + + vi.useFakeTimers(); + try { + const result = retryHandler.executeWithRetry(operation, 'test-provider'); + result.catch(() => {}); + + await vi.advanceTimersByTimeAsync(0); + expect(operation).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(1999); + expect(operation).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(1); + expect(operation).toHaveBeenCalledTimes(2); + + await expect(result).resolves.toBe('success'); + } finally { + vi.useRealTimers(); + } + }); + + it('applies jitter across the documented 50%-100% delay range', async () => { + const jitterRetryHandler = new RetryHandler( + { + maxAttempts: 3, + baseDelayMs: 1000, + backoffMultiplier: 1, + maxDelayMs: 5000, + jitter: true, + }, + silentLogger, ); + const operation = vi + .fn() + .mockRejectedValueOnce(serviceUnavailable()) + .mockRejectedValueOnce(serviceUnavailable()) + .mockResolvedValueOnce('success'); + const randomSpy = vi.spyOn(Math, 'random'); + randomSpy.mockReturnValueOnce(0); + randomSpy.mockReturnValueOnce(0.99); + + vi.useFakeTimers(); + try { + const result = jitterRetryHandler.executeWithRetry( + operation, + 'test-provider', + ); + result.catch(() => {}); - expect(result).toBe('success'); - expect(operation).toHaveBeenCalledTimes(3); + await vi.advanceTimersByTimeAsync(0); + expect(operation).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(499); + expect(operation).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(1); + expect(operation).toHaveBeenCalledTimes(2); + + await vi.advanceTimersByTimeAsync(994); + expect(operation).toHaveBeenCalledTimes(2); + + await vi.advanceTimersByTimeAsync(1); + expect(operation).toHaveBeenCalledTimes(3); + + await expect(result).resolves.toBe('success'); + } finally { + vi.useRealTimers(); + randomSpy.mockRestore(); + } }); it('should not retry non-transient errors', async () => { @@ -373,45 +507,22 @@ describe('RetryHandler', () => { expect(operation).toHaveBeenCalledTimes(1); }); - it('should respect specific retry delays from errors', async () => { - let attempts = 0; - const operation = vi.fn().mockImplementation(async () => { - attempts++; - if (attempts === 1) { - throw OAuthErrorFactory.rateLimited('test-provider', 2); // 2 second delay - } - return 'success'; - }); + it('fails after exhausting the maximum number of attempts', async () => { + const operation = vi.fn().mockRejectedValue(serviceUnavailable()); - // retryAfterMs from the error is capped by maxDelayMs: 0, so this completes - // without real delay. - const result = await retryHandler.executeWithRetry( - operation, - 'test-provider', - ); - - expect(result).toBe('success'); - expect(operation).toHaveBeenCalledTimes(2); - }); - - it('should fail after max attempts', async () => { - // Use a retry handler with no delay to avoid timing issues - const testRetryHandler = new RetryHandler({ - maxAttempts: 3, - baseDelayMs: 0, - backoffMultiplier: 1, - maxDelayMs: 0, - jitter: false, - }); + vi.useFakeTimers(); + try { + const result = retryHandler.executeWithRetry(operation, 'test-provider'); + result.catch(() => {}); - const operation = vi - .fn() - .mockRejectedValue(OAuthErrorFactory.networkError('test-provider')); + await vi.advanceTimersByTimeAsync(0); + await vi.advanceTimersByTimeAsync(100000); - await expect( - testRetryHandler.executeWithRetry(operation, 'test-provider'), - ).rejects.toThrow('Network error'); - expect(operation).toHaveBeenCalledTimes(3); // Initial + 2 retries + expect(operation).toHaveBeenCalledTimes(5); + await expect(result).rejects.toBeInstanceOf(OAuthError); + } finally { + vi.useRealTimers(); + } }); it('should convert non-OAuth errors to OAuth errors', async () => { @@ -421,25 +532,6 @@ describe('RetryHandler', () => { retryHandler.executeWithRetry(operation, 'test-provider'), ).rejects.toBeInstanceOf(OAuthError); }); - - it('should apply jitter when enabled', async () => { - const retryHandlerWithJitter = new RetryHandler({ - ...DEFAULT_RETRY_CONFIG, - baseDelayMs: 0, - maxDelayMs: 0, - jitter: true, - }); - - const operation = vi - .fn() - .mockRejectedValue(OAuthErrorFactory.networkError('test-provider')); - - // We can't easily test the exact jitter values, but we can verify it - // eventually rejects after retries without crashing. - await expect( - retryHandlerWithJitter.executeWithRetry(operation, 'test-provider'), - ).rejects.toBeInstanceOf(OAuthError); - }); }); describe('GracefulErrorHandler', () => { diff --git a/packages/auth/src/__tests__/run-bun-tests.behavior.test.ts b/packages/auth/src/__tests__/run-bun-tests.behavior.test.ts new file mode 100644 index 0000000000..03e8d13902 --- /dev/null +++ b/packages/auth/src/__tests__/run-bun-tests.behavior.test.ts @@ -0,0 +1,132 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect } from 'bun:test'; +import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { + formatFailureReason, + generateJUnit, + runTestFile, + type TestResult, +} from '../../run-bun-tests.js'; + +describe('auth run-bun-tests JUnit failure reporting', () => { + const baseResult: TestResult = { + file: 'src/sample.test.ts', + passed: false, + exitCode: null, + timedOut: false, + signal: null, + }; + + it('identifies signal termination in JUnit failure text', () => { + const xml = generateJUnit([{ ...baseResult, signal: 'SIGTERM' }], 1, 1); + expect(xml).toContain('Killed by signal SIGTERM'); + }); + + it('reports a nonzero numeric exit code', () => { + const xml = generateJUnit( + [{ ...baseResult, exitCode: 1, signal: null }], + 1, + 1, + ); + expect(xml).toContain('Exit code 1'); + }); + + it('reports a timeout', () => { + const xml = generateJUnit([{ ...baseResult, timedOut: true }], 1, 1); + expect(xml).toContain('Timed out after 60s'); + }); + + it('falls back to an exit code when neither signal nor timeout is present', () => { + const xml = generateJUnit( + [{ ...baseResult, exitCode: null, signal: null, timedOut: false }], + 1, + 1, + ); + expect(xml).toContain('Exit code -1'); + }); +}); + +describe('auth run-bun-tests failure reason formatting', () => { + const baseResult: TestResult = { + file: 'src/sample.test.ts', + passed: false, + exitCode: null, + timedOut: false, + signal: null, + }; + + it('prioritizes timeout over signal and exit code', () => { + const reason = formatFailureReason({ + ...baseResult, + timedOut: true, + signal: 'SIGTERM', + exitCode: 1, + }); + expect(reason).toBe('Timed out after 60s'); + }); + + it('reports a signal before an exit code', () => { + const reason = formatFailureReason({ + ...baseResult, + signal: 'SIGKILL', + exitCode: 1, + }); + expect(reason).toBe('Killed by signal SIGKILL'); + }); + + it('reports a numeric exit code when there is no timeout or signal', () => { + const reason = formatFailureReason({ + ...baseResult, + exitCode: 7, + signal: null, + }); + expect(reason).toBe('Exit code 7'); + }); + + it('falls back to exit code -1 when the exit code is null', () => { + const reason = formatFailureReason({ + ...baseResult, + exitCode: null, + signal: null, + }); + expect(reason).toBe('Exit code -1'); + }); +}); + +const isWindows = process.platform === 'win32'; + +describe('auth run-bun-tests real child signal propagation', () => { + it.skipIf(isWindows)( + 'carries a real SIGTERM child exit into the result and JUnit failure text', + async () => { + const fixtureDir = mkdtempSync(join(tmpdir(), 'auth-signal-')); + const fixturePath = join(fixtureDir, 'self-sigterm.test.ts'); + writeFileSync( + fixturePath, + `import { test } from 'bun:test'; +test('self-terminate', () => { + process.kill(process.pid, 'SIGTERM'); +}); +`, + ); + try { + const result = await runTestFile(fixturePath); + + expect(result.signal).toBe('SIGTERM'); + expect(result.passed).toBe(false); + + const xml = generateJUnit([result], 1, 1); + expect(xml).toContain('Killed by signal SIGTERM'); + } finally { + rmSync(fixtureDir, { recursive: true, force: true }); + } + }, + ); +}); diff --git a/packages/core/src/config/config.d.test.ts b/packages/core/src/config/config.d.test.ts index a7a1de2d6d..482c8aac3c 100644 --- a/packages/core/src/config/config.d.test.ts +++ b/packages/core/src/config/config.d.test.ts @@ -5,7 +5,7 @@ */ import path from 'node:path'; -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'bun:test'; import type { ConfigParameters } from './config.js'; import { Config, ApprovalMode } from './config.js'; import type { HookDefinition } from '../hooks/types.js'; @@ -59,7 +59,7 @@ const mcpInstances: Array<{ }> = []; vi.mock('@vybestack/llxprt-code-mcp', (importOriginal) => { - const actual = importOriginal() as Record; + const actual = importOriginal(); return { ...actual, McpClientManager: vi.fn().mockImplementation(() => { diff --git a/packages/core/src/config/config.folderTrustMcpWiring.test.ts b/packages/core/src/config/config.folderTrustMcpWiring.test.ts index 2f2a1484c3..19218c8f6b 100644 --- a/packages/core/src/config/config.folderTrustMcpWiring.test.ts +++ b/packages/core/src/config/config.folderTrustMcpWiring.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'bun:test'; interface InstanceMock { startConfiguredMcpServers: ReturnType; @@ -379,18 +379,20 @@ describe('Config MCP wiring on folder trust change', () => { (error: unknown) => error, ); outcome.catch(() => {}); - // Advance time past the hook initialization timeout and drain - // all pending timers/microtasks. - vi.runAllTimers(); - await vi.runAllTimersAsync(); + await vi.advanceTimersByTimeAsync(0); + const initializationSignal = hookInitializers[0].mock.calls[0][0]; + + await vi.advanceTimersByTimeAsync(29_999); + expect(initializationSignal).toBeInstanceOf(AbortSignal); + expect(initializationSignal.aborted).toBe(false); + + await vi.advanceTimersByTimeAsync(1); const result = await outcome; expect(result).toBeInstanceOf(Error); expect(result).toMatchObject({ message: expect.stringContaining('timed out'), }); - const initializationSignal = hookInitializers[0].mock.calls[0][0]; - expect(initializationSignal).toBeInstanceOf(AbortSignal); expect(initializationSignal.aborted).toBe(true); } finally { vi.useRealTimers(); diff --git a/packages/core/src/integration-tests/provider-settings-integration.spec.ts b/packages/core/src/integration-tests/provider-settings-integration.test.ts similarity index 92% rename from packages/core/src/integration-tests/provider-settings-integration.spec.ts rename to packages/core/src/integration-tests/provider-settings-integration.test.ts index 1a0cd8ccc7..e4d15d2f77 100644 --- a/packages/core/src/integration-tests/provider-settings-integration.spec.ts +++ b/packages/core/src/integration-tests/provider-settings-integration.test.ts @@ -2,7 +2,7 @@ * Integration tests for Phase 12: Provider Settings Integration */ -import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'bun:test'; import { SettingsService } from '@vybestack/llxprt-code-settings'; import { BaseProvider } from '@vybestack/llxprt-code-providers/BaseProvider.js'; import { getSettingsService } from '@vybestack/llxprt-code-settings'; @@ -61,7 +61,7 @@ describe('Provider Settings Integration', () => { { settingsService, runtimeId: `provider.settings.integration.${name}`, - metadata: { source: 'provider-settings-integration.spec.ts' }, + metadata: { source: 'provider-settings-integration.test.ts' }, }, ).provider; @@ -154,16 +154,9 @@ describe('Provider Settings Integration', () => { // Provider methods should work properly with SettingsService const provider = instantiateProvider('test-compat'); - // These should work with SettingsService integration - let error: unknown; - try { - await provider.getModelFromSettings(); - await provider.getApiKeyFromSettings(); - await provider.getBaseUrlFromSettings(); - await provider.getModelParamsFromSettings(); - } catch (caught) { - error = caught; - } - expect(error).toBeUndefined(); + await provider.getModelFromSettings(); + await provider.getApiKeyFromSettings(); + await provider.getBaseUrlFromSettings(); + await provider.getModelParamsFromSettings(); }); }); diff --git a/packages/core/src/recording/sessionManagement.test.ts b/packages/core/src/recording/sessionManagement.test.ts index 5a48130a9f..34021181d5 100644 --- a/packages/core/src/recording/sessionManagement.test.ts +++ b/packages/core/src/recording/sessionManagement.test.ts @@ -26,7 +26,7 @@ * Property-based tests use fast-check (≥30% of total tests). */ -import { describe, expect, beforeEach, afterEach, it } from 'vitest'; +import { describe, expect, beforeEach, afterEach, it } from 'bun:test'; import * as fc from 'fast-check'; import * as fs from 'node:fs/promises'; import * as path from 'node:path'; @@ -295,9 +295,7 @@ describe('sessionManagement @plan:PLAN-20260211-SESSIONRECORDING.P22', () => { expect(result.ok).toBe(true); const okResult = result as Extract; - { - expect(okResult.deletedSessionId).toBe(sessionId); - } + expect(okResult.deletedSessionId).toBe(sessionId); expect(await fileExists(filePath)).toBe(false); }); @@ -322,9 +320,7 @@ describe('sessionManagement @plan:PLAN-20260211-SESSIONRECORDING.P22', () => { expect(result.ok).toBe(true); const okResult = result as Extract; - { - expect(okResult.deletedSessionId).toBe(sessionId); - } + expect(okResult.deletedSessionId).toBe(sessionId); expect(await fileExists(filePath)).toBe(false); }); @@ -350,9 +346,7 @@ describe('sessionManagement @plan:PLAN-20260211-SESSIONRECORDING.P22', () => { expect(result.ok).toBe(true); const okResult = result as Extract; - { - expect(okResult.deletedSessionId).toBe('newest-del'); - } + expect(okResult.deletedSessionId).toBe('newest-del'); expect(await fileExists(newestPath)).toBe(false); }); @@ -398,9 +392,7 @@ describe('sessionManagement @plan:PLAN-20260211-SESSIONRECORDING.P22', () => { expect(result.ok).toBe(true); const okResult = result as Extract; - { - expect(okResult.deletedSessionId).toBe(sessionId); - } + expect(okResult.deletedSessionId).toBe(sessionId); }); it('lists live checkpoint blockers instead of deleting their source', async () => { @@ -481,9 +473,7 @@ describe('sessionManagement @plan:PLAN-20260211-SESSIONRECORDING.P22', () => { expect(result.ok).toBe(false); const errResult = result as Extract; - { - expect(errResult.error).toContain('in use'); - } + expect(errResult.error).toContain('in use'); // File should still exist expect(await fileExists(filePath)).toBe(true); } finally { @@ -513,9 +503,7 @@ describe('sessionManagement @plan:PLAN-20260211-SESSIONRECORDING.P22', () => { expect(result.ok).toBe(true); const okResult = result as Extract; - { - expect(okResult.deletedSessionId).toBe(sessionId); - } + expect(okResult.deletedSessionId).toBe(sessionId); expect(await fileExists(filePath)).toBe(false); expect(await fileExists(lockPath)).toBe(false); }); @@ -541,9 +529,7 @@ describe('sessionManagement @plan:PLAN-20260211-SESSIONRECORDING.P22', () => { expect(result.ok).toBe(false); const errResult = result as Extract; - { - expect(errResult.error).toContain('not found'); - } + expect(errResult.error).toContain('not found'); }); /** @@ -560,9 +546,7 @@ describe('sessionManagement @plan:PLAN-20260211-SESSIONRECORDING.P22', () => { expect(result.ok).toBe(false); const errResult = result as Extract; - { - expect(errResult.error).toContain('No sessions found'); - } + expect(errResult.error).toContain('No sessions found'); }); }); @@ -773,9 +757,7 @@ describe('sessionManagement @plan:PLAN-20260211-SESSIONRECORDING.P22', () => { expect(result.ok).toBe(true); const okResult = result as Extract; - { - expect(okResult.deletedSessionId).toBe(targetSession.sessionId); - } + expect(okResult.deletedSessionId).toBe(targetSession.sessionId); expect(await fileExists(targetSession.filePath)).toBe(false); } finally { await fs.rm(localTempDir, { recursive: true, force: true }); diff --git a/packages/core/test/utils/ripgrepPathResolver.test.ts b/packages/core/test/utils/ripgrepPathResolver.test.ts index c70083c52a..a2b4f831ca 100644 --- a/packages/core/test/utils/ripgrepPathResolver.test.ts +++ b/packages/core/test/utils/ripgrepPathResolver.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'bun:test'; import fs from 'fs'; import os from 'os'; import { @@ -14,6 +14,21 @@ import { clearRipgrepAvailabilityCache, } from '../../src/utils/ripgrepPathResolver.js'; +function mockRipgrepPackageUnavailable(): void { + vi.doMock( + '@lvce-editor/ripgrep', + () => + new Proxy( + {}, + { + get: () => { + throw new Error('Package not available'); + }, + }, + ), + ); +} + describe('RipgrepPathResolver - Cross-platform Path Resolution', () => { beforeEach(() => { vi.clearAllMocks(); @@ -78,18 +93,7 @@ describe('RipgrepPathResolver - Cross-platform Path Resolution', () => { }); it('should fall back to system ripgrep when package not available', async () => { // Mock package to fail - vi.doMock( - '@lvce-editor/ripgrep', - () => - new Proxy( - {}, - { - get: () => { - throw new Error('Package not available'); - }, - }, - ), - ); + mockRipgrepPackageUnavailable(); // Mock system ripgrep available const mockExecSync = vi.fn().mockReturnValue('/usr/local/bin/rg\n'); @@ -115,18 +119,7 @@ describe('RipgrepPathResolver - Cross-platform Path Resolution', () => { (os.platform as unknown) = mockPlatform; // Mock package and system ripgrep to fail - vi.doMock( - '@lvce-editor/ripgrep', - () => - new Proxy( - {}, - { - get: () => { - throw new Error('Package not available'); - }, - }, - ), - ); + mockRipgrepPackageUnavailable(); const mockExecSync = vi.fn().mockImplementation(() => { throw new Error('Command not found'); @@ -155,18 +148,7 @@ describe('RipgrepPathResolver - Cross-platform Path Resolution', () => { (os.platform as unknown) = mockPlatform; // Mock package and system ripgrep to fail - vi.doMock( - '@lvce-editor/ripgrep', - () => - new Proxy( - {}, - { - get: () => { - throw new Error('Package not available'); - }, - }, - ), - ); + mockRipgrepPackageUnavailable(); const mockExecSync = vi.fn().mockImplementation(() => { throw new Error('Command not found'); @@ -193,18 +175,7 @@ describe('RipgrepPathResolver - Cross-platform Path Resolution', () => { (process.pkg as unknown) = { entrypoint: '/path/to/bundle' }; // Mock package and system ripgrep to fail - vi.doMock( - '@lvce-editor/ripgrep', - () => - new Proxy( - {}, - { - get: () => { - throw new Error('Package not available'); - }, - }, - ), - ); + mockRipgrepPackageUnavailable(); const mockExecSync = vi.fn().mockImplementation(() => { throw new Error('Command not found'); @@ -235,18 +206,7 @@ describe('RipgrepPathResolver - Cross-platform Path Resolution', () => { it('should provide helpful error message when ripgrep not found', async () => { // Mock all ripgrep sources to fail - vi.doMock( - '@lvce-editor/ripgrep', - () => - new Proxy( - {}, - { - get: () => { - throw new Error('Package not available'); - }, - }, - ), - ); + mockRipgrepPackageUnavailable(); const mockExecSync = vi.fn().mockImplementation(() => { throw new Error('Command not found'); diff --git a/project-plans/issue2904/plan.md b/project-plans/issue2904/plan.md new file mode 100644 index 0000000000..0d48c70aa4 --- /dev/null +++ b/project-plans/issue2904/plan.md @@ -0,0 +1,137 @@ +# Issue 2904 implementation plan + +## Objective + +Resolve the still-valid test-quality findings recorded in issue 2904 without broadening Bun test discovery, changing unrelated runners, or introducing shared/public test abstractions. + +## Preflight evidence + +- The issue branch starts from current `origin/main` with a clean working tree. +- The current auth runner executes 33 `.test.*` files but omits all 9 `.spec.*` files, including `oauth-errors.spec.ts`. +- A direct Bun run of `oauth-errors.spec.ts` passes 38 tests, but its purported backoff and jitter tests wait on retry-after delays and do not exercise exponential or jitter calculations. +- Direct Bun runs of the five accepted core target suites pass 110 tests before changes. + +## Finding classification + +| Finding or reviewer suggestion | Classification | Rationale | +| --- | --- | --- | +| OAuth retry tests disable backoff and jitter timing | In-scope-Fix | This is the issue's only medium bug and requires behavioral timing evidence. | +| Folder-trust timeout test drains all timers | In-scope-Fix | Replace broad timer draining with advancement to the known 30-second boundary. | +| Provider-settings no-throw test uses try/catch | In-scope-Fix | Direct awaited calls are simpler, fail naturally, and avoid a broken matcher. | +| OAuth test duplicates the shared retry-handler configuration | In-scope-Fix | Reuse a meaningful shared non-jitter handler for the relevant cases. | +| OAuth jitter handler shadows the shared handler | In-scope-Fix | Give the timing-specific jitter handler a distinct name and configuration. | +| MCP mock uses a broad `Record` cast | In-scope-Fix | Use the established generic `importOriginal()` form, avoiding a type assertion. | +| Session property tests have inconsistent `numRuns` | Reject | Stale: every current property assertion already specifies `numRuns: 20`. | +| Ripgrep unavailable-package Proxy is duplicated | In-scope-Fix | A file-local helper removes the five identical mock factories without creating a shared abstraction. | +| Session assertions contain unnecessary bare scopes | In-scope-Fix | Remove only the redundant braces; preserve assertion behavior. | +| Auth runner reports a signal exit as a null/-1 exit code | In-scope-Fix | Carry the child signal into the result and emit explicit JUnit failure text. | +| `toolDeclaration.test.ts` has a misplaced fast-check array constraint | Reject | The issue already records this as a verified false positive. | +| Also change the core Bun runner's signal reporting | Reject | The literal finding concerns the auth runner; changing another runner is adjacent scope. | +| Add a shared `expectNoThrowAsync` test utility | Reject | It is an unplanned shared abstraction; direct awaits are sufficient. | +| Repair undefined `itProp` in session tests | Reject | Stale: the current suite imports and uses `it`. | +| Extract broader session result-assertion helpers | Reject | The accepted finding is redundant scopes, not a larger refactor. | +| Add a ripgrep available-package helper too | Reject | The available mocks are not identical, and no accepted change needs the abstraction. | + +## Accepted behavior and evidence + +### REQ-2904-001: Auth retry timing is real and CI-gated + +**GIVEN** a retryable `SERVICE_UNAVAILABLE` OAuth error without `retryAfterMs`, nonzero base delay, multiplier greater than one, and a finite maximum delay +**WHEN** retry attempts are scheduled +**THEN** no attempt occurs immediately before each expected boundary, an attempt occurs at each exact boundary, exponential growth is used, and later delays are capped at `maxDelayMs`. + +**GIVEN** the same retryable error and deterministic `Math.random()` values +**WHEN** jitter is enabled +**THEN** observed retry boundaries demonstrate the documented 50%-to-100% delay range rather than only eventual retry count. + +**GIVEN** a retryable error with an explicit `retryAfterMs` +**WHEN** a retry is scheduled +**THEN** the retry occurs at that explicit boundary and not one millisecond earlier. + +Evidence: + +- Rename only the directly related `oauth-errors.spec.ts` to `oauth-errors.test.ts`, because the auth runner already discovers `.test.ts`. Do not broaden discovery to the other eight unrelated spec suites. +- Convert the renamed suite to `bun:test`. +- Use fake timers and attach rejection handlers before advancing time. +- Use `SERVICE_UNAVAILABLE` without retry-after for exponential and jitter tests; factory network errors are unsuitable because they set `retryAfterMs`. +- Retain distinct local jitter configuration only where the behavior requires it; do not add a shared helper module. + +### REQ-2904-002: Folder-trust timeout advances only its timer + +**GIVEN** hook initialization remains pending during a live folder-trust transition +**WHEN** fake time reaches 30 seconds +**THEN** the transition rejects with the existing timeout error and its abort signal is aborted, without draining unrelated future timers. + +Evidence: convert the changed suite to `bun:test`, advance 29,999 ms to establish the pre-boundary state, then one additional millisecond and assert the existing rejection and aborted signal. + +### REQ-2904-003: Provider compatibility fails naturally on a thrown getter + +**GIVEN** a provider backed by the settings service +**WHEN** its model, API-key, base-URL, and model-parameter getters are awaited +**THEN** the test completes only when every call resolves; a thrown error fails the test directly. + +Evidence: convert the changed suite to `bun:test` and directly await the four calls without try/catch or a no-throw helper. + +### REQ-2904-004: MCP partial mock retains the module's precise type + +**GIVEN** the config test's partial MCP module mock +**WHEN** the original module is imported +**THEN** its known exports remain typed as `typeof import('@vybestack/llxprt-code-mcp')` and the existing config suite remains behaviorally unchanged. + +Evidence: convert the changed suite to `bun:test`, use generic `importOriginal()`, and run the full config suite plus type/lint gates. Do not add a type assertion. + +### REQ-2904-005: Accepted local test cleanup preserves behavior + +**GIVEN** the ripgrep resolver's package-unavailable scenarios +**WHEN** each scenario installs its mock +**THEN** a single file-local helper supplies the same throwing Proxy behavior to the five existing callers. + +**GIVEN** the existing session-management result assertions +**WHEN** the redundant bare scopes are removed +**THEN** each existing assertion remains unchanged and all property-test run counts stay at 20. + +Evidence: convert both changed suites to `bun:test`, run them in full, and make no helper extraction beyond the identical unavailable-package mock. + +### REQ-2904-006: Auth JUnit failures identify process termination + +**GIVEN** an auth test child exits because of a signal and has a null numeric exit code +**WHEN** the runner builds its result and JUnit report +**THEN** the result retains the signal and the failure message identifies it, for example `Killed by signal SIGTERM`. + +**GIVEN** a numeric nonzero exit, a timeout, or the null-without-signal fallback +**WHEN** JUnit is generated +**THEN** each condition has explicit, stable failure text and numeric exit-code reporting remains intact. + +Evidence: + +- Add a Bun test for the runner's observable JUnit output before production changes. +- Add only a narrow internal test seam (such as an exported JUnit helper) and `if (import.meta.main)` guard, following the existing script pattern; do not add a package export. +- Carry the standard child-process `exit` signal through `TestResult` and all result construction paths. +- Test signal, numeric exit, timeout, and null-without-signal output. Prefer real exit-event behavior where portable; do not mock child-process interactions merely to satisfy coverage. + +## TDD sequence + +1. Rename and strengthen the OAuth suite, run it through the auth runner, and confirm its timing boundaries pass against the existing production behavior. +2. Add the auth runner JUnit behavior test and demonstrate RED against current signal-unaware output/import behavior. +3. Add the minimal signal field, exit-event wiring, explicit failure formatting, and import guard; demonstrate GREEN. +4. Make each accepted core test-only refinement and run the complete affected suite immediately after the change. +5. Run all affected auth/core tests together, then the repository's complete verification suite and smoke test. +6. Review only the candidate diff; classify all findings using the same four categories and remediate every Blocker-Fix and In-scope-Fix finding. + +## Scope boundaries + +- No production retry behavior changes. +- No changes to core/CLI runners, auth discovery patterns, dependencies, workflows, lint configuration, test utilities shared across packages, or public package exports. +- No change to session property-test run counts or unrelated mocks. +- No TUI changes, so tmux verification is not required. +- No optional hardening after the accepted behavior and required gates pass. + +## Review remediation (follow-up findings) + +### Real child-signal evidence for REQ-2904-006 + +The initial signal reporting carried synthetic `TestResult` fixtures through `generateJUnit`. To prove the observable behavior end to end, the narrow internal `runTestFile` seam is now exported at the module level (not as a `package.json` export). A `bun:test` case writes a temporary fixture that self-terminates with `SIGTERM`, invokes the real `runTestFile`, and feeds the resulting `TestResult` to `generateJUnit`, asserting `result.signal === 'SIGTERM'`, `result.passed === false`, and `Killed by signal SIGTERM`. It is skipped via `it.skipIf(process.platform === 'win32')` where reliable POSIX signal reporting is unavailable; the exit callback is not faked. The existing synthetic numeric/timeout/null formatting cases are retained. This case fails the moment `runTestFile` stops propagating the child exit signal (verified RED by temporarily coercing the signal to `null`). + +### Provider suite rename for core runner discovery + +`packages/core/src/integration-tests/provider-settings-integration.spec.ts` is renamed to `provider-settings-integration.test.ts` so the existing core `run-bun-tests.ts` (which discovers only `.test.ts`/`.test.tsx`) includes this suite directly. Only this one spec is renamed; runner discovery is not broadened and no unrelated specs are renamed. The stale in-file `metadata.source` self-reference was updated to match. From 5b5fc9282015d4241555d73b4523ec5c83b8ad81 Mon Sep 17 00:00:00 2001 From: acoliver Date: Thu, 6 Aug 2026 20:04:21 -0300 Subject: [PATCH 2/3] Prevent cleanup from masking signal-test failures --- .../__tests__/run-bun-tests.behavior.test.ts | 28 ++++++------------- .../test-fixtures/self-sigterm.fixture.ts | 11 ++++++++ project-plans/issue2904/plan.md | 4 +++ 3 files changed, 23 insertions(+), 20 deletions(-) create mode 100644 packages/auth/test-fixtures/self-sigterm.fixture.ts 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 03e8d13902..fc2e516735 100644 --- a/packages/auth/src/__tests__/run-bun-tests.behavior.test.ts +++ b/packages/auth/src/__tests__/run-bun-tests.behavior.test.ts @@ -5,8 +5,6 @@ */ import { describe, it, expect } from 'bun:test'; -import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; -import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { formatFailureReason, @@ -106,27 +104,17 @@ describe('auth run-bun-tests real child signal propagation', () => { it.skipIf(isWindows)( 'carries a real SIGTERM child exit into the result and JUnit failure text', async () => { - const fixtureDir = mkdtempSync(join(tmpdir(), 'auth-signal-')); - const fixturePath = join(fixtureDir, 'self-sigterm.test.ts'); - writeFileSync( - fixturePath, - `import { test } from 'bun:test'; -test('self-terminate', () => { - process.kill(process.pid, 'SIGTERM'); -}); -`, + const fixturePath = join( + import.meta.dir, + '../../test-fixtures/self-sigterm.fixture.ts', ); - try { - const result = await runTestFile(fixturePath); + const result = await runTestFile(fixturePath); - expect(result.signal).toBe('SIGTERM'); - expect(result.passed).toBe(false); + expect(result.signal).toBe('SIGTERM'); + expect(result.passed).toBe(false); - const xml = generateJUnit([result], 1, 1); - expect(xml).toContain('Killed by signal SIGTERM'); - } finally { - rmSync(fixtureDir, { recursive: true, force: true }); - } + const xml = generateJUnit([result], 1, 1); + expect(xml).toContain('Killed by signal SIGTERM'); }, ); }); diff --git a/packages/auth/test-fixtures/self-sigterm.fixture.ts b/packages/auth/test-fixtures/self-sigterm.fixture.ts new file mode 100644 index 0000000000..60e149df31 --- /dev/null +++ b/packages/auth/test-fixtures/self-sigterm.fixture.ts @@ -0,0 +1,11 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { test } from 'bun:test'; + +test('self-terminate', () => { + process.kill(process.pid, 'SIGTERM'); +}); diff --git a/project-plans/issue2904/plan.md b/project-plans/issue2904/plan.md index 0d48c70aa4..34fead78b0 100644 --- a/project-plans/issue2904/plan.md +++ b/project-plans/issue2904/plan.md @@ -135,3 +135,7 @@ The initial signal reporting carried synthetic `TestResult` fixtures through `ge ### Provider suite rename for core runner discovery `packages/core/src/integration-tests/provider-settings-integration.spec.ts` is renamed to `provider-settings-integration.test.ts` so the existing core `run-bun-tests.ts` (which discovers only `.test.ts`/`.test.tsx`) includes this suite directly. Only this one spec is renamed; runner discovery is not broadened and no unrelated specs are renamed. The stale in-file `metadata.source` self-reference was updated to match. + +### PR OCR cleanup-failure finding + +The PR OCR finding that the real-signal test's `finally` cleanup could suppress an earlier behavioral assertion failure is classified **In-scope-Fix**. The concern applies directly to the new behavioral evidence. Instead of swallowing cleanup errors or adding defensive exception aggregation, the generated temporary source is replaced by a committed, non-discoverable auth test fixture. The behavioral test still invokes the real child process and verifies signal propagation, while no fallible cleanup can obscure its assertions. From 5c1b4387075c1543fff751511eaf5210ea482497 Mon Sep 17 00:00:00 2001 From: acoliver Date: Thu, 6 Aug 2026 20:07:41 -0300 Subject: [PATCH 3/3] Trigger CI after Actions webhook outage