From 4c46f5786b93005e0c898d1986384a2a39148b3c Mon Sep 17 00:00:00 2001 From: DrJKL Date: Mon, 12 Jan 2026 18:02:59 -0800 Subject: [PATCH] Address PR feedback: refactor browser tests for better type safety --- .../fixtures/utils/litegraphUtils.ts | 21 +-- browser_tests/fixtures/utils/subgraphUtils.ts | 9 ++ browser_tests/fixtures/utils/taskHistory.ts | 26 ++-- browser_tests/fixtures/utils/workflowUtils.ts | 22 +++ browser_tests/helpers/actionbar.ts | 5 +- browser_tests/tests/browserTabTitle.spec.ts | 29 ++-- browser_tests/tests/colorPalette.spec.ts | 10 +- browser_tests/tests/groupNode.spec.ts | 4 +- .../tests/sidebar/nodeLibrary.spec.ts | 9 +- browser_tests/tests/sidebar/workflows.spec.ts | 4 +- .../tests/subgraph-rename-dialog.spec.ts | 145 ++++++------------ browser_tests/tests/subgraph.spec.ts | 12 +- browser_tests/tests/widget.spec.ts | 4 +- docs/typescript/type-safety.md | 8 +- 14 files changed, 131 insertions(+), 177 deletions(-) create mode 100644 browser_tests/fixtures/utils/workflowUtils.ts diff --git a/browser_tests/fixtures/utils/litegraphUtils.ts b/browser_tests/fixtures/utils/litegraphUtils.ts index a44d61da5..2dbf0ba54 100644 --- a/browser_tests/fixtures/utils/litegraphUtils.ts +++ b/browser_tests/fixtures/utils/litegraphUtils.ts @@ -159,19 +159,6 @@ class NodeSlotReference { const rawPos = node.getConnectionPos(type === 'input', index) const convertedPos = app.canvas.ds.convertOffsetToCanvas(rawPos) - // Debug logging - convert Float64Arrays to regular arrays for visibility - - console.log( - `NodeSlotReference debug for ${type} slot ${index} on node ${id}:`, - { - nodePos: [node.pos[0], node.pos[1]], - nodeSize: [node.size[0], node.size[1]], - rawConnectionPos: [rawPos[0], rawPos[1]], - convertedPos: [convertedPos[0], convertedPos[1]], - currentGraphType: graph.constructor.name - } - ) - return convertedPos }, [this.type, this.node.id, this.index] as const @@ -256,8 +243,8 @@ class NodeWidgetReference { const pos: [number, number] = await this.node.comfyPage.page.evaluate( ([id, index]) => { const app = window.app - if (!app?.graph) throw new Error('App not initialized') - const node = app.graph.getNodeById(id) + if (!app?.canvas?.graph) throw new Error('App not initialized') + const node = app.canvas.graph.getNodeById(id) if (!node) throw new Error(`Node ${id} not found.`) if (!node.widgets) throw new Error(`Node ${id} has no widgets.`) const widget = node.widgets[index] @@ -310,8 +297,8 @@ class NodeWidgetReference { return await this.node.comfyPage.page.evaluate( ([id, index]) => { const app = window.app - if (!app?.graph) throw new Error('App not initialized') - const node = app.graph.getNodeById(id) + if (!app?.canvas?.graph) throw new Error('App not initialized') + const node = app.canvas.graph.getNodeById(id) if (!node) throw new Error(`Node ${id} not found.`) if (!node.widgets) throw new Error(`Node ${id} has no widgets.`) const widget = node.widgets[index] diff --git a/browser_tests/fixtures/utils/subgraphUtils.ts b/browser_tests/fixtures/utils/subgraphUtils.ts index c488d5f08..876fc1c8e 100644 --- a/browser_tests/fixtures/utils/subgraphUtils.ts +++ b/browser_tests/fixtures/utils/subgraphUtils.ts @@ -10,6 +10,15 @@ export interface SubgraphSlot { label?: string name?: string displayName?: string + labelPos?: [number, number] +} + +export interface SubgraphInputNode { + onPointerDown?: (e: unknown, pointer: unknown, linkConnector: unknown) => void +} + +export interface SubgraphGraphWithNodes extends SubgraphGraph { + inputNode?: SubgraphInputNode } /** diff --git a/browser_tests/fixtures/utils/taskHistory.ts b/browser_tests/fixtures/utils/taskHistory.ts index 8476d9fc3..7b60211b3 100644 --- a/browser_tests/fixtures/utils/taskHistory.ts +++ b/browser_tests/fixtures/utils/taskHistory.ts @@ -120,23 +120,15 @@ export default class TaskHistory { filenames: string[], filetype: OutputFileType ): TaskOutput { - return filenames.reduce( - (outputs, filename, i) => { - const nodeId = `${i + 1}` - outputs[nodeId] = { - [filetype]: [{ filename, subfolder: '', type: 'output' }] - } - const contentType = getContentType(filename, filetype) - this.outputContentTypes.set(filename, contentType) - return outputs - }, - {} as Record< - string, - { - [key: string]: { filename: string; subfolder: string; type: string }[] - } - > - ) + return filenames.reduce((outputs, filename, i) => { + const nodeId = `${i + 1}` + outputs[nodeId] = { + [filetype]: [{ filename, subfolder: '', type: 'output' }] + } + const contentType = getContentType(filename, filetype) + this.outputContentTypes.set(filename, contentType) + return outputs + }, {}) } private addTask(task: HistoryTaskItem) { diff --git a/browser_tests/fixtures/utils/workflowUtils.ts b/browser_tests/fixtures/utils/workflowUtils.ts new file mode 100644 index 000000000..dc89bf609 --- /dev/null +++ b/browser_tests/fixtures/utils/workflowUtils.ts @@ -0,0 +1,22 @@ +interface ExtensionManagerWorkflow { + workflow?: { + activeWorkflow?: { + filename?: string + delete?: () => Promise + } + } +} + +interface AppWithExtensionManager { + extensionManager: ExtensionManagerWorkflow +} + +export function getActiveWorkflowFilename(app: unknown): string | undefined { + const extMgr = (app as AppWithExtensionManager).extensionManager + return extMgr.workflow?.activeWorkflow?.filename +} + +export async function deleteActiveWorkflow(app: unknown): Promise { + const extMgr = (app as AppWithExtensionManager).extensionManager + await extMgr.workflow?.activeWorkflow?.delete?.() +} diff --git a/browser_tests/helpers/actionbar.ts b/browser_tests/helpers/actionbar.ts index f48646ae3..783853f36 100644 --- a/browser_tests/helpers/actionbar.ts +++ b/browser_tests/helpers/actionbar.ts @@ -47,9 +47,10 @@ class ComfyQueueButtonOptions { const extMgr = app.extensionManager as { queueSettings?: { mode: string } } - if (extMgr.queueSettings) { - extMgr.queueSettings.mode = mode + if (!extMgr.queueSettings) { + throw new Error('queueSettings not initialized') } + extMgr.queueSettings.mode = mode }, mode) } diff --git a/browser_tests/tests/browserTabTitle.spec.ts b/browser_tests/tests/browserTabTitle.spec.ts index 9faa22f2d..45bd461d1 100644 --- a/browser_tests/tests/browserTabTitle.spec.ts +++ b/browser_tests/tests/browserTabTitle.spec.ts @@ -1,6 +1,10 @@ import { expect } from '@playwright/test' import { comfyPageFixture as test } from '../fixtures/ComfyPage' +import { + deleteActiveWorkflow, + getActiveWorkflowFilename +} from '../fixtures/utils/workflowUtils' test.describe('Browser tab title', () => { test.describe('Beta Menu', () => { @@ -9,14 +13,11 @@ test.describe('Browser tab title', () => { }) test('Can display workflow name', async ({ comfyPage }) => { - const workflowName = await comfyPage.page.evaluate(async () => { + const workflowName = await comfyPage.page.evaluate(() => { const app = window['app'] if (!app) throw new Error('App not initialized') - const extMgr = app.extensionManager as { - workflow?: { activeWorkflow?: { filename?: string } } - } - return extMgr.workflow?.activeWorkflow?.filename - }) + return getActiveWorkflowFilename(app) + }, getActiveWorkflowFilename) expect(await comfyPage.page.title()).toBe(`*${workflowName} - ComfyUI`) }) @@ -25,14 +26,11 @@ test.describe('Browser tab title', () => { test.skip('Can display workflow name with unsaved changes', async ({ comfyPage }) => { - const workflowName = await comfyPage.page.evaluate(async () => { + const workflowName = await comfyPage.page.evaluate(() => { const app = window['app'] if (!app) throw new Error('App not initialized') - const extMgr = app.extensionManager as { - workflow?: { activeWorkflow?: { filename?: string } } - } - return extMgr.workflow?.activeWorkflow?.filename - }) + return getActiveWorkflowFilename(app) + }, getActiveWorkflowFilename) expect(await comfyPage.page.title()).toBe(`${workflowName} - ComfyUI`) await comfyPage.menu.topbar.saveWorkflow('test') @@ -47,11 +45,8 @@ test.describe('Browser tab title', () => { await comfyPage.page.evaluate(async () => { const app = window['app'] if (!app) throw new Error('App not initialized') - const extMgr = app.extensionManager as { - workflow?: { activeWorkflow?: { delete?: () => Promise } } - } - return extMgr.workflow?.activeWorkflow?.delete?.() - }) + await deleteActiveWorkflow(app) + }, deleteActiveWorkflow) }) }) diff --git a/browser_tests/tests/colorPalette.spec.ts b/browser_tests/tests/colorPalette.spec.ts index 11d5c3178..b49ce68aa 100644 --- a/browser_tests/tests/colorPalette.spec.ts +++ b/browser_tests/tests/colorPalette.spec.ts @@ -189,7 +189,15 @@ test.describe('Color Palette', () => { const extMgr = app.extensionManager as { colorPalette?: { addCustomColorPalette?: (p: unknown) => void } } - extMgr.colorPalette?.addCustomColorPalette?.(p) + if (!extMgr.colorPalette) { + throw new Error('colorPalette extension not found on extensionManager') + } + if (!extMgr.colorPalette.addCustomColorPalette) { + throw new Error( + 'addCustomColorPalette method not found on colorPalette extension' + ) + } + extMgr.colorPalette.addCustomColorPalette(p) }, customColorPalettes.obsidian_dark) expect(await comfyPage.getToastErrorCount()).toBe(0) diff --git a/browser_tests/tests/groupNode.spec.ts b/browser_tests/tests/groupNode.spec.ts index 25f22a525..4dad05254 100644 --- a/browser_tests/tests/groupNode.spec.ts +++ b/browser_tests/tests/groupNode.spec.ts @@ -23,9 +23,7 @@ test.describe('Group Node', () => { await libraryTab.open() }) - test('Is added to node library sidebar', async ({ - comfyPage: _comfyPage - }) => { + test('Is added to node library sidebar', async () => { expect(await libraryTab.getFolder('group nodes').count()).toBe(1) }) diff --git a/browser_tests/tests/sidebar/nodeLibrary.spec.ts b/browser_tests/tests/sidebar/nodeLibrary.spec.ts index 62f1a46c6..b4fcc3eea 100644 --- a/browser_tests/tests/sidebar/nodeLibrary.spec.ts +++ b/browser_tests/tests/sidebar/nodeLibrary.spec.ts @@ -268,10 +268,11 @@ test.describe('Node library sidebar', () => { const setting = (await comfyPage.getSetting( 'Comfy.NodeLibrary.BookmarksCustomization' )) as Record | undefined - await expect(setting).toHaveProperty(['foo/', 'color']) - await expect(setting?.['foo/'].color).not.toBeNull() - await expect(setting?.['foo/'].color).not.toBeUndefined() - await expect(setting?.['foo/'].color).not.toBe('') + expect(setting).toHaveProperty(['foo/', 'color']) + expect(setting?.['foo/']).toBeDefined() + expect(setting?.['foo/']?.color).not.toBeNull() + expect(setting?.['foo/']?.color).not.toBeUndefined() + expect(setting?.['foo/']?.color).not.toBe('') }) test('Can rename customized bookmark folder', async ({ comfyPage }) => { diff --git a/browser_tests/tests/sidebar/workflows.spec.ts b/browser_tests/tests/sidebar/workflows.spec.ts index d5bb16d5c..09c9bc53e 100644 --- a/browser_tests/tests/sidebar/workflows.spec.ts +++ b/browser_tests/tests/sidebar/workflows.spec.ts @@ -182,7 +182,9 @@ test.describe('Workflows sidebar', () => { // Compare the exported workflow with the original expect(downloadedContent).toBeDefined() expect(downloadedContentZh).toBeDefined() - if (!downloadedContent || !downloadedContentZh) return + if (!downloadedContent || !downloadedContentZh) { + throw new Error('Downloaded workflow content is undefined') + } delete downloadedContent.id delete downloadedContentZh.id expect(downloadedContent).toEqual(downloadedContentZh) diff --git a/browser_tests/tests/subgraph-rename-dialog.spec.ts b/browser_tests/tests/subgraph-rename-dialog.spec.ts index 277aab7e1..fc57a9810 100644 --- a/browser_tests/tests/subgraph-rename-dialog.spec.ts +++ b/browser_tests/tests/subgraph-rename-dialog.spec.ts @@ -11,6 +11,39 @@ const SELECTORS = { promptDialog: '.graphdialog input' } as const +interface SubgraphSlot { + label?: string + name?: string + displayName?: string +} + +interface SubgraphGraph { + inputs: SubgraphSlot[] + outputs: SubgraphSlot[] +} + +function getSubgraph(): SubgraphGraph { + const assertSubgraph = (graph: unknown): asserts graph is SubgraphGraph => { + if ( + graph === null || + typeof graph !== 'object' || + !('inputs' in graph) || + !('outputs' in graph) || + !Array.isArray((graph as { inputs: unknown }).inputs) || + !Array.isArray((graph as { outputs: unknown }).outputs) + ) { + throw new Error('Not in subgraph') + } + } + const app = window['app'] + if (!app) throw new Error('App not available') + const canvas = app.canvas + if (!canvas) throw new Error('Canvas not available') + const graph = canvas.graph + assertSubgraph(graph) + return graph +} + test.describe('Subgraph Slot Rename Dialog', () => { test.beforeEach(async ({ comfyPage }) => { await comfyPage.setSetting('Comfy.UseNewMenu', 'Disabled') @@ -25,32 +58,10 @@ test.describe('Subgraph Slot Rename Dialog', () => { await subgraphNode.navigateIntoSubgraph() // Get initial slot label - const initialInputLabel = await comfyPage.page.evaluate(() => { - const assertSubgraph = ( - graph: unknown - ): asserts graph is { - inputs: { label?: string; name?: string }[] - outputs: unknown[] - } => { - if ( - graph === null || - typeof graph !== 'object' || - !('inputs' in graph) || - !('outputs' in graph) || - !Array.isArray((graph as { inputs: unknown }).inputs) || - !Array.isArray((graph as { outputs: unknown }).outputs) - ) { - throw new Error('Not in subgraph') - } - } - const app = window['app'] - if (!app) throw new Error('App not available') - const canvas = app.canvas - if (!canvas) throw new Error('Canvas not available') - const graph = canvas.graph - assertSubgraph(graph) + const initialInputLabel = await comfyPage.page.evaluate((getSubgraph) => { + const graph = getSubgraph() return graph.inputs[0]?.label || graph.inputs[0]?.name || null - }) + }, getSubgraph) // First rename await comfyPage.rightClickSubgraphInputSlot(initialInputLabel ?? undefined) @@ -75,37 +86,15 @@ test.describe('Subgraph Slot Rename Dialog', () => { await comfyPage.nextFrame() // Verify the rename worked - const afterFirstRename = await comfyPage.page.evaluate(() => { - const assertSubgraph = ( - graph: unknown - ): asserts graph is { - inputs: { label?: string; name?: string; displayName?: string }[] - outputs: unknown[] - } => { - if ( - graph === null || - typeof graph !== 'object' || - !('inputs' in graph) || - !('outputs' in graph) || - !Array.isArray((graph as { inputs: unknown }).inputs) || - !Array.isArray((graph as { outputs: unknown }).outputs) - ) { - throw new Error('Not in subgraph') - } - } - const app = window['app'] - if (!app) throw new Error('App not available') - const canvas = app.canvas - if (!canvas) throw new Error('Canvas not available') - const graph = canvas.graph - assertSubgraph(graph) + const afterFirstRename = await comfyPage.page.evaluate((getSubgraph) => { + const graph = getSubgraph() const slot = graph.inputs[0] return { label: slot?.label || null, name: slot?.name || null, displayName: slot?.displayName || slot?.label || slot?.name || null } - }) + }, getSubgraph) expect(afterFirstRename.label).toBe(RENAMED_NAME) // Now rename again - this is where the bug would show @@ -139,32 +128,10 @@ test.describe('Subgraph Slot Rename Dialog', () => { await comfyPage.nextFrame() // Verify the second rename worked - const afterSecondRename = await comfyPage.page.evaluate(() => { - const assertSubgraph = ( - graph: unknown - ): asserts graph is { - inputs: { label?: string }[] - outputs: unknown[] - } => { - if ( - graph === null || - typeof graph !== 'object' || - !('inputs' in graph) || - !('outputs' in graph) || - !Array.isArray((graph as { inputs: unknown }).inputs) || - !Array.isArray((graph as { outputs: unknown }).outputs) - ) { - throw new Error('Not in subgraph') - } - } - const app = window['app'] - if (!app) throw new Error('App not available') - const canvas = app.canvas - if (!canvas) throw new Error('Canvas not available') - const graph = canvas.graph - assertSubgraph(graph) + const afterSecondRename = await comfyPage.page.evaluate((getSubgraph) => { + const graph = getSubgraph() return graph.inputs[0]?.label || null - }) + }, getSubgraph) expect(afterSecondRename).toBe(SECOND_RENAMED_NAME) }) @@ -177,32 +144,10 @@ test.describe('Subgraph Slot Rename Dialog', () => { await subgraphNode.navigateIntoSubgraph() // Get initial output slot label - const initialOutputLabel = await comfyPage.page.evaluate(() => { - const assertSubgraph = ( - graph: unknown - ): asserts graph is { - inputs: unknown[] - outputs: { label?: string; name?: string }[] - } => { - if ( - graph === null || - typeof graph !== 'object' || - !('inputs' in graph) || - !('outputs' in graph) || - !Array.isArray((graph as { inputs: unknown }).inputs) || - !Array.isArray((graph as { outputs: unknown }).outputs) - ) { - throw new Error('Not in subgraph') - } - } - const app = window['app'] - if (!app) throw new Error('App not available') - const canvas = app.canvas - if (!canvas) throw new Error('Canvas not available') - const graph = canvas.graph - assertSubgraph(graph) + const initialOutputLabel = await comfyPage.page.evaluate((getSubgraph) => { + const graph = getSubgraph() return graph.outputs[0]?.label || graph.outputs[0]?.name || null - }) + }, getSubgraph) // First rename await comfyPage.rightClickSubgraphOutputSlot( diff --git a/browser_tests/tests/subgraph.spec.ts b/browser_tests/tests/subgraph.spec.ts index 419a19b86..8ae3580a7 100644 --- a/browser_tests/tests/subgraph.spec.ts +++ b/browser_tests/tests/subgraph.spec.ts @@ -1,6 +1,7 @@ import { expect } from '@playwright/test' import { comfyPageFixture as test } from '../fixtures/ComfyPage' +import type { SubgraphGraphWithNodes } from '../fixtures/utils/subgraphUtils' // Constants const RENAMED_INPUT_NAME = 'renamed_input' @@ -322,16 +323,7 @@ test.describe('Subgraph Operations', () => { await comfyPage.page.evaluate(() => { const app = window['app'] if (!app) throw new Error('App not initialized') - const graph = app.canvas.graph as { - inputs?: { label?: string; labelPos?: [number, number] }[] - inputNode?: { - onPointerDown?: ( - e: unknown, - pointer: unknown, - linkConnector: unknown - ) => void - } - } | null + const graph = app.canvas.graph as SubgraphGraphWithNodes | null if (!graph || !('inputs' in graph)) { throw new Error('Not in a subgraph') } diff --git a/browser_tests/tests/widget.spec.ts b/browser_tests/tests/widget.spec.ts index 6d461e79f..84dce7bc3 100644 --- a/browser_tests/tests/widget.spec.ts +++ b/browser_tests/tests/widget.spec.ts @@ -109,7 +109,9 @@ test.describe('Slider widget', () => { await comfyPage.page.evaluate(() => { const app = window['app'] - if (!app?.graph?.nodes?.[0]?.widgets?.[0]) return + if (!app?.graph?.nodes?.[0]?.widgets?.[0]) { + throw new Error('widget not found') + } const widget = app.graph.nodes[0].widgets[0] widget.callback = (value: number) => { window.widgetValue = value diff --git a/docs/typescript/type-safety.md b/docs/typescript/type-safety.md index 1f8653fcf..3ebf840d6 100644 --- a/docs/typescript/type-safety.md +++ b/docs/typescript/type-safety.md @@ -203,9 +203,9 @@ const config: Config = { ## Prefer Interfaces Over Type Aliases for Objects -Use interfaces for object types - they have better error messages and performance: +Use interfaces for object types. While modern TypeScript has narrowed performance differences between interfaces and type aliases, interfaces still offer clearer error messages and support declaration merging: -❌ **Wrong:** +❌ **Avoid for object types:** ```typescript type NodeConfig = { @@ -214,7 +214,7 @@ type NodeConfig = { } ``` -✅ **Correct:** +✅ **Preferred:** ```typescript interface NodeConfig { @@ -385,7 +385,7 @@ declare function fn(arg: T): T ## Summary -1. **Never use `as`** outside of custom type guards +1. **Never use `as`** outside of custom type guards (exception: test files may use `as Partial as T` for partial mocks) 2. **Use `instanceof`** for class/DOM element checks 3. **Use `'prop' in obj`** for property existence checks 4. **Use `typeof`** for primitive type checks