mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-24 06:35:10 +00:00
*PR Created by the Glary-Bot Agent* --- ## Summary - Eliminates the confusing dual-helpers structure where `browser_tests/helpers/` and `browser_tests/fixtures/helpers/` coexisted one tier apart with overlapping purposes - Routes each file to its natural home based on what it actually *is*: page objects → `components/`, standalone utils → `utils/`, domain helper classes stay in `helpers/` - Adds an ESLint guard (`no-restricted-imports`) to prevent re-creating `browser_tests/helpers/` ## File Moves | File | From | To | Reason | |---|---|---|---| | `actionbar.ts` | `helpers/` | `fixtures/components/Actionbar.ts` | Page object class imported by ComfyPage | | `templates.ts` | `helpers/` | `fixtures/components/Templates.ts` | Page object class imported by ComfyPage | | `boundsUtils.ts` | `fixtures/helpers/` | `fixtures/utils/` | Pure function, not a helper class | | `mimeTypeUtil.ts` | `fixtures/helpers/` | `fixtures/utils/` | Pure function, not a helper class | | `builderTestUtils.ts` | `helpers/` | `fixtures/utils/` | Shared test setup functions | | `clipboardSpy.ts` | `helpers/` | `fixtures/utils/` | Page injection utility | | `fitToView.ts` | `helpers/` | `fixtures/utils/` | Canvas utility function | | `manageGroupNode.ts` | `helpers/` | `fixtures/utils/` | Litegraph interaction helper | | `painter.ts` | `helpers/` | `fixtures/utils/` | Test helper functions | | `perfReporter.ts` | `helpers/` | `fixtures/utils/` | Test infrastructure | | `promotedWidgets.ts` | `helpers/` | `fixtures/utils/` | Query helpers for specs | ## What Changed Beyond File Moves - **28 import statements** updated across test specs, fixtures, and infra files - **AGENTS.md** — directory tree diagram and architectural separation descriptions updated - **README.md** — "Leverage Existing Fixtures and Helpers" section updated - **`.claude/skills/perf-fix-with-proof/SKILL.md`** — perfReporter path reference updated - **`eslint.config.ts`** — added `@e2e/helpers/*` restricted import pattern to both spec and non-spec browser_tests rules ## Verification - `pnpm typecheck` — clean - `pnpm typecheck:browser` — clean - `pnpm lint` — 0 errors, 0 warnings - `pnpm format:check` — all files formatted - `pnpm knip` — clean - Pre-commit hooks passed full pipeline (oxfmt, oxlint, eslint, typecheck, typecheck:browser) ## Config Audit No changes needed to: `tsconfig.json` (`@e2e/*` alias covers all subdirs), `playwright.config.ts`, `vite.config.mts`, `knip.config.ts`, `.oxlintrc.json`, `nx.json` ## Manual Verification Note This is a pure structural refactoring (file moves + import updates) with zero behavioral or visual changes. The typecheck and lint passes confirm all imports resolve correctly. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11411-refactor-consolidate-browser_tests-helpers-into-fixtures-3476d73d3650816cb671ef7fa8433f66) by [Unito](https://www.unito.io) --------- Co-authored-by: glary-bot <glary-bot@comfy.org> Co-authored-by: Glary-Bot <glary-bot@users.noreply.github.com> Co-authored-by: DrJKL <DrJKL0424@gmail.com> Co-authored-by: Amp <amp@ampcode.com>
136 lines
6.4 KiB
Markdown
136 lines
6.4 KiB
Markdown
# E2E Testing Guidelines
|
|
|
|
See `@docs/guidance/playwright.md` for Playwright best practices (auto-loaded for `*.spec.ts`).
|
|
See `@browser_tests/FLAKE_PREVENTION_RULES.md` when triaging or editing
|
|
flaky browser tests.
|
|
|
|
## Directory Structure
|
|
|
|
```text
|
|
browser_tests/
|
|
├── assets/ - Test data (JSON workflows, images)
|
|
├── fixtures/
|
|
│ ├── ComfyPage.ts - Main fixture (delegates to helpers)
|
|
│ ├── ComfyMouse.ts - Mouse interaction helper
|
|
│ ├── VueNodeHelpers.ts - Vue Nodes 2.0 helpers
|
|
│ ├── selectors.ts - Centralized TestIds
|
|
│ ├── data/ - Static test data (mock API responses, workflow JSONs, node definitions)
|
|
│ ├── components/ - Page object classes (locators, user interactions)
|
|
│ │ ├── Actionbar.ts
|
|
│ │ ├── ContextMenu.ts
|
|
│ │ ├── ManageGroupNode.ts
|
|
│ │ ├── SettingDialog.ts
|
|
│ │ ├── SidebarTab.ts
|
|
│ │ ├── Templates.ts
|
|
│ │ ├── Topbar.ts
|
|
│ │ └── ...
|
|
│ ├── helpers/ - Focused helper classes (domain-specific actions)
|
|
│ │ ├── CanvasHelper.ts
|
|
│ │ ├── CommandHelper.ts
|
|
│ │ ├── KeyboardHelper.ts
|
|
│ │ ├── NodeOperationsHelper.ts
|
|
│ │ ├── SettingsHelper.ts
|
|
│ │ ├── WorkflowHelper.ts
|
|
│ │ └── ...
|
|
│ └── utils/ - Standalone utility functions (used by tests or fixtures)
|
|
│ ├── builderTestUtils.ts
|
|
│ ├── clipboardSpy.ts
|
|
│ ├── fitToView.ts
|
|
│ ├── perfReporter.ts
|
|
│ └── ...
|
|
└── tests/ - Test files (*.spec.ts)
|
|
```
|
|
|
|
### Architectural Separation
|
|
|
|
- **`fixtures/data/`** — Static test data only. Mock API responses, workflow JSONs, node definitions. No code, no imports from Playwright.
|
|
- **`fixtures/components/`** — Page object components. Classes that own locators for a specific UI region (e.g. `Actionbar`, `ContextMenu`, `ManageGroupNode`).
|
|
- **`fixtures/helpers/`** — Helper classes that coordinate actions across multiple regions without owning a locator surface of their own (e.g. `CanvasHelper`, `WorkflowHelper`, `NodeOperationsHelper`).
|
|
- **`fixtures/utils/`** — Standalone utility functions. Exported functions (not classes) used by tests or fixtures (e.g. `fitToView`, `clipboardSpy`, `builderTestUtils`).
|
|
|
|
### Placement Rule
|
|
|
|
When adding a new file, use this decision tree:
|
|
|
|
```mermaid
|
|
flowchart TD
|
|
A[New file in browser_tests/fixtures/] --> B{Has any code?}
|
|
B -- No, JSON/data only --> D[fixtures/data/]
|
|
B -- Yes --> C{Is it a class?}
|
|
C -- No, exported functions --> U[fixtures/utils/]
|
|
C -- Yes --> E{Owns locators for a<br/>specific UI region?}
|
|
E -- Yes --> P[fixtures/components/]
|
|
E -- No, coordinates actions<br/>across the app --> H[fixtures/helpers/]
|
|
```
|
|
|
|
## Page Object Locator Style
|
|
|
|
Define UI element locators as `public readonly` properties assigned in the constructor — not as getter methods. Getters that simply return a locator add unnecessary indirection and hide the object shape from IDE auto-complete.
|
|
|
|
```typescript
|
|
// ✅ Correct — public readonly, assigned in constructor
|
|
export class MyDialog extends BaseDialog {
|
|
public readonly submitButton: Locator
|
|
public readonly cancelButton: Locator
|
|
|
|
constructor(page: Page) {
|
|
super(page)
|
|
this.submitButton = this.root.getByRole('button', { name: 'Submit' })
|
|
this.cancelButton = this.root.getByRole('button', { name: 'Cancel' })
|
|
}
|
|
}
|
|
|
|
// ❌ Avoid — getter-based locators
|
|
export class MyDialog extends BaseDialog {
|
|
get submitButton() {
|
|
return this.root.getByRole('button', { name: 'Submit' })
|
|
}
|
|
}
|
|
```
|
|
|
|
**Keep as getters only when:**
|
|
|
|
- Lazy initialization is needed (`this._tab ??= new Tab(this.page)`)
|
|
- The value is computed from runtime state (e.g. `get id() { return this.userIds[index] }`)
|
|
- It's a private convenience accessor (e.g. `private get page() { return this.comfyPage.page }`)
|
|
|
|
When a class has cached locator properties, prefer reusing them in methods rather than rebuilding locators from scratch.
|
|
|
|
## Polling Assertions
|
|
|
|
Prefer `expect.poll()` over `expect(async () => { ... }).toPass()` when the block contains a single async call with a single assertion. `expect.poll()` is more readable and gives better error messages (shows actual vs expected on failure).
|
|
|
|
```typescript
|
|
// ✅ Correct — single async call + single assertion
|
|
await expect
|
|
.poll(() => comfyPage.nodeOps.getGraphNodesCount(), { timeout: 250 })
|
|
.toBe(0)
|
|
|
|
// ❌ Avoid — nested expect inside toPass
|
|
await expect(async () => {
|
|
expect(await comfyPage.nodeOps.getGraphNodesCount()).toBe(0)
|
|
}).toPass({ timeout: 250 })
|
|
```
|
|
|
|
Reserve `toPass()` for blocks with multiple assertions or complex async logic that can't be expressed as a single polled value.
|
|
|
|
## Gotchas
|
|
|
|
| Symptom | Cause | Fix |
|
|
| -------------------------------------------------- | ------------------------------------------- | ------------------------------------------------------------------------------------------------------- |
|
|
| `subtree intercepts pointer events` on DOM widgets | Canvas `z-999` overlay intercepts `click()` | Use Playwright's `locator.dispatchEvent('contextmenu', { bubbles: true, cancelable: true, button: 2 })` |
|
|
| 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 |
|
|
|
|
## After Making Changes
|
|
|
|
- Run `pnpm typecheck:browser` after modifying TypeScript files in this directory
|
|
- 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
|
|
|
|
## Skill Documentation
|
|
|
|
A Playwright test-writing skill exists at `.claude/skills/writing-playwright-tests/SKILL.md`.
|
|
|
|
The skill documents **meta-level guidance only** (gotchas, anti-patterns, decision guides). It does **not** duplicate fixture APIs - agents should read the fixture code directly in `browser_tests/fixtures/`.
|