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..fc2e516735 --- /dev/null +++ b/packages/auth/src/__tests__/run-bun-tests.behavior.test.ts @@ -0,0 +1,120 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect } from 'bun:test'; +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 fixturePath = join( + import.meta.dir, + '../../test-fixtures/self-sigterm.fixture.ts', + ); + 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'); + }, + ); +}); 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/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..34fead78b0 --- /dev/null +++ b/project-plans/issue2904/plan.md @@ -0,0 +1,141 @@ +# 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. + +### 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.