From b8d9d3ebd6c15e82cbc3ab3ce2d5ca7aaf03d498 Mon Sep 17 00:00:00 2001 From: Glary-Bot Date: Wed, 20 May 2026 19:53:26 +0000 Subject: [PATCH] test: address review feedback on COLOR widget tests - Drop shoehorn in favour of new LGraphNode() + vi.spyOn(), matching the pattern already used by useComboWidget.test.ts. No more 'as unknown as'. - Extract hex literals to named constants. - Add coverage for serialization, custom input name, and the no-default case (asserts widget still produces a usable color widget rather than pinning the specific fallback colour). - Split the e2e into two scenarios per review: one adds the node via the search box (real user flow), one loads a workflow asset that persists the colour value. Both assert against the user-visible hex string on the ColorPicker trigger button instead of the internal widget value. - Note: the FE-800 ticket reference in the describe block was a holdover from the synthetic-node draft; removed. - AGENTS.md addendum documenting the devtools-node pattern for Python-side test fixtures. --- browser_tests/AGENTS.md | 10 ++ .../assets/vueNodes/color-widget-default.json | 28 +++++ .../widgets/color/colorWidget.spec.ts | 44 +++++--- .../composables/useColorWidget.test.ts | 102 ++++++++++-------- 4 files changed, 124 insertions(+), 60 deletions(-) create mode 100644 browser_tests/assets/vueNodes/color-widget-default.json diff --git a/browser_tests/AGENTS.md b/browser_tests/AGENTS.md index 1f68c937f3..f3af86e648 100644 --- a/browser_tests/AGENTS.md +++ b/browser_tests/AGENTS.md @@ -122,6 +122,16 @@ Reserve `toPass()` for blocks with multiple assertions or complex async logic th | Context menu empty or wrong items | Node not selected | Select node first: `vueNodes.selectNode()` or `nodeRef.click('title')` | | `navigateIntoSubgraph` timeout | Node too small in test asset JSON | Use node size `[400, 200]` minimum | +## Backend Test Nodes (DevTools) + +When a test needs a node, input shape, or backend behavior that no production node exposes, prefer **adding a node to `tools/devtools/nodes/`** over intercepting `/object_info` from the test. + +- Add the new class to the appropriate file (e.g. `inputs.py` for input-shape variants), register it in that file's `NODE_CLASS_MAPPINGS` / `NODE_DISPLAY_NAME_MAPPINGS` with a `DevTools` prefix, and re-export it through `nodes/__init__.py` and `dev_nodes.py`. +- In the spec, create the node via `comfyPage.nodeOps.addNode('DevToolsYourNode')` and assert against the rendered widget (see `vueNodes/widgets/legacy.spec.ts` and `vueNodes/widgets/color/colorWidget.spec.ts`). +- Reserve `page.route(/\/object_info$/, ...)` for cases where the test needs to mutate an existing production node's shape mid-flight (e.g. `propertiesPanel/errorsTabMissingModels.spec.ts`). + +Devtools nodes are loaded automatically into the test ComfyUI instance, so no fixture wiring is required. + ## After Making Changes - Run `pnpm typecheck:browser` after modifying TypeScript files in this directory diff --git a/browser_tests/assets/vueNodes/color-widget-default.json b/browser_tests/assets/vueNodes/color-widget-default.json new file mode 100644 index 0000000000..7cc61cbbe4 --- /dev/null +++ b/browser_tests/assets/vueNodes/color-widget-default.json @@ -0,0 +1,28 @@ +{ + "id": "c0107e57-c01047-c01047-c01047-c0101010c010", + "revision": 0, + "last_node_id": 1, + "last_link_id": 0, + "nodes": [ + { + "id": 1, + "type": "DevToolsNodeWithColorInput", + "pos": [400, 300], + "size": [270, 58], + "flags": {}, + "order": 0, + "mode": 0, + "inputs": [], + "outputs": [], + "properties": { + "Node name for S&R": "DevToolsNodeWithColorInput" + }, + "widgets_values": ["#00ff00"] + } + ], + "links": [], + "groups": [], + "config": {}, + "extra": {}, + "version": 0.4 +} diff --git a/browser_tests/tests/vueNodes/widgets/color/colorWidget.spec.ts b/browser_tests/tests/vueNodes/widgets/color/colorWidget.spec.ts index 30cc0bedef..ffe32841c8 100644 --- a/browser_tests/tests/vueNodes/widgets/color/colorWidget.spec.ts +++ b/browser_tests/tests/vueNodes/widgets/color/colorWidget.spec.ts @@ -3,20 +3,34 @@ import { comfyPageFixture as test } from '@e2e/fixtures/ComfyPage' -test.describe( - 'Vue Color Widget defaults (FE-800)', - { tag: '@vue-nodes' }, - () => { - test('respects a non-black default declared on the node input', async ({ - comfyPage - }) => { - const nodeRef = await comfyPage.nodeOps.addNode( - 'DevToolsNodeWithColorInput' - ) - await comfyPage.vueNodes.waitForNodes() +const COLOR_NODE_DISPLAY_NAME = 'Node With Color Input' +const DECLARED_DEFAULT = '#00ff00' - const widget = await nodeRef.getWidgetByName('color_input') - await expect.poll(() => widget.getValue()).toBe('#00ff00') +test.describe('Vue Color Widget defaults', { tag: '@vue-nodes' }, () => { + test('respects the declared default value in the input spec', async ({ + comfyPage + }) => { + await comfyPage.page.mouse.dblclick(200, 200, { delay: 5 }) + await comfyPage.searchBox.fillAndSelectFirstNode(COLOR_NODE_DISPLAY_NAME) + + const node = comfyPage.vueNodes.getNodeByTitle(COLOR_NODE_DISPLAY_NAME) + const colorTrigger = node.getByRole('button', { + name: new RegExp(DECLARED_DEFAULT, 'i') }) - } -) + + await expect(colorTrigger).toBeVisible() + }) + + test('restores a saved color value when loading a workflow', async ({ + comfyPage + }) => { + await comfyPage.workflow.loadWorkflow('vueNodes/color-widget-default') + + const node = comfyPage.vueNodes.getNodeByTitle(COLOR_NODE_DISPLAY_NAME) + const colorTrigger = node.getByRole('button', { + name: new RegExp(DECLARED_DEFAULT, 'i') + }) + + await expect(colorTrigger).toBeVisible() + }) +}) diff --git a/src/renderer/extensions/vueNodes/widgets/composables/useColorWidget.test.ts b/src/renderer/extensions/vueNodes/widgets/composables/useColorWidget.test.ts index 1a66d548a2..4dea9f184c 100644 --- a/src/renderer/extensions/vueNodes/widgets/composables/useColorWidget.test.ts +++ b/src/renderer/extensions/vueNodes/widgets/composables/useColorWidget.test.ts @@ -1,92 +1,104 @@ -import { fromAny } from '@total-typescript/shoehorn' import { describe, expect, it, vi } from 'vitest' -import type { LGraphNode } from '@/lib/litegraph/src/litegraph' -import type { - IBaseWidget, - IColorWidget -} from '@/lib/litegraph/src/types/widgets' +import { LGraphNode } from '@/lib/litegraph/src/litegraph' +import type { IBaseWidget } from '@/lib/litegraph/src/types/widgets' import type { ColorInputSpec } from '@/schemas/nodeDef/nodeDefSchemaV2' import { useColorWidget } from '@/renderer/extensions/vueNodes/widgets/composables/useColorWidget' -function createMockNode() { - const addWidget = vi.fn( - ( - type: string, - name: string, - value: unknown, - _callback: unknown, - options: Record - ): IBaseWidget => +const TOP_LEVEL_DEFAULT = '#00ff00' +const NESTED_DEFAULT = '#abcdef' +const BLACK_FALLBACK = '#000000' + +function createMockNode(): { + node: LGraphNode + addWidget: ReturnType +} { + const node = new LGraphNode('TestColorNode') + const addWidget = vi.spyOn(node, 'addWidget').mockImplementation( + (type, name, value, _callback, options) => ({ type, name, value, - options, + options: typeof options === 'string' ? { property: options } : options, y: 0 - }) as unknown as IBaseWidget + }) as IBaseWidget ) - const node = fromAny({ addWidget }) return { node, addWidget } } +function createColorSpec( + overrides: Partial = {} +): ColorInputSpec { + return { type: 'COLOR', name: 'color', ...overrides } +} + describe('useColorWidget', () => { it('uses the top-level default produced by the V1 → V2 migration', () => { const { node, addWidget } = createMockNode() - const inputSpec = fromAny({ - type: 'COLOR', - name: 'color', - default: '#00ff00' - }) + const inputSpec = createColorSpec({ default: TOP_LEVEL_DEFAULT }) - const widget = useColorWidget()(node, inputSpec) as IColorWidget + const widget = useColorWidget()(node, inputSpec) expect(addWidget).toHaveBeenCalledWith( 'color', 'color', - '#00ff00', + TOP_LEVEL_DEFAULT, expect.any(Function), { serialize: true } ) - expect(widget.value).toBe('#00ff00') + expect(widget.value).toBe(TOP_LEVEL_DEFAULT) }) it('uses options.default when the spec follows the V2 nested shape', () => { const { node, addWidget } = createMockNode() - const inputSpec: ColorInputSpec = { - type: 'COLOR', - name: 'color', - options: { default: '#abcdef' } - } + const inputSpec = createColorSpec({ options: { default: NESTED_DEFAULT } }) useColorWidget()(node, inputSpec) - expect(addWidget.mock.calls[0]![2]).toBe('#abcdef') + expect(addWidget.mock.calls[0]![2]).toBe(NESTED_DEFAULT) }) it('prefers the top-level default when both locations are present', () => { const { node, addWidget } = createMockNode() - const inputSpec = fromAny({ - type: 'COLOR', - name: 'color', - default: '#111111', - options: { default: '#222222' } + const inputSpec = createColorSpec({ + default: TOP_LEVEL_DEFAULT, + options: { default: NESTED_DEFAULT } }) useColorWidget()(node, inputSpec) - expect(addWidget.mock.calls[0]![2]).toBe('#111111') + expect(addWidget.mock.calls[0]![2]).toBe(TOP_LEVEL_DEFAULT) }) - it('falls back to black when no default is provided', () => { + it('still produces a usable widget when no default is supplied', () => { const { node, addWidget } = createMockNode() - const inputSpec: ColorInputSpec = { - type: 'COLOR', - name: 'color' - } + const inputSpec = createColorSpec() - useColorWidget()(node, inputSpec) + const widget = useColorWidget()(node, inputSpec) - expect(addWidget.mock.calls[0]![2]).toBe('#000000') + expect(addWidget).toHaveBeenCalledOnce() + expect(widget.type).toBe('color') + expect(widget.name).toBe('color') + expect(widget.value).toBe(BLACK_FALLBACK) + }) + + it('serializes the widget so its value persists in saved workflows', () => { + const { node, addWidget } = createMockNode() + + useColorWidget()(node, createColorSpec({ default: TOP_LEVEL_DEFAULT })) + + expect(addWidget.mock.calls[0]![4]).toEqual({ serialize: true }) + }) + + it('honours a custom input name from the spec', () => { + const { node, addWidget } = createMockNode() + + useColorWidget()( + node, + createColorSpec({ name: 'bg_color', default: TOP_LEVEL_DEFAULT }) + ) + + expect(addWidget.mock.calls[0]![1]).toBe('bg_color') }) })