fix: make the Jest wiring guard read the runner's test list reliably - #3576
Conversation
The guard asserted on the stdout of a nested `jest --listTests` child. A clipped stdout became a short set, and the guard then reported healthy suites as undiscovered, which reads as a wiring break rather than a bad read. Read the list through `--json --outputFile` instead. Jest then writes it with one synchronous writeFileSync rather than console.log, so the payload cannot be cut short by a stdout pipe the process exits before draining, and the JSON array fails to parse if it is ever partial, so an incomplete listing is reported as incomplete rather than as a list of missing files. The child also inherited any JEST_* discovery override the caller had set. A run with JEST_IGNORE_PATTERN pointing at scripts/automation made the child report 230 of 282 files and the guard then named 50 healthy suites; the listing now always describes the default root configuration. Failures to obtain a listing now throw with that diagnosis, including the child's exit status and stderr, instead of a bare "expected 0".
|
Warning Review limit reachedNext included review available in 32 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: lightspeedwp/.github/.coderabbit.yml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Template RoutingBranch Type: This PR was automatically routed based on the branch naming strategy. |
📋 Changelog Quality Validation
Status✅ Validation PASSED - No new failures introduced by this PR. No action required. |
PR Summary by QodoMake Jest wiring test listings reliable
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Linked issues
Fixes #3575
Related: #3552 and #3496 (the guard this hardens), #3572 (the separate timing-boundary flakes), #3487 (made Jest a required gate, which is why a false positive here now blocks arbitrary PRs).
Context
scripts/validation/__tests__/test-wiring.test.jsondevelopsincef58448f45d. Both nested listings it spawns are affected: the rootjest --listTestsand the pr-agentjest-vm.js --listTestspass-through.Reproduction
JEST_IGNORE_PATTERN='<rootDir>/scripts/automation/' npx jest --config .jest.config.cjs scripts/validation/__tests__/test-wiring.test.js.scripts/automation/as undiscovered.Root Cause
console.log; its--outputFilepath uses a single synchronousfs.writeFileSync. A stdout pipe is asynchronous in Node, so a payload can be cut short if the process exits before the pipe drains, and newline-delimited paths carry no completeness marker. A clipped list is therefore indistinguishable from a genuinely shorter one, and the only thing the guard could do with it was blame files.JEST_*discovery overrides the root config reads (JEST_IGNORE_PATTERN,JEST_TEST_MATCH_1..6). The guard was asserting "the default root runner discovers every file" using a runner configured by whoever invoked the test. This is a deterministic false positive, and it is the same wrong diagnosis as the reported flake.expect(result.status).toBe(0)was the only check on the child. A child that failed, was killed, or wrote nothing produced a confusing downstream diff rather than "the listing did not work".Fix Summary
--listTests --json --outputFile <tmpfile>:--outputFilemakes Jest write with one synchronouswriteFileSyncinstead ofconsole.log, so the payload cannot be clipped by a pipe the process exits before draining.--jsonmakes the payload an array, so a partial file fails to parse. Incompleteness becomes a detectable state instead of a silently short list — this replaces the size assertion the issue suggested, which cannot distinguish a clipped listing from a genuine break and is, once the listing is known complete, implied by the existing check.expect(result.status).toBe(0).finallyblock; the counter and pid keep concurrent workers from colliding.Verification
Evidence:
JEST_IGNORE_PATTERNrun that deterministically failed before now passes; same for aJEST_TEST_MATCH_1scoped run.coverage/fault-injection.test.js(a directory the root config ignores) makes the guard fail with+ "coverage/fault-injection.test.js". The assertion was not weakened or removed.the nested Jest listing exited with status 1, so its result is unknown, naming no suites.SUMMARY pass=20 fail=0, every run14 todo, 5551 passed, 5565 total.SyntaxErroronJSON.parse, which the helper converts into "the listing is incomplete rather than short".eslint0 problems andprettier --checkclean on the changed file (it was clean at HEAD, so the change introduces no new lint or format debt).semgrep scan --config p/security-audit --config p/secrets --config p/php: 0 findings.markdownlint-cli2 CHANGELOG.md: 0 issues.actionlint: not applicable, no workflow files changed.Risk & Rollback
.jest-skip/exclusions, must be discovered. Only how the listing is obtained and how failures are reported changed.compareDiscoveredwas inlined and the suggested size assertion dropped as provably redundant; the diff is the smallest change that makes the read trustworthy.Changelog
Added under
## [Unreleased]→### FixedinCHANGELOG.md: