fix: address review findings for subgraph test utils

- Return disposer from collectConsoleWarnings to prevent listener leaks - Unify slot type params to singular 'input'|'output' across SubgraphHelper - Replace || with ?? in getSlotCount and getSlotLabel for correctness - Remove dead assertSubgraph export and unused imports - Migrate subgraph-duplicate-ids.spec.ts to shared isInSubgraph helper - Add trust-boundary comment on serializeAndReload cast - Use early-throw narrowing in expectWidgetBelowHeader Amp-Thread-ID: https://ampcode.com/threads/T-019d311b-133d-71a8-87bb-e628ea82f90c Co-authored-by: Amp <amp@ampcode.com>
2026-04-20 14:30:41 +00:00 · 2026-03-27 14:42:58 -07:00
parent 1c181d6c3a
commit a4ce1a8e37
7 changed files with 41 additions and 61 deletions
--- a/browser_tests/fixtures/helpers/SubgraphHelper.ts
+++ b/browser_tests/fixtures/helpers/SubgraphHelper.ts
@@ -414,24 +414,24 @@ export class SubgraphHelper {
    })
  }

-  async getSlotCount(type: 'inputs' | 'outputs'): Promise<number> {
-    return this.page.evaluate((slotType: 'inputs' | 'outputs') => {
+  async getSlotCount(type: 'input' | 'output'): Promise<number> {
+    return this.page.evaluate((slotType: 'input' | 'output') => {
      const graph = window.app!.canvas.graph
      if (!graph || !('inputNode' in graph)) return 0
-      return graph[slotType]?.length || 0
+      return graph[`${slotType}s`]?.length ?? 0
    }, type)
  }

  async getSlotLabel(
-    type: 'inputs' | 'outputs',
+    type: 'input' | 'output',
    index = 0
  ): Promise<string | null> {
    return this.page.evaluate(
      ([slotType, idx]) => {
        const graph = window.app!.canvas.graph
        if (!graph || !('inputNode' in graph)) return null
-        const slot = graph[slotType]?.[idx]
-        return slot?.label || slot?.name || null
+        const slot = graph[`${slotType}s`]?.[idx]
+        return slot?.label ?? slot?.name ?? null
      },
      [type, index] as const
    )
--- a/browser_tests/helpers/subgraphTestUtils.ts
+++ b/browser_tests/helpers/subgraphTestUtils.ts
@@ -1,27 +1,11 @@
 import { expect } from '@playwright/test'
-import type { Locator, Page } from '@playwright/test'
+import type { ConsoleMessage, Locator, Page } from '@playwright/test'

 import type { ComfyWorkflowJSON } from '@/platform/workflow/validation/schemas/workflowSchema'

-import type { LGraph, Subgraph } from '../../src/lib/litegraph/src/litegraph'
-import { isSubgraph } from '../../src/utils/typeGuardUtil'
 import type { ComfyPage } from '../fixtures/ComfyPage'
 import type { NodeReference } from '../fixtures/utils/litegraphUtils'

-/**
- * Assertion helper for tests where being in a subgraph is a precondition.
- * Throws a clear error if the graph is not a Subgraph.
- */
-export function assertSubgraph(
-  graph: LGraph | Subgraph | null | undefined
-): asserts graph is Subgraph {
-  if (!isSubgraph(graph)) {
-    throw new Error(
-      'Expected to be in a subgraph context, but graph is not a Subgraph'
-    )
-  }
-}
-
 /**
 * Returns the widget-input slot Y position and the node title height
 * for the promoted "text" input on the SubgraphNode.
@@ -50,6 +34,7 @@ export function getTextSlotPosition(page: Page, nodeId: string) {
 }

 export async function serializeAndReload(comfyPage: ComfyPage): Promise<void> {
+  // serialize() returns ComfyWorkflowJSON; Playwright JSON-serializes it across the boundary
  const serialized = await comfyPage.page.evaluate(() =>
    window.app!.graph!.serialize()
  )
@@ -79,9 +64,9 @@ export async function expectWidgetBelowHeader(
    .locator('[data-testid^="node-header-"]')
    .boundingBox()
  const widgetBox = await widgetLocator.boundingBox()
-  expect(headerBox).not.toBeNull()
-  expect(widgetBox).not.toBeNull()
-  expect(widgetBox!.y).toBeGreaterThan(headerBox!.y + headerBox!.height)
+  if (!headerBox || !widgetBox)
+    throw new Error('Header or widget bounding box not found')
+  expect(widgetBox.y).toBeGreaterThan(headerBox.y + headerBox.height)
 }

 export async function packAllInteriorNodes(
@@ -111,13 +96,14 @@ export function collectConsoleWarnings(
    'Failed to resolve legacy -1',
    'No inner link found'
  ]
-): string[] {
+): { warnings: string[]; dispose: () => void } {
  const warnings: string[] = []
-  page.on('console', (msg) => {
+  const handler = (msg: ConsoleMessage) => {
    const text = msg.text()
    if (patterns.some((p) => text.includes(p))) {
      warnings.push(text)
    }
-  })
-  return warnings
+  }
+  page.on('console', handler)
+  return { warnings, dispose: () => page.off('console', handler) }
 }
--- a/browser_tests/tests/subgraph-duplicate-ids.spec.ts
+++ b/browser_tests/tests/subgraph-duplicate-ids.spec.ts
@@ -110,16 +110,11 @@ test.describe('Subgraph duplicate ID remapping', { tag: ['@subgraph'] }, () => {
    const subgraphNode = await comfyPage.nodeOps.getNodeRefById('5')
    await subgraphNode.navigateIntoSubgraph()

-    const isInSubgraph = () =>
-      comfyPage.page.evaluate(
-        () => window.app!.canvas.graph?.isRootGraph === false
-      )
-
-    expect(await isInSubgraph()).toBe(true)
+    expect(await comfyPage.subgraph.isInSubgraph()).toBe(true)

    await comfyPage.page.keyboard.press('Escape')
    await comfyPage.nextFrame()

-    expect(await isInSubgraph()).toBe(false)
+    expect(await comfyPage.subgraph.isInSubgraph()).toBe(false)
  })
 })
--- a/browser_tests/tests/subgraph-rename-dialog.spec.ts
+++ b/browser_tests/tests/subgraph-rename-dialog.spec.ts
@@ -25,7 +25,7 @@ test.describe('Subgraph Slot Rename Dialog', { tag: '@subgraph' }, () => {
    await subgraphNode.navigateIntoSubgraph()

    // Get initial slot label
-    const initialInputLabel = await comfyPage.subgraph.getSlotLabel('inputs')
+    const initialInputLabel = await comfyPage.subgraph.getSlotLabel('input')

    if (initialInputLabel === null) {
      throw new Error(
@@ -102,7 +102,7 @@ test.describe('Subgraph Slot Rename Dialog', { tag: '@subgraph' }, () => {
    await comfyPage.nextFrame()

    // Verify the second rename worked
-    const afterSecondRename = await comfyPage.subgraph.getSlotLabel('inputs')
+    const afterSecondRename = await comfyPage.subgraph.getSlotLabel('input')
    expect(afterSecondRename).toBe(SECOND_RENAMED_NAME)
  })

@@ -115,7 +115,7 @@ test.describe('Subgraph Slot Rename Dialog', { tag: '@subgraph' }, () => {
    await subgraphNode.navigateIntoSubgraph()

    // Get initial output slot label
-    const initialOutputLabel = await comfyPage.subgraph.getSlotLabel('outputs')
+    const initialOutputLabel = await comfyPage.subgraph.getSlotLabel('output')

    if (initialOutputLabel === null) {
      throw new Error(
--- a/browser_tests/tests/subgraph.spec.ts
+++ b/browser_tests/tests/subgraph.spec.ts
@@ -33,7 +33,7 @@ test.describe('Subgraph Operations', { tag: ['@slow', '@subgraph'] }, () => {
      const subgraphNode = await comfyPage.nodeOps.getNodeRefById('2')
      await subgraphNode.navigateIntoSubgraph()

-      const initialCount = await comfyPage.subgraph.getSlotCount('inputs')
+      const initialCount = await comfyPage.subgraph.getSlotCount('input')
      const [vaeEncodeNode] = await comfyPage.nodeOps.getNodeRefsByType(
        'VAEEncode',
        true
@@ -42,7 +42,7 @@ test.describe('Subgraph Operations', { tag: ['@slow', '@subgraph'] }, () => {
      await comfyPage.subgraph.connectFromInput(vaeEncodeNode, 0)
      await comfyPage.nextFrame()

-      const finalCount = await comfyPage.subgraph.getSlotCount('inputs')
+      const finalCount = await comfyPage.subgraph.getSlotCount('input')
      expect(finalCount).toBe(initialCount + 1)
    })

@@ -52,7 +52,7 @@ test.describe('Subgraph Operations', { tag: ['@slow', '@subgraph'] }, () => {
      const subgraphNode = await comfyPage.nodeOps.getNodeRefById('2')
      await subgraphNode.navigateIntoSubgraph()

-      const initialCount = await comfyPage.subgraph.getSlotCount('outputs')
+      const initialCount = await comfyPage.subgraph.getSlotCount('output')
      const [vaeEncodeNode] = await comfyPage.nodeOps.getNodeRefsByType(
        'VAEEncode',
        true
@@ -61,7 +61,7 @@ test.describe('Subgraph Operations', { tag: ['@slow', '@subgraph'] }, () => {
      await comfyPage.subgraph.connectToOutput(vaeEncodeNode, 0)
      await comfyPage.nextFrame()

-      const finalCount = await comfyPage.subgraph.getSlotCount('outputs')
+      const finalCount = await comfyPage.subgraph.getSlotCount('output')
      expect(finalCount).toBe(initialCount + 1)
    })

@@ -71,7 +71,7 @@ test.describe('Subgraph Operations', { tag: ['@slow', '@subgraph'] }, () => {
      const subgraphNode = await comfyPage.nodeOps.getNodeRefById('2')
      await subgraphNode.navigateIntoSubgraph()

-      const initialCount = await comfyPage.subgraph.getSlotCount('inputs')
+      const initialCount = await comfyPage.subgraph.getSlotCount('input')
      expect(initialCount).toBeGreaterThan(0)

      await comfyPage.subgraph.removeSlot('input')
@@ -80,7 +80,7 @@ test.describe('Subgraph Operations', { tag: ['@slow', '@subgraph'] }, () => {
      await comfyPage.canvas.click({ position: { x: 100, y: 100 } })
      await comfyPage.nextFrame()

-      const finalCount = await comfyPage.subgraph.getSlotCount('inputs')
+      const finalCount = await comfyPage.subgraph.getSlotCount('input')
      expect(finalCount).toBe(initialCount - 1)
    })

@@ -90,7 +90,7 @@ test.describe('Subgraph Operations', { tag: ['@slow', '@subgraph'] }, () => {
      const subgraphNode = await comfyPage.nodeOps.getNodeRefById('2')
      await subgraphNode.navigateIntoSubgraph()

-      const initialCount = await comfyPage.subgraph.getSlotCount('outputs')
+      const initialCount = await comfyPage.subgraph.getSlotCount('output')
      expect(initialCount).toBeGreaterThan(0)

      await comfyPage.subgraph.removeSlot('output')
@@ -99,7 +99,7 @@ test.describe('Subgraph Operations', { tag: ['@slow', '@subgraph'] }, () => {
      await comfyPage.canvas.click({ position: { x: 100, y: 100 } })
      await comfyPage.nextFrame()

-      const finalCount = await comfyPage.subgraph.getSlotCount('outputs')
+      const finalCount = await comfyPage.subgraph.getSlotCount('output')
      expect(finalCount).toBe(initialCount - 1)
    })

@@ -109,7 +109,7 @@ test.describe('Subgraph Operations', { tag: ['@slow', '@subgraph'] }, () => {
      const subgraphNode = await comfyPage.nodeOps.getNodeRefById('2')
      await subgraphNode.navigateIntoSubgraph()

-      const initialInputLabel = await comfyPage.subgraph.getSlotLabel('inputs')
+      const initialInputLabel = await comfyPage.subgraph.getSlotLabel('input')

      await comfyPage.subgraph.rightClickInputSlot(initialInputLabel!)
      await comfyPage.contextMenu.clickLitegraphMenuItem('Rename Slot')
@@ -125,7 +125,7 @@ test.describe('Subgraph Operations', { tag: ['@slow', '@subgraph'] }, () => {
      await comfyPage.canvas.click({ position: { x: 100, y: 100 } })
      await comfyPage.nextFrame()

-      const newInputName = await comfyPage.subgraph.getSlotLabel('inputs')
+      const newInputName = await comfyPage.subgraph.getSlotLabel('input')

      expect(newInputName).toBe(RENAMED_INPUT_NAME)
      expect(newInputName).not.toBe(initialInputLabel)
@@ -137,7 +137,7 @@ test.describe('Subgraph Operations', { tag: ['@slow', '@subgraph'] }, () => {
      const subgraphNode = await comfyPage.nodeOps.getNodeRefById('2')
      await subgraphNode.navigateIntoSubgraph()

-      const initialInputLabel = await comfyPage.subgraph.getSlotLabel('inputs')
+      const initialInputLabel = await comfyPage.subgraph.getSlotLabel('input')

      await comfyPage.subgraph.doubleClickInputSlot(initialInputLabel!)

@@ -151,7 +151,7 @@ test.describe('Subgraph Operations', { tag: ['@slow', '@subgraph'] }, () => {
      await comfyPage.canvas.click({ position: { x: 100, y: 100 } })
      await comfyPage.nextFrame()

-      const newInputName = await comfyPage.subgraph.getSlotLabel('inputs')
+      const newInputName = await comfyPage.subgraph.getSlotLabel('input')

      expect(newInputName).toBe(RENAMED_INPUT_NAME)
      expect(newInputName).not.toBe(initialInputLabel)
@@ -163,8 +163,7 @@ test.describe('Subgraph Operations', { tag: ['@slow', '@subgraph'] }, () => {
      const subgraphNode = await comfyPage.nodeOps.getNodeRefById('2')
      await subgraphNode.navigateIntoSubgraph()

-      const initialOutputLabel =
-        await comfyPage.subgraph.getSlotLabel('outputs')
+      const initialOutputLabel = await comfyPage.subgraph.getSlotLabel('output')

      await comfyPage.subgraph.doubleClickOutputSlot(initialOutputLabel!)

@@ -179,7 +178,7 @@ test.describe('Subgraph Operations', { tag: ['@slow', '@subgraph'] }, () => {
      await comfyPage.canvas.click({ position: { x: 100, y: 100 } })
      await comfyPage.nextFrame()

-      const newOutputName = await comfyPage.subgraph.getSlotLabel('outputs')
+      const newOutputName = await comfyPage.subgraph.getSlotLabel('output')

      expect(newOutputName).toBe(renamedOutputName)
      expect(newOutputName).not.toBe(initialOutputLabel)
@@ -193,7 +192,7 @@ test.describe('Subgraph Operations', { tag: ['@slow', '@subgraph'] }, () => {
      const subgraphNode = await comfyPage.nodeOps.getNodeRefById('2')
      await subgraphNode.navigateIntoSubgraph()

-      const initialInputLabel = await comfyPage.subgraph.getSlotLabel('inputs')
+      const initialInputLabel = await comfyPage.subgraph.getSlotLabel('input')

      // Test that right-click still works for renaming
      await comfyPage.subgraph.rightClickInputSlot(initialInputLabel!)
@@ -211,7 +210,7 @@ test.describe('Subgraph Operations', { tag: ['@slow', '@subgraph'] }, () => {
      await comfyPage.canvas.click({ position: { x: 100, y: 100 } })
      await comfyPage.nextFrame()

-      const newInputName = await comfyPage.subgraph.getSlotLabel('inputs')
+      const newInputName = await comfyPage.subgraph.getSlotLabel('input')

      expect(newInputName).toBe(rightClickRenamedName)
      expect(newInputName).not.toBe(initialInputLabel)
@@ -225,7 +224,7 @@ test.describe('Subgraph Operations', { tag: ['@slow', '@subgraph'] }, () => {
      const subgraphNode = await comfyPage.nodeOps.getNodeRefById('2')
      await subgraphNode.navigateIntoSubgraph()

-      const initialInputLabel = await comfyPage.subgraph.getSlotLabel('inputs')
+      const initialInputLabel = await comfyPage.subgraph.getSlotLabel('input')

      // Use direct pointer event approach to double-click on label
      await comfyPage.page.evaluate(() => {
@@ -283,7 +282,7 @@ test.describe('Subgraph Operations', { tag: ['@slow', '@subgraph'] }, () => {
      await comfyPage.canvas.click({ position: { x: 100, y: 100 } })
      await comfyPage.nextFrame()

-      const newInputName = await comfyPage.subgraph.getSlotLabel('inputs')
+      const newInputName = await comfyPage.subgraph.getSlotLabel('input')

      expect(newInputName).toBe(labelClickRenamedName)
      expect(newInputName).not.toBe(initialInputLabel)
--- a/browser_tests/tests/subgraphLegacyPrefixedProxyWidgets.spec.ts
+++ b/browser_tests/tests/subgraphLegacyPrefixedProxyWidgets.spec.ts
@@ -31,7 +31,7 @@ test.describe(
    test('Loads without console warnings about failed widget resolution', async ({
      comfyPage
    }) => {
-      const warnings = collectConsoleWarnings(comfyPage.page)
+      const { warnings } = collectConsoleWarnings(comfyPage.page)

      await comfyPage.workflow.loadWorkflow(WORKFLOW)

--- a/browser_tests/tests/subgraphNestedConfigureOrder.spec.ts
+++ b/browser_tests/tests/subgraphNestedConfigureOrder.spec.ts
@@ -9,7 +9,7 @@ test.describe('Nested subgraph configure order', { tag: ['@subgraph'] }, () => {
  test('Loads without "No link found" or "Failed to resolve legacy -1" console warnings', async ({
    comfyPage
  }) => {
-    const warnings = collectConsoleWarnings(comfyPage.page)
+    const { warnings } = collectConsoleWarnings(comfyPage.page)

    await comfyPage.workflow.loadWorkflow(WORKFLOW)