From 8d2607b26580d1434d6c9a395fb693c2894d96b5 Mon Sep 17 00:00:00 2001 From: Rudy Celekli Date: Tue, 22 Sep 2026 22:28:29 -0400 Subject: [PATCH 1/2] fix(test-execution): reject missing Vitest report evidence --- .../handlers/test-execution-handlers.ts | 8 +- .../test-execution/services/flaky-detector.ts | 22 ++- .../test-execution/services/retry-handler.ts | 13 +- .../test-execution/services/test-executor.ts | 49 ++++-- src/shared/vitest-json-report.ts | 38 +++-- .../executors/vitest-executor.ts | 8 +- .../test-executor-vitest-report.test.ts | 144 ++++++++++++++++++ .../vitest-missing-report-consumers.test.ts | 67 ++++++++ tests/unit/shared/vitest-json-report.test.ts | 22 ++- 9 files changed, 324 insertions(+), 47 deletions(-) create mode 100644 tests/unit/domains/test-execution/test-executor-vitest-report.test.ts create mode 100644 tests/unit/domains/test-execution/vitest-missing-report-consumers.test.ts diff --git a/src/coordination/handlers/test-execution-handlers.ts b/src/coordination/handlers/test-execution-handlers.ts index ee48617b7..a8056515c 100644 --- a/src/coordination/handlers/test-execution-handlers.ts +++ b/src/coordination/handlers/test-execution-handlers.ts @@ -223,17 +223,19 @@ export function registerTestExecutionHandlers(ctx: TaskHandlerContext): void { let output: string; try { execution = spawnSync('npx', ['vitest', 'run', ...testFiles, '--reporter=json', ...report.args], options); - output = report.read(execution.stdout || ''); + output = execution.error || execution.signal + ? '' + : report.read(execution.stdout || '') ?? ''; } finally { report.cleanup(); } // Preserve the existing Jest fallback when Vitest cannot produce a report. - if (!output.includes('{') && execution.status !== 0) { + if (!output.includes('{') && execution.status !== 0 && !execution.error && !execution.signal) { runner = 'jest'; execution = spawnSync('npx', ['jest', ...testFiles, '--json'], options); output = execution.stdout || ''; } - const diagnostics = [execution.error?.message, execution.stderr, output].filter(Boolean).join('\n'); + const diagnostics = [execution.error?.message, execution.signal && `Terminated by ${execution.signal}`, execution.stderr, output].filter(Boolean).join('\n'); if (execution.error) { return err(new TestRunnerExecutionError(`${runner} could not complete: ${diagnostics.slice(0, 4000)}`)); } diff --git a/src/domains/test-execution/services/flaky-detector.ts b/src/domains/test-execution/services/flaky-detector.ts index fae41768d..821b07d9f 100644 --- a/src/domains/test-execution/services/flaky-detector.ts +++ b/src/domains/test-execution/services/flaky-detector.ts @@ -526,8 +526,19 @@ export class FlakyDetectorService implements IFlakyTestDetector { child.on('close', (code) => { clearTimeout(timeout); const duration = Date.now() - startTime; - const reportText = report ? report.read(stdout) : stdout; + let reportText: string | undefined; + try { + reportText = report ? report.read(stdout) : stdout; + } catch (error) { + report?.cleanup(); + reject(toError(error)); + return; + } report?.cleanup(); + if (reportText === undefined) { + reject(new Error(`The current Vitest JSON report is missing for ${file}.`)); + return; + } try { // Parse the test results from the JSON report (or stdout for other runners) @@ -543,6 +554,7 @@ export class FlakyDetectorService implements IFlakyTestDetector { // If parsing fails but we have an exit code, create a single result for the file if (parsedResults.size === 0) { + if (report) throw new Error(`The current Vitest JSON report has no test results for ${file}.`); const testId = this.generateTestId(file, 'main'); results.set(testId, [ { @@ -565,6 +577,10 @@ export class FlakyDetectorService implements IFlakyTestDetector { resolve(results); } catch (parseError) { + if (report) { + reject(toError(parseError)); + return; + } // If we can't parse output but process completed, create result from exit code const testId = this.generateTestId(file, 'main'); results.set(testId, [ @@ -610,7 +626,7 @@ export class FlakyDetectorService implements IFlakyTestDetector { const parsed = safeJsonParse(jsonOutput); return this.parseVitestJson(parsed, file, runId, runIndex); } - } catch (error) { + } catch { // Non-critical: not valid JSON, try other formats logger.debug('Vitest JSON parse failed:'); } @@ -624,7 +640,7 @@ export class FlakyDetectorService implements IFlakyTestDetector { return this.parseJestJson(parsed, file, runId, runIndex); } } - } catch (error) { + } catch { // Non-critical: not Jest format logger.debug('Jest JSON parse failed:'); } diff --git a/src/domains/test-execution/services/retry-handler.ts b/src/domains/test-execution/services/retry-handler.ts index 2d8ef39dc..39a8c5071 100644 --- a/src/domains/test-execution/services/retry-handler.ts +++ b/src/domains/test-execution/services/retry-handler.ts @@ -517,7 +517,7 @@ export class RetryHandlerService implements IRetryHandler { if ('jest' in devDeps) return 'jest'; if ('mocha' in devDeps) return 'mocha'; } - } catch (error) { + } catch { // Non-critical: package.json read errors during test runner detection logger.debug('package.json read failed:'); } @@ -621,8 +621,15 @@ export class RetryHandlerService implements IRetryHandler { // Parse result based on exit code and output (Vitest writes the JSON // report to the --outputFile; read it back rather than trusting stdout). - const result = this.parseTestResult(code, report ? report.read(stdout) : stdout, stderr); - settle(resolve, result); + try { + const reportText = report ? report.read(stdout) : stdout; + if (reportText === undefined) { + throw new Error('The current Vitest JSON report is missing for the retry run.'); + } + settle(resolve, this.parseTestResult(code, reportText, stderr)); + } catch (error) { + settle(reject, toError(error)); + } }); proc.on('error', (err: Error) => { diff --git a/src/domains/test-execution/services/test-executor.ts b/src/domains/test-execution/services/test-executor.ts index 556996ba7..41c94b655 100644 --- a/src/domains/test-execution/services/test-executor.ts +++ b/src/domains/test-execution/services/test-executor.ts @@ -390,7 +390,7 @@ Provide: maxTokens: this.config.llmMaxTokens, }); return response.content; - } catch (error) { + } catch { logger.warn('LLM analysis failed:'); return null; } @@ -519,28 +519,37 @@ Provide: let stdout = ''; let stderr = ''; let killed = false; - const finish = (result: Result): void => { - report?.cleanup(); + let settled = false; + const finish = (result: Result, cleanup = true): void => { + if (settled) return; + settled = true; + if (cleanup) report?.cleanup(); resolve(result); }; // Spawn the test runner process // Note: shell: false (default) to prevent command injection (CWE-78) // Arguments are passed as array to avoid shell interpretation - const proc: ChildProcess = spawn(command, args, { - cwd: process.cwd(), - env: { - ...process.env, - FORCE_COLOR: '0', // Disable color codes for easier parsing - CI: 'true', // Enable CI mode for consistent output - }, - }); + let proc: ChildProcess; + try { + proc = spawn(command, args, { + cwd: process.cwd(), + env: { + ...process.env, + FORCE_COLOR: '0', // Disable color codes for easier parsing + CI: 'true', // Enable CI mode for consistent output + }, + }); + } catch (error) { + finish(err(new Error(`Failed to spawn test runner: ${toErrorMessage(error)}. Is '${command}' installed?`))); + return; + } // Set timeout const timeoutId = setTimeout(() => { killed = true; proc.kill('SIGTERM'); - finish(err(new Error(`Test execution timed out after ${timeout}ms for files: ${fileLabel}`))); + finish(err(new Error(`Test execution timed out after ${timeout}ms for files: ${fileLabel}`)), false); }, timeout); proc.stdout?.on('data', (data: Buffer) => { @@ -555,12 +564,23 @@ Provide: clearTimeout(timeoutId); if (killed) { - return; // Already handled by timeout + report?.cleanup(); + return; // Timeout result was already returned; child has now closed. } // Parse results based on framework. Vitest 5 writes the JSON report to // a file instead of stdout, so read it back through the report handle. - const reportText = report ? report.read(stdout) : stdout; + let reportText: string | undefined; + try { + reportText = report ? report.read(stdout) : stdout; + } catch (error) { + finish(err(toError(error))); + return; + } + if (reportText === undefined) { + finish(err(new Error(`The current Vitest JSON report is missing for ${fileLabel}.`))); + return; + } const parseResult = this.parseTestOutput(reportText, stderr, fileLabel, framework, code); // If no coverage in stdout JSON, try reading from disk @@ -580,6 +600,7 @@ Provide: proc.on('error', (error: Error) => { clearTimeout(timeoutId); + if (killed) return; finish(err(new Error(`Failed to spawn test runner: ${error.message}. Is '${command}' installed?`))); }); }); diff --git a/src/shared/vitest-json-report.ts b/src/shared/vitest-json-report.ts index 8caf0e668..2b8716200 100644 --- a/src/shared/vitest-json-report.ts +++ b/src/shared/vitest-json-report.ts @@ -12,7 +12,7 @@ * instead of parsing stdout directly. */ -import { existsSync, mkdtempSync, readFileSync, rmSync } from 'node:fs'; +import { mkdtempSync, readFileSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -22,29 +22,35 @@ export interface VitestJsonReport { /** CLI arguments to append to `vitest run --reporter=json`. */ readonly args: readonly string[]; /** - * Return the text that carries the JSON document: the report file when the - * runner wrote one, otherwise the captured stdout (older runners, or a run - * that died before reporting). + * Read this invocation's report. Missing reports return undefined so callers + * can distinguish them from a completed run; malformed reports throw. + * Stdout is diagnostic text, never a substitute for the requested report. */ - read(stdout: string): string; + read(stdout: string): string | undefined; /** Remove the temporary report directory. Safe to call more than once. */ cleanup(): void; } /** - * Resolve the JSON document text for a finished Vitest run. - * Exported separately so the precedence rule is unit-testable without spawning. + * Read the report owned by a finished Vitest run. The stdout parameter stays + * for existing callers but is never treated as result evidence. */ -export function resolveVitestJsonOutput(stdout: string, reportPath: string | undefined): string { - if (reportPath && existsSync(reportPath)) { - try { - const content = readFileSync(reportPath, 'utf-8'); - if (content.trim().length > 0) return content; - } catch { - // Unreadable report: fall back to stdout below. - } +export function resolveVitestJsonOutput(_stdout: string, reportPath: string | undefined): string | undefined { + if (!reportPath) return undefined; + let content: string; + try { + content = readFileSync(reportPath, 'utf-8'); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return undefined; + throw new Error('Could not read the current Vitest JSON report.'); } - return stdout; + try { + const parsed = JSON.parse(content); + if (!parsed || !Array.isArray(parsed.testResults)) throw new Error('Invalid report'); + } catch { + throw new Error('The current Vitest JSON report is malformed.'); + } + return content; } /** diff --git a/src/test-scheduling/executors/vitest-executor.ts b/src/test-scheduling/executors/vitest-executor.ts index 7b1102bed..53e4f24e7 100644 --- a/src/test-scheduling/executors/vitest-executor.ts +++ b/src/test-scheduling/executors/vitest-executor.ts @@ -189,7 +189,11 @@ export class VitestPhaseExecutor implements PhaseExecutor { timeoutMs ); exitCode = run.exitCode; - document = report.read(run.stdout); + const currentReport = report.read(run.stdout); + if (currentReport === undefined) { + throw new Error('The current Vitest JSON report is missing for this phase.'); + } + document = currentReport; } finally { report.cleanup(); } @@ -205,7 +209,7 @@ export class VitestPhaseExecutor implements PhaseExecutor { const jsonStr = document.slice(jsonStart, jsonEnd + 1); return safeJsonParse(jsonStr); - } catch (parseError) { + } catch { // If JSON parsing fails, create a basic result from exit code return { numTotalTestSuites: 0, diff --git a/tests/unit/domains/test-execution/test-executor-vitest-report.test.ts b/tests/unit/domains/test-execution/test-executor-vitest-report.test.ts new file mode 100644 index 000000000..8d7631832 --- /dev/null +++ b/tests/unit/domains/test-execution/test-executor-vitest-report.test.ts @@ -0,0 +1,144 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { EventEmitter } from 'node:events'; +import { existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { TestExecutorService } from '../../../../src/domains/test-execution/services/test-executor.js'; +import { registerTestExecutionHandlers } from '../../../../src/coordination/handlers/test-execution-handlers.js'; +import type { InstanceTaskHandler, TaskHandlerContext } from '../../../../src/coordination/handlers/handler-types.js'; +import type { QueenTask } from '../../../../src/coordination/queen-coordinator.js'; +import type { Result } from '../../../../src/shared/types/index.js'; + +const { spawn, spawnSync } = vi.hoisted(() => ({ spawn: vi.fn(), spawnSync: vi.fn() })); +vi.mock('node:child_process', () => ({ spawn, spawnSync })); +vi.mock('child_process', () => ({ spawn, spawnSync })); + +const validReport = JSON.stringify({ success: true, numTotalTests: 1, numPassedTests: 1, + numFailedTests: 0, testResults: [{ status: 'passed', assertionResults: [{ status: 'passed' }] }] }); + +describe('Vitest report process boundaries', () => { + let fixture: string; + let file: string; + let handler: InstanceTaskHandler; + let reportPaths: string[]; + let run: (timeout?: number) => Promise>; + + beforeEach(() => { + vi.clearAllMocks(); + fixture = mkdtempSync(join(tmpdir(), 'aqe-report-boundary-')); + file = join(fixture, 'sample.test.js'); + writeFileSync(file, ''); + reportPaths = []; + const executor = new TestExecutorService({ memory: {} as never }); + run = (timeout = 1000) => (executor as unknown as { + spawnTestRunner(files: string[], framework: string, timeout: number): Promise>; + }).spawnTestRunner([file], 'vitest', timeout); + registerTestExecutionHandlers({ registerHandler(name, value) { + if (name === 'execute-tests') handler = value; + } } as TaskHandlerContext); + }); + + afterEach(() => { + vi.useRealTimers(); + for (const path of reportPaths) rmSync(dirname(path), { recursive: true, force: true }); + rmSync(fixture, { recursive: true, force: true }); + }); + + function reportPath(args: string[]): string { + const argument = args.find(arg => arg.startsWith('--outputFile=')); + expect(argument).toBeDefined(); + const path = argument!.slice('--outputFile='.length); + reportPaths.push(path); + return path; + } + + function child() { + return Object.assign(new EventEmitter(), { + stdout: new EventEmitter(), stderr: new EventEmitter(), kill: vi.fn(() => true), + }); + } + + it.each([ + { name: 'missing report despite passing stdout JSON', body: undefined, code: 0 }, + { name: 'malformed report despite passing stdout JSON', body: '{', code: 0 }, + { name: 'nonzero exit despite passing report', body: validReport, code: 1 }, + { name: 'signal termination despite passing report', body: validReport, code: null }, + ])('rejects $name on both paths and cleans the reports', async ({ body, code }) => { + spawn.mockImplementation((_command, args) => { + const path = reportPath(args); + const proc = child(); + queueMicrotask(() => { + if (body !== undefined) writeFileSync(path, body); + proc.stdout.emit('data', Buffer.from(validReport)); + proc.emit('close', code); + }); + return proc; + }); + spawnSync.mockImplementation((_command, args) => { + const path = reportPath(args); + if (body !== undefined) writeFileSync(path, body); + return { status: code, signal: code === null ? 'SIGTERM' : null, stdout: validReport, stderr: '' }; + }); + + expect((await run()).success).toBe(false); + const taskResult = await handler({ payload: { testFiles: [file] } } as QueenTask); + expect(taskResult.success).toBe(false); + if (!taskResult.success && code === null) expect(taskResult.error.message).toContain('SIGTERM'); + expect(spawnSync).toHaveBeenCalledTimes(1); + expect(new Set(reportPaths).size).toBe(2); + expect(reportPaths.every(path => !existsSync(dirname(path)))).toBe(true); + }); + + it('retains a timeout and cleans only after the asynchronous child closes', async () => { + vi.useFakeTimers(); + const proc = child(); + spawn.mockImplementation((_command, args) => { reportPath(args); return proc; }); + const resultPromise = run(20); + await vi.advanceTimersByTimeAsync(20); + const result = await resultPromise; + expect(result.success).toBe(false); + if (!result.success) expect(result.error.message).toContain('timed out'); + expect(proc.kill).toHaveBeenCalledWith('SIGTERM'); + expect(existsSync(dirname(reportPaths[0]))).toBe(true); + proc.emit('close', null); + expect(existsSync(dirname(reportPaths[0]))).toBe(false); + }); + + it.each(['throw', 'error-event'])('cleans the report after a spawn %s', async mode => { + spawn.mockImplementation((_command, args) => { + reportPath(args); + if (mode === 'throw') throw new Error('spawn fixture failure'); + const proc = child(); + queueMicrotask(() => proc.emit('error', new Error('spawn fixture failure'))); + return proc; + }); + expect((await run()).success).toBe(false); + expect(existsSync(dirname(reportPaths[0]))).toBe(false); + }); + + it('preserves synchronous timeout diagnostics without retrying Jest', async () => { + spawnSync.mockImplementation((_command, args) => { + writeFileSync(reportPath(args), '{'); + return { status: null, signal: 'SIGTERM', error: new Error('ETIMEDOUT fixture'), stdout: '', stderr: '' }; + }); + const result = await handler({ payload: { testFiles: [file] } } as QueenTask); + expect(result.success).toBe(false); + if (!result.success) expect(result.error.message).toContain('ETIMEDOUT fixture'); + expect(spawnSync).toHaveBeenCalledTimes(1); + expect(existsSync(dirname(reportPaths[0]))).toBe(false); + }); + + it('retains Jest fallback only for an unsuccessful Vitest run without a report', async () => { + spawnSync.mockImplementation((_command, args) => { + if (args[0] === 'vitest') { + reportPath(args); + return { status: 1, signal: null, stdout: '', stderr: 'Vitest unavailable' }; + } + expect(args).toEqual(['jest', file, '--json']); + return { status: 0, signal: null, stdout: validReport, stderr: '' }; + }); + expect((await handler({ payload: { testFiles: [file] } } as QueenTask)).success).toBe(true); + expect(spawnSync).toHaveBeenCalledTimes(2); + expect(existsSync(dirname(reportPaths[0]))).toBe(false); + }); +}); diff --git a/tests/unit/domains/test-execution/vitest-missing-report-consumers.test.ts b/tests/unit/domains/test-execution/vitest-missing-report-consumers.test.ts new file mode 100644 index 000000000..3f7aa544c --- /dev/null +++ b/tests/unit/domains/test-execution/vitest-missing-report-consumers.test.ts @@ -0,0 +1,67 @@ +import { EventEmitter } from 'node:events'; +import { existsSync } from 'node:fs'; +import { dirname } from 'node:path'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { createVitestJsonReport } from '../../../../src/shared/vitest-json-report.js'; +import { RetryHandlerService } from '../../../../src/domains/test-execution/services/retry-handler.js'; +import { FlakyDetectorService } from '../../../../src/domains/test-execution/services/flaky-detector.js'; +import { VitestPhaseExecutor } from '../../../../src/test-scheduling/executors/vitest-executor.js'; + +const { spawn } = vi.hoisted(() => ({ spawn: vi.fn() })); +vi.mock('node:child_process', () => ({ spawn })); +vi.mock('child_process', () => ({ spawn })); + +function successfulChild() { + const child = Object.assign(new EventEmitter(), { + stdout: new EventEmitter(), stderr: new EventEmitter(), kill: vi.fn(() => true), + }); + queueMicrotask(() => child.emit('close', 0)); + return child; +} + +describe('missing owned Vitest report cannot become a successful result', () => { + beforeEach(() => vi.clearAllMocks()); + + it('rejects a retry run even when its process exits zero', async () => { + spawn.mockImplementation(() => successfulChild()); + const report = createVitestJsonReport(); + const service = new RetryHandlerService({} as never); + const run = (service as unknown as { + spawnTestProcess(command: string, args: string[], cwd: string, report: typeof report): Promise; + }).spawnTestProcess('npx', ['vitest', 'run', ...report.args], process.cwd(), report); + await expect(run).rejects.toThrow(/report/i); + expect(existsSync(dirname(report.path))).toBe(false); + }); + + it('rejects a flaky-detection run instead of fabricating a passing test', async () => { + let reportPath = ''; + spawn.mockImplementation((_command, args: string[]) => { + reportPath = args.find(arg => arg.startsWith('--outputFile='))!.slice('--outputFile='.length); + return successfulChild(); + }); + const service = new FlakyDetectorService({} as never, { + testRunner: 'vitest', testRunnerArgs: ['run', '--reporter=json'], + }); + const run = (service as unknown as { + executeTestFile(file: string, runIndex: number): Promise; + }).executeTestFile('sample.test.ts', 0); + await expect(run).rejects.toThrow(/report/i); + expect(existsSync(dirname(reportPath))).toBe(false); + }); + + it('rejects a scheduling run instead of deriving success from exit code zero', async () => { + const service = new VitestPhaseExecutor(); + let reportPath = ''; + (service as unknown as { + runCommand(command: string, args: string[], timeout: number): Promise; + }).runCommand = vi.fn(async (_command, args) => { + reportPath = args.find(arg => arg.startsWith('--outputFile='))!.slice('--outputFile='.length); + return { stdout: '', stderr: '', exitCode: 0 }; + }); + const run = (service as unknown as { + runVitest(args: string[], timeout: number): Promise; + }).runVitest([], 1000); + await expect(run).rejects.toThrow(/report/i); + expect(existsSync(dirname(reportPath))).toBe(false); + }); +}); diff --git a/tests/unit/shared/vitest-json-report.test.ts b/tests/unit/shared/vitest-json-report.test.ts index cb053c43e..23f06fdfb 100644 --- a/tests/unit/shared/vitest-json-report.test.ts +++ b/tests/unit/shared/vitest-json-report.test.ts @@ -22,20 +22,30 @@ describe('vitest JSON report resolution (Vitest 4 and 5 parity)', () => { } }); - it('falls back to stdout when no report file exists', () => { + it('does not accept passing stdout when no report file exists', () => { const report = createVitestJsonReport(); try { - expect(report.read(REPORT)).toBe(REPORT); + expect(report.read(REPORT)).toBeUndefined(); } finally { report.cleanup(); } }); - it('falls back to stdout when the report file is empty', () => { + it('rejects an empty report even if stdout looks successful', () => { const report = createVitestJsonReport(); try { writeFileSync(report.path, ' \n'); - expect(report.read('stdout-doc')).toBe('stdout-doc'); + expect(() => report.read(REPORT)).toThrow(/malformed/); + } finally { + report.cleanup(); + } + }); + + it('rejects a report without testResults even if stdout looks successful', () => { + const report = createVitestJsonReport(); + try { + writeFileSync(report.path, '{"success":true}'); + expect(() => report.read(REPORT)).toThrow(/malformed/); } finally { report.cleanup(); } @@ -53,8 +63,8 @@ describe('vitest JSON report resolution (Vitest 4 and 5 parity)', () => { expect(() => a.cleanup()).not.toThrow(); }); - it('resolves undefined report paths to stdout', () => { - expect(resolveVitestJsonOutput('doc', undefined)).toBe('doc'); + it('does not use stdout when no report path is requested', () => { + expect(resolveVitestJsonOutput(REPORT, undefined)).toBeUndefined(); }); it('detects vitest json invocations that still need an output file', () => { From d46231bd13336e8071d609c7940e694ff03bb0bf Mon Sep 17 00:00:00 2001 From: Rudy Celekli Date: Tue, 22 Sep 2026 23:04:14 -0400 Subject: [PATCH 2/2] fix(test-execution): honor runner exits in auxiliary verdicts --- .../test-execution/services/flaky-detector.ts | 27 ++++++ .../test-execution/services/retry-handler.ts | 23 +++-- .../executors/vitest-executor.ts | 18 +++- .../vitest-missing-report-consumers.test.ts | 92 ++++++++++++++++++- 4 files changed, 146 insertions(+), 14 deletions(-) diff --git a/src/domains/test-execution/services/flaky-detector.ts b/src/domains/test-execution/services/flaky-detector.ts index 821b07d9f..d406165df 100644 --- a/src/domains/test-execution/services/flaky-detector.ts +++ b/src/domains/test-execution/services/flaky-detector.ts @@ -18,6 +18,7 @@ import { MemoryBackend } from '../../../kernel/interfaces'; import { TEST_EXECUTION_CONSTANTS, RETRY_CONSTANTS } from '../../constants.js'; import { toError } from '../../../shared/error-utils.js'; import { safeJsonParse } from '../../../shared/safe-json.js'; +import { getTestRunnerExecutionError } from '../../../shared/test-runner-verdict.js'; import { createVitestJsonReport, needsVitestJsonReportFile } from '../../../shared/vitest-json-report.js'; import { secureRandom } from '../../../shared/utils/crypto-random.js'; @@ -552,6 +553,32 @@ export class FlakyDetectorService implements IFlakyTestDetector { duration ); + if (report) { + const verdictReport = safeJsonParse(reportText) as { + success?: boolean; + numFailedTestSuites?: number; + numRuntimeErrorTestSuites?: number; + testResults?: Array<{ + status?: string; + message?: string; + assertionResults?: Array<{ status?: string }>; + }>; + }; + const assertions = verdictReport.testResults?.flatMap( + suite => suite.assertionResults ?? [] + ) ?? []; + const executionError = getTestRunnerExecutionError( + 'vitest', file, code, + { + passed: assertions.filter(test => test.status === 'passed').length, + failed: assertions.filter(test => test.status === 'failed').length, + skipped: assertions.filter(test => test.status === 'skipped' || test.status === 'pending').length, + }, + stderr, verdictReport + ); + if (executionError) throw executionError; + } + // If parsing fails but we have an exit code, create a single result for the file if (parsedResults.size === 0) { if (report) throw new Error(`The current Vitest JSON report has no test results for ${file}.`); diff --git a/src/domains/test-execution/services/retry-handler.ts b/src/domains/test-execution/services/retry-handler.ts index 39a8c5071..2a88fd8bd 100644 --- a/src/domains/test-execution/services/retry-handler.ts +++ b/src/domains/test-execution/services/retry-handler.ts @@ -652,18 +652,14 @@ export class RetryHandlerService implements IRetryHandler { stdout: string, stderr: string ): { passed: boolean; error?: string } { - // Exit code 0 typically means all tests passed - if (exitCode === 0) { - return { passed: true }; - } - - // Try to parse JSON output for more detailed error info + // A passing process and a passing report must agree when JSON is available. try { // Vitest JSON output const vitestMatch = stdout.match(/\{[\s\S]*"testResults"[\s\S]*\}/); if (vitestMatch) { const result = safeJsonParse(vitestMatch[0]); - if (result.success === true || result.numFailedTests === 0) { + if (exitCode === 0 && result.success !== false + && result.numFailedTests === 0 && result.numFailedTestSuites === 0) { return { passed: true }; } const failedTest = result.testResults?.[0]?.assertionResults?.find( @@ -671,7 +667,7 @@ export class RetryHandlerService implements IRetryHandler { ); return { passed: false, - error: failedTest?.failureMessages?.join('\n') ?? `Test failed with exit code ${exitCode}`, + error: failedTest?.failureMessages?.join('\n') || stderr || `Test failed with exit code ${exitCode}`, }; } @@ -679,7 +675,8 @@ export class RetryHandlerService implements IRetryHandler { const jestMatch = stdout.match(/\{[\s\S]*"numFailedTests"[\s\S]*\}/); if (jestMatch) { const result = safeJsonParse(jestMatch[0]); - if (result.success === true || result.numFailedTests === 0) { + if (exitCode === 0 && result.success !== false + && result.numFailedTests === 0 && result.numFailedTestSuites === 0) { return { passed: true }; } const failedTest = result.testResults?.[0]?.assertionResults?.find( @@ -687,7 +684,7 @@ export class RetryHandlerService implements IRetryHandler { ); return { passed: false, - error: failedTest?.failureMessages?.join('\n') ?? `Test failed with exit code ${exitCode}`, + error: failedTest?.failureMessages?.join('\n') || stderr || `Test failed with exit code ${exitCode}`, }; } @@ -695,19 +692,21 @@ export class RetryHandlerService implements IRetryHandler { const mochaMatch = stdout.match(/\{[\s\S]*"stats"[\s\S]*"failures"[\s\S]*\}/); if (mochaMatch) { const result = safeJsonParse(mochaMatch[0]); - if (result.stats?.failures === 0) { + if (exitCode === 0 && result.stats?.failures === 0) { return { passed: true }; } const failure = result.failures?.[0]; return { passed: false, - error: failure?.err?.message ?? `Test failed with exit code ${exitCode}`, + error: failure?.err?.message || stderr || `Test failed with exit code ${exitCode}`, }; } } catch { // JSON parsing failed, fall back to simple exit code check } + if (exitCode === 0) return { passed: true }; + // Non-zero exit code means failure const errorOutput = stderr || stdout || `Test failed with exit code ${exitCode}`; return { diff --git a/src/test-scheduling/executors/vitest-executor.ts b/src/test-scheduling/executors/vitest-executor.ts index 53e4f24e7..3544f9ef8 100644 --- a/src/test-scheduling/executors/vitest-executor.ts +++ b/src/test-scheduling/executors/vitest-executor.ts @@ -14,6 +14,7 @@ import type { } from '../interfaces'; import type { FlakyTestTracker } from '../flaky-tracking/flaky-tracker'; import { safeJsonParse } from '../../shared/safe-json.js'; +import { getTestRunnerExecutionError } from '../../shared/test-runner-verdict.js'; import { createVitestJsonReport } from '../../shared/vitest-json-report.js'; // ============================================================================ @@ -182,6 +183,7 @@ export class VitestPhaseExecutor implements PhaseExecutor { const report = createVitestJsonReport(); let document: string; let exitCode: number; + let stderr: string; try { const run = await this.runCommand( this.config.vitestPath || 'npx', @@ -189,6 +191,7 @@ export class VitestPhaseExecutor implements PhaseExecutor { timeoutMs ); exitCode = run.exitCode; + stderr = run.stderr; const currentReport = report.read(run.stdout); if (currentReport === undefined) { throw new Error('The current Vitest JSON report is missing for this phase.'); @@ -199,6 +202,7 @@ export class VitestPhaseExecutor implements PhaseExecutor { } // Parse JSON report from Vitest + let result: VitestJsonResult; try { const jsonStart = document.indexOf('{'); const jsonEnd = document.lastIndexOf('}'); @@ -208,7 +212,7 @@ export class VitestPhaseExecutor implements PhaseExecutor { } const jsonStr = document.slice(jsonStart, jsonEnd + 1); - return safeJsonParse(jsonStr); + result = safeJsonParse(jsonStr); } catch { // If JSON parsing fails, create a basic result from exit code return { @@ -224,6 +228,18 @@ export class VitestPhaseExecutor implements PhaseExecutor { testResults: [], }; } + + const executionError = getTestRunnerExecutionError( + 'vitest', 'phase', exitCode, + { + passed: result.numPassedTests, + failed: result.numFailedTests, + skipped: result.numPendingTests, + }, + stderr, result + ); + if (executionError) throw executionError; + return result; } private runCommand( diff --git a/tests/unit/domains/test-execution/vitest-missing-report-consumers.test.ts b/tests/unit/domains/test-execution/vitest-missing-report-consumers.test.ts index 3f7aa544c..0356b085c 100644 --- a/tests/unit/domains/test-execution/vitest-missing-report-consumers.test.ts +++ b/tests/unit/domains/test-execution/vitest-missing-report-consumers.test.ts @@ -1,5 +1,5 @@ import { EventEmitter } from 'node:events'; -import { existsSync } from 'node:fs'; +import { existsSync, writeFileSync } from 'node:fs'; import { dirname } from 'node:path'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { createVitestJsonReport } from '../../../../src/shared/vitest-json-report.js'; @@ -19,6 +19,36 @@ function successfulChild() { return child; } +function testReport(failed = false) { + return { + success: !failed, + numTotalTestSuites: 1, + numPassedTestSuites: failed ? 0 : 1, + numFailedTestSuites: failed ? 1 : 0, + numTotalTests: 1, + numPassedTests: failed ? 0 : 1, + numFailedTests: failed ? 1 : 0, + numPendingTests: 0, + startTime: Date.now(), + testResults: [{ + name: 'sample.test.ts', status: failed ? 'failed' : 'passed', + assertionResults: [{ + ancestorTitles: [], fullName: 'sample', title: 'sample', + status: failed ? 'failed' : 'passed', duration: 1, + failureMessages: failed ? ['assertion failed'] : [], + }], + }], + }; +} + +function reportedChild(exitCode: number) { + const child = Object.assign(new EventEmitter(), { + stdout: new EventEmitter(), stderr: new EventEmitter(), kill: vi.fn(() => true), + }); + queueMicrotask(() => child.emit('close', exitCode)); + return child; +} + describe('missing owned Vitest report cannot become a successful result', () => { beforeEach(() => vi.clearAllMocks()); @@ -65,3 +95,63 @@ describe('missing owned Vitest report cannot become a successful result', () => expect(existsSync(dirname(reportPath))).toBe(false); }); }); + +describe('runner exit status remains authoritative after a valid report', () => { + beforeEach(() => vi.clearAllMocks()); + + it('does not classify a retry as passed when the report says passed but the runner exits one', () => { + const service = new RetryHandlerService({} as never); + const parse = (service as unknown as { + parseTestResult(code: number, report: string, stderr: string): { passed: boolean; error?: string }; + }).parseTestResult.bind(service); + expect(parse(1, JSON.stringify(testReport()), 'post-run crash').passed).toBe(false); + expect(parse(0, JSON.stringify(testReport()), '').passed).toBe(true); + expect(parse(1, JSON.stringify(testReport(true)), '').passed).toBe(false); + expect(parse(0, JSON.stringify(testReport(true)), '').passed).toBe(false); + expect(parse(1, JSON.stringify({ success: true, numFailedTests: 0 }), 'jest crash').passed).toBe(false); + expect(parse(1, JSON.stringify({ stats: { failures: 0 }, failures: [] }), 'mocha crash').passed).toBe(false); + }); + + it('rejects a crashed flaky-detection run but accepts an ordinary failed assertion', async () => { + for (const failed of [false, true]) { + spawn.mockImplementation((_command, args: string[]) => { + const reportPath = args.find(arg => arg.startsWith('--outputFile='))!.slice('--outputFile='.length); + writeFileSync(reportPath, JSON.stringify(testReport(failed))); + return reportedChild(1); + }); + const service = new FlakyDetectorService({} as never, { + testRunner: 'vitest', testRunnerArgs: ['run', '--reporter=json'], + }); + const run = (service as unknown as { + executeTestFile(file: string, runIndex: number): Promise>>; + }).executeTestFile('sample.test.ts', 0); + if (failed) { + const results = await run; + expect([...results.values()][0][0].passed).toBe(false); + } else { + await expect(run).rejects.toThrow(/exit code 1/i); + } + } + }); + + it('rejects a crashed phase but retains assertion failures for threshold evaluation', async () => { + for (const failed of [false, true]) { + const service = new VitestPhaseExecutor(); + (service as unknown as { + runCommand(command: string, args: string[], timeout: number): Promise; + }).runCommand = vi.fn(async (_command, args) => { + const reportPath = args.find(arg => arg.startsWith('--outputFile='))!.slice('--outputFile='.length); + writeFileSync(reportPath, JSON.stringify(testReport(failed))); + return { stdout: '', stderr: 'post-run crash', exitCode: 1 }; + }); + const run = (service as unknown as { + runVitest(args: string[], timeout: number): Promise<{ numFailedTests: number }>; + }).runVitest([], 1000); + if (failed) { + await expect(run).resolves.toMatchObject({ numFailedTests: 1 }); + } else { + await expect(run).rejects.toThrow(/exit code 1/i); + } + } + }); +});