test: simplify test file mocking patterns#8320
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughReplaces the legacy Changes
Sequence Diagram(s)sequenceDiagram
participant GraphCanvas
participant useNewUserService
participant SettingStore
participant LocalStorage
GraphCanvas->>useNewUserService: useNewUserService().initializeIfNewUser()
useNewUserService->>SettingStore: read settings (e.g., tutorial/workflow flags)
useNewUserService->>LocalStorage: check workflow/install indicators
alt determined as new user
useNewUserService->>LocalStorage: set Comfy.InstalledVersion
useNewUserService->>useNewUserService: execute pending callbacks (try/catch)
else not new user
useNewUserService->>useNewUserService: clear pending callbacks
end
useNewUserService-->>GraphCanvas: initialization complete
Possibly related PRs
Suggested reviewers
✨ Finishing touches
Comment |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/28/2026, 07:40:15 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Tests: ✅ PassedResults: 507 passed, 0 failed, 0 flaky, 8 skipped (Total: 515) 📊 Browser Reports
|
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 25.9 kB (baseline 25.9 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 960 kB (baseline 960 kB) • 🔴 +317 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 80.7 kB (baseline 80.7 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 471 kB (baseline 471 kB) • 🟢 -8 BConfiguration panels, inspectors, and settings screens
Status: 12 added / 12 removed User & Accounts — 3.94 kB (baseline 3.94 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 3 added / 3 removed Editors & Dialogs — 2.89 kB (baseline 2.89 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 33.7 kB (baseline 33.7 kB) • ⚪ 0 BReusable component library chunks
Status: 4 added / 4 removed Data & Services — 2.7 MB (baseline 2.7 MB) • 🔴 +1 BStores, services, APIs, and repositories
Status: 8 added / 8 removed Utilities & Hooks — 25.3 kB (baseline 25.3 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 7 added / 7 removed Vendor & Third-Party — 10.7 MB (baseline 10.7 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.05 MB (baseline 7.05 MB) • 🟢 -198 BBundles that do not match a named category
Status: 34 added / 34 removed |
DrJKL
force-pushed
the
drjkl/mock-yeah-ing-yeah
branch
from
a0cf3b6 to
0eb4a23
Compare
DrJKL
requested review from
a team,
Myestery,
christian-byrne,
jtydhr88,
ltdrdata,
pythongosssss and
viva-jinyi
as code owners
dosubot
bot
added
the
size:XL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/platform/updates/common/versionCompatibilityStore.test.ts (1)
1-55: Replaceany-typed mock stores with explicit types.
anyobscures the mock store API surface. Please type the mocks with the real store signatures and usePartial<...> as ...for incomplete mock shapes.♻️ Suggested typing for mocks
- let mockSystemStatsStore: any - let mockSettingStore: any + let mockSystemStatsStore: ReturnType<typeof useSystemStatsStore> + let mockSettingStore: ReturnType<typeof useSettingStore> @@ - mockSystemStatsStore = { + mockSystemStatsStore = { systemStats: null, isInitialized: false, refetchSystemStats: vi.fn() - } + } as Partial<ReturnType<typeof useSystemStatsStore>> as ReturnType< + typeof useSystemStatsStore + > @@ - mockSettingStore = { + mockSettingStore = { get: vi.fn(() => false) // Default to warnings enabled - } + } as Partial<ReturnType<typeof useSettingStore>> as ReturnType< + typeof useSettingStore + >src/platform/updates/common/releaseStore.test.ts (1)
1-83: Type the mocked stores/services instead ofany.Please align the mocks with the real store/service signatures and use
Partial<...> as ...for partial mock shapes.♻️ Suggested typing for mocks
- let mockReleaseService: any - let mockSettingStore: any - let mockSystemStatsStore: any + let mockReleaseService: ReturnType<typeof useReleaseService> + let mockSettingStore: ReturnType<typeof useSettingStore> + let mockSystemStatsStore: ReturnType<typeof useSystemStatsStore> @@ - mockReleaseService = { + mockReleaseService = { getReleases: vi.fn(), isLoading: { value: false }, error: { value: null } - } + } as Partial<ReturnType<typeof useReleaseService>> as ReturnType< + typeof useReleaseService + > @@ - mockSettingStore = { + mockSettingStore = { get: vi.fn(), set: vi.fn() - } + } as Partial<ReturnType<typeof useSettingStore>> as ReturnType< + typeof useSettingStore + > @@ - mockSystemStatsStore = { + mockSystemStatsStore = { systemStats: { system: { comfyui_version: '1.0.0' } }, isInitialized: true, refetchSystemStats: vi.fn(), getFormFactor: vi.fn(() => 'git-windows') - } + } as Partial<ReturnType<typeof useSystemStatsStore>> as ReturnType< + typeof useSystemStatsStore + >
🤖 Fix all issues with AI agents
In `@src/lib/litegraph/src/LGraph.test.ts`:
- Around line 49-52: The test asserting equality between LiteGraph.LGraph and
LGraph ("expect(LiteGraph.LGraph).toBe(LGraph)") is duplicated elsewhere; remove
one of the duplicates to avoid redundancy—specifically delete the test named "is
exactly the same type" that contains the assertion, or alternatively remove the
identical assertion from the "Legacy LGraph Compatibility Layer" block (the test
named "is correctly assigned to LiteGraph"), ensuring only a single test remains
verifying LiteGraph.LGraph === LGraph.
In `@src/renderer/extensions/minimap/composables/useMinimap.test.ts`:
- Around line 156-157: The new static imports for useMinimap and api are
triggering import-x/no-unresolved due to the '@' alias; either update the ESLint
import resolver config used for tests to recognize '@' (ensure test eslint
config extends resolver settings or add "import/resolver" mapping) or change the
two imports in useMinimap.test.ts to relative paths that point to the same
modules (replace "@/renderer/extensions/minimap/composables/useMinimap" and
"@/scripts/api" with their relative filesystem equivalents) so CI lint passes;
locate the import lines referencing useMinimap and api and apply one of these
fixes.
In `@src/renderer/extensions/minimap/composables/useMinimapViewport.test.ts`:
- Around line 6-10: The test import of calculateMinimapScale,
calculateNodeBounds, and enforceMinimumBounds from
'@/renderer/core/spatial/boundsCalculator' is triggering import-x/no-unresolved;
update the import in useMinimapViewport.test.ts to use the project's relative
path to the module (e.g., navigate up from the test file into
renderer/core/spatial/boundsCalculator) OR update the test ESLint/TS resolver
config so the '@' alias is recognized in tests; modify the import statement
referencing those three functions accordingly so CI no longer flags the
unresolved alias.
In `@src/services/jobOutputCache.test.ts`:
- Around line 260-264: The test uses an unsafe cast
mockResolvedValue(mockWorkflow as any) for extractWorkflow; replace the any with
a properly typed mock that matches extractWorkflow's return type (e.g., derive
the type via ReturnType<typeof extractWorkflow> or import the extractWorkflow
return/interface) and update mockWorkflow to conform to that type so
vi.mocked(extractWorkflow).mockResolvedValue(mockWorkflow) is fully typed; keep
api.getJobDetail mocking unchanged.
In `@src/services/useNewUserService.test.ts`:
- Around line 29-30: The test currently silences TypeScript with
//@ts-expect-error for global.__COMFYUI_FRONTEND_VERSION__; either add a brief
inline comment explaining why the expect-error is necessary for this test global
(e.g., "define test-only global used by ComfyUI snapshots") or, preferably, add
a test type declaration (e.g., declare const __COMFYUI_FRONTEND_VERSION__:
string) in a test globals file so the symbol is properly typed; update
useNewUserService.test.ts to remove the expect-error or reference the new test
globals declaration accordingly.
In
`@src/workbench/extensions/manager/components/manager/packCard/PackCard.test.ts`:
- Around line 50-60: The test's vi.mock for '@vueuse/core' returns raw values
from the mocked useStorage, but VueUse's useStorage returns a Ref/RemovableRef,
so update the useStorage mock in PackCard.test.ts to return a ref-like object
(e.g., an object with a .value property holding the stored value, and persist
updates back into storageMap when .value changes) so components reading `.value`
behave correctly; locate the mocked useStorage inside the vi.mock block and
replace its return with a ref-compatible shape (or use Vue's ref() to wrap the
default value) and ensure createSharedComposable and whenever mocks remain
unchanged.
Comment on lines
+156
to
+204
| import { useMinimap } from '@/renderer/extensions/minimap/composables/useMinimap' | ||
| import { api } from '@/scripts/api' |
Contributor
There was a problem hiding this comment.
Fix lint-breaking unresolved alias on new static imports.
import-x/no-unresolved now fires on these static imports, which can fail CI. Either update the ESLint resolver to honor the @ alias for test files or switch to relative imports as a local fix.
✅ Example fallback using relative imports
-import { useMinimap } from '@/renderer/extensions/minimap/composables/useMinimap'
-import { api } from '@/scripts/api'
+import { useMinimap } from './useMinimap'
+import { api } from '../../../../scripts/api'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { useMinimap } from '@/renderer/extensions/minimap/composables/useMinimap' | |
| import { api } from '@/scripts/api' | |
| import { useMinimap } from './useMinimap' | |
| import { api } from '../../../../scripts/api' |
🧰 Tools
🪛 ESLint
[error] 156-156: Unable to resolve path to module '@/renderer/extensions/minimap/composables/useMinimap'.
(import-x/no-unresolved)
[error] 157-157: Unable to resolve path to module '@/scripts/api'.
(import-x/no-unresolved)
🤖 Prompt for AI Agents
In `@src/renderer/extensions/minimap/composables/useMinimap.test.ts` around lines
156 - 157, The new static imports for useMinimap and api are triggering
import-x/no-unresolved due to the '@' alias; either update the ESLint import
resolver config used for tests to recognize '@' (ensure test eslint config
extends resolver settings or add "import/resolver" mapping) or change the two
imports in useMinimap.test.ts to relative paths that point to the same modules
(replace "@/renderer/extensions/minimap/composables/useMinimap" and
"@/scripts/api" with their relative filesystem equivalents) so CI lint passes;
locate the import lines referencing useMinimap and api and apply one of these
fixes.
Comment on lines
+6
to
+11
| import { | ||
| calculateMinimapScale, | ||
| calculateNodeBounds, | ||
| enforceMinimumBounds | ||
| } from '@/renderer/core/spatial/boundsCalculator' |
Contributor
There was a problem hiding this comment.
Resolve ESLint alias failure for boundsCalculator import.
The new static import is flagged by import-x/no-unresolved. Either ensure the ESLint resolver uses the TS path alias for @ in tests, or use a relative path to unblock CI.
✅ Example fallback using a relative import
-import {
- calculateMinimapScale,
- calculateNodeBounds,
- enforceMinimumBounds
-} from '@/renderer/core/spatial/boundsCalculator'
+import {
+ calculateMinimapScale,
+ calculateNodeBounds,
+ enforceMinimumBounds
+} from '../../../core/spatial/boundsCalculator'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { | |
| calculateMinimapScale, | |
| calculateNodeBounds, | |
| enforceMinimumBounds | |
| } from '@/renderer/core/spatial/boundsCalculator' | |
| import { | |
| calculateMinimapScale, | |
| calculateNodeBounds, | |
| enforceMinimumBounds | |
| } from '../../../core/spatial/boundsCalculator' |
🧰 Tools
🪛 ESLint
[error] 10-10: Unable to resolve path to module '@/renderer/core/spatial/boundsCalculator'.
(import-x/no-unresolved)
🤖 Prompt for AI Agents
In `@src/renderer/extensions/minimap/composables/useMinimapViewport.test.ts`
around lines 6 - 10, The test import of calculateMinimapScale,
calculateNodeBounds, and enforceMinimumBounds from
'@/renderer/core/spatial/boundsCalculator' is triggering import-x/no-unresolved;
update the import in useMinimapViewport.test.ts to use the project's relative
path to the module (e.g., navigate up from the test file into
renderer/core/spatial/boundsCalculator) OR update the test ESLint/TS resolver
config so the '@' alias is recognized in tests; modify the import statement
referencing those three functions accordingly so CI no longer flags the
unresolved alias.
Comment on lines
260
to
264
| const mockWorkflow = { version: 1 } | ||
|
|
||
| vi.mocked(fetchJobDetail).mockResolvedValue(mockDetail) | ||
| vi.mocked(api.getJobDetail).mockResolvedValue(mockDetail) | ||
| vi.mocked(extractWorkflow).mockResolvedValue(mockWorkflow as any) | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid as any in the workflow mock.
mockResolvedValue(mockWorkflow as any) violates the test typing rule and can hide mismatches. Use a concrete type derived from extractWorkflow instead.
🔧 Suggested fix
- const mockWorkflow = { version: 1 }
+ type Workflow = NonNullable<Awaited<ReturnType<typeof extractWorkflow>>>
+ const mockWorkflow: Workflow = { version: 1 }
vi.mocked(api.getJobDetail).mockResolvedValue(mockDetail)
- vi.mocked(extractWorkflow).mockResolvedValue(mockWorkflow as any)
+ vi.mocked(extractWorkflow).mockResolvedValue(mockWorkflow)As per coding guidelines, avoid any in test files.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const mockWorkflow = { version: 1 } | |
| vi.mocked(fetchJobDetail).mockResolvedValue(mockDetail) | |
| vi.mocked(api.getJobDetail).mockResolvedValue(mockDetail) | |
| vi.mocked(extractWorkflow).mockResolvedValue(mockWorkflow as any) | |
| type Workflow = NonNullable<Awaited<ReturnType<typeof extractWorkflow>>> | |
| const mockWorkflow: Workflow = { version: 1 } | |
| vi.mocked(api.getJobDetail).mockResolvedValue(mockDetail) | |
| vi.mocked(extractWorkflow).mockResolvedValue(mockWorkflow) |
🤖 Prompt for AI Agents
In `@src/services/jobOutputCache.test.ts` around lines 260 - 264, The test uses an
unsafe cast mockResolvedValue(mockWorkflow as any) for extractWorkflow; replace
the any with a properly typed mock that matches extractWorkflow's return type
(e.g., derive the type via ReturnType<typeof extractWorkflow> or import the
extractWorkflow return/interface) and update mockWorkflow to conform to that
type so vi.mocked(extractWorkflow).mockResolvedValue(mockWorkflow) is fully
typed; keep api.getJobDetail mocking unchanged.
Comment on lines
29
to
30
| //@ts-expect-error Define global for the test | ||
| global.__COMFYUI_FRONTEND_VERSION__ = '1.24.0' |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting the @ts-expect-error usage.
Per coding guidelines, @ts-expect-error should be avoided and the underlying type issue fixed. However, for global test definitions like __COMFYUI_FRONTEND_VERSION__, this may be necessary. Consider adding a brief comment explaining why this is required or declaring the global in a test types file.
💡 Optional: Add type declaration for test globals
You could add a test globals declaration file (e.g., tests/globals.d.ts):
declare const __COMFYUI_FRONTEND_VERSION__: stringOr add a more descriptive comment:
-//@ts-expect-error Define global for the test
+// `@ts-expect-error` - Required: __COMFYUI_FRONTEND_VERSION__ is a compile-time constant injected by Vite
global.__COMFYUI_FRONTEND_VERSION__ = '1.24.0'🤖 Prompt for AI Agents
In `@src/services/useNewUserService.test.ts` around lines 29 - 30, The test
currently silences TypeScript with //@ts-expect-error for
global.__COMFYUI_FRONTEND_VERSION__; either add a brief inline comment
explaining why the expect-error is necessary for this test global (e.g., "define
test-only global used by ComfyUI snapshots") or, preferably, add a test type
declaration (e.g., declare const __COMFYUI_FRONTEND_VERSION__: string) in a test
globals file so the symbol is properly typed; update useNewUserService.test.ts
to remove the expect-error or reference the new test globals declaration
accordingly.
Comment on lines
+50
to
+59
| vi.mock('@vueuse/core', () => ({ | ||
| whenever: vi.fn(), | ||
| useStorage: vi.fn((key: string, defaultValue: unknown) => { | ||
| if (!storageMap.has(key)) storageMap.set(key, defaultValue) | ||
| return storageMap.get(key) | ||
| }), | ||
| createSharedComposable: vi.fn((fn) => { | ||
| let cached: ReturnType<typeof fn> | ||
| return (...args: Parameters<typeof fn>) => (cached ??= fn(...args)) | ||
| }) | ||
| })) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
useStorage mock should return a ref to match VueUse's actual API.
VueUse's useStorage returns a RemovableRef<T> (extending Ref), but this mock returns the raw value. If the component (or a future change) accesses .value, the mock will behave incorrectly.
♻️ Suggested fix to match VueUse API
+import { ref } from 'vue'
+
vi.mock('@vueuse/core', () => ({
whenever: vi.fn(),
useStorage: vi.fn((key: string, defaultValue: unknown) => {
if (!storageMap.has(key)) storageMap.set(key, defaultValue)
- return storageMap.get(key)
+ return ref(storageMap.get(key))
}),
createSharedComposable: vi.fn((fn) => {
let cached: ReturnType<typeof fn>
return (...args: Parameters<typeof fn>) => (cached ??= fn(...args))
})
}))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| vi.mock('@vueuse/core', () => ({ | |
| whenever: vi.fn(), | |
| useStorage: vi.fn((key: string, defaultValue: unknown) => { | |
| if (!storageMap.has(key)) storageMap.set(key, defaultValue) | |
| return storageMap.get(key) | |
| }), | |
| createSharedComposable: vi.fn((fn) => { | |
| let cached: ReturnType<typeof fn> | |
| return (...args: Parameters<typeof fn>) => (cached ??= fn(...args)) | |
| }) | |
| })) | |
| import { ref } from 'vue' | |
| vi.mock('@vueuse/core', () => ({ | |
| whenever: vi.fn(), | |
| useStorage: vi.fn((key: string, defaultValue: unknown) => { | |
| if (!storageMap.has(key)) storageMap.set(key, defaultValue) | |
| return ref(storageMap.get(key)) | |
| }), | |
| createSharedComposable: vi.fn((fn) => { | |
| let cached: ReturnType<typeof fn> | |
| return (...args: Parameters<typeof fn>) => (cached ??= fn(...args)) | |
| }) | |
| })) |
🤖 Prompt for AI Agents
In
`@src/workbench/extensions/manager/components/manager/packCard/PackCard.test.ts`
around lines 50 - 60, The test's vi.mock for '@vueuse/core' returns raw values
from the mocked useStorage, but VueUse's useStorage returns a Ref/RemovableRef,
so update the useStorage mock in PackCard.test.ts to return a ref-like object
(e.g., an object with a .value property holding the stored value, and persist
updates back into storageMap when .value changes) so components reading `.value`
behave correctly; locate the mocked useStorage inside the vi.mock block and
replace its return with a ref-compatible shape (or use Vue's ref() to wrap the
default value) and ensure createSharedComposable and whenever mocks remain
unchanged.
christian-byrne
previously approved these changes
Jan 28, 2026
DrJKL
force-pushed
the
drjkl/mock-yeah-ing-yeah
branch
from
a9a4dce to
0eb4a23
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/platform/updates/common/releaseStore.test.ts (1)
4-91: Fix TS type mismatches in mocked stores (CI is failing due to type incompatibility).The partial mocks being assigned to
vi.mocked(...).mockReturnValue(...)don't match the full store types.mockSettingStoreomits properties likesettingValues,settingsById,isReady,load,addSetting,exists,getDefaultValue, andisLoading/error.mockSystemStatsStoreis missingisLoadinganderror. Cast these partial mocks explicitly usingas Partial<StoreType> as StoreTypeto satisfy the type system while acknowledging the incomplete implementation.🐛 Proposed fix
import type { Mock } from 'vitest' import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { Ref } from 'vue' import { ref } from 'vue' @@ - let mockReleaseService: { - getReleases: Mock - isLoading: ReturnType<typeof ref<boolean>> - error: ReturnType<typeof ref<string | null>> - } - let mockSettingStore: { get: Mock; set: Mock } - let mockSystemStatsStore: { - systemStats: { - system: { - comfyui_version: string - argv?: string[] - [key: string]: unknown - } - devices?: unknown[] - } | null - isInitialized: boolean - refetchSystemStats: Mock - getFormFactor: Mock - } + type ReleaseService = ReturnType<typeof useReleaseService> + type SettingStore = ReturnType<typeof useSettingStore> + type SystemStatsStore = ReturnType<typeof useSystemStatsStore> + + let mockReleaseService: ReleaseService + let mockSettingStore: SettingStore + let mockSystemStatsStore: SystemStatsStore @@ - mockReleaseService = { - getReleases: vi.fn(), - isLoading: ref(false), - error: ref(null) - } + mockReleaseService = { + getReleases: vi.fn<ReleaseService['getReleases']>(), + isLoading: ref(false) as Ref<boolean>, + error: ref<string | null>(null) + } - mockSettingStore = { - get: vi.fn(), - set: vi.fn() - } + mockSettingStore = { + get: vi.fn(), + set: vi.fn() + } as Partial<SettingStore> as SettingStore - mockSystemStatsStore = { + mockSystemStatsStore = { systemStats: { system: { comfyui_version: '1.0.0' } }, isInitialized: true, refetchSystemStats: vi.fn(), getFormFactor: vi.fn(() => 'git-windows') - } + } as Partial<SystemStatsStore> as SystemStatsStore
🤖 Fix all issues with AI agents
In
`@src/workbench/extensions/manager/components/manager/packCard/PackCard.test.ts`:
- Around line 3-4: Tests import unused symbols Card and setActivePinia; remove
these unused imports from the PackCard.test.ts file to satisfy static analysis.
Locate the import statements that reference Card and setActivePinia and delete
those identifiers (or the entire import line if it only contains unused symbols)
so there are no unused imports remaining.
src/workbench/extensions/manager/components/manager/packCard/PackCard.test.ts
Outdated
Show resolved
Hide resolved
- releaseStore.test.ts: remove 5 dynamic imports, sync beforeEach - useMinimap.test.ts: static imports for useMinimap and api - jobOutputCache.test.ts: static imports, uniqueId() for cache isolation Amp-Thread-ID: https://ampcode.com/threads/T-019bfbc0-fe5e-75d3-b4fb-b7427ef0dada Co-authored-by: Amp <amp@ampcode.com>
Replace await import() with static imports for mocked modules where module isolation is not required. This simplifies test code and improves readability. Amp-Thread-ID: https://ampcode.com/threads/T-019bfbe0-e248-7002-9a0a-e36b42bdfbc1 Co-authored-by: Amp <amp@ampcode.com>
- Use createSharedComposable from VueUse for singleton pattern - Move module-scoped state into reactive refs - Encapsulate reset() method within the composable - Update GraphCanvas.vue to use new composable Amp-Thread-ID: https://ampcode.com/threads/T-019bfc08-b23e-72ed-8d69-fb04e42b4d14 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019bfc0e-7557-776c-9187-365aac373684 Co-authored-by: Amp <amp@ampcode.com>
Refactor test files to use cleaner mocking approaches: - Remove redundant vi.hoisted() calls where not needed - Simplify mock implementations - Clean up unused imports and variables Amp-Thread-ID: https://ampcode.com/threads/T-019bfc31-6862-7278-96c3-a17e0b2cb350 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c05d4-e92f-71e8-8d7c-71976bbbe057 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c05f8-3eae-75ed-a734-d723415676e5 Co-authored-by: Amp <amp@ampcode.com>
DrJKL
force-pushed
the
drjkl/mock-yeah-ing-yeah
branch
from
0eb4a23 to
6cf7e17
Compare
dosubot
bot
added
size:XXL
🔧 Auto-fixes AppliedThis PR has been automatically updated to fix linting and formatting issues.
Changes made:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/components/graph/selectionToolbox/ExecuteButton.test.ts (1)
75-81:vi.clearAllMocks()clears the mock configuration set above it.Calling
vi.clearAllMocks()aftermockReturnValue()will reset the mock, causinguseSelectionStateto returnundefinedinstead of the configured value withmockSelectedNodes. Movevi.clearAllMocks()to the beginning ofbeforeEach.🐛 Proposed fix
beforeEach(() => { + vi.clearAllMocks() + // Set up Pinia with testing utilities setActivePinia( createTestingPinia({ createSpy: vi.fn }) ) // Reset mocks const partialCanvas: Partial<LGraphCanvas> = { setDirty: vi.fn() } mockCanvas = partialCanvas as Partial<LGraphCanvas> as LGraphCanvas mockSelectedNodes = [] // Get store instances and mock methods const canvasStore = useCanvasStore() const commandStore = useCommandStore() vi.spyOn(canvasStore, 'getCanvas').mockReturnValue(mockCanvas) vi.spyOn(commandStore, 'execute').mockResolvedValue() // Update the useSelectionState mock vi.mocked(useSelectionState).mockReturnValue({ selectedNodes: { value: mockSelectedNodes } } as ReturnType<typeof useSelectionState>) - - vi.clearAllMocks() })src/workbench/extensions/manager/components/manager/packCard/PackCard.test.ts (2)
133-158: Avoid testing CSS utility classes; test behavior instead.Tests asserting on
.ring-3and.rounded-lgclasses are dependent on implementation details rather than behavior. If the styling implementation changes (e.g., different Tailwind classes or CSS approach), these tests will break even though the visual behavior is unchanged.Consider testing the user-visible outcome or accessible properties instead:
♻️ Suggested approach
it('should apply selected ring when isSelected is true', () => { const wrapper = createWrapper({ nodePack: mockNodePack, isSelected: true }) - expect(wrapper.find('.ring-3').exists()).toBe(true) + // Test via aria attribute, data attribute, or computed style if visual distinction is important + expect(wrapper.classes()).toContain('ring-3') // If class check is necessary, use classes() on the root }) - it('should not apply selected ring when isSelected is false', () => { - const wrapper = createWrapper({ - nodePack: mockNodePack, - isSelected: false - }) - - expect(wrapper.find('.ring-3').exists()).toBe(false) - }) + // Consider removing the negative test - testing absence of a style is low value }) describe('component behavior', () => { it('should render without errors', () => { const wrapper = createWrapper({ nodePack: mockNodePack }) expect(wrapper.exists()).toBe(true) - expect(wrapper.find('.rounded-lg').exists()).toBe(true) }) })As per coding guidelines: "Do not write tests dependent on non-behavioral features like utility classes or styles."
161-178: Consider consolidating redundant tests.The tests for
should display package name,should display package description, andshould display author name(lines 162-178) duplicate assertions already made inshould render package card with basic information(lines 118-125).♻️ Suggested consolidation
describe('package information display', () => { - it('should display package name', () => { - const wrapper = createWrapper({ nodePack: mockNodePack }) - - expect(wrapper.text()).toContain('Test Package') - }) - - it('should display package description', () => { - const wrapper = createWrapper({ nodePack: mockNodePack }) - - expect(wrapper.text()).toContain('Test package description') - }) - - it('should display author name', () => { - const wrapper = createWrapper({ nodePack: mockNodePack }) - - expect(wrapper.text()).toContain('Test Author') - }) - it('should handle missing description', () => { // ... keep edge case tests })As per coding guidelines: "Do not write redundant tests; be parsimonious in testing."
src/lib/litegraph/src/litegraph.test.ts (1)
11-36: Avoid direct LGraph import; keep barrel-only.The direct import from
@/lib/litegraph/src/LGraphbypasses the barrel export policy and risks circular dependencies. Drop the direct import and its assertion.♻️ Proposed change
-import { LGraph as DirectLGraph } from '@/lib/litegraph/src/LGraph' - describe('Import order dependency', () => { test('Imports reference the same types', ({ expect }) => { - // Both imports should reference the same LGraph class - expect(LiteGraph.LGraph).toBe(DirectLGraph) expect(LiteGraph.LGraph).toBe(LGraph) }) })Based on learnings: Always import from the barrel export for subgraph code to avoid circular dependencies (import from '@/lib/litegraph/src/litegraph' instead of individual modules like '@/lib/litegraph/src/LGraph').
🤖 Fix all issues with AI agents
In `@docs/testing/unit-testing.md`:
- Around line 317-330: The markdown contains a stray empty fenced code block
that triggers MD040; remove the empty triple-backtick block or give it a
language tag and ensure the intended snippet (the
vi.mocked(useMyComposable).mockReturnValue({ isLoading: ref(false) }) line) is
inside a properly delimited fenced block (e.g., ```typescript) instead of
leaving an empty block; update the section around the examples referencing
mockService, useMyComposable, vi.mocked and beforeEach so there are no
standalone empty triple-backtick fences.
In `@src/platform/updates/common/releaseStore.test.ts`:
- Around line 373-385: The test "should fetch normally when argv is undefined"
(using useReleaseStore, useReleaseService, useSystemStatsStore and mutating
systemStats.system.argv) is flagged as a smell because it violates TypeScript
types; either remove the test if you rely on static types to guarantee argv
presence or convert it into an explicit runtime-safety test: keep it but rename
to indicate it intentionally breaks types (e.g., "handles missing argv at
runtime"), add a comment explaining why we bypass types, and wrap the argv
deletion and expectations in a clearly scoped block so future readers know this
is a deliberate runtime-only guard; update test name and comment or delete the
test accordingly.
In `@src/services/jobOutputCache.test.ts`:
- Around line 17-20: The mock object in the vi.mock call includes an unused
fetchJobDetail mock which should be removed to avoid confusion; update the mock
to only export extractWorkflow (remove the fetchJobDetail property from the
mocked object in jobOutputCache.test.ts) and ensure any imports or references to
fetchJobDetail are removed or updated so the tests only rely on extractWorkflow.
Comment on lines
+317
to
+330
| ```typescript | ||
| // ❌ Don't configure mock return values in beforeEach with shared variable | ||
| let mockService: { doSomething: Mock } | ||
| beforeEach(() => { | ||
| mockService = { doSomething: vi.fn() } | ||
| vi.mocked(useMyComposable).mockReturnValue(mockService) | ||
| }) | ||
|
|
||
| // ❌ Don't auto-mock then override — reactive refs won't work correctly | ||
| vi.mock('@/path/to/composable') | ||
| vi.mocked(useMyComposable).mockReturnValue({ isLoading: ref(false) }) | ||
| ``` | ||
|
|
||
| ``` |
Contributor
There was a problem hiding this comment.
Remove the stray fenced block or add a language tag (MD040).
An empty fenced block without a language spec trips markdownlint.
🧹 Proposed fix
-vi.mocked(useMyComposable).mockReturnValue({ isLoading: ref(false) })
-```
-
-```
+vi.mocked(useMyComposable).mockReturnValue({ isLoading: ref(false) })
+```🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
330-330: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@docs/testing/unit-testing.md` around lines 317 - 330, The markdown contains a
stray empty fenced code block that triggers MD040; remove the empty
triple-backtick block or give it a language tag and ensure the intended snippet
(the vi.mocked(useMyComposable).mockReturnValue({ isLoading: ref(false) }) line)
is inside a properly delimited fenced block (e.g., ```typescript) instead of
leaving an empty block; update the section around the examples referencing
mockService, useMyComposable, vi.mocked and beforeEach so there are no
standalone empty triple-backtick fences.
Comment on lines
373
to
385
| it('should fetch normally when argv is undefined', async () => { | ||
| mockSystemStatsStore.systemStats!.system.argv = undefined | ||
| mockReleaseService.getReleases.mockResolvedValue([mockRelease]) | ||
| const store = useReleaseStore() | ||
| const releaseService = useReleaseService() | ||
| const systemStatsStore = useSystemStatsStore() | ||
| // TODO: Consider deleting this test since the types have to be violated for it to be relevant | ||
| delete (systemStatsStore.systemStats!.system as { argv?: string[] }).argv | ||
| vi.mocked(releaseService.getReleases).mockResolvedValue([mockRelease]) | ||
|
|
||
| await store.initialize() | ||
|
|
||
| expect(mockReleaseService.getReleases).toHaveBeenCalled() | ||
| expect(releaseService.getReleases).toHaveBeenCalled() | ||
| expect(store.releases).toEqual([mockRelease]) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The TODO comment appropriately flags a potential test smell.
The comment at line 377 correctly identifies that this test requires violating TypeScript types to be relevant. Consider whether this edge case is worth testing given the type system should prevent argv from being undefined. The test provides runtime safety validation but may be redundant if types are enforced throughout.
🤖 Prompt for AI Agents
In `@src/platform/updates/common/releaseStore.test.ts` around lines 373 - 385, The
test "should fetch normally when argv is undefined" (using useReleaseStore,
useReleaseService, useSystemStatsStore and mutating systemStats.system.argv) is
flagged as a smell because it violates TypeScript types; either remove the test
if you rely on static types to guarantee argv presence or convert it into an
explicit runtime-safety test: keep it but rename to indicate it intentionally
breaks types (e.g., "handles missing argv at runtime"), add a comment explaining
why we bypass types, and wrap the argv deletion and expectations in a clearly
scoped block so future readers know this is a deliberate runtime-only guard;
update test name and comment or delete the test accordingly.
Comment on lines
17
to
20
| vi.mock('@/platform/remote/comfyui/jobs/fetchJobs', () => ({ | ||
| fetchJobDetail: vi.fn(), | ||
| extractWorkflow: vi.fn() | ||
| })) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unused fetchJobDetail mock.
The fetchJobDetail mock is defined but never used in these tests—only extractWorkflow is imported and tested. Keeping unused mocks adds confusion about what the tests actually depend on.
♻️ Suggested fix
vi.mock('@/platform/remote/comfyui/jobs/fetchJobs', () => ({
- fetchJobDetail: vi.fn(),
extractWorkflow: vi.fn()
}))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| vi.mock('@/platform/remote/comfyui/jobs/fetchJobs', () => ({ | |
| fetchJobDetail: vi.fn(), | |
| extractWorkflow: vi.fn() | |
| })) | |
| vi.mock('@/platform/remote/comfyui/jobs/fetchJobs', () => ({ | |
| extractWorkflow: vi.fn() | |
| })) |
🤖 Prompt for AI Agents
In `@src/services/jobOutputCache.test.ts` around lines 17 - 20, The mock object in
the vi.mock call includes an unused fetchJobDetail mock which should be removed
to avoid confusion; update the mock to only export extractWorkflow (remove the
fetchJobDetail property from the mocked object in jobOutputCache.test.ts) and
ensure any imports or references to fetchJobDetail are removed or updated so the
tests only rely on extractWorkflow.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/platform/updates/common/releaseStore.test.ts`:
- Around line 28-39: The mock creates shared mutable refs (isLoading, error)
that persist between tests; update the mock for useReleaseService to return
fresh refs per test or expose a reset helper on the mock (e.g.,
mockReleaseService.reset) that reinitializes isLoading and error, and call that
reset in beforeEach; specifically change the vi.mock for
'@/platform/updates/common/releaseService' so useReleaseService returns new ref
instances each invocation (or provide a reset function alongside
getReleases/isLoading/error) and invoke that reset in the test suite setup to
mirror the mockSystemStatsState.reset pattern.
Comment on lines
+28
to
+39
| vi.mock('@/platform/updates/common/releaseService', () => { | ||
| const getReleases = vi.fn() | ||
| const isLoading = ref(false) | ||
| const error = ref<string | null>(null) | ||
| return { | ||
| useReleaseService: () => ({ | ||
| getReleases, | ||
| isLoading, | ||
| error | ||
| }) | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Mutable ref state is not reset between tests.
The isLoading and error refs are created once and shared across all tests. Unlike mock functions, Vue refs are not reset by vi.resetAllMocks(). When a test sets releaseService.error.value = 'API Error' (line 390), this value persists into subsequent tests.
Consider adding a reset mechanism similar to mockSystemStatsState.reset():
♻️ Proposed fix
vi.mock('@/platform/updates/common/releaseService', () => {
const getReleases = vi.fn()
const isLoading = ref(false)
const error = ref<string | null>(null)
+ const reset = () => {
+ isLoading.value = false
+ error.value = null
+ }
return {
useReleaseService: () => ({
getReleases,
isLoading,
error
- })
+ }),
+ __resetReleaseServiceMock: reset
}
})Then in beforeEach:
+import { __resetReleaseServiceMock } from '@/platform/updates/common/releaseService'
+
beforeEach(() => {
setActivePinia(createTestingPinia({ stubActions: false }))
vi.resetAllMocks()
mockSystemStatsState.reset()
+ __resetReleaseServiceMock()
})Based on learnings: "Leverage Vitest utilities for mocking where possible and keep module mocks contained without global mutable state".
🤖 Prompt for AI Agents
In `@src/platform/updates/common/releaseStore.test.ts` around lines 28 - 39, The
mock creates shared mutable refs (isLoading, error) that persist between tests;
update the mock for useReleaseService to return fresh refs per test or expose a
reset helper on the mock (e.g., mockReleaseService.reset) that reinitializes
isLoading and error, and call that reset in beforeEach; specifically change the
vi.mock for '@/platform/updates/common/releaseService' so useReleaseService
returns new ref instances each invocation (or provide a reset function alongside
getReleases/isLoading/error) and invoke that reset in the test suite setup to
mirror the mockSystemStatsState.reset pattern.
christian-byrne
approved these changes
Jan 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Simplifies test mocking patterns across multiple test files.
vi.hoisted()calls┆Issue is synchronized with this Notion page by Unito