Comfy-Org / Comfy-Org/ComfyUI_frontend
test: follow-up improvements to error systems E2E tests (from PR #10848 review)
- Dominant language
- TypeScript
- Stars
- 2k
- Forks
- 699
- Avg merge
- 1d 7h
- Merged PRs (30d)
- 490
Description
## Follow-up: Constructive Review Items from DrJKL on PR #10848
This issue tracks all actionable improvement notes raised by @DrJKL during the review of #10848 (comprehensive E2E tests for error dialog, overlay, and errors tab).
> **Status update (2026-05-04):** Items 8 and 11 are resolved. Remaining items below are still actionable on `main`.
---
### 1. Use `ComfyPage` type in `triggerExecutionError` helper
**File:** `browser_tests/tests/errorOverlay.spec.ts` (line 126)
**Comment:** https://github.com/Comfy-Org/ComfyUI_frontend/pull/10848#discussion_r3047307203
The hand-rolled structural type `{ canvasOps: ..., page: Page, command: ... }` should be replaced with the proper `ComfyPage` import to avoid silent type drift.
```ts
async function triggerExecutionError(comfyPage: ComfyPage) {
```
---
### 2. Add `{ tag: '@ui' }` to the `Error dialog` describe block
**File:** `browser_tests/tests/errorDialog.spec.ts` (line 37)
**Comment:** https://github.com/Comfy-Org/ComfyUI_frontend/pull/10848#discussion_r3047307228
This is the only error test suite missing the `@ui` tag, causing it to be excluded from tag-filtered CI runs.
```ts
test.describe('Error dialog', { tag: '@ui' }, () => {
```
---
### 3. Strengthen weak `/\S/` assertions
**Files:** `browser_tests/tests/errorDialog.spec.ts` (line 83), `browser_tests/tests/errorOverlay.spec.ts` (line 143)
**Comment:** https://github.com/Comfy-Org/ComfyUI_frontend/pull/10848#discussion_r3047307240
The assertion `/\S/` only confirms at least one non-whitespace character exists. For a safety-critical report section, assert a known keyword from the error message or report structure (e.g., `Error on configure!` or `ComfyUI`).
---
### 4. Replace hardcoded `'properties-panel'` string with `TestIds`
**File:** `browser_tests/tests/errorOverlay.spec.ts` (lines 178, 203)
**Comment:** https://github.com/Comfy-Org/ComfyUI_frontend/pull/10848#discussion_r3047307245
The file correctly uses `TestIds.dialogs.*` everywhere else. Register `'properties-panel'` in `TestIds` if not already present and use it consistently.
---
### 5. Merge redundant missing-nodes tests
**File:** `browser_tests/tests/propertiesPanel/errorsTabMissingNodes.spec.ts` (lines 15, 23)
**Comment:** https://github.com/Comfy-Org/ComfyUI_frontend/pull/10848#discussion_r3047307250
"Should show MissingNodeCard" and "Should show missing node packs group" both load the same fixture and assert visibility of nested elements. Since `missingNodePacksGroup` is rendered inside `missingNodeCard`, the second test is implied by the first. Consider merging into a single test with two assertions.
---
### 6. Confirm `@oss` tag is wired into the test runner
**File:** `browser_tests/tests/propertiesPanel/errorsTabMissingModels.spec.ts` (line 80)
**Comment:** https://github.com/Comfy-Org/ComfyUI_frontend/pull/10848#discussion_r3047307251
`@oss` is the only usage of this tag across the entire test suite. Currently `playwright.config.ts` only references it via `grepInvert: /@oss/` in the `cloud` project, meaning these tests are excluded from cloud runs but there is no dedicated project that specifically runs `@oss`-tagged tests. Confirm the intended behavior and update `playwright.config.ts` accordingly.
---
### 7. Extract `openExecutionErrorTab` into `ErrorsTabHelper.ts`
**File:** `browser_tests/tests/propertiesPanel/errorsTab.spec.ts` (line 45)
**Comment:** https://github.com/Comfy-Org/ComfyUI_frontend/pull/10848#discussion_r3047307257
The "Search and filter" block manually replicates the execution-error tab-opening flow already present as `openExecutionErrorTab` in `errorsTabExecution.spec.ts` (line 17). Extract a shared `openExecutionErrorTab` into `ErrorsTabHelper.ts` alongside `openErrorsTabViaSeeErrors`.
---
### ~~8. Extract `cleanup_fake_model` into `ErrorsTabHelper.ts`~~ ✅ Done
Resolved: `cleanupFakeModel` is exported from `browser_tests/fixtures/helpers/ErrorsTabHelper.ts` and consumed across `errorOverlay.spec.ts`, `propertiesPanel/errorsTabMissingModels.spec.ts`, `propertiesPanel/errorsTabModeAware.spec.ts`, and `vueNodes/nodeStates/error.spec.ts`.
---
### 9. Consider making `runtimePanel` a Page Object
**File:** `browser_tests/tests/propertiesPanel/errorsTab.spec.ts` (line 67)
**Comment:** https://github.com/Comfy-Org/ComfyUI_frontend/pull/10848#discussion_r3047356567
The `runtimePanel` locator logic is a good candidate for encapsulation within a Page Object.
---
### 10. Encapsulate `openExecutionErrorTab` logic in the main Page Object
**File:** `browser_tests/tests/propertiesPanel/errorsTabExecution.spec.ts` (line 17)
**Comment:** https://github.com/Comfy-Org/ComfyUI_frontend/pull/10848#discussion_r3047362229
The local helper `openExecutionErrorTab` contains logic that belongs in the main Page Object for better reusability and maintainability.
---
### ~~11. Move `ErrorsTabHelper.ts` out of the `/tests/` directory~~ ✅ Done
Resolved: `ErrorsTabHelper.ts` now lives at `browser_tests/fixtures/helpers/ErrorsTabHelper.ts` (no longer under `/tests/`). All call sites import from `@e2e/fixtures/helpers/ErrorsTabHelper`.
---
### 12. Avoid hardcoded canvas coordinates in tests
**File:** `browser_tests/tests/errorOverlay.spec.ts` (line ~105)
**Comment:** https://github.com/Comfy-Org/ComfyUI_frontend/pull/10848#discussion_r3047391337
Specific pixel coordinates are brittle. Prefer using named positions from `defaultGraphPositions` or semantically locating elements instead of hardcoded coordinates.
---
**Source PR:** #10848
**Requested by:** @jaeone94
Contributor guide
Assessment
This issue has not been assessed yet.