mirror of
https://github.com/NanmiCoder/claude-code-haha.git
synced 2026-10-10 03:43:11 +08:00
docs(agents): say what shape a regression test has to have
The existing rule — production changes need a same-area regression test — is satisfied by exactly the tests that failed to hold. ContextUsageIndicator.tsx was fixed three times in ninety minutes; all three fixes shipped tests, and all three tests covered only their own hop. 21 of the last 70 fix commits edit lines another fix wrote within 30 days. Coverage was never the missing signal. That file sits at 87% branch coverage. What the tests had in common was shape, so this writes the shape down, with the commit behind each rule: - drive the transition instead of assigning the state it produces (component tests call setState 744 times and a real store action 3 times, and assigned state cannot expose "A did not update B") - assert the invariant, not today's output (2262973a4asserted the bug and the next fix inverted that exact line) - cover both directions of any drop/merge rule (the replay guard was only ever tested for "discard the replay") - test the join, not each end (deleting ChatInput's refreshNonce term left 314 tests green) - never retune an existing test's inputs to keep it green (128f75ab5changed five tests' props rather than accept they described unreachable states) - do not mock the module under test - comparing content to decide identity means the identity exists upstream Plus the two blind spots worth checking rather than trusting: desktop/electron is not instrumented at all, and Bun's LCOV emits no branch records, so src/ and adapters/ report 100% branch coverage for data that was never collected. The desktop file gains the module-placement rules the Settings split and the reachability guard produced.
This commit is contained in:
@@ -26,12 +26,52 @@ Rules closer to the code take precedence. Before editing `.github/`, `src/`, `de
|
||||
|
||||
- Make narrow, owned diffs. Every changed line must trace to the request, a failing test, or a verified compatibility constraint.
|
||||
- Prefer existing utilities, stores, services, and test harnesses. Do not add dependencies or speculative abstractions unless the task requires them.
|
||||
- Production changes under `src/`, `desktop/src/`, or `adapters/` require a same-area regression test unless a maintainer explicitly approves an exception.
|
||||
- Production changes under `src/`, `desktop/src/`, or `adapters/` require a same-area regression test unless a maintainer explicitly approves an exception. A test that only covers the hop you just changed satisfies this rule and still lets the next change break — see "Writing a test that holds" below.
|
||||
- Keep TypeScript ESM style: 2-space indentation, no semicolons, `PascalCase` components, and `camelCase` functions/hooks/stores.
|
||||
- Use structured parsers and existing boundaries instead of ad hoc string manipulation. Add comments only for non-obvious control flow or external constraints.
|
||||
- Do not commit generated output such as `artifacts/`, coverage reports, `node_modules/`, build directories, or Rust `target/` trees.
|
||||
- When publishing is explicitly requested, use Conventional Commit subjects and normal product branch prefixes such as `fix/`, `feat/`, or `docs/`; do not create `codex/` branches in this repository.
|
||||
|
||||
## Writing a Test That Holds
|
||||
|
||||
Most regressions here are repairs of a recent repair: 21 of the last 70 `fix` commits
|
||||
edit lines another `fix` wrote within 30 days. Coverage is not the missing signal —
|
||||
`ContextUsageIndicator.tsx` sits at 87% branch coverage and was fixed three times in
|
||||
ninety minutes. What those tests had in common is shape, so choose it deliberately.
|
||||
|
||||
- **Drive the transition; never hand-write the state it produces.** Component tests in
|
||||
`desktop/src` call `setState` 744 times and a real store action 3 times. State you
|
||||
assigned is self-consistent by construction and cannot expose "transition A did not
|
||||
update B" — which is where these bugs live. Use `handleServerMessage`, store actions,
|
||||
and real user events.
|
||||
- **Assert the invariant, not today's output.** `2262973a4` shipped
|
||||
`expect(getByText('deepseek-reasoner'))` at a moment when the screen showed another
|
||||
model's number: it wrote the bug in as a passing assertion, and the next fix had to
|
||||
invert that exact line. Ask what must be true after this step, not what it prints now.
|
||||
- **Cover both directions of any rule that drops or merges something.** The replay guard
|
||||
was tested for "a replay must be discarded" and never for "a genuine repeat must be
|
||||
kept", so it shipped dropping real replies.
|
||||
- **Test the join, not each end.** Server, store, and component each had a test for
|
||||
`runtime_config_applied`; nothing crossed them, and deleting the term that joins them
|
||||
(`ChatInput.tsx` `refreshNonce`) left 314 tests green.
|
||||
- **Never retune an existing test's inputs to keep it green.** `128f75ab5` changed five
|
||||
tests' props (`messageCount={0}` → `{1}`) instead of accepting that they described
|
||||
states a real session cannot reach. If a test only passes after you edit its inputs,
|
||||
the test was describing the implementation.
|
||||
- **Do not mock the module under test.** A hand-written factory freezes an interface
|
||||
snapshot: the store can be renamed or gutted and the test still passes.
|
||||
- **If you are comparing content to decide identity, the identity exists upstream.**
|
||||
Deduping by text cannot separate a replay from a legitimate repeat; forward the id
|
||||
(`uuid`, `toolUseId`) instead of guessing.
|
||||
|
||||
Blind spots to check rather than trust:
|
||||
|
||||
- `desktop/electron/` is not instrumented at all (`vitest.config.ts` collects only
|
||||
`desktop/src`), so main-process diffs score zero covered lines.
|
||||
- Bun's LCOV emits no branch records, so `src/` and `adapters/` report **100% branch
|
||||
coverage** for data that was never collected (`pct(0, 0) === 100`). Only `desktop/`
|
||||
has real branch numbers.
|
||||
|
||||
## Verification
|
||||
|
||||
1. Run the narrowest relevant test while iterating.
|
||||
|
||||
@@ -4,6 +4,9 @@ These rules apply to `desktop/` changes in addition to the root instructions.
|
||||
|
||||
- Before adding or editing anything under `desktop/src/components/`, read `desktop/src/components/AGENTS.md`. It is the authoritative index of reusable components, the placement rules for new ones, and the required style/i18n/a11y/test conventions. Do not add a component that duplicates one listed there, and do not add new files to `components/shared/` or `components/common/`.
|
||||
- Reuse the existing desktop store/API patterns. Use `lucide-react` for common icons and keep operational UI dense, stable, and readable.
|
||||
- A new feature panel is its own module from the start. `Settings.tsx` reached 4639 lines holding seven unrelated panels before it was split into `pages/settings/*`; the four most-repeatedly-fixed files in the repository are also its four largest. Put a panel in its own file, and put anything two panels share in an explicit shared module rather than leaving it in the page that happens to host both.
|
||||
- Wire the component into its route in the same change that creates it. `src/__tests__/componentReachability.test.ts` fails on any `.tsx` no entry point can reach. Three components once lost their last import, and the coverage gate read "zero coverage" as "needs a test" — someone wrote suites for two of them, and a UI redesign then restyled all three.
|
||||
- Every translation key a component uses must be added to all five files in `src/i18n/locales/`, including keys chosen inside an expression (`t(count === 1 ? 'a' : 'b')`) — a literal-only scan misses those in both directions.
|
||||
- Add focused Vitest or Testing Library coverage for UI, store, or API behavior. Run it first, then follow `bun run check:impact`; desktop product changes normally select `bun run check:desktop`.
|
||||
- Chat transport, WebSocket lifecycle, first-turn runtime selection, reconnect, or session changes also require the offline `bun run check:chat-contract` when selected, plus `bun run check:agent-flow` for the end-to-end session/tool/permission/reconnect protocol.
|
||||
- Permission dialog, tool-call rendering, or approval-flow changes should also run `bun run check:desktop-ui-smoke`: it exercises the real dialog in a real browser against the mock runtime, with no provider.
|
||||
|
||||
Reference in New Issue
Block a user