fs/internal/imptest: cross-backend ocflfs contract test suite - #170
Merged
Conversation
fs/local and fs/s3 were tested independently, with no shared assertions. Nothing caught the two backends disagreeing about the ocflfs.WriteFS contract, which is where the defects in #163-#168 live: a backend can look correct against its own tests and still be wrong in a way that only shows up when a caller swaps local storage for S3. Five entry points, each run against both backends: TestWriteFSWriteContract a shorter overwrite fully replaces (a tail of the previous content is the tell for an in-place write); a failing source leaves the target as it was; an invalid path is rejected with fs.ErrInvalid and no side effect. TestWriteFSRemoveContract a missing file is fs.ErrNotExist on every backend; "." errors and leaves the storage root readable. TestWriteFSRemoveAllContract absent is success, the documented inversion of Remove; the subtree goes and nothing else does; "a" is a path element, not a string prefix, so "ab", "abc.txt" and "a-sibling.txt" survive. TestDirEntriesContract entries are base names, one level deep, sorted. A flat listing naturally yields full keys and the whole subtree, and a caller that joins the entry onto the directory it asked for then builds "dir/dir/file". TestWalkFilesContract early termination when yield returns false; a walk error delivered as the pair's error element with nothing yielded after it; every *FileRef carrying the backend's own instance in FS. Each config struct carries only what is legitimately backend-specific (RemoveDotIsNotExist, RemoveAllDotIsError, RemoveAllOnFileRemovesIt, WriteDotIsError, and the WalkFiles error fixture). Where both backends agree the suite asserts outright, which is why TestDirEntriesContract takes no struct at all. This lands before the fixes, so three cases main does not honor are guarded by a Skip field naming the issue that closes them, for the fixing PR to delete: local Write is not atomic (#163), s3 Remove of a missing key returns nil (#166), and fileWalk panics on an error-yielding DirEntries (#165). The last also gets a full regression test in fs/walk_contract_test.go, skipped for now. All three were verified to fail with the skip removed. The suite lives under fs/internal rather than the top-level internal so it stays scoped to the fs tree. Contract callers are external test packages (local_test, s3_test) so the suite can grow backend fixtures without an import cycle. fs/s3/internal/mock gains an ordered call log with query helpers (Calls, CallsFor, CallCount, KeysFor, KeyBatchesFor), guarded by its own mutex because every method records, including the ones already holding the MPU lock. KeyBatchesFor is the batching-sensitive view #166 needs to tell one DeleteObjects of 500 keys from 500 DeleteObject calls. Recording lives in the mock rather than in per-test wrapper types, so it covers every method rather than the two a given test remembered to wrap. Two mock fidelity gaps surfaced while writing the contracts, both of which limited every S3 test to asserting which requests were sent rather than what the bucket ended up holding: PutObject did not materialize the object, and DeleteObject recorded the key in the Deleted map without removing it. Both now behave like a real store; Deleted stays as call bookkeeping, documented as such. No existing test depended on the old behavior. Refs #162. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JnxfAdBrXieiGaAENQ74Hh
The suite was one level deeper, at fs/internal/testutil, which put a second package named testutil in the module alongside the top-level internal/testutil. Nothing needed the extra directory: fs/internal holds only this suite, and the shorter path spells out that it belongs to the fs tree. Mechanical move — package testutil becomes package internal, testutil.go becomes doc.go, and the callers in fs/local and fs/s3 now read internal.TestWriteFSRemoveContract(...). No contract, knob, or skip changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JnxfAdBrXieiGaAENQ74Hh
The package had no name of its own — it was `package internal` at fs/internal,
which says where it lives and nothing about what it is. It is now imptest, the
fs implementation test suite: assertions every implementation of the ocflfs
interfaces must satisfy, run against each backend from that backend's own
tests.
The word "contract" is gone from exports, file names and docs. It was doing
two jobs at once — naming the suite and naming the thing being tested — and
neither reading was carried by the word itself:
TestWriteFSWriteContract -> TestWriteFSWrite
TestWriteFSRemoveContract -> TestWriteFSRemove
TestWriteFSRemoveAllContract -> TestWriteFSRemoveAll
TestDirEntriesContract -> TestDirEntries
TestWalkFilesContract -> TestWalkFiles
and each entry point's options struct loses the suffix with it
(WriteFSWriteContract -> WriteFSWrite, and so on), so the struct now reads as
the options for the test that takes it: imptest.TestWalkFiles(t, fsys,
imptest.WalkFiles{...}). Files follow: write_contract.go is write.go, the
backend callers are imptest_test.go, and fs/walk_contract_test.go is
walk_test.go with TestWalkFiles_DirEntriesError.
Prose that said "the contract" now says what it means — the behavior every
implementation must share, what the interface documents, what the suite pins.
The fixture path prefixes move with the package (write-contract/new.txt is
imptest-write/new.txt) so a stray file in a failing test still names its
source.
No assertion, knob, or skip changed. The same three skips stand: local Write
atomicity (#163), s3 Remove of a missing key (#166), and the fileWalk
nil-deref (#165).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JnxfAdBrXieiGaAENQ74Hh
doc.go says only that the file is documentation; imptest.go names the package it introduces, matching the file-per-entry-point naming beside it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JnxfAdBrXieiGaAENQ74Hh
WriteDotIsError let a backend opt out of rejecting "." as a write target, on the theory that a backend without directories might treat it as an ordinary key. No backend wants that: both callers set it true, and the one backend the escape hatch was written for — s3, which has no directories — guards "." explicitly anyway. Worse, the knob only asserted that the write failed, not how, which let the local backend pass without a guard of its own: fs.ValidPath accepts ".", so the rejection came from the kernel refusing to open the storage root for writing. That EISDIR matches neither fs.ErrInvalid nor anything else a caller can test for, and it would have quietly disappeared under a write-to-temp-and-rename refactor. Drop the field, assert errors.Is(err, fs.ErrInvalid) outright, and add the guard local.FS.Write was missing — matching the s3 backend and the suite's treatment of every other bad name.
RemoveDotIsNotExist let each backend classify Remove(".") its own way: the s3
backend returned fs.ErrNotExist, the local backend a bare errors.New that
matched nothing. A caller could not recognize the refusal without knowing
which storage it was talking to, which is exactly the disagreement this suite
exists to catch — so the knob was encoding a defect rather than a difference.
fs.ErrNotExist is the wrong half of that pair to standardize on: "." names the
storage root, which is the one path guaranteed to exist. Reporting it absent
invites a caller to conclude there is nothing to clean up. It is a bad name,
not a missing file — the same reading openFile and Write already give it (s3
rejects "." with fs.ErrInvalid in both, and local.FS.Write gained the matching
guard in the previous commit). Both backends now return fs.ErrInvalid, both
render as "remove .: invalid argument", and WriteFS.Remove says so.
RemoveAll(".") is untouched and keeps RemoveAllDotIsError: there the backends
genuinely differ in capability rather than in error text — s3 can empty its
bucket, local cannot remove its own root — so no single answer is available to
standardize on.
Verified by mutation: restoring the s3 fs.ErrNotExist fails the dot subtest.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JnxfAdBrXieiGaAENQ74Hh
The Skip-fields example still passed RemoveDotIsNotExist, a field that no longer exists, and the options-struct section still described a world where knobs were the norm. Two have since been deleted — WriteDotIsError and RemoveDotIsNotExist — leaving WriteFSRemoveAll as the only struct with real knobs, WriteFSWrite and WriteFSRemove carrying nothing but Skip fields, and TestDirEntries taking no struct at all. Say that, and say what the two deletions taught: a field that lets each backend name a different error is usually recording a defect rather than a difference, and belongs in an assertion once the backends agree. Also distinguish WalkFiles.ErrWalk, which is a fixture and not a knob — the assertions about the failure it supplies are the same for every backend — and name the three skips currently outstanding. Doc-only; no code changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JnxfAdBrXieiGaAENQ74Hh
…p fields The Skip* fields made each caller declare which assertions its backend fails, threading a reason string through the options struct to reach a t.Skip inside the suite. That is machinery for something simpler: these subtests are notes about work to come, not per-backend configuration. Skip them where they are written, under a TODO naming the issue that closes the gap. SkipFailedSourceKeepsFile -> TODO(#163) + t.Skip in write.go, x2 SkipMissingIsNotExist -> TODO(#166) + t.Skip in remove.go SkipWalkErrors -> TODO(#165) + t.Skip in walkfiles.go With the fields gone, WriteFSWrite and WriteFSRemove hold nothing at all, so both entry points drop their options parameter and join TestDirEntries in taking just a backend. WalkFiles keeps ErrWalk: the fixture stays wired up through the skipped subtest so closing #165 is a one-line change, and its doc now says so. The tradeoff, recorded in each TODO: an unconditional skip covers both backends, and every one of these is one-sided. Local satisfies the missing-key Remove today and s3 satisfies both Write cases and the walk-error case, so each skip now gives up a backend that was passing. The TODO names which one, so the fixing PR knows what it is switching back on. The package doc's "Skip fields" section is replaced by one describing the convention, and the options-struct section no longer describes structs that carry only Skip fields, because none do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JnxfAdBrXieiGaAENQ74Hh
The suite grew a file per method — write.go, remove.go, removeall.go, direntries.go, walkfiles.go — which split the ocflfs.WriteFS story across three files when it is one interface with three methods, and left no obvious home for a new assertion. Regroup by the interface each file covers: writefs.go TestWriteFSWrite, TestWriteFSRemove, TestWriteFSRemoveAll direntriesfs.go TestDirEntries filewalker.go TestWalkFiles writefs.go orders the three to match ocflfs.WriteFS's own method order in fs/fs.go and keeps failingReader at the bottom, below the entry points it serves. The three merged files had byte-identical import blocks, so the merge reconciles nothing. A pure move: no signature, type, assertion, skip or TODO changed, and neither caller needed an edit. Verified by diffing the -v subtest list before and after — 68 subtests, identical. Note for reviewers: git needs -M30% to see writefs.go as a rename of write.go, since absorbing the other two files drops the similarity below the default threshold. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JnxfAdBrXieiGaAENQ74Hh
Author
Review (from Shelley, adversarial pass per repo conventions)Full suite passes ( Critical / contract-level1. Divergence: 2. Mock 3. Mock Robustness / correctness (non-blocking)4. 5. 6. Invalid-path tables never include a nested-but-missing Verified non-issues
Bottom line: #1 and #3 are the ones I'd block on. #1 is a doc/contract coherence question worth answering deliberately; #3 is a latent race in freshly-added concurrency-sensitive test infrastructure. #2 is worth a comment on #166 so the fix doesn't stop at error-mapping. |
WriteFS.Remove now pins "." as fs.ErrInvalid on every backend, while WriteFS.RemoveAll leaves "." to the implementation and the package-level RemoveAll special-cases "." without consulting the backend at all. A caller hitting any one of the three docs had no way to learn the other two existed. Say so in both doc comments: the interface doc names the two permitted backend behaviors and points root-emptying callers at the package-level helper; the helper doc notes that the backend behavior it bypasses is only observable directly. Doc-only; no code changed. Co-authored-by: Shelley <shelley@exe.dev>
The call log added a mutex while the rest of the mock — objects, UpdatedETags, Deleted, the MPU flags — stayed unsynchronized, and its comment referred to "the mock's MPU mutex", which does not exist. The s3 backend issues PutObject, UploadPart and friends from the uploader's goroutines, so any test that drives the mock concurrently while reading those fields (the shape #166-era assertions will take) is a data race; -race passed only because no test does that yet. One mutex now guards objects, UpdatedETags, Deleted and the MPU flags, with the same discipline the call log already used: parts keeps its sync.Map and the log its own mutex, and no handler holds the state lock while touching either, so there is no lock ordering to reason about. - Handlers lock around map/flag access; ListObjectsV2 holds the lock for its whole scan so the listing is a consistent snapshot rather than a torn read of a map being written. - GetObject, CopyObject and UploadPartCopy clone the body bytes they hand out or hash, under the lock: a read of a stored slice must not race a PutObject replacing it, and a returned buffer must stay valid if one does. This also fixes the aliasing where GetObject returned a subslice of the stored body a caller could mutate into mock state. - The exported fields stay for compatibility, with a doc steer to the new accessors (WasDeleted, UpdatedETag, MPU*Flag) that take the lock; the existing fs_test.go assertions switch to them. - New TestMockConcurrentUse pins the guarantee: 8 goroutines of overlapping puts/gets/heads/lists/deletes, fatal under -race if any state access lost its guard. Co-authored-by: Shelley <shelley@exe.dev>
This was referenced Aug 25, 2026
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
2 participants
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.
Closes #162.
Adds
fs/internal/imptest, a shared contract test suite for theocflfsinterfaces, and runs it against both thefs/localandfs/s3backends. Nothing in the previous suite caught the two backends disagreeing about theocflfs.WriteFScontract — this suite is where those disagreements now surface.What lands here
fs/internal/imptest— one entry point per interface under test, one file per interface:TestWriteFSWrite/TestWriteFSRemove/TestWriteFSRemoveAll(writefs.go)TestDirEntries(direntriesfs.go) — takes no options struct: the backends agree on every case it coversTestWalkFiles(filewalker.go) — keeps itsErrWalkfixture wired through the skipped subtestfs/s3/internal/mockgains a mutex-guarded request log (Call{Op, Keys},Calls(),CallsFor(op)), so a test can tell oneDeleteObjectsof 500 keys apart from 500 separateDeleteObjectcalls (needed by fs/s3: delete semantics — missing keys, batching, and partial failures #166).Write(".")andRemove(".")now returnfs.ErrInvalidon every backend, asserted directly rather than through per-backend knobs.Skips
Per the issue's sequencing constraint, the suite only asserts what
mainalready honors; each known gap is skipped with a TODO naming the issue that closes it:TODO(#163)— failing source leaves previous content intact (write, x2)TODO(#165)— walk error delivered as the pair's error elementTODO(#166)—Removeof a missing key returnsfs.ErrNotExistEach TODO names which backend was passing before the unconditional skip, so the fixing PR knows what it is switching back on.