chore(test-infra): add afterEach(jest.clearAllMocks) hygiene to unit tests

[steilerDev]

Summary

Scope-narrowed from the original memory-audit PR. This PR contains only test-file changes — adding afterEach(jest.clearAllMocks) hygiene to 8 unit test files to prevent mock state accumulation across tests within a worker lifetime.

Changes

• CostBreakdownTable.test.tsx: add afterEach/within imports + afterEach(() => jest.clearAllMocks())
• GanttSidebar.test.tsx: add afterEach import + afterEach(() => jest.clearAllMocks())
• WorkItemDetailPage.test.tsx: same pattern
• WorkItemDetailPage.harmonization.test.tsx: same pattern
• HouseholdItemDetailPage.test.tsx: same pattern
• HouseholdItemDetailPage.inline-edit.test.tsx: same pattern
• InvoiceBudgetLinesSection.test.tsx: add jest.clearAllMocks() to existing afterEach alongside jest.restoreAllMocks()
• MilestoneDetailPage.test.tsx: same pattern

What was deferred

The jest.config.ts change (adding clearMocks: true globally) and dependency upgrades originally in this PR have been deferred:

• feat(test-infra): adopt Jest 30.4.x – ts-node, workerGracefulExitTimeout & test:list #1620 (jest.config.ts + dep upgrades) was closed — it triggered a root/client Konva type collision (miterLimit / RefObject<Stage>) on lockfile regeneration.
• chore(deps): align root and client Konva versions to eliminate type-collision #1646 tracks the Konva collision for future resolution.

No jest.config.ts, package.json, or package-lock.json changes in this PR. Only explicit per-file afterEach(jest.clearAllMocks) additions.

[steilerDev]

[product-architect]

