🐛 fix: address Copilot review on #3966 — fix mock leaks and remove duplicate test#3971
🐛 fix: address Copilot review on #3966 — fix mock leaks and remove duplicate test#3971clubanderson merged 1 commit intomainfrom
Conversation
…plicate test - Replace Object.defineProperty/global assignment with vi.stubGlobal() for WebSocket and fetch mocks so vitest tracks and restores them - Add vi.unstubAllGlobals() to afterEach in all 3 test files - Re-establish WebSocket mock in beforeEach (cleared by unstubAllGlobals) - Delete duplicate useClusterProgress.test.ts from hooks/ (canonical copy lives in hooks/__tests__/) Signed-off-by: Andrew Anderson <andy@clubanderson.com>
kubestellar-prow
bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ Deploy Preview for kubestellarconsole ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
kubestellar-prow
bot
added
the
size/L
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
There was a problem hiding this comment.
Pull request overview
This PR tightens Vitest test isolation for several hooks by switching global mocking to vi.stubGlobal() with explicit cleanup, and removes a duplicate useClusterProgress test file so the canonical suite lives under src/hooks/__tests__/.
Changes:
- Replace ad-hoc global mocking (
Object.defineProperty,global.fetch = ...) withvi.stubGlobal()in the affected tests. - Add
vi.unstubAllGlobals()toafterEach()to prevent global mock leakage across tests/files. - Delete the duplicate
web/src/hooks/useClusterProgress.test.ts(keeping the__tests__version).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| web/src/hooks/useClusterProgress.test.ts | Removes duplicate test file to avoid duplicate execution. |
| web/src/hooks/tests/useUpdateProgress.test.ts | Uses vi.stubGlobal('WebSocket', ...) + vi.unstubAllGlobals() for tracked, restorable global stubbing. |
| web/src/hooks/tests/useLocalAgent.test.ts | Uses vi.stubGlobal('fetch', ...) + vi.unstubAllGlobals() to avoid global fetch mock leaks between tests. |
| web/src/hooks/tests/useClusterProgress.test.ts | Uses vi.stubGlobal('WebSocket', ...) + vi.unstubAllGlobals() and keeps per-test state reset. |
| // Assign mock to global before importing the hook | ||
| Object.defineProperty(globalThis, 'WebSocket', { | ||
| writable: true, | ||
| value: MockWebSocket, | ||
| }) | ||
| vi.stubGlobal('WebSocket', MockWebSocket) | ||
|
|
||
| import { useClusterProgress } from '../useClusterProgress' | ||
|
|
||
| describe('useClusterProgress', () => { | ||
| beforeEach(() => { | ||
| vi.useFakeTimers() | ||
| wsInstances = [] | ||
| vi.stubGlobal('WebSocket', MockWebSocket) | ||
| }) |
There was a problem hiding this comment.
WebSocket is stubbed twice: once at module scope (before the import) and again in beforeEach(). Since useClusterProgress only instantiates WebSocket inside useEffect (not at module-eval time), the module-scope vi.stubGlobal is unnecessary and can be removed to avoid double-stubbing and make global cleanup more predictable (keep the per-test stub + vi.unstubAllGlobals() in afterEach).
| // Assign mock to global before importing the hook | ||
| Object.defineProperty(globalThis, 'WebSocket', { | ||
| writable: true, | ||
| value: MockWebSocket, | ||
| }) | ||
| vi.stubGlobal('WebSocket', MockWebSocket) | ||
|
|
||
| import { useUpdateProgress } from '../useUpdateProgress' | ||
|
|
||
| describe('useUpdateProgress', () => { | ||
| beforeEach(() => { | ||
| vi.useFakeTimers() | ||
| wsInstances = [] | ||
| vi.stubGlobal('WebSocket', MockWebSocket) | ||
| }) |
There was a problem hiding this comment.
WebSocket is stubbed both at module scope and again in beforeEach(). Because useUpdateProgress only calls new WebSocket(...) inside useEffect (not during module import), the module-scope vi.stubGlobal('WebSocket', ...) isn’t needed and leads to double-stubbing; consider removing the module-scope stub and relying on the per-test stub + vi.unstubAllGlobals() cleanup.
🔄 Auto-Applying Copilot Code ReviewCopilot code review found 0 code suggestion(s) and 2 general comment(s). Also address these general comments:
Push all fixes in a single commit. Run Auto-generated by copilot-review-apply workflow. |
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
Summary
Object.defineProperty/global.fetch =withvi.stubGlobal()in all 3 test files so vitest properly tracks and restores global mocksvi.unstubAllGlobals()toafterEachfor proper cleanup between testsuseClusterProgress.test.tsfromhooks/(canonical copy inhooks/__tests__/)Addresses all 4 Copilot review comments from #3966.
Test plan