matrixorigin Review Code: Test Quality
This is not a general code review. Answer one question: do the tests prove the changed behavior, especially when things go wrong?
Hard rule: flag production control flow that depends on natural-language text
matching. Safety, admission, routing, blocking, retry, recovery, evaluation, and
state transitions must use typed fields such as enums, ErrorKind,
result_class, exit_semantics, structured JSON, protocol parsers, AST/token
parsers, or exact machine-owned sentinel fields. Text matching is acceptable
only for UI display/search, rendered-text tests, or explicitly named legacy
fallback parsers that are not the primary decision path.
Task
$ARGUMENTS
Step 1: Resolve Changed Behavior
git status --short
git diff --stat
git diff --name-only
For each changed behavior, identify:
- public entrypoint or caller;
- state/persistence side effect;
- error/cancellation path;
- existing test file, if any.
Step 2: Look For Required Test Evidence
| Change signal | Required evidence |
|---|---|
| Public function/API/CLI command | Happy path plus at least one invalid input/error path |
| DB write or projection | Test asserts persisted state, not just Ok(()) |
| Restore/sync/checkpoint | Test covers missing, stale, duplicate, or partial data |
| Auth/permission/capability | Denied case and allowed case |
| State machine/lifecycle | Out-of-order, double-submit, retry, cancellation, terminal-state behavior; every consumer uses the producer-owned status vocabulary |
| Async task/channel/lock | Cancellation/timeout/cleanup or bounded queue behavior |
| Fanout/background work | Partial child completion does not wake/synthesize; lookup miss is not terminal; one canonical group wake after settlement |
| Prompt/tool/skill selection | Test proves the selection rule, not just string presence |
Use rg to find tests before reading them:
rg -n "<function|type|route|event_name>" crates tests web packages --glob '!target/**'
Step 3: Classify Gaps
Missing test:
- A reachable changed behavior has no test at the owning layer.
- A persistence mutation has no state assertion.
- An unhappy path is the main risk and only happy paths are tested.
Weak test:
- Only checks
is_ok()/is_err()with no effect assertion. - Mocks away the behavior being changed.
- Tests a helper while the bug lives at the integration boundary.
- Snapshot/string test is brittle and does not assert the contract.
Covered:
- Test reaches the public path, asserts the relevant state/output, and includes at least one failure/lifecycle edge when that edge is material.
- Async journey tests assert causal counts (model requests, terminal transitions, durable rows, and wake boundaries), not merely final rendered text or elapsed time. At least one real transport/public entrypoint test complements producer unit tests when the change crosses CLI/server/edge topology.
Output Contract
Missing Tests:
- <file:line> <behavior not proven, consequence>
Weak Tests:
- <test file:line> <why it can pass while behavior is broken>
Covered:
- <behavior> proven by <test>
Suggested Checks:
- <exact command(s), usually cargo test -p <crate> <filter>>
If coverage is adequate, say so and name the tests that prove it.