Comfy-Org / Comfy-Org/ComfyUI_frontend

test: follow-up improvements to error systems E2E tests (from PR #10848 review)

Open
#10,960 0 comments 0 reactions 1 assignee Claimed by @jaeone94 View on GitHub
area:testing developer experience
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.