chore: test cleanup #2116
chore: test cleanup #2116
Conversation
Cleans up some issues in tests: - Adds missing getUidGid and listVMs mocks. - Simplify ResizeObserver mocking using the same pattern as Podman Desktop renderer. - Import correct beforeEach. - Import node types to avoid NodeJS import warnings. Fixes podman-desktop#2115. Signed-off-by: Tim deBoer <git@tdeboer.ca>
deboer-tim
requested review from
MarsKubeX,
benoitf and
gastoner
and removed request for
a team
📝 WalkthroughWalkthroughTest infrastructure consolidation: moved ResizeObserver mocking to centralized setup file, updated timer type annotations to browser-compatible equivalents (ReturnType<typeof setInterval/setTimeout>), standardized test utility imports from vitest, and added deterministic VM list mock setup. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/frontend/tsconfig.json (1)
22-22: Node typings now available globally in frontend TSAdding
"node"tocompilerOptions.typessatisfies the editor warnings goal and is consistent with the PR description, but it also makes Node globals (e.g.,process,Buffer) typecheck in all frontend sources. If you’d prefer to keep browser code guarded from accidental Node usage, consider scoping Node types to a test-specific tsconfig instead (e.g., a dedicatedtsconfig.vitest.jsonthat extends this one).packages/frontend/src/CreateVM.spec.ts (1)
22-48: Deterministic default listVMs mock via beforeEachImporting
beforeEachfrom vitest and settingbootcClient.listVMsto resolve to[]before every test gives all cases a sane default and removes the missing-mock noise described in the issue. Tests that need a non-empty VM list can still override this mock as you do later in the file; for extra clarity, you might consider setting those overrides before callingrenderso the component never briefly sees the default list, but that’s optional given current behavior.packages/frontend/vite.tests.setup.js (1)
21-28: Centralized ResizeObserver mock is good; consider also setting global.ResizeObserverThe
ResizeObserverMockwith no-op methods is sufficient for tests that just need the API surface (e.g.,@floating-ui/dom) and centralizing it here removes duplication from individual specs. To make this more robust against code that referencesResizeObserverdirectly (withoutwindow.), you could also assign the mock to the global symbol:-class ResizeObserverMock { - observe() {} - unobserve() {} - disconnect() {} -} - -global.window.ResizeObserver = ResizeObserverMock; +class ResizeObserverMock { + observe() {} + unobserve() {} + disconnect() {} +} + +global.window.ResizeObserver = ResizeObserverMock; +global.ResizeObserver = ResizeObserverMock;packages/frontend/src/lib/disk-image/DiskImageDetailsBuild.spec.ts (1)
67-73: Use of folderLocation variable in expectation reduces duplicationAsserting
loadLogsFromFolderwithfolderLocationinstead of a string literal keeps the test aligned with the prop passed intorenderand avoids drift if the folder path changes later. If you want to go further, you could apply the same pattern to the error-message assertion in the “Handles empty logs correctly” test, but that’s purely a readability improvement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/frontend/src/Build.spec.ts(1 hunks)packages/frontend/src/CreateVM.spec.ts(2 hunks)packages/frontend/src/lib/disk-image/DiskImageDetailsBuild.spec.ts(1 hunks)packages/frontend/src/lib/disk-image/DiskImageDetailsVirtualMachine.spec.ts(1 hunks)packages/frontend/src/lib/disk-image/DiskImagesList.spec.ts(1 hunks)packages/frontend/tsconfig.json(1 hunks)packages/frontend/vite.tests.setup.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/frontend/src/CreateVM.spec.ts (1)
packages/frontend/src/api/client.ts (1)
bootcClient(28-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: linter, formatter, and tests / macos-14
- GitHub Check: linter, formatter, and tests / windows-2022
- GitHub Check: e2e tests
🔇 Additional comments (3)
packages/frontend/src/Build.spec.ts (1)
100-117: bootcClient.getUidGid mock completes the test client surfaceAdding
getUidGid: vi.fn()to the localbootcClientmock matches the expected API and should eliminate missing-method noise when Build uses chown-related options in tests. For any tests that depend on a specific UID/GID, you can still override this mock withmockResolvedValuein those specs.packages/frontend/src/lib/disk-image/DiskImageDetailsVirtualMachine.spec.ts (1)
18-20: Vitest import now matches actual usage and shared test setupSwitching the import to
{ vi, test, expect }and relying on the global test setup for DOM shims (likeResizeObserver) keeps this spec minimal and consistent with the rest of the suite. No remaining hooks here depend on the removed per-file setup.packages/frontend/src/lib/disk-image/DiskImagesList.spec.ts (1)
18-18: Use of vitest’s beforeEach aligns lifecycle hooks with the test runnerImporting
beforeEachfromvitest(instead of Node’s test module) makes the lifecycle hook consistent with the rest of the suite and with the mocking utilities fromvi. This should avoid subtle issues from mixing different test frameworks.
packages/frontend/tsconfig.json
Outdated
| "allowJs": true, | ||
| "checkJs": true, | ||
| "types": ["@testing-library/jest-dom"] | ||
| "types": ["@testing-library/jest-dom", "node"] |
There was a problem hiding this comment.
question: is that sage to use node in the frontend ?
There was a problem hiding this comment.
it should not use node on frontend yes
There was a problem hiding this comment.
Removed node types and replaced code instances with ReturnType<typeof ...> to avoid any typing errors.
Replaces NodeJS types with node/javascript compatible types. Signed-off-by: Tim deBoer <git@tdeboer.ca>
5 participants
What does this PR do?
Cleans up some issues in tests:
Screenshot / video of UI
N/A
What issues does this PR fix or reference?
Fixes #2115.
How to test this PR?
Code looks ok; tests green.