Summary
expect(mockIntervalPause).toHaveBeenCalled() in useMinimap.test.ts no longer constrains
the behaviour it was added for. Removing pauseChangeDetection() from destroy() leaves
the suite green.
Mechanism
The assertion was added during review of #15029 to pin that destroy() stops the change
detection interval. It passes for the wrong reason now: watch(shouldPoll, ..., { immediate: true }) fires during setup with active === false, which calls pauseChangeDetection()
before destroy() is ever reached. So the spy has "been called" regardless.
Evidence
Verified by mutation with a control at both commits, in an isolated worktree:
- Remove the
pauseChangeDetection() call from destroy() only
- At
bfeb2e55 (before the shouldPoll refactor): 1 test fails - the guard worked
- At
37e86ce0 and on main today: all tests pass - the guard is vacuous
Neutering every pauseChangeDetection() call site does still fail 2 tests, so the interval
is not completely unguarded. What is unguarded is specifically the destroy path.
Why it is worth fixing
This is the third time in this PR's history that a fix silently weakened a test rather than
breaking loudly. The code is correct; the test that is supposed to keep it correct is not.
A future refactor that drops the destroy() pause would ship a leaked 100ms interval per
minimap instance with a green suite, and there are three instances (MiniMap.vue,
GraphCanvasMenu.vue, ZoomControlsModal.vue).
Suggested fix
Assert the call count across the transition rather than the fact of a call, or reset the spy
after setup so only the destroy-path invocation is observed:
mockIntervalPause.mockClear()
minimap.destroy()
expect(mockIntervalPause).toHaveBeenCalledTimes(1)
Affected area
src/renderer/extensions/minimap/composables/useMinimap.test.ts
Found while reviewing #15029. Verified against main at 4c5afc8.
Summary
expect(mockIntervalPause).toHaveBeenCalled()inuseMinimap.test.tsno longer constrainsthe behaviour it was added for. Removing
pauseChangeDetection()fromdestroy()leavesthe suite green.
Mechanism
The assertion was added during review of #15029 to pin that
destroy()stops the changedetection interval. It passes for the wrong reason now:
watch(shouldPoll, ..., { immediate: true })fires during setup withactive === false, which callspauseChangeDetection()before
destroy()is ever reached. So the spy has "been called" regardless.Evidence
Verified by mutation with a control at both commits, in an isolated worktree:
pauseChangeDetection()call fromdestroy()onlybfeb2e55(before theshouldPollrefactor): 1 test fails - the guard worked37e86ce0and onmaintoday: all tests pass - the guard is vacuousNeutering every
pauseChangeDetection()call site does still fail 2 tests, so the intervalis not completely unguarded. What is unguarded is specifically the destroy path.
Why it is worth fixing
This is the third time in this PR's history that a fix silently weakened a test rather than
breaking loudly. The code is correct; the test that is supposed to keep it correct is not.
A future refactor that drops the
destroy()pause would ship a leaked 100ms interval perminimap instance with a green suite, and there are three instances (
MiniMap.vue,GraphCanvasMenu.vue,ZoomControlsModal.vue).Suggested fix
Assert the call count across the transition rather than the fact of a call, or reset the spy
after setup so only the destroy-path invocation is observed:
Affected area
src/renderer/extensions/minimap/composables/useMinimap.test.tsFound while reviewing #15029. Verified against
mainat 4c5afc8.