feat(conventions): front-load testing + forms idiom guides (test churn 61%→16% live)#147
Merged
Conversation
Tests were 61% of edits on a live CRUD build (measured), with 16 iterations on a single test file — because the model has never known the stack's test idioms. New front-loaded `testing` guide, derived from the actual failure log + the scaffold's own reference tests, teaches each failure mode: - .test.ts vs .test.tsx (and never create BOTH — the unused twin is a knip orphan; the model agonized over this repeatedly and made both). - the hoisted api-client mock idiom (vi.hoisted + vi.mock) returning a CONCRETE typed object so `data` isn't `any` (the model reinvented this ~16×, fighting no-unsafe). - mockResolvedValueOnce vs mockResolvedValue + mock RESET between tests (deepseek-flagged pass-alone-fail-in-suite trap). - API route tests: createApp() + app.handle(new Request(...)) + type-guard the body (no `as`) + a local loginCookie helper. - the enforced test rules (no-focused-tests, no-conditional-expect, fake-timers, mirror). - re-read after the harness auto-formats a test (the not-found edit-reject churn). Wired: testing topic + TOPIC_RULES (test-sibling etc → pushes guide on first red) + pull_conventions enum. Tests lock the idioms + enum parity.
… smoke idiom
Live build4 (guide v1, A/B vs build3): the UI-mock idiom crushed the biggest hotspot —
Supplier.mutations.test.tsx 16 edits → 1, queries 3 → 1, route test 15 → 7. Residual
was a NEW 14-edit grind on supplier.service.test.ts: the model tried to unit-test a
DB-hitting Drizzle service in isolation (fighting `string | SQLWrapper` types) and mixed
test runners. Add two idioms from the scaffold's reference service test:
- apps/api tests run under `bun:test` (NOT vitest — vitest/vi.* are UI-only and fail to
resolve in apps/api).
- a DB-hitting `*.service.ts` is covered by the route test; its `*.service.test.ts` only
needs a minimal smoke test (`expect(typeof fn).toBe('function')`) to satisfy the
test-sibling floor — don't unit-test it against a live DB.
…earAllMocks Panel caught (2 reviewers): the testing guide offered vi.clearAllMocks() as an alternative for preventing leaked mock state between tests, but clearAllMocks() clears only call history — it does NOT reset queued mockResolvedValue(Once) return values, so the pass-alone/fail-in-suite leak the guide warns about would REMAIN. Correct to vi.resetAllMocks() (or per-mock mockReset), and explicitly steer AWAY from clearAllMocks.
…il() (kill invented FormEvent)
Live-build residual (build5 + build6, testing guide already active): the model
repeatedly invented React's `FormEvent` to type the submit handler ('FormEvent
doesn't actually exist') and used the deprecated `z.string().email()`, burning turns
fixing both. The scaffold's real idiom (read from valbuild4 auth forms): useForm<T>({
resolver: zodResolver(schema) }) + handleSubmit(onSubmit); the submit event is a
BaseSyntheticEvent fired as `void handleSubmit(onSubmit)(event)`; Zod uses top-level
z.email()/z.url()/z.uuid(). Front-load it so the model writes forms right first-try.
… + lock all 6 rule mappings Panel findings on the testing guide: - WRONG-IDIOM (real): the guide said no-conditional-expect bans expect inside try/catch, but the rule only walks if/loop/switch/ternary (NOT TryStatement) — teaching that false positive would make the model avoid legal try/catch. Corrected to 'if/loop/switch/ternary — assert unconditionally'. - MISSING-TEST: topicForRule only locked 3 of the 6 testing rules; now locks all 6 so a typo in test-file-mirrors-source / no-conditional-expect / fake-timers-must-be-restored can't silently disable the reactive PUSH. (Enum-parity finding was already covered by convention-index.test.ts's PULL_CONVENTIONS_TOOL sync test; smoke-test idiom is the scaffold's own sanctioned pattern for route-covered DB services, already scoped in the guide.)
…n audit event build6 hard-gate residual (#64): the model wrote update/delete service methods that the gate rejected — 'Mutating method X does not record an audit event' (a boringstack-own rule). The model can't satisfy it without knowing auditLogService exists. Front-load the idiom (from valbuild4 oauth/accounts services): import { AUDIT_ACTIONS, auditLogService } and `void auditLogService.record({ userId, action: AUDIT_ACTIONS.X, metadata })` after each create/update/delete; reads exempt; throw ApiError not error envelopes. New api-service topic + pull_conventions enum (+ parity lock) + guide test.
… handlers
build7 parked near-green (~65min) on react/jsx-no-bind: SupplierRow's edit/delete
handlers were defined as body-level arrows (recreated each render) → flagged where
passed to onClick, and the model couldn't operationalize the fix. The old jsx guide
('extract inline functions') wasn't specific enough — extracting to the BODY still
fails. Add the exact idiom (from valbuild4 NotificationListItem): a function passed to
a JSX prop must be a STABLE ref — for a row, give it an onEdit(id)/onDelete(id) prop
passed straight to onClick, with the parent supplying each via useCallback in
<feature>.hooks.ts; an arg-bound handler is a useCallback in the row's hook, never an
inline/body arrow. This is build7's capstone blocker — validating via build8.
…se schema + rhf resolver types Ran build8's parked log through the reviewer panel (harness-diagnose). Verdict: near-green-oscillation on two type residuals the model knew but couldn't land: - ROOT (deepseek-pro): the Readable<SuccessResponse> UI error comes from Elysia routes lacking a single-content-type response schema → openapi-fetch types data as Readable. api-service guide now: every route declares response: <Schema>; a Readable<SuccessResponse> in the UI means fix the ROUTE's response schema, not the UI (the model cycled UI/API ~30×). - rhf resolver (#67): .optional()/.default() on a form field makes Zod input≠output → Resolver/SubmitHandler mismatch. forms guide now: keep fields required + use defaultValues, type useForm<z.infer<typeof schema>>. Both grounded in the 4-reviewer diagnosis, not solo guessing. Validating via build9.
… set incl .stories.tsx build9 got past build8's type walls (panel fixes worked) but sprayed near-green restructuring SupplierList, tripping component-folder-structure: 'missing required siblings: SupplierList.hooks.ts, .types.ts, .stories.tsx, .test.tsx'. The guide listed tsx/hooks/index but omitted the REQUIRED .stories.tsx (Storybook) + .types.ts, so the model didn't create them. Now lists the full set (verified vs scaffold BillingPage / NotificationListItem folders): <Name>.tsx, .hooks.ts, .types.ts, .stories.tsx, .test.tsx, index.ts. Validating via build10.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Front-loads the boringstack testing and forms idioms the model kept getting wrong first-try, mined from live build logs (builds 3–6) and the scaffold's own reference tests.
Impact (measured, same supplier feature, pristine clone)
Supplier.mutations.test.tsx16 edits → 1;supplier.service.test.ts14 → 1.Testing guide covers
.test.ts vs .test.tsx (never both — knip orphan); the
vi.hoistedapi-client mock returning a concrete-typed object (kills theanythrash);mockResolvedValueOnce+ resetAllMocks between tests (pass-alone/fail-in-suite);bun:testfor apps/api (not vitest);createApp/app.handle/type-guard route tests + localloginCookie; DB-service smoke-test idiom; the enforced test rules; re-read after auto-format.Forms guide covers
useForm({ resolver: zodResolver })+handleSubmit(event isBaseSyntheticEvent, NOT the inventedFormEvent);z.email()not deprecatedz.string().email().Reviews
Multiple panel rounds; findings addressed (clearAllMocks→resetAllMocks, no-conditional-expect scope, all-6 rule mappings locked). Panel's remaining BLOCKs are the validate-flake false-positive (task #63), not content.