DEVX-104: Adopt Fishery as standard test-factory library - #15
Merged
allenahner merged 4 commits intoApr 20, 2026
Merged
Conversation
Phase 1 (audit) → Phase 2 (migrate) → Phase 3 (codify) per the ticket. Factories - shared/src/test-factories/ (via `@rootscribe/shared/test-factories` subpath export): recordingDetail, jiraLink, inboxMutationResponse, recordingRow, plaudRawRecording. Chainable traits encode status variants (`reviewed()`, `snoozed()`, `archived()`, `pendingAudio()`, `audioOnly()`) and data variants (`withJiraLinks`, `withTags`, `withTranscript`, `withSummary`, `withError`). - web/src/test-factories/: appConfig, syncStatusResponse, recordingsListResponse, recordingDetailResponse, inboxFiltersProps. - server/src/test-factories/: fileDetailData, seedRecording. - inbox-mcp/tests/factories/: seedRecording (lives under tests/ because the SeedRecording type is test-only in tests/helpers/schema.ts). Call-site migrations - web (12 files): InboxActions, InboxFilters, InboxNotesEditor, CategoryEditor, JiraLinksEditor, SyncStatusBadge, TagEditor, Dashboard, RecordingDetail, Settings, ReviewStep, JiraStep, recordingCache. Collapses 3 `makeConfig` clones and 8 inline SyncStatusResponse literals. - server (5 files): plaud/list, plaud/detail, sync/poller, webhook/post, sync/state. poller's dead `sn`/`is_historical`/`trans_status` fields (cast past TypeScript) are dropped — no production code reads them. - inbox-mcp's 38 `seedRecording(dbFile, sparse)` call sites stay as-is: the existing helper is already factory-shaped (sparse-input + pick() defaults), so wrapping it in Fishery would add ceremony without clarity. Factory exists for future tests that want a fully-hydrated SeedRecording with traits. Config + infra - Added `./test-factories` subpath export to shared/package.json. - Extended web/ and server/ vitest.config.ts alias arrays so Vite resolves `@rootscribe/shared/test-factories` to src (matching the existing `@rootscribe/shared` → src pattern). - Added `**/test-factories/**` to coverage exclude in root vitest.config.ts. Factory modules are test support, not production code; every exercised line is covered via the specs that call them, but counting rarely-used traits toward production coverage would create pressure to inline factories. - Removed `makeRecordingDetail` and `makeInboxMutationResponse` from web/src/test-utils.tsx — superseded by the shared factories. The module now only owns render infrastructure (TestProviders, renderWithProviders, stubFetch, jsonResponse). Docs - CLAUDE.md Testing + CI: new bullet pointing at each factory location + the 2-file-or-5-field cutoff rule. - README.md Running tests → Philosophy: new Fishery bullet alongside the existing TDD / real-behavior / harness-per-package items. Verification - 776 unit + integration tests pass (25 web files, 25 server, 2 inbox-mcp, 2 shared) via `pnpm test:coverage`. - Coverage 95.35% stmts / 86.57% branches / 95.89% funcs / 97.10% lines — all above the ratchet #7 thresholds from DEVX-102 (95/86/95/96). - `pnpm lint` + `pnpm typecheck` both clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR standardizes on Fishery for constructing test fixtures across the RootScribe monorepo, introducing shared and package-local factories and migrating existing tests off ad-hoc make* helpers and repeated object literals.
Changes:
- Added shared and per-package Fishery factories (with chainable traits) and migrated impacted web/server tests to use them.
- Introduced
@rootscribe/shared/test-factoriesas a subpath export and updated Vitest alias resolution to support it without requiring a shared build step. - Updated docs and tooling (coverage excludes, READMEs/CLAUDE.md guidance) to codify the new fixture-building standard.
Reviewed changes
Copilot reviewed 45 out of 46 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| web/vitest.config.ts | Adds subpath alias for @rootscribe/shared/test-factories (ordered before base alias). |
| web/src/test-utils.tsx | Removes makeRecordingDetail / makeInboxMutationResponse; keeps render/test harness utilities only. |
| web/src/test-factories/syncStatusResponse.ts | Introduces Fishery factory for SyncStatusResponse variants used in UI tests. |
| web/src/test-factories/recordingsListResponse.ts | Adds list-response factory with transient items and derived totalBytes. |
| web/src/test-factories/recordingDetailResponse.ts | Adds detail-response factory wrapping shared recordingDetailFactory. |
| web/src/test-factories/index.ts | Central export barrel for web-only test factories. |
| web/src/test-factories/inboxFiltersProps.ts | Adds factory for InboxFilters props. |
| web/src/test-factories/appConfig.ts | Adds AppConfig factory consolidating duplicated config builders across tests. |
| web/src/routes/setup/ReviewStep.test.tsx | Migrates config fixture creation to appConfigFactory. |
| web/src/routes/setup/JiraStep.test.tsx | Migrates config fixture creation to appConfigFactory. |
| web/src/routes/Settings.test.tsx | Migrates config and sync-status fixtures to factories. |
| web/src/routes/RecordingDetail.test.tsx | Migrates detail response construction to factories. |
| web/src/routes/Dashboard.test.tsx | Migrates row/list response fixtures to factories. |
| web/src/lib/recordingCache.test.ts | Migrates RecordingDetail fixtures to shared recordingDetailFactory. |
| web/src/components/TagEditor.test.tsx | Migrates mutation response fixtures to shared factories. |
| web/src/components/SyncStatusBadge.test.tsx | Replaces repeated inline sync-status literals with factory traits. |
| web/src/components/JiraLinksEditor.test.tsx | Migrates config + mutation response + link fixtures to factories. |
| web/src/components/InboxNotesEditor.test.tsx | Migrates mutation response fixtures to shared factories. |
| web/src/components/InboxFilters.test.tsx | Migrates prop construction to inboxFiltersPropsFactory. |
| web/src/components/InboxActions.test.tsx | Migrates recording/mutation fixtures to shared factories and traits. |
| web/src/components/CategoryEditor.test.tsx | Migrates mutation response fixtures to shared factories. |
| web/package.json | Adds Fishery as a dev dependency for web tests. |
| vitest.config.ts | Excludes **/test-factories/** from coverage aggregation. |
| shared/src/test-factories/recordingRow.ts | Adds shared factory for RecordingRow with status/audio traits. |
| shared/src/test-factories/recordingDetail.ts | Adds shared factory for RecordingDetail with status/content traits. |
| shared/src/test-factories/plaudRawRecording.ts | Adds shared factory for upstream PlaudRawRecording. |
| shared/src/test-factories/jiraLink.ts | Adds shared factory for JiraLink including unsafe-url trait. |
| shared/src/test-factories/index.ts | Adds shared factories barrel export for subpath consumption. |
| shared/src/test-factories/inboxMutationResponse.ts | Adds shared factory for inbox mutation response wrapper. |
| shared/package.json | Adds ./test-factories exports subpath and Fishery dev dependency. |
| server/vitest.config.ts | Adds subpath alias for @rootscribe/shared/test-factories (ordered before base alias). |
| server/tests/webhook/post.test.ts | Migrates row fixtures to recordingRowFactory. |
| server/tests/sync/state.test.ts | Migrates Plaud raw recording fixtures to plaudRawRecordingFactory. |
| server/src/test-factories/seedRecording.ts | Adds server-only factory for SeedRecording (unit-test support). |
| server/src/test-factories/index.ts | Adds server test factories barrel export. |
| server/src/test-factories/fileDetailData.ts | Adds factory for Plaud /file/detail FileDetailData and content list traits. |
| server/src/sync/poller.test.ts | Migrates Plaud item + row fixtures to shared factories. |
| server/src/plaud/list.test.ts | Migrates Plaud raw recording fixtures to shared factory. |
| server/src/plaud/detail.test.ts | Migrates file detail fixtures to fileDetailDataFactory. |
| server/package.json | Adds Fishery as a dev dependency for server tests. |
| pnpm-lock.yaml | Locks Fishery and its transitive deps across the workspace. |
| inbox-mcp/tests/factories/seedRecording.ts | Adds inbox-mcp-local SeedRecording factory for future tests. |
| inbox-mcp/tests/factories/index.ts | Adds inbox-mcp factories barrel export. |
| inbox-mcp/package.json | Adds Fishery as a dev dependency for inbox-mcp tests. |
| README.md | Documents Fishery as the standard approach for test fixtures and where factories live. |
| CLAUDE.md | Codifies Fishery usage rules and the “2+ specs or 5+ fields ⇒ factory” cutoff. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Three unresolved threads — all legitimate concerns worth the round-trip.
1. web/src/test-factories/inboxFiltersProps.ts — InboxFilters was
imported as `import type`, then referenced via `typeof InboxFilters`.
Under NodeNext/ESM, `import type` erases the value side so `typeof`
doesn't resolve to the component's type. The current code happened
to compile because React's global namespace carries ComponentProps,
but the pattern is fragile. Switched to a value import of
InboxFilters and an explicit `import type { ComponentProps } from
"react"` — portable and doesn't lean on the global.
2. shared/src/test-factories/recordingRow.ts — `pendingAudio()` set
`audioPath: null`. Real server rows produced by upsertFromPlaud
(server/src/sync/state.ts:52-74) populate all four `*_path`
columns at INSERT time from recordingPaths(); status is derived
purely from `audio_downloaded_at IS NULL` (server/src/db.ts:222).
Changed pendingAudio() to set plausible non-null paths
(audio.ogg / transcript.json / summary.md / metadata.json) so
factory output models real DB rows. Tests that model the pending
audio state now see the same shape production does.
3. shared/src/test-factories/recordingDetail.ts — same divergence,
same fix. Both factory traits now stay in lockstep with server
semantics.
Verification: `pnpm typecheck` + `pnpm test` clean (776 passed across
54 files). No changes needed in call sites — existing tests that
overrode audioPath explicitly still pass, and tests that relied on
pendingAudio defaults get paths that now match production instead of
a divergent null.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 46 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Three more legitimate Copilot concerns after the previous push.
1. web/src/components/JiraLinksEditor.test.tsx — ROOT_101 fixture
overrode `issueKey: "ROOT-101"` but let `issueUrl` inherit from the
factory's sequence-derived default. Module-scope builds run at file
import time, so if another file in the same Vitest worker has
already used jiraLinkFactory, the sequence counter advances and
issueUrl drifts to .../ROOT-107 (mismatching the pinned issueKey).
Added explicit `issueUrl` to the override.
2. web/src/components/InboxActions.test.tsx — mutationResponseFor()
spread `{ inboxStatus }` onto recordingDetailFactory.build(),
leaving effectiveInboxStatus/reviewedAt/snoozedUntil at their "new"
defaults. That produced impossible combinations (e.g. inboxStatus=
reviewed + effectiveInboxStatus=new) that real API responses never
emit and that could mask cache-update bugs. Switched to the
chainable status traits (.reviewed() / .archived()) so all derived
fields stay consistent.
3. web/src/test-factories/recordingDetailResponse.ts — mediaBase was
hard-coded to "/media/rec-1" regardless of the wrapped recording.
Callers that override the recording's id via .withRecording(...)
would get a wrapper still pointing at /media/rec-1 — a divergence
that could let URL-building bugs slip past tests. Derived
mediaBase from the built recording's id.
Verification: pnpm typecheck + pnpm test clean (776 passed).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 46 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot flagged `filter: "all" as RecordingsListFilter` as a type-safety
weakener: the cast compiles even if the union loses "all" later.
Fishery's `.define(() => ({...}))` widens string literals, so just
writing `filter: "all"` fails to compile (inferred as `string`). The
right fix is `as const`, which preserves the "all" literal through the
inference AND still errors if the union narrows to exclude "all" —
unlike `as RecordingsListFilter`, which would silently compile.
Also: dropped the now-unused `RecordingsListFilter` import and retyped
`withFilter`'s parameter as `InboxFiltersProps["filter"]` so the factory
stays coupled to whatever the component's prop type currently is.
Verification: pnpm typecheck clean; InboxFilters.test passes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 46 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Summary
Establishes Fishery as the standard for building test objects across all four RootScribe packages, replacing hand-rolled
make*helpers with chainable-trait factories. Three-phase delivery per the ticket: audit → migrate → codify.Resolves: DEVX-104
Phase 1 — Audit inventory
Ripgrep across every
*.test.ts/*.test.tsxinweb/,server/,inbox-mcp/identified these shapes. Cutoff: 2+ spec files OR 5+ fields → factory; one-offs <5 fields stay inline.shared — became factories (all 5 are cross-package types)
RecordingDetailRecordingRowPlaudRawRecordingJiraLinkInboxMutationResponseweb — became factories
AppConfigmakeConfig(Settings, ReviewStep, JiraStep)SyncStatusResponseSyncStatusBadge.test.tsxRecordingsListResponseRecordingDetailResponseInboxFiltersPropsserver — became factories
FileDetailDatacontent_listSeedRecording(server)SEED_RECORDINGSfixed-data array is intentionally NOT migrated — its specific values are locked by Playwright assertions)inbox-mcp — exemption (documented in CLAUDE.md)
SeedRecording(inbox-mcp)inbox-mcp/tests/factories/seedRecording.tsfor future use, but the 38 existingseedRecording(dbFile, sparseObj)call sites are deliberately NOT migrated. The existingseedRecording()helper already provides factory-shaped semantics via sparse-input +pick()defaults; wrapping each call site in Fishery would add ceremony without clarity. The factory is there for any new inbox-mcp test that needs a fully-hydrated SeedRecording with multiple traits combined.Stays inline (below cutoff)
PlaudListResponse(4 fields),FileDetailResponse(4 fields),TranssummResponse(6 fields, single file, single-purpose),TranscriptSegment(5 fields, single file),ContentListItem(2 fields),linkJiraparams (4 fields),Configpartials in route tests (2-4 fields each).Phase 2 — Migration highlights
class extends Factory<T>+this.params(). Status traits are past-tense verbs (reviewed(),archived(),snoozed(),pendingAudio(),audioOnly()), data traits usewith*(withJiraLinks(...),withTags(...),withTranscript()). Matches FactoryBot's:admin(state) vswith_items(association) distinction.shared/src/test-factories/and export via a new@rootscribe/shared/test-factoriessubpath. The web/ and server/ vitest configs gained a subpath alias ahead of the bare-package alias (array form, more-specific-first) so Vite resolves@rootscribe/shared/test-factoriesto source without a shared-build step.makeRecordingDetailandmakeInboxMutationResponsedeleted fromweb/src/test-utils.tsx; that module now owns only render-infrastructure helpers (TestProviders,renderWithProviders,stubFetch,jsonResponse).server/src/sync/poller.test.ts'smakePlaudItemhadsn/is_historical/trans_statusfields cast past TypeScript; grep confirmed no production code reads them. Normalized to the realPlaudRawRecordingshape via the factory.**/test-factories/**added to rootvitest.config.ts. Factories are test support, not production code; every exercised line is covered via the specs that call them, but counting rarely-used traits toward production coverage would pressure contributors to inline factories or delete traits to move the percentage.Phase 3 — Codification
Test plan
pnpm test:coverage— 776 tests pass. Coverage: 95.35% stmts / 86.57% branches / 95.89% funcs / 97.10% lines — all above the ratchet DEVX-103: Rename applaud to rootscribe across the repo #7 thresholds (95/86/95/96) from DEVX-102.pnpm lint— clean.pnpm typecheck— clean across all four packages.ci.yml) runs the same three above + the Vitest suite on Node 20 and 22.e2e.yml) exercises the Playwright smoke suite — E2E seed data (SEED_RECORDINGSinserver/src/test-seed/fixtures.ts) was intentionally left as hand-written values to keep Playwright assertions stable, so this should pass unchanged.Follow-ups (not in scope)
inbox-mcptests should prefer the factory; existing sparse call sites stay.SEED_RECORDINGSarray could be rewritten usingseedRecordingFactorywith explicit per-entry traits — deferred because the current fixed-value approach is what Playwright asserts against and changing it risks e2e churn without clear benefit.🤖 Generated with Claude Code