reviewing-unit-tests

Reviewing Unit Tests for ComfyUI_frontend

Overview

Review for behavior and current repo rules, not motion. Compare to authoritative rules, not prior diffs or legacy snippets.

Review Workflow

1. Identify the test type: component, store, composable, util, or bugfix regression.
2. Name the behavior the test proves. If you cannot say it in one sentence, request changes.
3. Open the authoritative doc section before judging structure.
4. Scan the red flags below.
5. State the verdict first. Name the failure mode. Cite the doc or rule.

Source of Truth / Precedence

When docs and examples conflict, use this order:

1. Explicit repo rules, lint rules, and note blocks.
2. docs/testing/vitest-patterns.md
3. Rule sections in docs/testing/unit-testing.md, docs/testing/store-testing.md, and docs/testing/component-testing.md
4. Example snippets
5. Prior diffs

Apply these repo-specific clarifications:

• docs/testing/component-testing.md starts with the authoritative rule: new component tests use @testing-library/vue with @testing-library/user-event. The @vue/test-utils snippets below it are legacy examples.
• docs/testing/store-testing.md still contains as any examples. Treat them as legacy snippets, not approval for new or edited test code.
• If docs conflict, prefer the stricter newer rule and call out the doc ambiguity. Do not approve through it.
• Motion != fix.

30-Second Red Flags

Rationalization Table

Mocking Rules

• Fail helpers that do not remove repeated setup, encode domain meaning, or simplify assertions. Barely earning the abstraction is not enough.
• For composables with reactive or singleton state, define stable mock state inside the vi.mock() factory. Access it per test via the composable itself. See docs/testing/unit-testing.md "Mocking Composables with Reactive State".
• This does not ban local test data builders or per-test vi.spyOn(...).
• Mock seams, not the project-owned module you are trying to exercise. For store tests, prefer real Pinia plus createTestingPinia({ stubActions: false }) per docs/testing/vitest-patterns.md and docs/testing/store-testing.md.

Alias-by-Renaming

// Before
const mockAdd = vi.hoisted(() => vi.fn())

// After: same indirection, new name
function getToastAddMock() {
  return useToast().add
}

If the wrapper only renames or relays a mocked value, fail it. Inline the lookup at the call site or fetch the singleton mock via the documented pattern.

vi.mocked() Scope

• Flag casts whenever vi.mocked() would narrow correctly.
• Do not add vi.mocked() around assertion-only references just for style.

Reset Hygiene

• Flag per-mock mockClear() or mockReset() when vi.clearAllMocks() or vi.resetAllMocks() already runs in the relevant hook chain.
• Review for redundancy or broken state management. Do not bikeshed clearAllMocks vs resetAllMocks unless behavior depends on it.

Third-Party Seams

• Distinguish trivial hooks from behavior-rich APIs.
• Mocking single-method third-party hooks like primevue/usetoast is usually acceptable.
• That exception does not justify mocking behavior-rich third-party modules.

vue-i18n

• Never mock vue-i18n in component tests.
• Use real createI18n per docs/testing/vitest-patterns.md and the shared testI18n setup.

Test-Body Rules

Bugfix Regression Proof

For fix: PRs or bugfix diffs:

1. Identify the production change that fixes the bug.
2. Verify the new test fails on pre-fix code, or ask the author to show it.
3. If the test passes on broken code, request changes.

A regression test that never proves red does not pin the bug.

Review Output Rules

• State verdict before procedural questions.
• Do not lead with approval language like LGTM, just one nit or approve and move on?.
• Name the failure mode directly: alias-by-renaming, unnecessary cast, mocked i18n, mock-only assertion, unproven regression.
• Link the authoritative doc section in the review comment.
• If an explicit repo rule, lint rule, or authoritative doc note is violated, do not downgrade it to "minor deviation" or "nit".

Quick Reference

Key Files to Read