Flaky tests and observable state (#1526)

* Fix missing await * Fix flaky tests - keyboard combos Old code is causing playwright &/ changeTracker to add an undo step. Using combo mode resolves flakiness until that can be investigated thoroughly. * Restore skipped tests * Fix flaky tests * Async clean up * Fix test always fails on retry * Add TS types (tests) * Fix flaky test * Add observable busy state to workflow store * Add workflow store busy wait to tests * Rename test for clarity * Fix flaky tests - use press() from locator API Ref: https://playwright.dev/docs/api/class-keyboard#keyboard-press * Fix flaky test - wait next frame * Add delay between mouse events Litegraph pointer handling is all custom coded, so a adding a delay between events for a bit of reality is actually beneficial.
2026-02-20 14:54:12 +00:00 · 2024-11-14 01:35:22 +11:00
parent ddab149f16
commit 7e0d1d441d
10 changed files with 140 additions and 98 deletions
--- a/browser_tests/changeTracker.spec.ts
+++ b/browser_tests/changeTracker.spec.ts
@@ -3,6 +3,9 @@ import {
  comfyPageFixture as test,
  comfyExpect as expect
 } from './fixtures/ComfyPage'
+import type { useWorkspaceStore } from '../src/stores/workspaceStore'
+
+type WorkspaceStore = ReturnType<typeof useWorkspaceStore>

 async function beforeChange(comfyPage: ComfyPage) {
  await comfyPage.page.evaluate(() => {
@@ -19,38 +22,40 @@ test.describe('Change Tracker', () => {
  test.describe('Undo/Redo', () => {
    test.beforeEach(async ({ comfyPage }) => {
      await comfyPage.setSetting('Comfy.UseNewMenu', 'Top')
+      await comfyPage.setupWorkflowsDirectory({})
    })

    test('Can undo multiple operations', async ({ comfyPage }) => {
      function isModified() {
        return comfyPage.page.evaluate(async () => {
-          return window['app'].extensionManager.workflow.activeWorkflow
-            .isModified
+          return !!(window['app'].extensionManager as WorkspaceStore).workflow
+            .activeWorkflow?.isModified
        })
      }

      function getUndoQueueSize() {
        return comfyPage.page.evaluate(() => {
-          const workflow =
-            window['app'].extensionManager.workflow.activeWorkflow
-          return workflow.changeTracker.undoQueue.length
+          const workflow = (window['app'].extensionManager as WorkspaceStore)
+            .workflow.activeWorkflow
+          return workflow?.changeTracker.undoQueue.length
        })
      }

      function getRedoQueueSize() {
        return comfyPage.page.evaluate(() => {
-          const workflow =
-            window['app'].extensionManager.workflow.activeWorkflow
-          return workflow.changeTracker.redoQueue.length
+          const workflow = (window['app'].extensionManager as WorkspaceStore)
+            .workflow.activeWorkflow
+          return workflow?.changeTracker.redoQueue.length
        })
      }
      expect(await getUndoQueueSize()).toBe(0)
      expect(await getRedoQueueSize()).toBe(0)

+      // Save, confirm no errors & workflow modified flag removed
      await comfyPage.menu.topbar.saveWorkflow('undo-redo-test')
-      await comfyPage.page.waitForFunction(
-        () => !window['app'].extensionManager.workflow.activeWorkflow.isModified
-      )
+      expect(await comfyPage.getToastErrorCount()).toBe(0)
+      expect(await isModified()).toBe(false)
+
      // TODO(huchenlei): Investigate why saving the workflow is causing the
      // undo queue to be triggered.
      expect(await getUndoQueueSize()).toBe(1)
@@ -75,13 +80,11 @@ test.describe('Change Tracker', () => {
      expect(await getUndoQueueSize()).toBe(2)
      expect(await getRedoQueueSize()).toBe(1)

-      // TODO(huchenlei): Following assertion is flaky.
-      // Seems like ctrlZ() is not triggered correctly.
-      // await comfyPage.ctrlZ()
-      // await expect(node).not.toBeCollapsed()
-      // expect(await isModified()).toBe(false)
-      // expect(await getUndoQueueSize()).toBe(1)
-      // expect(await getRedoQueueSize()).toBe(2)
+      await comfyPage.ctrlZ()
+      await expect(node).not.toBeCollapsed()
+      expect(await isModified()).toBe(false)
+      expect(await getUndoQueueSize()).toBe(1)
+      expect(await getRedoQueueSize()).toBe(2)
    })
  })

@@ -109,7 +112,7 @@ test.describe('Change Tracker', () => {
    await expect(node).not.toBeCollapsed()

    // Run again, but within a change transaction
-    beforeChange(comfyPage)
+    await beforeChange(comfyPage)

    await node.click('collapse')
    await comfyPage.ctrlB()
@@ -117,7 +120,7 @@ test.describe('Change Tracker', () => {
    await expect(node).toBeBypassed()

    // End transaction
-    afterChange(comfyPage)
+    await afterChange(comfyPage)

    // Ensure undo reverts both changes
    await comfyPage.ctrlZ()
@@ -125,7 +128,7 @@ test.describe('Change Tracker', () => {
    await expect(node).not.toBeCollapsed()
  })

-  test('Can group multiple transaction calls into a single one', async ({
+  test('Can nest multiple change transactions without adding undo steps', async ({
    comfyPage
  }) => {
    const node = (await comfyPage.getFirstNodeRef())!
--- a/browser_tests/copyPaste.spec.ts
+++ b/browser_tests/copyPaste.spec.ts
@@ -15,9 +15,9 @@ test.describe('Copy Paste', () => {
    await textBox.click()
    const originalString = await textBox.inputValue()
    await textBox.selectText()
-    await comfyPage.ctrlC()
-    await comfyPage.ctrlV()
-    await comfyPage.ctrlV()
+    await comfyPage.ctrlC(null)
+    await comfyPage.ctrlV(null)
+    await comfyPage.ctrlV(null)
    const resultString = await textBox.inputValue()
    expect(resultString).toBe(originalString + originalString)
  })
@@ -31,7 +31,7 @@ test.describe('Copy Paste', () => {
        y: 643
      }
    })
-    await comfyPage.ctrlC()
+    await comfyPage.ctrlC(null)
    // KSampler's seed
    await comfyPage.canvas.click({
      position: {
@@ -39,7 +39,7 @@ test.describe('Copy Paste', () => {
        y: 281
      }
    })
-    await comfyPage.ctrlV()
+    await comfyPage.ctrlV(null)
    await comfyPage.page.keyboard.press('Enter')
    await expect(comfyPage.canvas).toHaveScreenshot('copied-widget-value.png')
  })
@@ -51,14 +51,14 @@ test.describe('Copy Paste', () => {
    comfyPage
  }) => {
    await comfyPage.clickEmptyLatentNode()
-    await comfyPage.ctrlC()
+    await comfyPage.ctrlC(null)
    const textBox = comfyPage.widgetTextBox
    await textBox.click()
    await textBox.inputValue()
    await textBox.selectText()
-    await comfyPage.ctrlC()
-    await comfyPage.ctrlV()
-    await comfyPage.ctrlV()
+    await comfyPage.ctrlC(null)
+    await comfyPage.ctrlV(null)
+    await comfyPage.ctrlV(null)
    await expect(comfyPage.canvas).toHaveScreenshot(
      'paste-in-text-area-with-node-previously-copied.png'
    )
@@ -69,10 +69,10 @@ test.describe('Copy Paste', () => {
    await textBox.click()
    await textBox.inputValue()
    await textBox.selectText()
-    await comfyPage.ctrlC()
+    await comfyPage.ctrlC(null)
    // Unfocus textbox.
    await comfyPage.page.mouse.click(10, 10)
-    await comfyPage.ctrlV()
+    await comfyPage.ctrlV(null)
    await expect(comfyPage.canvas).toHaveScreenshot('no-node-copied.png')
  })

--- a/browser_tests/dialog.spec.ts
+++ b/browser_tests/dialog.spec.ts
@@ -36,6 +36,7 @@ test.describe('Execution error', () => {
  }) => {
    await comfyPage.loadWorkflow('execution_error')
    await comfyPage.queueButton.click()
+    await comfyPage.nextFrame()

    // Wait for the element with the .comfy-execution-error selector to be visible
    const executionError = comfyPage.page.locator('.comfy-error-report')
--- a/browser_tests/extensionAPI.spec.ts
+++ b/browser_tests/extensionAPI.spec.ts
@@ -48,7 +48,7 @@ test.describe('Topbar commands', () => {
      })
    })

-    const menuItem: Locator = await comfyPage.menu.topbar.getMenuItem('ext')
+    const menuItem = comfyPage.menu.topbar.getMenuItem('ext')
    expect(await menuItem.count()).toBe(0)
  })

--- a/browser_tests/fixtures/ComfyPage.ts
+++ b/browser_tests/fixtures/ComfyPage.ts
@@ -349,6 +349,12 @@ export class ComfyPage {
    await this.nextFrame()
  }

+  async getToastErrorCount() {
+    return await this.page
+      .locator('.p-toast-message.p-toast-message-error')
+      .count()
+  }
+
  async getVisibleToastCount() {
    return await this.page.locator('.p-toast:visible').count()
  }
@@ -550,7 +556,7 @@ export class ComfyPage {
  }

  async doubleClickCanvas() {
-    await this.page.mouse.dblclick(10, 10)
+    await this.page.mouse.dblclick(10, 10, { delay: 5 })
    await this.nextFrame()
  }

@@ -586,43 +592,42 @@ export class ComfyPage {
    await this.nextFrame()
  }

-  async ctrlSend(keyToPress: string) {
-    await this.page.keyboard.down('Control')
-    await this.page.keyboard.press(keyToPress)
-    await this.page.keyboard.up('Control')
+  async ctrlSend(keyToPress: string, locator: Locator | null = this.canvas) {
+    const target = locator ?? this.page.keyboard
+    await target.press(`Control+${keyToPress}`)
    await this.nextFrame()
  }

-  async ctrlA() {
-    await this.ctrlSend('KeyA')
+  async ctrlA(locator?: Locator | null) {
+    await this.ctrlSend('KeyA', locator)
  }

-  async ctrlB() {
-    await this.ctrlSend('KeyB')
+  async ctrlB(locator?: Locator | null) {
+    await this.ctrlSend('KeyB', locator)
  }

-  async ctrlC() {
-    await this.ctrlSend('KeyC')
+  async ctrlC(locator?: Locator | null) {
+    await this.ctrlSend('KeyC', locator)
  }

-  async ctrlV() {
-    await this.ctrlSend('KeyV')
+  async ctrlV(locator?: Locator | null) {
+    await this.ctrlSend('KeyV', locator)
  }

-  async ctrlZ() {
-    await this.ctrlSend('KeyZ')
+  async ctrlZ(locator?: Locator | null) {
+    await this.ctrlSend('KeyZ', locator)
  }

-  async ctrlY() {
-    await this.ctrlSend('KeyY')
+  async ctrlY(locator?: Locator | null) {
+    await this.ctrlSend('KeyY', locator)
  }

-  async ctrlArrowUp() {
-    await this.ctrlSend('ArrowUp')
+  async ctrlArrowUp(locator?: Locator | null) {
+    await this.ctrlSend('ArrowUp', locator)
  }

-  async ctrlArrowDown() {
-    await this.ctrlSend('ArrowDown')
+  async ctrlArrowDown(locator?: Locator | null) {
+    await this.ctrlSend('ArrowDown', locator)
  }

  async closeMenu() {
--- a/browser_tests/fixtures/components/Topbar.ts
+++ b/browser_tests/fixtures/components/Topbar.ts
@@ -13,35 +13,46 @@ export class Topbar {
    await this.page.locator('.p-menubar-mobile .p-menubar-button').click()
  }

-  async getMenuItem(itemLabel: string): Promise<Locator> {
+  getMenuItem(itemLabel: string): Locator {
    return this.page.locator(`.p-menubar-item-label:text-is("${itemLabel}")`)
  }

-  async getWorkflowTab(tabName: string): Promise<Locator> {
+  getWorkflowTab(tabName: string): Locator {
    return this.page
      .locator(`.workflow-tabs .workflow-label:has-text("${tabName}")`)
      .locator('..')
  }

  async closeWorkflowTab(tabName: string) {
-    const tab = await this.getWorkflowTab(tabName)
+    const tab = this.getWorkflowTab(tabName)
    await tab.locator('.close-button').click({ force: true })
  }

-  async saveWorkflow(workflowName: string) {
-    await this.triggerTopbarCommand(['Workflow', 'Save'])
-    await this.page.locator('.p-dialog-content input').fill(workflowName)
-    await this.page.keyboard.press('Enter')
-    // Wait for the dialog to close.
-    await this.page.waitForTimeout(300)
+  getSaveDialog(): Locator {
+    return this.page.locator('.p-dialog-content input')
  }

-  async saveWorkflowAs(workflowName: string) {
-    await this.triggerTopbarCommand(['Workflow', 'Save As'])
-    await this.page.locator('.p-dialog-content input').fill(workflowName)
+  saveWorkflow(workflowName: string): Promise<void> {
+    return this._saveWorkflow(workflowName, 'Save')
+  }
+
+  saveWorkflowAs(workflowName: string): Promise<void> {
+    return this._saveWorkflow(workflowName, 'Save As')
+  }
+
+  async _saveWorkflow(workflowName: string, command: 'Save' | 'Save As') {
+    await this.triggerTopbarCommand(['Workflow', command])
+    await this.getSaveDialog().fill(workflowName)
    await this.page.keyboard.press('Enter')
+
+    // Wait for workflow service to finish saving
+    await this.page.waitForFunction(
+      () => !window['app'].extensionManager.workflow.isBusy,
+      undefined,
+      { timeout: 3000 }
+    )
    // Wait for the dialog to close.
-    await this.page.waitForTimeout(300)
+    await this.getSaveDialog().waitFor({ state: 'hidden', timeout: 500 })
  }

  async triggerTopbarCommand(path: string[]) {
--- a/browser_tests/interaction.spec.ts
+++ b/browser_tests/interaction.spec.ts
@@ -285,7 +285,8 @@ test.describe('Node Interaction', () => {
      position: {
        x: 50,
        y: 10
-      }
+      },
+      delay: 5
    })
    await comfyPage.page.keyboard.type('Hello World')
    await comfyPage.page.keyboard.press('Enter')
@@ -300,7 +301,8 @@ test.describe('Node Interaction', () => {
      position: {
        x: 50,
        y: 50
-      }
+      },
+      delay: 5
    })
    expect(await comfyPage.page.locator('.node-title-editor').count()).toBe(0)
  })
@@ -352,7 +354,8 @@ test.describe('Group Interaction', () => {
      position: {
        x: 50,
        y: 10
-      }
+      },
+      delay: 5
    })
    await comfyPage.page.keyboard.type('Hello World')
    await comfyPage.page.keyboard.press('Enter')
@@ -504,7 +507,7 @@ test.describe('Widget Interaction', () => {
    await expect(textBox).toHaveValue('')
    await textBox.fill('Hello World')
    await expect(textBox).toHaveValue('Hello World')
-    await comfyPage.ctrlZ()
+    await comfyPage.ctrlZ(null)
    await expect(textBox).toHaveValue('')
  })

@@ -515,9 +518,9 @@ test.describe('Widget Interaction', () => {
    await textBox.fill('1girl')
    await expect(textBox).toHaveValue('1girl')
    await textBox.selectText()
-    await comfyPage.ctrlArrowUp()
+    await comfyPage.ctrlArrowUp(null)
    await expect(textBox).toHaveValue('(1girl:1.05)')
-    await comfyPage.ctrlZ()
+    await comfyPage.ctrlZ(null)
    await expect(textBox).toHaveValue('1girl')
  })
 })
--- a/browser_tests/menu.spec.ts
+++ b/browser_tests/menu.spec.ts
@@ -504,6 +504,7 @@ test.describe('Menu', () => {
      await comfyPage.menu.topbar.saveWorkflow(workflowName)
      expect(await comfyPage.menu.topbar.getTabNames()).toEqual([workflowName])
      await comfyPage.menu.topbar.closeWorkflowTab(workflowName)
+      await comfyPage.nextFrame()
      expect(await comfyPage.menu.topbar.getTabNames()).toEqual([
        'Unsaved Workflow'
      ])
@@ -525,8 +526,7 @@ test.describe('Menu', () => {
    })

    test('Displays keybinding next to item', async ({ comfyPage }) => {
-      const workflowMenuItem =
-        await comfyPage.menu.topbar.getMenuItem('Workflow')
+      const workflowMenuItem = comfyPage.menu.topbar.getMenuItem('Workflow')
      await workflowMenuItem.click()
      const exportTag = comfyPage.page.locator('.keybinding-tag', {
        hasText: 'Ctrl + s'
--- a/browser_tests/nodeSearchBox.spec.ts
+++ b/browser_tests/nodeSearchBox.spec.ts
@@ -15,7 +15,7 @@ test.describe('Node search box', () => {

  test(`Can trigger on group body double click`, async ({ comfyPage }) => {
    await comfyPage.loadWorkflow('single_group_only')
-    await comfyPage.page.mouse.dblclick(50, 50)
+    await comfyPage.page.mouse.dblclick(50, 50, { delay: 5 })
    await comfyPage.nextFrame()
    await expect(comfyPage.searchBox.input).toHaveCount(1)
  })
--- a/src/stores/workflowStore.ts
+++ b/src/stores/workflowStore.ts
@@ -284,32 +284,45 @@ export const useWorkflowStore = defineStore('workflow', () => {
    workflows.value.filter((workflow) => workflow.isModified)
  )

+  /** A filesystem operation is currently in progress (e.g. save, rename, delete) */
+  const isBusy = ref<boolean>(false)
+
  const renameWorkflow = async (workflow: ComfyWorkflow, newPath: string) => {
-    // Capture all needed values upfront
-    const oldPath = workflow.path
-    const wasBookmarked = bookmarkStore.isBookmarked(oldPath)
-
-    const openIndex = detachWorkflow(workflow)
-    // Perform the actual rename operation first
+    isBusy.value = true
    try {
-      await workflow.rename(newPath)
-    } finally {
-      attachWorkflow(workflow, openIndex)
-    }
+      // Capture all needed values upfront
+      const oldPath = workflow.path
+      const wasBookmarked = bookmarkStore.isBookmarked(oldPath)

-    // Update bookmarks
-    if (wasBookmarked) {
-      bookmarkStore.setBookmarked(oldPath, false)
-      bookmarkStore.setBookmarked(newPath, true)
+      const openIndex = detachWorkflow(workflow)
+      // Perform the actual rename operation first
+      try {
+        await workflow.rename(newPath)
+      } finally {
+        attachWorkflow(workflow, openIndex)
+      }
+
+      // Update bookmarks
+      if (wasBookmarked) {
+        bookmarkStore.setBookmarked(oldPath, false)
+        bookmarkStore.setBookmarked(newPath, true)
+      }
+    } finally {
+      isBusy.value = false
    }
  }

  const deleteWorkflow = async (workflow: ComfyWorkflow) => {
-    await workflow.delete()
-    if (bookmarkStore.isBookmarked(workflow.path)) {
-      bookmarkStore.setBookmarked(workflow.path, false)
+    isBusy.value = true
+    try {
+      await workflow.delete()
+      if (bookmarkStore.isBookmarked(workflow.path)) {
+        bookmarkStore.setBookmarked(workflow.path, false)
+      }
+      delete workflowLookup.value[workflow.path]
+    } finally {
+      isBusy.value = false
    }
-    delete workflowLookup.value[workflow.path]
  }

  /**
@@ -317,12 +330,17 @@ export const useWorkflowStore = defineStore('workflow', () => {
   * @param workflow The workflow to save.
   */
  const saveWorkflow = async (workflow: ComfyWorkflow) => {
-    // Detach the workflow and re-attach to force refresh the tree objects.
-    const openIndex = detachWorkflow(workflow)
+    isBusy.value = true
    try {
-      await workflow.save()
+      // Detach the workflow and re-attach to force refresh the tree objects.
+      const openIndex = detachWorkflow(workflow)
+      try {
+        await workflow.save()
+      } finally {
+        attachWorkflow(workflow, openIndex)
+      }
    } finally {
-      attachWorkflow(workflow, openIndex)
+      isBusy.value = false
    }
  }

@@ -333,6 +351,7 @@ export const useWorkflowStore = defineStore('workflow', () => {
    openedWorkflowIndexShift,
    openWorkflow,
    isOpen,
+    isBusy,
    closeWorkflow,
    createTemporary,
    renameWorkflow,