PR Review — chore(test-infra): audit Jest unit tests for memory growth (#1568)

Verdict: ✅ APPROVED — No architectural violations. All findings are informational or low severity.

---

Scope Verification

This PR touches zero production files. All changes are confined to:

• jest.config.ts — root-level Jest configuration
• 8 *.test.tsx files — client unit tests

No ADR implications, no schema changes, no API contract changes. ✅

---

Finding 1 — clearMocks: true placement and scope (Informational ✅)

Location: jest.config.ts, top-level config object

Assessment: Correct and idiomatic. Placing clearMocks: true at the top-level config object means it propagates to all three projects (server, client, shared), which is the intended behaviour. It clears .mock.calls, .mock.instances, and .mock.results after every test — it does not restore spy implementations (restoreMocks would do that). This is the right choice here.

Interaction with setupTests.ts spies: Both server/src/test/setupTests.ts (jest.spyOn(console, 'warn') + jest.spyOn(console, 'log')) and client/src/test/setupTests.ts (jest.spyOn(console, 'info')) use mockImplementation() to suppress noise. clearMocks: true will reset the call counts after each test but will preserve the suppression implementation — correct behaviour, no suppression is lost between tests.

---

Finding 2 — Redundancy of explicit afterEach calls alongside clearMocks: true (Informational ✅)

Location: All 8 *.test.tsx files

Assessment: With clearMocks: true in the global config, the explicit afterEach(() => jest.clearAllMocks()) calls in each file are logically redundant — Jest will already clear mocks automatically. However, they are:

1. Not harmful — calling clearAllMocks() twice per test cycle is a no-op on the second call
2. Defensively useful — they make the intent explicit at the file level and guard against someone removing clearMocks: true from the global config in the future
3. Consistent with existing art — the codebase already has files (InvoiceBudgetLinesSection.test.tsx, etc.) with explicit afterEach teardown

The only true value-add would be if clearMocks: true were ever removed from the global config, in which case these files stay correct. Acceptable pattern.

---

Finding 3 — afterEach placement inside top-level describe in inline-edit file (Low / Informational ✅)

Location: HouseholdItemDetailPage.inline-edit.test.tsx, line ~393

Assessment: The afterEach is placed inside the top-level describe block, positioned between the renderPage() helper closure and the first nested describe. This is semantically correct — an afterEach inside a describe applies to all tests within that describe and its children.

Minor style nit: it appears between a comment separator and the first describe block rather than directly after the beforeEach, which is slightly unconventional ordering. This is purely cosmetic and does not affect behaviour.

---

Finding 4 — clearAllMocks ordering relative to restoreAllMocks in InvoiceBudgetLinesSection (Low / Informational ✅)

Location: InvoiceBudgetLinesSection.test.tsx, afterEach block

Assessment: The clearAllMocks() call now runs before restoreAllMocks(). This is the correct ordering — restoreAllMocks() is a strict superset of clearAllMocks() (it clears call state AND restores original implementations), so calling clearAllMocks() first is a redundant but safe no-op before restoreAllMocks() does its full work. No issue.

---

Architectural Compliance

Check	Result
clearMocks at correct config level (top-level, not per-project)	✅
No new Jest config keys that conflict with ADR-005 conventions	✅
No changes to test discovery patterns (testMatch)	✅
No changes to transform pipeline or module mappers	✅
No production code modified	✅
Commit type (chore) appropriate for test-infra changes	✅
PR targets main (correct — chore PRs don't require beta integration)	✅
No ADR required (no new architectural decision — this is configuration tuning)	✅

---

Summary

This is a clean, well-reasoned test infrastructure improvement. The global clearMocks: true addition is the highest-value change and is correctly placed. The per-file afterEach additions are redundant given the global config but are harmless and defensively reasonable. No blocking concerns from an architecture standpoint.

[steilerDev]

[qa-integration-tester self-review] QA self-review of trimmed PR #1622 — verdict: COMMENT (approve with noted issues)

---

Review scope

8 test files reviewed against: (1) mock-clearing semantics, (2) afterEach placement consistency, (3) orphan imports, (4) memory-hygiene impact of trimmed scope, (5) coverage of worst offenders.

---

Finding 1 — Orphan import: within imported but never used in CostBreakdownTable.test.tsx ⚠️ Minor

client/src/components/CostBreakdownTable/CostBreakdownTable.test.tsx line 5:

import { render, screen, fireEvent, within } from '@testing-library/react';

within appears zero times in the file body. It was imported in anticipation of use (likely the original PR intended to add scoped queries) but no tests were added that call it. This is a dead import. It won't cause a runtime error but it will generate an ESLint/TS no-unused-vars warning and adds noise. Should be removed before merge.

---

Finding 2 — Placement inconsistency: HouseholdItemDetailPage.inline-edit.test.tsx ⚠️ Minor

In this file the afterEach is placed after the renderPage helper but before the first nested describe block (line 393), with a stray comment // ─── Schedule row display sandwiched between the helper and the hook:

  }  // end renderPage helper

  // ─── Schedule row display ──────────────────────────────────────────────────
  afterEach(() => {          ← sits here
    jest.clearAllMocks();
  });

  describe('Schedule row — target vs actual date display', () => {

Every other file in the 8 puts afterEach immediately after beforeEach and immediately before the first describe (no stray comment between them). Minor style inconsistency; the hook is correctly scoped to the outer describe so it is functionally identical. Still, it looks like the insert landed in the wrong spot (the comment // ─── Schedule row display now dangles above the afterEach rather than above the describe). No functional defect; just aesthetics.

---

Finding 3 — Mock-clearing semantics: clearAllMocks vs resetAllMocks ✅ Correct for most files

clearAllMocks resets .mock.calls / .mock.instances / .mock.results — it does not remove mockReturnValue / mockResolvedValue implementations. That is the correct choice here because:

• WorkItemDetailPage.test.tsx / harmonization / MilestoneDetailPage / HouseholdItemDetailPage.test.tsx / HouseholdItemDetailPage.inline-edit: all use an explicit beforeEach that calls .mockReset() on every named mock — which does clear implementations. The afterEach(clearAllMocks) addition is therefore belt-and-suspenders for the call-history leakage window between the last test in one suite and the first beforeEach of the next. Semantically sound.

• CostBreakdownTable.test.tsx / GanttSidebar.test.tsx: these files create jest.fn() instances inline per test (not module-scope mocks with persisted return values), so clearing call history is sufficient and correct. clearAllMocks is the right tool.

• InvoiceBudgetLinesSection.test.tsx: the PR adds clearAllMocks before the pre-existing restoreAllMocks. There are no spyOn calls in this file, making restoreAllMocks a no-op (it was already defensive code). Adding clearAllMocks first is harmless and slightly more explicit. Correct order (clear call history, then restore implementations).

⚠️ One nuance worth flagging: InvoiceBudgetLinesSection.test.tsx line 215 has getBaseUrl: jest.fn().mockReturnValue('/api') at module-scope. clearAllMocks will clear the call history but will NOT clear the mockReturnValue chain — so the return value is intentionally preserved across tests. This is the desired behavior here since the base URL should always return '/api'. Correct.

---

Finding 4 — Memory hygiene: trimmed scope still worth landing ✅ Yes

With jest.config.ts changes deferred, the global clearMocks: true flag is gone. The 8 per-file afterEach(jest.clearAllMocks) additions still provide real value:

1. They prevent call-history accumulation on module-scope mock refs across tests within a worker lifetime — the exact leak the PR targeted.
2. The 3 largest files modified (CostBreakdownTable at 4529 lines/127 tests, WorkItemDetailPage.test at 949 lines/~38 mocks, HouseholdItemDetailPage.test at 1851 lines/~20 mocks) hold the most module-scope mock references and benefit most.
3. Without clearMocks: true globally, these explicit afterEach additions are now the only safety net for those 8 files — making them more important, not less.

The memory improvement will be file-scoped rather than global, but it is real and measurable.

---

Finding 5 — Coverage of worst offenders: notable gaps remain ℹ️ Informational

The 8 files chosen were the highest-priority targets (largest test files with most module-scope mocks). However several files of comparable size and similar mock patterns were not touched:

File	Lines	Mock count	Has afterEach?
AutoItemizePage.test.tsx	2758	~30	✅ (restoreAllMocks only — no clearAllMocks)
BudgetSourcesPage.test.tsx	2208	11	❌ none
SubsidyProgramsPage.test.tsx	1702	11	❌ none
VendorDetailPage.test.tsx	1676	11	❌ none
HouseholdItemDetailPage.budget.test.tsx	1062	many	❌ none
DashboardPage.test.tsx	933	many	❌ none

AutoItemizePage is particularly notable: it already has afterEach(() => restoreAllMocks()) but no clearAllMocks — the exact same gap being fixed in InvoiceBudgetLinesSection.

These are follow-on work, not a blocker for this PR. The original audit was scoped to the 8 worst memory-accumulation offenders.

---

Summary verdict

Approve with two action items before merge (both Minor):

1. Blocker-for-cleanliness: Remove unused within import from CostBreakdownTable.test.tsx line 5.
2. Cosmetic: Fix stray comment placement in HouseholdItemDetailPage.inline-edit.test.tsx — move // ─── Schedule row display to be directly above the describe block, not above afterEach.

The mock-clearing semantics (clearAllMocks) are correct for all 8 files. The trimmed scope (no jest.config.ts) is still worth landing — the per-file hygiene additions provide real value and are now more important without the global fallback. CI pending at time of review.

[steilerDev]

[dev-team-lead]

VERDICT: CHANGES_REQUIRED

Issue 1: Orphan within import in CostBreakdownTable.test.tsx

• File: client/src/components/CostBreakdownTable/CostBreakdownTable.test.tsx
• Line(s): 5
• Problem: The diff adds within to the @testing-library/react import (alongside render, screen, fireEvent), but within is never called anywhere in the file — confirmed by full-file grep. This import was absent on beta and appears to have been accidentally added during the trim/rebase.
• Fix: Remove within from the import line; restore it to import { render, screen, fireEvent } from '@testing-library/react';
• Agent: qa-integration-tester

---

Everything else: APPROVED ✅

Consistency: Six of eight files add a standalone afterEach(() => jest.clearAllMocks()) (import + block). InvoiceBudgetLinesSection adds jest.clearAllMocks() to its existing afterEach alongside jest.restoreAllMocks() — correct. Placement variation (top-level vs inside outer describe) is benign in Jest semantics.

Flakiness risk: None. jest.clearAllMocks() calls mockClear() on each mock — it clears call-tracking (calls/instances/results) only. It does not clear implementations set via mockResolvedValue, mockReturnValue, or mockImplementation. Files that use top-level jest.fn().mockResolvedValue() patterns (e.g. mockFetchHICCategories in inline-edit.test.tsx) retain their configured return values across tests — no inter-test breakage introduced.

No lockfile touch: Verified — no changes to jest.config.ts, package.json, or package-lock.json.

Coverage gap: ~130 other .test.tsx files still lack clearAllMocks/resetAllMocks, but the PR description explicitly defers this to a global clearMocks: true in jest.config.ts (tracked in #1620/#1646). Acceptable scope.

---

Single blocker: remove the unused within import from CostBreakdownTable.test.tsx line 5, then this PR is ready to merge.