test: cover docscheck's filesystem-error branches with an error-injecting fs.FS - #20
Conversation
…tree fstest.MapFS cannot produce a filesystem error: every path in one either exists or is cleanly absent, so the branches that report a repository the checks cannot read went unexercised. Those branches are the ones that keep a check's silence honest — no documents collected reads exactly like no drift found — so leaving them unpinned left the gate's failure mode untested. unreadableRepository wraps the existing fixture and fails ReadDir for one named directory. ReadDir alone reaches all three branches: fs.WalkDir reads each directory as it descends, and rootEntries reads the root directly. Everything else is the embedded fixture, unchanged. Four subtests pin the walk's mid-descent failure, the unlistable root, CheckAll's collection failure, and CheckNamedPaths' unreadable root. Each asserts the injected cause reaches the report rather than being replaced with a generic one. Package coverage 95.4% -> 96.8%, with CheckAll, DocumentFiles, and rootEntries now fully covered.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe tests add an error-injecting filesystem wrapper and cover unreadable directory and repository-root behavior in ChangesDocscheck filesystem error coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR adds focused filesystem-error coverage without changing production behavior; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Peer review noted that the three error cases have a boundary nothing states: fs.WalkDir offers each directory to the callback before reading it, so a directory DocumentFiles skips is never a directory it can fail on. That is what keeps an unreadable testdata or dist from breaking a walk that never wanted to enter them. The order is load-bearing and invisible. Dropping testdata from the skip list makes the new subtest fail and nothing else, so the invariant is now pinned where a reader of the skip list will find it. Also renames two subtests to the file's scenario-first phrasing, which they had broken by opening with a bare function name.
PR #20 was opened for #15 five minutes before this branch's pull request and implements the same thing — an fs.FS that fails on ReadDir, exercising the walk and collection error paths. Two sessions picked up the issue in parallel. Theirs was first and is scoped to #15 alone, where this branch only carried those tests because #18 touches the same package. Removing them here resolves the conflict in docscheck_test.go and leaves this branch as what it should have been: the scan bound and nothing else. The #18 work is untouched. Package coverage returns to 95.5%, with #20 carrying the rest.
|
Deferring to this PR for #15 — it was opened five minutes before mine and is properly scoped to the issue. I had picked up #15 alongside #18 because both touch One thing from my version that may be worth folding in, if it isn't already covered: the branch where Fourth duplicate dispatch tonight (#11, #9, #18, now #15). Worth raising with whoever runs the dispatcher: checking assignee and |
|
All peer reviews addressed. Ready for final review @scottescue Security, Go, testing, and documentation reviews all approved. Two minor findings were fixed in One note worth surfacing: grooming caught that the implementer guidance I originally wrote into #22 was wrong. It said the |
2 participants
Closes #15
Summary
fstest.MapFScannot produce a filesystem error — every path in one either exists or is cleanly absent — so a set of error branches ininternal/docscheckhad no test that could reach them. This adds anfs.FSthat fails on demand and pins all three, plus the invariant that bounds them.Those branches are what keep the gate's silence honest: an unreadable tree that collected no documents reads exactly like a tree with no drift, so a check that swallowed the error would fail open on the one input it most needs to reject.
What changed
internal/docscheck/docscheck_test.goonly. No production code changed — this PR fixes no bug and alters no behavior.unreadableRepository— embeds the existingrepository(nil)fixture and returns a sentinel error fromReadDirfor one named directory.ReadDiralone reaches every target branch:fs.WalkDirreads each directory as it descends, androotEntriesreads the root directly.Open,Stat, and file contents come from the embedded fixture unchanged.TestAnUnreadableTreeIsReportedRatherThanIgnored— five subtests:DocumentFiles—WalkDircallback error path + walk's error returnCheckAllat document collectionCheckAllcollection-failure branchCheckNamedPathscannot listrootEntries/CheckNamedPathsfs.ReadDir(".")pathfs.WalkDiroffers a directory to the callback before reading it, so a skippedtestdata/distis never one the walk can fail onEach subtest asserts the injected cause reaches the caller — by identity via
errors.Iswhere the error is returned, by message fragment where a check turns it into a finding — so an error swallowed and replaced with a generic one would still fail.CheckNamedPathsis called directly rather than throughCheckAll, because the same unreadable root stopsCheckAllat document collection first.Verification
go test ./...andgo test -race ./internal/docscheck/green;gofmt -l .andgo vet ./...clean.internal/docscheck: 95.4% → 96.8%.CheckAll,DocumentFiles, androotEntriesare now at 100%.rootEntrieserror, returningSkipDirinstead of propagating in the walk callback, and droppingtestdatafrom the skip list each make their own subtest fail and no other. The tests pin behavior, not line coverage.Peer review
Security, Go, testing, and documentation reviews all approved. Two minor review findings were addressed in
256d295rather than deferred: two subtest names that had broken the file's scenario-first phrasing, and the unpinned skip invariant. Deferred findings are captured in #22 (remaining content/parse edge-case branches) and #23 (CheckLinksmislabels an unreadable target as missing).Acceptance criteria
fs.FSwrapper exists in the package's testsWalkDir,ReadDir, andCheckAllcollection-failure branches are each exercised and assert a specific messageinternal/docscheckdoes not regressSummary by CodeRabbit