Repository navigation
Add a repository conformance suite - #469
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough
Merge Risk: 🔵 Low · up to
|
19c6b07 to
4233b4c
Compare
4233b4c to
2fb6b12
Compare
This comment was marked as outdated.
This comment was marked as outdated.
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
These comments were generated by Claude Opus 5.5 (Claude Code) and reviewed by me before posting.
|
⛏️ #475 changes the repository interface this suite defines. Cases to add:
|
describeAnalysisRepositoryConformance declares the tests every AnalysisRepository implementation must pass, given a store it can open repositories over and plant a newer build's draft in. It covers the whole-draft contract: an empty draft when none is stored, round trips of every record kind and optional field within one repository and across reopening, whole replacement (with or without a prior load), a payload shared across books kept and edited as one record, model-version stamping, JSON-plain values with optional fields omitted rather than null, copy semantics in both directions, isolation between source projects, and refusal to read or overwrite a newer build's draft. Record lists are compared in id order, since the seam promises no list order; arrays inside records must keep theirs. The AnalysisRepository docs now state the empty-draft, ordering and stamping rules the suite checks. draftRepository.conformance.test.ts runs it against DraftRepository over map-backed storage, failing on any warning DraftRepository logs. The map storage and silent logger move to test-helpers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The draft that replaces the full fixture kept every metadata field, so a backend that merged the saved draft over the stored one passed. It now drops every optional field and changes the required ones. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Keep any extra key on a loaded analysis when sorting record lists, so strict comparisons catch it. - Split the JSON-plain check into a saved-draft test and a never-saved one. - Describe exodusOnlyDraft as what it is: EXO's records, without the draft's optional fields. - Drop the two DraftRepository tests the suite already covers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A repository that loaded a draft but never saved it still caches what each shard held, so saving after another repository's save could skip a book that save deleted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DraftRepository reads a saved draft's contents after its first await, and overlapping saves from two repositories can store a mix of both, so the interface now promises caller changes only once a call settles and forbids overlapping calls for one draft. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A repository that kept the caller's saved draft by reference to serve the next load would pass a check through a newly opened repository alone. - AGENTS.md: the suite takes a factory for a fresh store per test. - memoryStorage: state each fact once in its doc comment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- After another repository replaces the draft, expect this one's load to return that draft, so serving loads from a cached copy of its own last save fails. - After changing a saved draft in place, save it again and expect the change stored, so keeping the passed draft as the baseline for the next save's diff fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The interface now names the books a load could not read, keeps such a book as stored on save unless told to discard it, settles a save once its content lands, and adds exportForCopy and markClean. The suite checks each of those at the seam, so a later backend owes them too. - ConformanceStore gains hooks to damage one book or the whole draft, fail a save's cleanup after its content lands, and report leftovers. - The DraftRepository runner tolerates warnings only in tests that damage storage or fail cleanup. - DraftRepository tests the suite now covers are dropped; those that check which keys are written or what bytes stay remain. - exportForCopy's doc names the empty draft it returns when none is stored. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A repository that copies a draft only shallowly keeps aliases to the glosses, morphemes and segmentation inside it, so the tests that change a draft after saving or loading it now change those too. Helper and hook docs in the suite say what each returns or does rather than which helper it delegates to or what each argument value means. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
f9ae365 to
71d3068
Compare
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
These comments were generated by Claude Opus 5.5 (Claude Code) and reviewed by me before posting.
- Change every record and link, and the values nested in them and in the segmentation, when testing what a repository holds on to. - Give saving a draft changed in place its own test. - Expect a failed cleanup to leave something behind, so a store whose failCleanup or holdsLeftovers hook is mis-wired fails. - Retitle the markClean test with no saved draft to what the seam shows. - In the DraftRepository runner, refuse only the failing draft's other records, and only between a save landing and the next call. - State in replaceAll's doc that a save leaves nothing the draft no longer needs and that a later save finishes a failed cleanup. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
These comments were generated by Claude Opus 5.5 (Claude Code) and reviewed by me before posting.
The Evidence table's "none 38 passed" is stale: splitting the in-place-change test made it 39.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
These comments were generated by Claude Opus 5.5 (Claude Code) and reviewed by me before posting.
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc reviewed 6 files and all commit messages, and resolved 21 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on imnasnainaec).
Part of #424; closes #432.
Summary
This adds a conformance suite at the repository seam: the tests every
AnalysisRepositoryimplementation must pass. It runs in CI againstDraftRepository.What the suite checks
exportAll)null.exportForCopy)replaceAll)discardUnreadable, a save writes or wipes the book like any other.markClean)Ground rules
AnalysisRepositorystates the empty-draft, ordering, stamping and caller-ownership rules, so the suite tests only what the interface promises.DraftRepositorystays in its own tests: the shard journal, rewriting only changed shards, the bytes a kept shard holds, the error each refusal throws.DraftRepositorytests the suite now covers are dropped. AGENTS.md says where each kind of test goes.Evidence
Unit suite: passes at 100% coverage, along with
tsc --noEmit, ESLint and Prettier.The suite can fail. Each mutation of
DraftRepositorywas run, then reverted:No unexplained warnings. The runner fails a test if
DraftRepositorylogs a warning, unless that test damaged storage or failed cleanup. The fixtures are valid drafts it accepts without complaint.Deviations from the issue and the plan
exportAll,exportForCopy,replaceAllandmarkClean, the calls the interface has.apply, undo and the index lookups join it as 3b ([3b] Add apply with inverse capture and per-session undo #441) and 3c ([3c] Add derived indexes with a rebuild-and-diff command #442) build them. 3b's done-when already names conformance tests.exportAllandreplaceAll. Most of today's tests get rewritten against the reads that replace them. The suite itself stays.DraftRepositoryover map-backed storage, in the same sense as Route getDraft and saveDraft through an async repository (#431) #466.memoryStorageandsilentLoggermoved totest-helpers.ts. The new runner needs both, andsilentLoggeralready had two copies.Decisions with a strong alternative
Reviewer push-back is welcome.
DraftRepositoryalready regroups records by book on load.describe.eachover implementations.projectStorage's guards"), and Move Save and Save As into the host #475 made unreadable books and settling once content lands part of the interface. The suite checks each through the seam: a load still refuses the draft or still names the book, andholdsLeftoverssays whether a later save finished cleanup.openis asyncopen, matchingDraftRepository's constructor.perf/storage.ts)DraftRepositorysafe under them.DraftRepositorydoesn't have.AnalysisRepositoryinstead forbids overlapping calls for one draft, whether on one repository or several. After one repository saves a draft, any other that had already loaded or saved it must load it again before its next save.DraftRepositorylearns which books are unreadable only by loading, so such a save writes over a corrupt book, or wipes it if the draft carries nothing for it.replaceAll's doc says the book is kept unlessoptionsdiscards it, without that condition. Either the doc orDraftRepositoryshould change; that belongs in a follow-up, not a test PR.Comments on other issues
Merge Danger
Door: two-way
Only tests and docs change, plus one shared-helper move. A revert removes the suite and restores the dropped
DraftRepositorytests.Blast Radius: tests
Nothing ships differently. A later change to
DraftRepositoryor the seam must keep the suite passing, and a new backend must implement the store hooks to run it.🤖 Generated with Claude Code
This change is
Summary by CodeRabbit