Skip to content

Conversation

benceruleanlu
Copy link
Member

@benceruleanlu benceruleanlu commented Sep 19, 2025

Tests added

  • Should show a link dragging out from a slot when dragging on a slot
  • Should create a link when dropping on a compatible slot
  • Should not create a link when dropping on an incompatible slot(s)

┆Issue is synchronized with this Notion page by Unito

Copy link

github-actions bot commented Sep 19, 2025

🎭 Playwright Test Results

⚠️ Tests passed with flaky tests

⏰ Completed at: 09/19/2025, 08:14:13 PM UTC

📈 Summary

  • Total Tests: 453
  • Passed: 423 ✅
  • Failed: 0
  • Flaky: 1 ⚠️
  • Skipped: 29 ⏭️

📊 Test Reports by Browser

  • chromium: View Report • ✅ 416 / ❌ 0 / ⚠️ 1 / ⏭️ 29
  • chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • mobile-chrome: View Report • ✅ 4 / ❌ 0 / ⚠️ 0 / ⏭️ 0

🎉 Click on the links above to view detailed test results for each browser configuration.

@benceruleanlu
Copy link
Member Author

SyntaxError: /home/runner/work/ComfyUI_frontend/ComfyUI_frontend/ComfyUI_frontend/src/lib/litegraph/src/LGraphNode.ts: TypeScript 'declare' fields must first be transformed by @babel/plugin-transform-typescript.
If you have already enabled that plugin (or '@babel/preset-typescript'), make sure that it runs before any plugin related to additional class features:

  • @babel/plugin-transform-class-properties
  • @babel/plugin-transform-private-methods
  • @babel/plugin-proposal-decorators
    405 | showAdvanced?: boolean
    406 |

407 | declare comfyClass?: string
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
408 | declare isVirtualNode?: boolean
409 | applyToGraph?(extraLinks?: LLink[]): void
410 |

speed-ishowspeed

let me try fixing this

@benceruleanlu
Copy link
Member Author

the failing test is the new screenshot needed, feel free to check it out

@benceruleanlu benceruleanlu added the New Browser Test Expectations New browser test screenshot should be set by github action label Sep 19, 2025
@benceruleanlu benceruleanlu marked this pull request as ready for review September 19, 2025 12:19
@benceruleanlu benceruleanlu requested a review from a team as a code owner September 19, 2025 12:19
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Sep 19, 2025
@benceruleanlu benceruleanlu removed the New Browser Test Expectations New browser test screenshot should be set by github action label Sep 19, 2025
@benceruleanlu
Copy link
Member Author

by the way, I asked design if they had something for links, we can change it at a later time, they haven't made anything just yet, so that's why the links look odd ending abruptly


type WorkspaceStore = ReturnType<typeof useWorkspaceStore>

type Bounds = readonly [number, number, number, number]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we import the type from src?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this has been removed

Comment on lines 343 to 349
async fitToView(
options: {
selectionOnly?: boolean
zoom?: number
padding?: number
} = {}
) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is a lot of logic to put in a fixture.
Normally I like fixtures to be as close to describing what a user would do as possible.

The general rule of browser tests is: unless you expect a user to open the console and type in a series of commands, the test shouldn't do that either.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved it to a helper

Comment on lines +9 to +16
async function getCenter(locator: Locator): Promise<{ x: number; y: number }> {
const box = await locator.boundingBox()
if (!box) throw new Error('Slot bounding box not available')
return {
x: box.x + box.width / 2,
y: box.y + box.height / 2
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use dragTo?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

Copy link
Contributor

@christian-byrne christian-byrne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, appreciate adding e2e tests a ton, will give more confidence going forward.

@christian-byrne christian-byrne merged commit 893409d into main Sep 19, 2025
29 checks passed
@christian-byrne christian-byrne deleted the bl-tests branch September 19, 2025 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:links area:testing size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants