docs/type/test cleanup pass: README, help text, type ergonomics, test coverage - #106
Merged
Conversation
Verified every documented command, flag, and example against the current implementation. Corrects the scopes table (read:page:confluence, read:folder:confluence, and read:confluence-content.summary all over/under-claimed which commands need them), a stale error-message quote, two example outputs that no longer match real formatting/sorting, the --json envelope example (missing roots/dry_run), a stale schema command list, an outdated lossy-transforms claim for read (internal links and image paths now round-trip), a missing error-output prefix, and an overstated PowerShell completions claim.
Verified every subcommand's --help text against its actual implementation. Fixes read's frontmatter field list (parent was missing), create's misleading link/parent-cycle claim, and adds material omissions that were real usage traps: an unknown find --space key errors rather than returning empty, search --cql refuses --space/--type, export's --file id fallback, and attachment-download's outside-root refusal. Also hides create's --persist flag, which only existed to give --no-persist a positive counterpart and was never documented anywhere.
parentInfo.kind was a bare string compared against four hand-typed literals with no compile-time exhaustiveness check; a typo would silently fall through every switch instead of failing to build. pageIDFailure.title was set but never read by any caller.
jsonUploadResult and jsonDownloadResult were the same shape -- ok/status/ dry_run/filename/error/code -- with download only adding dest_path. Replaced both with jsonout.AttachmentActionResult, which upload leaves dest_path nil on. Schema updated to match: attachmentUploadResult now requires dest_path (always null).
convert.Attachment and client.LocalAttachment were byte-identical structs (Filename, Path, Source), manually field-copied at two call sites through near-duplicate toLocalAttachments helpers in create.go and update.go. internal/convert is deliberately client-free, so the shared shape now lives in a new internal/attachref package that both sides alias to, letting create/update pass pageContent.Attachments straight through.
Options and Config read as arbitrarily paired names for what is really a two-stage pipeline (Resolve(Options) -> Config -> New(Config)); the new name makes Options' role as Resolve's input side explicit.
TestSubcommandsRegistered duplicated TestCommandEnumMatchesRegisteredCommands for every command except schema (the one enum-exempt case nothing else checks); shrunk to just that. Removed an orphaned test-stub comment in attachment-download left over from when the escape-refusal logic moved to internal/attachfile (tested there now). Consolidated schema_test.go's duplicated cobra SetOut/SetErr/SetArgs boilerplate into runCmd. Dropped TestAttachmentCommentBudget's constant-arithmetic assertion, which checked Go's arithmetic rather than attachmentComment's behavior.
create.go and update.go each defined a byte-identical pageURL function and an identical TestPageURLUsesSiteNotGateway test. Moved it to internal/client as a ConfluenceClient method, since it only needs SiteURL and a Page, and consolidated the test into internal/client's own suite.
internal/client had 5 different named httptest-server-plus-client helpers (newServer, countingServer, updateServer, newFindServer, newSearchServer) plus 3 bare inline repeats of the same 3-line construction. Added internal/clienttest for every other package's tests to build on, and a local newTestServer for internal/client's own tests, which can't import clienttest: an internal (white-box) test file may not import a package that itself imports the package under test.
Each had its own single-purpose local reimplementation of the same httptest-server-plus-client boilerplate; switched all three to build on the shared internal/clienttest.New.
…ange guarantee cmd/fix had no non-json test file: json_test.go only covered output shaping. Added direct tests for locatePage's page_id/title-search paths (found, not-found, zero/one/multiple title matches), plannedChanges' per-field diffing (fill-when-missing, update-when-differing, the parent-null normalization, title never overwritten, page_width's default-to-max and case-insensitive comparison), and processFile's central guarantee -- fix never writes the file unless there's a real change, and never writes at all under --dry-run or on failure.
…ss path
update's most distinctive behavior -- skip republishing unless the file was
modified after the page's last version, unless --force -- had no test
driving processFile through a real skip decision; every existing test
hand-built an updateResult{status: statusSkipped} instead. Added tests for
the skip itself, --force bypassing it, and a full success path (through
UpdatePage) that also covers the file-newer-than-page half of the same
check. Migrated the file's existing httptest usage to internal/clienttest
in the same pass.
…File's own validation Three gaps: topoSort's parent-cycle detection was never actually triggered by any test (buildRecords always t.Fatalf's on error); the plain "--parent <id>" (parentExternal) path -- as opposed to a .md reference -- was never exercised at all; and resolveFile's own validation errors (no title, no space, --space/frontmatter conflict) were only ever used as arbitrary example strings for testing the abort/JSON machinery, never asserted as resolveFile's actual output.
Write's Create truncates (or makes) the destination file before DownloadAttachment runs. On failure the file was left in place -- empty, or holding whatever was written before the error -- so a retry without --force saw it via the existence check and reported skipped, silently treating a download that never completed as "already there." Found while adding test coverage for this path; the accompanying test reproduces the bug and confirms the fix.
…l-rejection integration The credential-rejection-vs-404 distinction (notFound/RejectedCredential) had a unit test for the detection logic itself but nothing feeding that exact response shape through GetPageOrNil/GetPageBodyOrNil/GetFolderOrNil to confirm they actually propagate the error instead of reading it as "page absent" -- the specific integration notFound exists to protect. Also added GetUser (display-name lookup, empty-id short-circuit, best-effort-empty-on-failure) and CreatePage's conditional parentId field, neither of which had any coverage.
…omWebUI Adds: attempt-count pinning for a persistently unreachable server (maxRetries+1 total) and confirmation a non-idempotent POST gets exactly one attempt on a transport failure; a retried POST resending the same body (verified true today via net/http's own GetBody handling for the bytes.Reader/bytes.Buffer types this client always uses, not attempt's explicit rebuild -- confirmed by mutation testing, comment corrected to say so); parseRetryAfter's future-HTTP-date branch, which the existing table only exercised with a fixed past date; isIdempotent's method table; SetContentProperty returning the second (not first) error when both attempts fail; and SpaceKeyFromWebUI's non-matching/empty cases.
…ction functions Cache -- the whole point of the type -- had no test file at all; verified it builds once per root.Dir and reuses that index across later Get calls (proven by mutating the tree on disk between two Get calls and checking the second doesn't see the change), keeps separate roots separate, and propagates a build error. GithubSlug/ConfluenceSlug/ extractHeadings had no direct test anywhere despite Unicode-aware slugging being a documented design point: added cases for Unicode letters, punctuation stripping, the per-character-vs-collapsed whitespace-to-hyphen difference between the two sluggers, and extractHeadings' fenced-code-block skip (including an unterminated fence).
walkAndCache's backfill can point several byDir keys at the same *Root, so Close -- which closes every key's Root unconditionally -- calls FS.Close() on the same underlying os.Root handle more than once. Confirmed empirically that os.Root.Close is idempotent (returns nil on every call, not just the first), so this was already safe; added a test that builds a real backfilled scenario and closes it, so a future change to Close's iteration or to os.Root's semantics can't regress it silently.
find had no test of run() itself, only its pure table() helper. Added the success path, "No matches found.", the empty-title usage error, an unknown --space usage error, and --json output shape -- using a bare *cobra.Command carrying just the flags run() reads (CONFLUENCE_TOKEN via the environment, since it's never a flag) rather than the real root command tree.
children had no test of run() itself, only parseDepth and tree(). Added the success path, "No children.", an invalid --depth usage error, and --json output shape, using the same bare-*cobra.Command pattern as find's run() tests.
info had no test of run() itself, only its pure formatting helpers. Added the success path, a missing-page operational failure, --properties triggering the content-properties fetch and section, and --json output shape.
read had no test file exercising run() at all -- only json_test.go's marshaling helpers. Added markdown and storage format output, the unsupported-format usage error, a missing-page operational failure, the no-readable-body check (the one thing standing between reading a folder and a confusing downstream error), and --json output shape.
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.
Summary
A broad cleanup pass across documentation, type ergonomics, and test coverage, done in four stages:
--jsonenvelope shape, lossy-transforms claims).--helptext, fixing a stale frontmatter field list, a misleading cycle claim, and several missing usage-trap warnings (unknown--space,--cqlflag conflicts, an id fallback, a path-escape refusal).parentKindenum replacing magic strings, removal of a dead struct field, a sharedjsonout.AttachmentActionResultreplacing two near-identical per-command types, a newinternal/attachrefpackage removing a duplicated struct betweeninternal/convertandinternal/client, andclient.Optionsrenamed toclient.ResolveOptionsfor clarity.internal/clienttesthelper (replacing ~10 independent reimplementations of the same httptest-server-plus-client boilerplate), and added real test coverage forcmd/fix(previously near-zero),cmd/update's mtime-skip guarantee,cmd/create's cycle rejection and--parenthandling,internal/client's retry/backoff edges and credential-rejection integration,internal/linkindex'sCacheand slug functions, andrun()-level tests forfind/children/info/read(previously untested end-to-end). Found and fixed one real bug along the way: a failed attachment download left a partial file behind that a later retry mistook for "already there."Every test added was verified with mutation testing (breaking the code under test and confirming the test actually fails).
Test plan
make checkpasses (vet, fmt-check, test, build, lint) after every commit in this branchinternal/attachfile)