Skip to content

fix(test-execution): reject incomplete Vitest execution evidence - #712

Merged
proffesor-for-testing merged 2 commits into
proffesor-for-testing:mainfrom
rudycelekli:fix/vitest-report-files
Sep 27, 2026
Merged

proffesor-for-testing merged 2 commits into
proffesor-for-testing:mainfrom
rudycelekli:fix/vitest-report-files

Conversation

@rudycelekli

@rudycelekli rudycelekli commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Vitest 4/5 writes its JSON report to a per-run --outputFile, but the shared reader could substitute stdout if the owned file was missing or empty. A stale or unrelated passing JSON string could then be accepted as this run's evidence. A second false-pass path remained when the owned report existed: retry, flaky detection, and phase scheduling could accept passing assertions even though the runner exited nonzero after writing the report.

Require the owned report for all five Vitest consumers. Retry now requires the process exit and parsed report to agree, including its Jest and Mocha parsing paths. Flaky detection and phase scheduling use the shared runner-verdict rules to reject execution failures without failed assertions while retaining ordinary assertion failures for analysis and threshold evaluation. The async test executor also keeps its report directory until a timed-out child closes and cleans up synchronous spawn failures.

Verification

  • On current main, three new regression cases failed: retry reported passed: true, flaky detection recorded a passing test, and phase scheduling returned success: true for a passing report followed by exit code 1.
  • After this change, 203/203 tests pass across 12 focused unit and integration files, including real Vitest 5 healthy/failure/skip/todo controls, report-file failures, all affected consumers, timeout/spawn cleanup, and the Jest fallback. Tests used fresh AQE_PROJECT_ROOT and TMPDIR before imports.
  • Report and runner controls also passed 32/32 tests on Vitest 4.1.4 before the exit-verdict addition; the new exit-verdict regression tests ran on Vitest 5.0.1.
  • npm run typecheck, npm run build, changed-source ESLint, and git diff --check pass.

Failure modes

  • A missing, empty, or malformed owned report could be replaced by passing stdout JSON. Covered by tests/unit/shared/vitest-json-report.test.ts and tests/unit/domains/test-execution/test-executor-vitest-report.test.ts.
  • Retry, flaky detection, or scheduling could fabricate a pass from exit code 0 without report evidence. Covered by tests/unit/domains/test-execution/vitest-missing-report-consumers.test.ts.
  • Retry, flaky detection, or scheduling could accept a passing report despite nonzero runner exit; a retry could also accept a failed report after a zero exit. Covered by the same consumer tests, with an ordinary assertion-failure control.
  • A timeout could remove the report directory before child close, or a synchronous spawn throw could leave it behind. Covered by tests/unit/domains/test-execution/test-executor-vitest-report.test.ts.
  • A nonzero or signalled main test-runner exit could be masked by a passing report; the existing Jest fallback could be lost. Covered by the process-boundary tests and real runner controls in tests/integration/test-runner-outcomes.test.ts.
  • Terminating an entire child process tree after cancellation remains tracked in fix(coordination): make cancellation abort in-flight work #707; this change does not claim quiescence.

Required check (issue #401)

  • Every failure mode mentioned in this PR description has either (a) a test that exercises it, or (b) a linked tracking issue. "Unlikely" is not an acceptable substitute. If you wrote "I don't think this can happen but...", that sentence is a failure mode and needs a test or an issue link.

Optional context

@rudycelekli
rudycelekli force-pushed the fix/vitest-report-files branch from 0726bc5 to 8d2607b Compare September 23, 2026 02:28
@rudycelekli rudycelekli changed the title fix(test-execution): read isolated Vitest report files fix(test-execution): reject missing Vitest report evidence Sep 23, 2026
@rudycelekli rudycelekli changed the title fix(test-execution): reject missing Vitest report evidence fix(test-execution): reject incomplete Vitest execution evidence Sep 23, 2026

@proffesor-for-testing proffesor-for-testing left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rudycelekli — this closes a real false-pass hole. Checked against real Vitest 4.1.4 on every case (pass, fail, skip/todo, empty file, no files, module crash, --passWithNoTests): each writes the owned report, so legitimate runs aren't rejected, and the Jest/Mocha paths are untouched. 298/298 focused tests pass merged with current main; the Performance Gates red is the known RVF timing flake fixed in #718. Merging.

Two small follow-ups if you're up for them:

  1. In test-executor.ts (~L577/581) a missing/malformed report becomes a plain Error, so the CLI path counts it as N failed tests while the MCP handler reports TestRunnerExecutionError. Using TestRunnerExecutionError there would give CLI/MCP parity (same for report.read() throwing in test-execution-handlers.ts:228).
  2. Todo-only files/phases count as "zero tests" in the phase executor and flaky detector (only numPendingTests/pending are counted, not numTodoTests).

@proffesor-for-testing
proffesor-for-testing merged commit fa50f61 into proffesor-for-testing:main Sep 27, 2026
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants