fix(ci): execute coherence tests and preserve runner failures - #704
Conversation
proffesor-for-testing
left a comment
There was a problem hiding this comment.
Reviewed with a full reproduction: on main, npm run test:safe fails immediately on the unknown --maxForks flag and continue-on-error hid it, so the coherence job never executed a test; junit is not a valid istanbul coverage reporter, so coverage runs died after passing. This PR fixes both, the wrapper now propagates real exit codes, and all checks on the head are green with 224 coherence tests actually running. Codex adversarial pass: no findings. One known trade-off: the wrapper no longer masks the historical post-test vitest hang; if that returns we fix the handle leak rather than reinstating the mask.
fe3f4c5
into
proffesor-for-testing:main
|
Thank you, @rudycelekli! This one caught a genuine false green: the coherence job had never actually run a test since the |
Summary
CI could report success without completing its checks: the coherence command used a Vitest 4 option that prevents test collection, while the shared Vitest wrapper converted nonzero exits into success after seeing a passing test summary. The configured
junitcoverage reporter also failed after the tests passed.Use the supported coherence worker flag and propagate test failures. Preserve every shared-runner exit, including timeouts, and use the same strict wrapper for MCP integration. Keep the separate JUnit test reporter and generate valid JSON, JSON summary and HTML coverage reports. Guard optional PR comments for fork tokens while retaining test jobs and report artifacts.
Verification
Final published head
8152bc57: all 27 checks pass. Full coverage job: 23,601 tests passed / 62 skipped; 975 files passed / six skipped. Coverage artifacts upload successfully; actual line coverage is 65.42%, below the existing advisory 80% comparison. No reporter errors or success normalization occur.Exact published head
8152bc57: Coherence Verification passes 224 tests / nine files; strict MCP integration passes 59 tests / nine files, with no exit-status normalization.Exact coherence workflow command: original script exits 1 with
Unknown option --maxForksand collects no tests; corrected script passes 224 tests across nine files, exit 0. The existing memory/GC settings are retained.Nine shell-wrapper regressions: unpatched 7 fail / 2 pass; patched 9 pass. They execute the actual wrapper with a controlled runner and cover successful argument forwarding, nonzero exits after a passing summary, failure before a summary, and a real timeout after all tests report passing.
Wrapper regressions plus existing fork-comment guard tests: 21 pass. The 12 reporting cases keep fork reporting jobs available while skipping unauthorized comments, retain same-repository comments, and prevent comments on non-PR events.
Actual Vitest 4.1.4 through the corrected wrapper: 18 tests pass, exit 0, with valid
coverage-final.json,coverage-summary.json, HTML and test JUnit artifacts in a fresh temporary fixture. Coverage was restricted to the exercised formatter module for this report-generation check; this is not a full coverage run.Negative report-generation control: the same 18 tests pass and JUnit reports zero failures, but an intentionally invalid coverage reporter still produces exit 1 through the wrapper.
Shell tests run with real Bash/GNU timeout where available. A stock-macOS PATH control explicitly skips the nine shell cases when GNU timeout is absent; evaluating the same prerequisite check for Linux confirms missing tools fail instead of skipping.
Actionlint passes for the coherence and MCP workflows with optional ShellCheck/Pyflakes integrations disabled. Shell syntax and whitespace checks pass. ESLint does not cover these paths under the existing configuration: tests are ignored and
vitest.config.tsis outside its configured TypeScript project.The full repository coverage job has now completed on this exact head, as recorded above.
Failure modes
No production API, runner assertion, timing limit, or test exclusion is changed by this follow-up.
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.
Affects published API or CLI surface: no; changes are development scripts, CI workflow behavior and test configuration.
Touches the init flow /
npm-publish.yml/tests/fixtures/init-corpus/: no.