ComfyUI_frontend/.claude/skills/red-green-fix/reference/testing-anti-patterns.md

Testing Anti-Patterns for Red-Green Fixes

Common mistakes that undermine the red-green proof. Avoid these when writing the test commit (Step 1).

Testing Implementation Details

Test observable behavior, not internal state.

Bad — coupling to internals:

it('uses cache internally', () => {
  const service = new UserService()
  service.getUser(1)
  expect(service._cache.has(1)).toBe(true) // Implementation detail
})

Good — testing through the public interface:

it('returns same user on repeated calls', async () => {
  const service = new UserService()
  const user1 = await service.getUser(1)
  const user2 = await service.getUser(1)
  expect(user1).toBe(user2) // Behavior, not implementation
})

Why this matters for red-green: if your test is coupled to internals, a valid fix that changes the implementation may break the test — even though the bug is fixed. The green commit should only require changing source code, not rewriting the test.

Assertion-Free Tests

Every test must assert something meaningful. A test without assertions always passes — it cannot produce the red X needed in Step 1.

Bad:

it('processes the download', () => {
  processDownload('/models/checkpoints', 'model.safetensors')
  // No expect()!
})

Good:

it('processes the download to correct path', () => {
  const result = processDownload('/models/checkpoints', 'model.safetensors')
  expect(result.savePath).toBe('/models/checkpoints/model.safetensors')
})

Over-Mocking

Mock only system boundaries (network, filesystem, Electron APIs). If you mock the module under test, you are testing your mocks — the test will not detect the real bug.

Bad — mocking everything:

vi.mock('./pathResolver')
vi.mock('./validator')
vi.mock('./downloader')

it('downloads model', () => {
  // This only tests that mocks were called, not that the bug exists
})

Good — mock only the boundary:

vi.mock('./electronAPI') // Boundary: Electron IPC

it('resolves absolute path correctly', () => {
  const result = resolveModelPath('/root/models', '/root/models/checkpoints')
  expect(result).toBe('/root/models/checkpoints')
})

See also: Don't Mock What You Don't Own

Giant Tests

A test that covers the entire flow makes it hard to pinpoint which part catches the bug. Keep it focused — one concept per test.

Bad:

it('full model download flow', async () => {
  // 80 lines: load workflow, open dialog, select model,
  // click download, verify path, check progress, confirm completion
})

Good:

it('resolves absolute savePath without nesting under modelsDirectory', () => {
  const result = getLocalSavePath(
    '/models',
    '/models/checkpoints',
    'file.safetensors'
  )
  expect(result).toBe('/models/checkpoints/file.safetensors')
})

If the bug is in path resolution, test path resolution — not the entire download flow.

Test Duplication

Duplicated test code hides what actually differs between cases. Use parameterized tests.

Bad:

it('resolves checkpoints path', () => {
  expect(resolve('/models', '/models/checkpoints', 'a.safetensors')).toBe(
    '/models/checkpoints/a.safetensors'
  )
})
it('resolves loras path', () => {
  expect(resolve('/models', '/models/loras', 'b.safetensors')).toBe(
    '/models/loras/b.safetensors'
  )
})

Good:

it.each([
  ['/models/checkpoints', 'a.safetensors', '/models/checkpoints/a.safetensors'],
  ['/models/loras', 'b.safetensors', '/models/loras/b.safetensors']
])('resolves %s/%s to %s', (dir, file, expected) => {
  expect(resolve('/models', dir, file)).toBe(expected)
})

Flaky Tests

A flaky test cannot prove anything — it may show red for reasons unrelated to the bug, or green despite the bug still existing.

Common causes in this codebase:

Cause	Fix
Missing nextFrame() after canvas ops	Add await comfyPage.nextFrame()
waitForTimeout instead of assertions	Use toBeVisible(), toHaveText()
Shared state between tests	Isolate with afterEach / beforeEach
Timing-dependent logic	Use expect.poll() or toPass()

Gaming the Red-Green Process

These are ways the red-green proof gets invalidated during Step 2 (the fix commit). The test from Step 1 is immutable — if any of these happen, restart from Step 1.

Weakening the assertion to make it pass:

// Step 1 (red) — strict assertion
expect(result).toBe('/external/drive/models/checkpoints/file.safetensors')

// Step 2 (green) — weakened to pass without a real fix
expect(result).toBeDefined() // This proves nothing

Updating snapshots to bless the bug:

# Instead of fixing the code, just updating the snapshot to match buggy output
pnpm test:unit -- --update

If a snapshot needs updating, the fix should change the code behavior, not the expected output.

Adding mocks in Step 2 that hide the failure:

// Step 2 adds a mock that didn't exist in Step 1
vi.mock('./pathResolver', () => ({
  resolve: () => '/expected/path' // Hardcoded to pass
}))

Step 2 should only change source code — not test infrastructure.

Testing the Happy Path Only

The red-green pattern specifically requires the test to exercise the broken path. If you only test the case that already works, the test will pass (green) on Step 1 — defeating the purpose.

Bad — testing the default case that works:

it('downloads to default models directory', () => {
  // This already works — it won't produce a red X
  const result = resolve('/models', 'checkpoints', 'file.safetensors')
  expect(result).toBe('/models/checkpoints/file.safetensors')
})

Good — testing the case that is actually broken:

it('downloads to external models directory configured via extra_model_paths', () => {
  // This is the broken case — absolute path from folder_paths API
  const result = resolve(
    '/models',
    '/external/drive/models/checkpoints',
    'file.safetensors'
  )
  expect(result).toBe('/external/drive/models/checkpoints/file.safetensors')
})