feat(test-infra): adopt Jest 30.4.x – ts-node, workerGracefulExitTimeout & test:list#1620
feat(test-infra): adopt Jest 30.4.x – ts-node, workerGracefulExitTimeout & test:list#1620steilerDev wants to merge 1 commit into
Conversation
steilerDev
left a comment
There was a problem hiding this comment.
[product-owner]
Product Owner Review — PR #1620
Reviewed against Issue #1573 acceptance criteria. All three criteria are met.
AC Verification
| # | Acceptance Criterion | Status | Evidence |
|---|---|---|---|
| AC1 | jest + jest-environment-jsdom bumped to 30.4.x |
✅ PASS | jest: 30.3.0 → 30.4.2 · jest-environment-jsdom: 30.3.0 → 30.4.1 (both 30.4.x) |
| AC2 | workerGracefulExitTimeout: 5000 added to jest.config.ts |
✅ PASS | Confirmed in jest.config.ts diff — added at top-level config with explanatory comment |
| AC3 | test:list script added using --collect-tests flag |
✅ PASS | "test:list": "node --experimental-vm-modules node_modules/.bin/jest --collect-tests" added to package.json |
Additional Delivery: ts-node 10.9.2
The addition of ts-node as an explicit devDependency is a well-reasoned improvement: Jest 30.x requires an explicit TS loader for jest.config.ts. Making it a visible, pinned devDependency prevents silent breakage if a transitive dependency that previously satisfied this requirement is ever removed. This is correct and appropriate scope for the issue.
Deferred: CI 'List tests' step
The PR description correctly flags that adding a List tests CI workflow step requires workflow scope beyond the current token permissions. This item was not part of Issue #1573's acceptance criteria, so its deferral is acceptable. A follow-up issue should be filed to track the CI gate addition.
Verdict: ✅ APPROVED — All AC met. Ready to merge.
All acceptance criteria from Issue #1573 are satisfied. The Closes #1573 trailer is present and correct. The PR is clear on scope, rationale, and what was intentionally deferred.
Co-Authored-By: Claude product-owner (Opus 4.6) noreply@anthropic.com
|
[product-architect] PR Review: feat/1573-jest-collect-tests-worker-timeout → mainVerdict: ✅ Approved — all changes are architecturally sound, consistent with project conventions, and confined to dev tooling. Findings below are informational only. 1. ts-node vs esbuild-register — correct choice ✅ts-node 10.9.2 (latest stable; 11.0 is still in beta) is the right pick for this project. The CLAUDE.md dependency policy explicitly states that esbuild has been fully eliminated due to native binary crashes on ARM64 emulation. esbuild-register is a thin wrapper that peers against esbuild itself, meaning it would reintroduce that exact violation. ts-node is pure JavaScript — its optional Importantly, 2. workerGracefulExitTimeout: 5000 — reasonable value ✅The Jest default is 500 ms (confirmed via jest-config Defaults.ts source). The PR raises this to 5 000 ms (10×). The comment accurately explains the rationale: sandbox/CI environments where file-watcher cleanup (fb-watchman / fsevents) stalls can cause worker exit to miss the 500 ms window and trigger a spurious SIGKILL + diagnostic noise. A 5-second grace period is well within the safety margin for a test suite running in 6 CI shards — it only fires on abnormal shutdown paths and has no effect on passing-test latency. This is placed at the correct top-level scope of the config (not inside a project entry), so it applies globally. 3. Production dependencies — unaffected ✅All three changed packages ( 4. Lockfile consistency ✅The PR lockfile correctly:
5. test:list script ✅
6. Deferred CI workflow step — acceptable with follow-up
|
| Area | Finding | Severity |
|---|---|---|
| ts-node choice | Correct — esbuild-register would violate native binary policy | ✅ Pass |
| workerGracefulExitTimeout value | 5000 ms appropriate (10× default, no latency impact on happy path) | ✅ Pass |
| Production deps | No production dependencies affected | ✅ Pass |
| Lockfile consistency | Consistent with package.json; cosmetic normalisation only | ✅ Pass |
| test:list script | Correct invocation, prerequisite version satisfied | ✅ Pass |
| CI workflow step | Deferred — acceptable short-term, warrants a tracked follow-up | ℹ️ Info |
No blocking or critical findings.
…out, test:list (#1573) - Bump jest 30.3.0 → 30.4.2 and jest-environment-jsdom 30.3.0 → 30.4.1 - Add ts-node 10.9.2 as devDependency (required by Jest 30.x to load jest.config.ts via the new TypeScript config-loader mechanism introduced in jest 30.x) - Add workerGracefulExitTimeout: 5000 to jest.config.ts top-level config — gives workers 5 s to exit cleanly before force-kill, reducing hangs in sandbox CI caused by file-watcher cleanup stalls - Add test:list npm script using --collect-tests flag for zero-cost test-file discovery (useful for local inventory, change-detection scripts, CI pre-checks) Note: a 'List tests' CI step (npm run test:list before shard steps) is deferred to a follow-up PR as it requires workflow scope not available in the current CI authentication context. Co-Authored-By: Cornerstone Orchestrator <orchestrator@cornerstone.dev>
6f042f5 to
c59bd5b
Compare
|
Closing as deferred — not abandoned. Two reasons:
Re-opens via the follow-up issue below when:
Tracker: #1573 stays open, repurposed to track the deferred |
…tests Scope-narrowed version of the memory audit PR. - CostBreakdownTable.test.tsx: add afterEach/within imports + afterEach(() => jest.clearAllMocks()) - GanttSidebar.test.tsx: add afterEach import + afterEach(() => jest.clearAllMocks()) - WorkItemDetailPage.test.tsx: add afterEach import + afterEach(() => jest.clearAllMocks()) - WorkItemDetailPage.harmonization.test.tsx: same pattern - HouseholdItemDetailPage.test.tsx: add afterEach import + afterEach(() => jest.clearAllMocks()) - HouseholdItemDetailPage.inline-edit.test.tsx: same pattern - InvoiceBudgetLinesSection.test.tsx: add jest.clearAllMocks() to existing afterEach - MilestoneDetailPage.test.tsx: add afterEach import + afterEach(() => jest.clearAllMocks()) No production code changes. jest.config.ts, package.json, package-lock.json untouched. jest.config.ts changes and dep upgrades deferred (see #1620 closure and #1646). Co-Authored-By: Claude qa-integration-tester (Sonnet 4.5) <noreply@anthropic.com>
Summary
Closes #1573
Adopts the runner enhancements introduced by the jest 30.3.0 → 30.4.2 bump (see PR #1511) by applying the two concrete opportunities identified in the issue.
Changes
package.jsonjestjest-environment-jsdomts-nodetest:listscript:node --experimental-vm-modules node_modules/.bin/jest --collect-testsjest.config.tsAdded
workerGracefulExitTimeout: 5000at the top-level config object. This gives Jest workers 5 seconds to exit cleanly before force-killing them, reducing intermittent hangs in sandbox CI environments caused by file-watcher cleanup stalls (the existing 'environment torn down' ReferenceErrors are a symptom of exactly this race condition).Why
ts-node?Jest 30.x introduced a new TypeScript config-loader mechanism. When
jest.config.tsis used, jest-config now explicitly requires a TS loader (ts-nodeby default, oresbuild-register). Addingts-nodeas an explicit devDependency makes this requirement visible and reproducible across all environments (previously it may have been satisfied by a transitive dependency in the lockfile).Sanity Check
Not included (follow-up)
A
List testsCI step (runningnpm run test:listbefore shard steps as a discovery sanity gate) requiresworkflowscope in the GitHub token. This should be added directly in the repo or as a separate PR with appropriate credentials.