test(browser): refactor browser tests for reliability and maintainability#8510
test(browser): refactor browser tests for reliability and maintainability#8510
Conversation
- widget.spec.ts: use expect().toPass() for combo refresh - subgraph.spec.ts: use comfyPage.setup() after reload - nodeLibrary.spec.ts: use toHaveCount() for debounced search - remoteWidgets.spec.ts: use expect().toPass() for backoff test - litegraphUtils.ts: refactor navigateIntoSubgraph to use toPass() Amp-Thread-ID: https://ampcode.com/threads/T-019c11f1-e839-772c-8890-b77cfdf70124 Co-authored-by: Amp <amp@ampcode.com>
Per Playwright best practices, assertions belong in tests, not fixtures.
Fixture methods should use waitFor() for preconditions instead.
- vueNodeFixtures.ts: setTitle(), cancelTitleEdit() use waitFor()
- Topbar.ts: closeTopbarMenu() uses waitFor({ state: 'hidden' })
- ComfyPage.ts: ConfirmDialog.click(), closeDialog(), clickDialogButton(),
closeToasts() all converted to waitFor()
- templates.ts: renamed waitForMinimumCardCount to expectMinimumCardCount
to clarify it contains an assertion (uses expect().toPass() pattern)
Amp-Thread-ID: https://ampcode.com/threads/T-019c11f8-acd2-7429-8bec-525ad47a47c4
Co-authored-by: Amp <amp@ampcode.com>
- Replace 4 CSS selectors with semantic locators in nodeLibrary.spec.ts - Uses PrimeVue SelectButton's accessible radio button names - Part of Playwright best practices remediation Amp-Thread-ID: https://ampcode.com/threads/T-019c11ff-0d10-771a-abaf-7d8d908800b3 Co-authored-by: Amp <amp@ampcode.com>
- Create browser_tests/fixtures/constants/defaultGraphPositions.ts with all hardcoded default graph positions - Create browser_tests/fixtures/constants/types.ts with Position and Size interfaces - Update ComfyPage.ts to import and use DefaultGraphPositions - Reduces duplication and prepares for further modular extraction Amp-Thread-ID: https://ampcode.com/threads/T-019c1201-adec-7363-96d5-446d0a98e4cf Co-authored-by: Amp <amp@ampcode.com>
- Create browser_tests/fixtures/helpers/DebugHelper.ts with 7 methods - Add debug: DebugHelper property to ComfyPage - Replace debug methods with deprecation proxies delegating to this.debug - Reduces ComfyPage.ts by ~140 lines Amp-Thread-ID: https://ampcode.com/threads/T-019c1206-3625-71ef-b976-1796574b9642 Co-authored-by: Amp <amp@ampcode.com>
- Create browser_tests/fixtures/helpers/SubgraphHelper.ts with 10 methods - Add subgraph: SubgraphHelper property to ComfyPage - Replace original methods with deprecation proxy stubs - Reduces ComfyPage.ts from 1633 to 1409 lines (~224 lines removed) Amp-Thread-ID: https://ampcode.com/threads/T-019c12cc-5256-7359-b830-28d1c3639daa Co-authored-by: Amp <amp@ampcode.com>
- Create CanvasHelper class for canvas viewport operations - Extract pan, zoom, dragAndDrop, rightClick, doubleClick methods - Extract moveMouseToEmptyArea, convertOffsetToCanvas methods - Add deprecation proxies for backward compatibility - Add canvasOps property to ComfyPage Amp-Thread-ID: https://ampcode.com/threads/T-019c1300-e933-769c-b05f-ea00c2d32dd1 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c1300-e933-769c-b05f-ea00c2d32dd1 Co-authored-by: Amp <amp@ampcode.com>
Phase 5 of ComfyPage refactoring: extracts node query, selection, resize, and group conversion operations into NodeOperationsHelper. Methods extracted: getGraphNodesCount, getSelectedGraphNodesCount, getNodes, waitForGraphNodes, getFirstNodeRef, getNodeRefById, getNodeRefsByType, getNodeRefsByTitle, selectNodes, select2Nodes, resizeNode, convertAllNodesToGroupNode ComfyPage.ts: 1383 -> 1329 lines (-54) Amp-Thread-ID: https://ampcode.com/threads/T-019c1313-7d0e-718d-911b-f6954a15eb9b Co-authored-by: Amp <amp@ampcode.com>
Migrate all test callers to use helper classes directly: - nodeOps: getGraphNodesCount, getNodeRefById, selectNodes, etc. - canvasOps: dragAndDrop, zoom, pan, resetView, etc. - subgraph: rightClickInputSlot, connectToOutput, etc. Reduces ComfyPage.ts from 1335 to 1085 lines (-250 lines). Amp-Thread-ID: https://ampcode.com/threads/T-019c1318-58b7-757f-801c-5b676f0d3421 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c134d-cf30-7028-aca7-cfc6227c90ef Co-authored-by: Amp <amp@ampcode.com>
Remove redundant setTimeout calls inside page.evaluate() in SubgraphHelper.ts. The waits were unnecessary since proper Playwright waitForSelector calls already exist after the evaluate returns. Amp-Thread-ID: https://ampcode.com/threads/T-019c134d-cf30-7028-aca7-cfc6227c90ef Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c1359-48cc-7676-abc8-2de8c57b52cf Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c1359-48cc-7676-abc8-2de8c57b52cf Co-authored-by: Amp <amp@ampcode.com>
- Add data-testid='settings-dialog' to settings container
- Add data-testid='about-panel' to AboutPanel
- Add data-testid='settings-tab-{key}' to all sidebar navigation items
Amp-Thread-ID: https://ampcode.com/threads/T-019c1359-48cc-7676-abc8-2de8c57b52cf
Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c1359-48cc-7676-abc8-2de8c57b52cf Co-authored-by: Amp <amp@ampcode.com>
…ialog.ts Amp-Thread-ID: https://ampcode.com/threads/T-019c1359-48cc-7676-abc8-2de8c57b52cf Co-authored-by: Amp <amp@ampcode.com>
- nodeSearchBox.spec.ts: Use NodeReference.getOutput() for CLIP output slot position - rightClickMenu.spec.ts: Use NodeReference.getTitlePosition() for node dragging - Add getTitlePosition() helper to NodeReference for drag operations Amp-Thread-ID: https://ampcode.com/threads/T-019c1367-6a59-7752-9329-17cbf152fc62 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c1373-f6d0-7426-a3ee-5673891f9dcc Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c1373-f6d0-7426-a3ee-5673891f9dcc Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c137b-34ba-736a-afb6-0a41baf9358f Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c137b-34ba-736a-afb6-0a41baf9358f Co-authored-by: Amp <amp@ampcode.com>
Replace fragile CSS selectors with Playwright getByRole() for better accessibility-based testing: - bottomPanelShortcuts.spec.ts: button[aria-label] -> getByRole - execution.spec.ts: .p-dialog-close-button -> getByRole - backgroundImageUpload.spec.ts: button:has(.pi-*) -> getByRole - nodeHelp.spec.ts: button:has(.pi-question) -> getByRole - BaseDialog.ts: closeButton uses getByRole - ComfyNodeSearchBox.ts: Add button uses getByRole - Topbar.ts: close-button uses getByRole Amp-Thread-ID: https://ampcode.com/threads/T-019c155c-92e1-76f3-b6ce-7fc3ffa11582 Co-authored-by: Amp <amp@ampcode.com>
- Remove unused saveButton selector from ComfyMenu - Replace widgetTextBox .nth(1) with node-scoped selector - Replace .nth(3) toast selector with .p-toast-message filter - Replace .nth(1) category selector with getByTestId - Replace .nth(1) node selector with getNodeInFolder() helper - Add nodeSelector(), folderSelector(), getNodeInFolder() methods - Add saveButton and nodeLibrary TestIds to selectors.ts Amp-Thread-ID: https://ampcode.com/threads/T-019c155c-92e1-76f3-b6ce-7fc3ffa11582 Co-authored-by: Amp <amp@ampcode.com>
- Remove select2Nodes helper method from NodeOperationsHelper - Replace usages with selectNodes(['CLIP Text Encode (Prompt)']) - Increase flaky timeout in mobileBaseline test from 256ms to 5000ms Amp-Thread-ID: https://ampcode.com/threads/T-019c15b9-efed-72eb-b092-1e92a11f44e3 Co-authored-by: Amp <amp@ampcode.com>
Remove duplicated nextFrame and ctrlSend methods from ClipboardHelper by delegating to KeyboardHelper instead. Amp-Thread-ID: https://ampcode.com/threads/T-019c15b9-efed-72eb-b092-1e92a11f44e3 Co-authored-by: Amp <amp@ampcode.com>
📝 WalkthroughWalkthroughLarge refactor and expansion of Playwright E2E tests: adds many modular test helpers and components, centralizes selectors/types/constants, migrates ~100 tests to new ComfyPage namespaces, tightens typings, adds test utilities, updates CI/tooling, lints, and TypeScript config, and normalizes .gitattributes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Runner as Test Runner
participant ComfyPage as ComfyPage fixture
participant Page as Playwright Page
participant App as window.app (in-browser)
Runner->>ComfyPage: call comfyPage.workflow.loadWorkflow(name)
ComfyPage->>Page: perform locator actions / page.evaluate (getByTestId/getByRole/waitFor)
Page->>App: invoke window.app APIs (settings, graph, commands)
App-->>Page: update UI/state (graph changes, toasts)
Page-->>ComfyPage: locator.waitFor / nextFrame resolves
ComfyPage-->>Runner: return control / assertions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
🎭 Playwright Tests:
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 02/03/2026, 06:54:10 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
- Add missing test IDs to TestIds object - Migrate all hardcoded getByTestId calls to use TestIds - Update TestIdValue type to include new categories Amp-Thread-ID: https://ampcode.com/threads/T-019c2266-b2ca-72cb-ad4b-1a13f42cbf94 Co-authored-by: Amp <amp@ampcode.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
browser_tests/tests/interaction.spec.ts (2)
355-355:⚠️ Potential issue | 🟡 MinorAvoid
delay(300)- use deterministic waits.This
comfyPage.delay(300)also violates the guideline againstwaitForTimeout. Consider using a retrying assertion on the dialog visibility state instead of a fixed delay. As per coding guidelines: "Do not use waitForTimeout in Playwright tests; use Locator actions and retrying assertions."🔧 Suggested approach
- await comfyPage.delay(300) + // The dialog is already visible, proceed directly with the clickAlternatively, if there's a specific animation you need to wait for, use
waitForFunctionor a retrying assertion that checks for the desired state.
378-378:⚠️ Potential issue | 🟡 MinorAvoid
delay(300)- same issue as above.Another instance of
comfyPage.delay(300)that should be replaced with a deterministic wait mechanism. As per coding guidelines: "Do not use waitForTimeout in Playwright tests; use Locator actions and retrying assertions."
christian-byrne
added
the
claude-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| async getScale(): Promise<number> { | ||
| return this.page.evaluate(() => { | ||
| return window.app!.canvas.ds.scale |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Inconsistent non-null assertion pattern
Context: Line 96 uses non-null assertion on window.app when evaluating canvas scale. This assumes window.app is always defined in page.evaluate contexts, but the codebase shows mixed patterns of optional chaining vs assertions.
Suggestion: Standardize on one approach - either always use optional chaining or document the invariant that window.app exists in evaluate contexts.
There was a problem hiding this comment.
It's assumed to always be present in browser_tests
|
|
||
| private async nextFrame(): Promise<void> { | ||
| await this.page.evaluate(() => { | ||
| return new Promise<number>(requestAnimationFrame) |
There was a problem hiding this comment.
[quality] low Priority
Issue: Redundant type annotation on Promise return type
Context: Line 15 explicitly types the Promise return as Promise when requestAnimationFrame already returns number. This is unnecessary verbosity.
Suggestion: Simplify to return new Promise(requestAnimationFrame) without the type annotation since TypeScript infers it correctly.
| ) {} | ||
|
|
||
| private async nextFrame(): Promise<void> { | ||
| await this.page.evaluate(() => { |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Duplicated nextFrame implementation
Context: Line 14-18 implements nextFrame() using the same pattern as CanvasHelper. This violates DRY principles and creates maintenance burden if the implementation needs to change.
Suggestion: Extract nextFrame to a shared utility function or accept it as a dependency in the constructor from the parent ComfyPage fixture.
| * @param action - 'rightClick' or 'doubleClick' | ||
| * @param slotName - Optional specific slot name to target | ||
| */ | ||
| private async interactWithSubgraphSlot( |
There was a problem hiding this comment.
[architecture] medium Priority
Issue: Deeply nested conditional logic increases cognitive complexity
Context: Lines 27-153 contain a complex interactWithSubgraphSlot method with nested conditionals, switch-like logic, and multiple execution paths. The cyclomatic complexity is high.
Suggestion: Extract separate methods for rightClick and doubleClick actions rather than parameterizing the behavior. Consider: private async rightClickSlot() and private async doubleClickSlot() with shared helper for slot resolution.
| /** | ||
| * Helper type for accessing nested TestIds (excludes function values) | ||
| */ | ||
| export type TestIdValue = |
There was a problem hiding this comment.
[quality] low Priority
Issue: Unnecessary complex type definition for TestIdValue
Context: Lines 63-77 define a union type TestIdValue that is never used in the codebase. This adds complexity without providing value.
Suggestion: Remove the TestIdValue type export unless it has a specific consumer. If needed for future type safety, add a comment explaining its purpose.
| const waitIfExists = async (locator: Locator, menuName: string) => { | ||
| const count = await locator.count() | ||
| if (count > 0) { | ||
| await locator.waitFor({ state: 'hidden' }).catch((error: Error) => { |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Swallowed errors with silent fallback in waitForHidden
Context: Lines 40-44 catch and only log errors via console.warn, which may hide legitimate test failures or timing issues. The silent failure mode could mask flaky tests.
Suggestion: Consider whether these errors should propagate or at least be collected and reported in test output. If silence is intentional, document why with a comment.
| { | ||
| "files": ["browser_tests/**/*.ts"], | ||
| "rules": { | ||
| "typescript/no-explicit-any": "error", |
There was a problem hiding this comment.
[quality] high Priority
Issue: Stricter lint rules enabled for browser_tests may conflict with existing code patterns
Context: Line 117 enables typescript/no-explicit-any as error for browser_tests. While this improves type safety, the PR description states it purged as any casts, but enabling this rule post-facto may cause issues if any remain or are added accidentally.
Suggestion: Verify all browser_tests files pass the new no-explicit-any rule. Consider adding a test or pre-commit hook to prevent regressions.
| window.app!.canvas.ds!.convertOffsetToCanvas(rawPos) | ||
|
|
||
| // Debug logging - convert Float64Arrays to regular arrays for visibility | ||
| console.warn( |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Debug logging left in production test code
Context: Lines 124-133 contain console.warn debug logging that outputs detailed position information. This appears to be leftover debugging code that should be removed before merge.
Suggestion: Remove the console.warn statement or wrap it in a DEBUG flag if this logging is intentionally kept for troubleshooting flaky tests.
| } | ||
|
|
||
| async getThemeId() { | ||
| return await this.page.evaluate(async () => { |
There was a problem hiding this comment.
[quality] low Priority
Issue: Redundant async wrapper in getThemeId
Context: Line 104 uses async/await when the page.evaluate already returns a Promise. The inner async function is unnecessary since getSettingValue likely returns a Promise that can be awaited in the outer scope.
Suggestion: Simplify to return this.page.evaluate(() => window.app!.ui.settings.getSettingValue('Comfy.ColorPalette'))
| */ | ||
| export const DefaultGraphPositions = { | ||
| // Node click positions | ||
| textEncodeNode1: { x: 618, y: 191 }, |
There was a problem hiding this comment.
[architecture] medium Priority
Issue: Hardcoded pixel coordinates create brittle tests vulnerable to UI changes
Context: Lines 9-36 define hardcoded x/y positions that couple tests tightly to specific viewport dimensions and node layouts. Any change to default workflow layout or canvas sizing will break these tests.
Suggestion: Consider computing positions dynamically from node references at runtime, or at minimum document which default workflow file these positions correspond to and add validation that viewport matches expected dimensions.
| pos: { x: 473, y: 579 }, | ||
| size: { width: 315, height: 136 } | ||
| } | ||
| } as const satisfies { |
There was a problem hiding this comment.
[quality] low Priority
Issue: Redundant type constraint with satisfies
Context: Lines 37-51 use both as const and satisfies with a fully specified type. The satisfies clause provides type checking but the explicit type definition duplicates the structure definition above.
Suggestion: Consider removing the explicit type after satisfies since the structure above already defines the shape, or add a comment explaining why both are needed for documentation purposes.
There was a problem hiding this comment.
Comprehensive PR Review
This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.
Review Summary
PR: test(browser): refactor browser tests for reliability and maintainability (#8510)
Impact: 4073 additions, 3021 deletions across 99 files
Issue Distribution
- Critical: 0
- High: 1
- Medium: 6
- Low: 4
Category Breakdown
- Architecture: 2 issues
- Code Quality: 9 issues
- Security: 0 issues
- Performance: 0 issues
Key Findings
Architecture & Design
Positive: The decomposition of ComfyPage.ts from ~1000 lines into focused helpers is excellent architectural improvement. The separation of concerns (CanvasHelper, SubgraphHelper, NodeOperationsHelper, etc.) makes the codebase more maintainable and follows single-responsibility principle.
Concerns:
- Hardcoded pixel coordinates in DefaultGraphPositions create brittle tests. While extracting these into constants is good, they remain tightly coupled to specific viewport dimensions and workflow layouts.
- High cyclomatic complexity in SubgraphHelper.interactWithSubgraphSlot() method (150+ lines with nested conditionals). Consider extracting separate methods for different action types.
Code Quality
Excellent improvements:
- Centralized selectors with TestIds constants
- Elimination of as any casts
- Addition of strict TypeScript rules for browser tests
- Removal of setTimeout anti-patterns
- Better type safety throughout
Issues identified:
- DRY violations: nextFrame() method duplicated across multiple helpers
- Debug logging: console.warn statements left in litegraphUtils.ts that should be removed
- Error handling: Silent error swallowing in ContextMenu.waitForHidden may hide test flakiness
- Type redundancy: Some unnecessary type annotations and constraints
Type Safety & Testing Standards
The PR successfully achieves its goal of improving type safety by:
- Enabling typescript/no-explicit-any for browser_tests
- Adding proper Window type augmentation
- Removing type assertions throughout
- Using data-testid attributes for stable selectors
The migration from fragile CSS selectors to getByTestId/getByRole is a significant reliability improvement aligned with Playwright best practices.
Integration Points
The refactoring maintains backward compatibility - all helpers are accessed through ComfyPage fixture, so existing tests continue to work. The decomposition is purely internal and doesn't break the public API of the test fixtures.
Positive Observations
- Excellent decomposition strategy: Breaking down the monolithic ComfyPage into focused helpers follows SOLID principles
- Test reliability improvements: Replacing waitForTimeout with retrying assertions and using stable selectors dramatically improves test stability
- Type safety enhancements: Stricter linting and removal of any casts improves long-term maintainability
- Documentation: Good inline comments explaining complex interactions, especially in SubgraphHelper
- Consistent patterns: Helper classes follow consistent constructor injection patterns
- Better organization: Constants extracted into dedicated files (defaultGraphPositions, selectors)
References
Next Steps
- Address high priority issue: Verify the new no-explicit-any lint rule passes on all files
- Consider architectural feedback: Extract shared utilities to reduce duplication (nextFrame, common patterns)
- Remove debug artifacts: Clean up console.warn statements before merge
- Document hardcoded positions: Add comments explaining which workflow file DefaultGraphPositions corresponds to
- Simplify complex methods: Refactor SubgraphHelper.interactWithSubgraphSlot to reduce cognitive complexity
Overall Assessment
This is a high-quality refactoring that substantially improves the browser test infrastructure. The decomposition is well-executed, and the focus on reliability through stable selectors and proper async handling is commendable. The issues identified are mostly minor code quality improvements that don't block the merge but would enhance maintainability.
The PR successfully achieves its stated goals of improving reliability, maintainability, and type safety. All browser tests passing is a strong signal that the refactoring preserves functionality while improving structure.
This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.
github-actions
bot
removed
the
claude-review
| # Force TS to LF to make the unixy scripts not break on Windows | ||
| *.cjs text eol=lf | ||
| *.js text eol=lf | ||
| *.json text eol=lf | ||
| *.mjs text eol=lf | ||
| *.mts text eol=lf | ||
| *.snap text eol=lf | ||
| *.ts text eol=lf | ||
| *.vue text eol=lf | ||
| *.yaml text eol=lf | ||
| *.yml text eol=lf |
| - Run `pnpm exec eslint browser_tests/path/to/file.ts` to lint specific files | ||
| - Run `pnpm exec oxlint browser_tests/path/to/file.ts` to check with oxlint |
There was a problem hiding this comment.
We need two separate commands?
There was a problem hiding this comment.
I was being lazy and didn't want to add a lint:browser to go with typecheck:browser and test:browser.
We really should leverage the workspaces more...
| public async setMode(mode: AutoQueueMode) { | ||
| await this.page.evaluate((mode) => { | ||
| window['app'].extensionManager.queueSettings.mode = mode | ||
| ;(window.app!.extensionManager as WorkspaceStore).queueSettings.mode = |
There was a problem hiding this comment.
@coderabbitai make a tracking Issue to eliminate all these casts, possibly using approach described above.
There was a problem hiding this comment.
Want to make a tracking issue?
| await expect(comfyPage.getVisibleToastCount()).resolves.toBe(2) | ||
| await comfyPage.settings.setSetting('Comfy.Validation.Workflows', true) | ||
| await comfyPage.workflow.loadWorkflow('links/bad_link') | ||
| await expect.poll(() => comfyPage.toast.getVisibleToastCount()).toBe(2) |
There was a problem hiding this comment.
Wait, expect.poll exists? I thought you said the idiomatic way was different?
There was a problem hiding this comment.
It's an order thing
- If there's an auto-retrying assertion built in, use that
- If you have to poll for values, use
.poll - If you have steps that might fail until they don't, use
.toPass - If you hate yourself and your team and don't respect their time, add a sleep
| .app!.graph!.nodes.find( | ||
| (node) => node.title === 'Node With Optional Combo Input' | ||
| )! |
There was a problem hiding this comment.
I guess we shuold use the helper for this.
| await expect(async () => { | ||
| const refreshedComboValues = await getComboValues() | ||
| expect(refreshedComboValues).not.toEqual(initialComboValues) | ||
| }).toPass({ timeout: 5000 }) |
There was a problem hiding this comment.
So what's the difference between this vs. expect.poll?
There was a problem hiding this comment.
This can fail internally for various reasons without failing the test, as long as it succeeds once before the timeout.
|
|
||
| 1. ✅ **No assertion** — Properly typed from the start | ||
| 2. ✅ **Type narrowing** — `if ('prop' in obj)` or type guards | ||
| 3. ⚠️ **Specific assertion** — `as SpecificType` when you truly know the type |
There was a problem hiding this comment.
No mention of Satisfies?
| @@ -1,5 +1,6 @@ | |||
| <template> | |||
| <Tree | |||
| v-bind="$attrs" | |||
There was a problem hiding this comment.
Should we favor just binding the test id attr to avoid unknown side effects of adding passthrough attrs on a widely used component?
There was a problem hiding this comment.
Passthrough is the default. The only reason it wasn't working before was the ContextMenu's existence blocking it.
| <template #folder="{ node }"> | ||
| <NodeTreeFolder :node="node" /> | ||
| </template> |
There was a problem hiding this comment.
Somewhat curious why this was necessary.
3 participants
Summary
Major refactoring of browser tests to improve reliability, maintainability, and type safety.
Changes
Test Infrastructure Decomposition
ComfyPage.ts(~1000 lines) into focused helpers:CanvasHelper,DebugHelper,SubgraphHelper,NodeOperationsHelperSettingsHelper,WorkflowHelper,ClipboardHelper,KeyboardHelperContextMenupage object,BaseDialogbase class, andBottomPanelpage objectDefaultGraphPositionsconstantsLocator Stability
data-testidattributes to Vue components (sidebar, dialogs, node library)selectors.tswith test ID constants.nth(),:nth-child()) withgetByTestId/getByRolePerformance & Reliability
setTimeoutanti-patterns (replaced withwaitForFunction)waitForTimeoutwith retrying assertionsNodeReferencepositionsType Safety
no-explicit-anylint rule for browser_tests via oxlintas anycasts from browser_testsBug Fixes
ExtensionManagerAPI contractTesting
Summary by CodeRabbit
Tests
Chores
Documentation
UI