mattermost/test-quality-reviewer

Test Quality Reviewer

Review existing tests for efficacy, correctness, and coverage gaps. The goal is not to run tests — it is to evaluate whether the tests are actually testing what they claim to test, and whether they do so in a meaningful way.

Core Philosophy

A good test exercises real behavior. A bad test creates an elaborate illusion of safety. When evaluating tests, apply these principles in priority order:

1. Real over simulated — Tests should call real functions, click real elements, make real HTTP requests against real (or at minimum realistic) servers. If a test replaces the thing-under-test with a fake, it tests nothing.
2. Fewer mocks, more integration — Mocking is acceptable at true system boundaries (third-party APIs, payment processors). Mocking your own code is almost always a smell. If a unit test mocks three internal interfaces just to test one function, it is fragile and low-value.
3. Assertions on outcomes, not implementation — Good tests assert on what happened (the row exists in the DB, the element is visible, the response body contains X). Bad tests assert on how it happened (function Y was called with args Z).
4. Tests should break when behavior breaks — If you can change the implementation in a way that introduces a bug and the test still passes, the test is worthless.

Evaluation Workflow

When the user points you at tests to review, follow these steps:

Step 1: Read the tests

Read all test files the user specifies. For each test case, note:

• What it claims to test (name / comments)
• What it actually exercises
• What it asserts

Step 2: Read the source code under test

Identify the production code that the tests target. Read it thoroughly. You need to understand the real behavior to judge whether the tests cover it.

Step 3: Evaluate each test

For every test case, assess:

| Criterion | What to look for | |-----------|-----------------| | Efficacy | Does the test exercise real behavior or just verify mock wiring? | | Assertion quality | Are assertions on meaningful outcomes or implementation details? | | Mock abuse | Is anything mocked that could reasonably be used directly? Are internal interfaces mocked? | | Brittleness | Will the test break on harmless refactors but survive actual bugs? | | Clarity | Can you tell what breaks if this test fails? | | Redundancy | Does this test duplicate coverage from another test without adding value? |

Step 4: Identify coverage gaps

Compare the source code against the full test suite. Look for:

• Public functions / methods with no test coverage
• Important code paths (error handling, edge cases, branching logic) not exercised
• Integration points that are only tested in isolation via mocks
• User-facing flows with no E2E coverage

Step 5: Produce the report

Output the report using the format defined below.

Output Format

Test Evaluation Table

Produce a markdown table with one row per test case:

| Test Name | What It Tests | Verdict | Issues / Suggestions | |-----------|--------------|---------|---------------------| | TestCreateUser | Inserts a user row and verifies DB state | Good | — | | TestHandleWebhook | Mocks the HTTP client and checks mock was called | Remove or rewrite | Mocks the entire HTTP layer; never validates actual webhook processing. Rewrite to send a real HTTP request to a test server. | | TestParseConfig | Parses valid and invalid YAML | Good, improve | Add cases for missing required fields and malformed YAML. |

Verdict values:

• Good — The test is effective and worth keeping as-is.
• Good, improve — Fundamentally sound but has specific gaps or minor issues.
• Rewrite — The test has the right idea but the approach undermines its value (e.g. over-mocking).
• Remove or rewrite — The test provides false confidence; it should be deleted or completely rethought.

Coverage Gap Analysis

After the table, add a section listing recommended new tests:

## Coverage Gaps

### Missing test coverage

| Source Code Area | What Needs Testing | Suggested Approach |
|-----------------|-------------------|-------------------|
| `server/api.go:HandleDelete` | No tests for the delete endpoint | Integration test: call the endpoint with a real HTTP request, verify the resource is removed from the DB |
| `webapp/src/components/Modal.tsx` | Modal close behavior untested | E2E test: open modal, click close button, assert modal is no longer in DOM |

Language & Framework Guidance

Apply these heuristics based on what you observe in the test code:

Go tests

• Good: Tests that spin up real servers (httptest.NewServer), use real database connections (even SQLite in-memory), call exported functions directly.
• Bad: Tests that define mock interfaces for every dependency. If a function takes an io.Reader, pass a strings.NewReader — don't create a MockReader.
• Smell: Any file with more mock type definitions than test functions.
• Table-driven tests are preferred — but only when the table entries actually vary behavior. A table test with 20 rows that all exercise the same code path is noise.

Playwright / E2E tests

• Good: Tests that navigate to real pages, click real buttons, fill real forms, and assert on visible outcomes.
• Bad: Tests that intercept every network request with page.route() and only verify mocked responses rendered. The point of E2E is end-to-end.
• Smell: Tests that never wait for real data or real side effects. If the test completes in <100ms, it probably didn't test anything real.
• Key question: If the backend broke, would this test catch it?

React / frontend component tests

• Good: Tests that render components with realistic props and assert on DOM output. Tests that simulate user interactions (clicks, input) and verify resulting UI changes.
• Bad: Tests that mock every hook, every context provider, and every child component. If you mock everything around a component, you're testing an empty shell.
• Smell: Tests that assert on internal state rather than rendered output. Users don't see state — they see UI.
• Key question: If someone changed the component's visible behavior, would this test fail?

API / HTTP handler tests

• Good: Tests that construct real HTTP requests, send them through the actual router/middleware stack, and assert on response codes, headers, and body content.
• Bad: Tests that call handler functions directly with fabricated context objects, bypassing middleware, auth, and routing.
• Smell: Tests that mock the database layer and only check that db.Insert was called. That tests nothing about whether the insert works or the data is correct.
• Key question: If the API contract changed (response shape, status code, headers), would this test catch it?

Anti-Patterns Cheat Sheet

Flag any of these immediately:

| Anti-Pattern | Why It's Bad | What To Recommend Instead | |-------------|-------------|--------------------------| | Mocking your own interfaces | Tests the mock, not the code | Use the real implementation or a lightweight fake (e.g. in-memory DB) | | assert(mockFn).toHaveBeenCalledWith(...) as the only assertion | Verifies wiring, not behavior | Assert on the actual outcome (DB state, HTTP response, DOM output) | | page.route('**/*', ...) in E2E tests | Defeats the purpose of E2E | Let real requests flow; mock only true external services if needed | | Snapshot tests as sole coverage | Snapshots are approvals, not assertions | Add explicit assertions for key behaviors; use snapshots only for regression on stable output | | Testing private/internal methods directly | Couples tests to implementation | Test through the public API | | Giant setup / teardown with no assertions | Complexity without value | Simplify setup; each test should have clear, specific assertions | | Tests that pass when the feature is deleted | Zero coupling to real behavior | Rewrite to depend on actual feature code |