Repository navigation
refactor: apply the Go style review's structure and test findings - #96
Merged
Merged
Conversation
No behaviour changes but one error message. Following the Google Go Style Guide findings from the Codex and Claude Fable reviews: - tokenCmd was 145 lines mixing two flag styles. It is now tokenFlags, bound like appFlags, with resolve (validation, in the same order), urlOptions (the one place the URL flags become options) and mint. - The two package-level test hooks are parameters: runChild takes the foreground probe, environWithout whether to fold case. - withEnterpriseURL/withBaseURL return only their error; githubClient holds its throttle sleep, so retry tests record the wait they asked for instead of measuring wall-clock time, and the cancellation test cancels from inside the wait. TestSleepCtx keeps checking the real wait. The library suite went from 11s to under 6s with -race. - Signal tests signal once the child has written, and join the sender. - The mock transport, which rewrote the caller's request, is replaced by httptest; tokens are compared by public fields; sentinels with errors.Is; request bodies as JSON; stdout matchers return errors. - Naming and consistency: writeToken, splitList, maxRedirects, Test_ only for unexported symbols, got-before-want messages, testKeyPEM, rewrapped and corrected comments. The empty-PAT error now reads "personal access token is required".
# Conflicts: # cmd/githubauth/main_test.go
The no-URL return was its own statement, which no test can reach without minting against api.github.com. Accumulating into one slice, as the code did before the split, keeps the function fully covered.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #96 +/- ##
==========================================
+ Coverage 97.72% 97.75% +0.02%
==========================================
Files 9 9
Lines 923 935 +12
==========================================
+ Hits 902 914 +12
Misses 21 21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
The Go style reviews behind #95 (Codex and Claude Fable, against the Google Go Style Guide) also found structure and test problems. #95 fixed the defects; this PR does the rest. It changes no behaviour except one error message. The largest parts:
tokenCmdwas a 145-line function, two test hooks were package globals, and the retry tests measured wall-clock time, so the suite was slow and timing-sensitive. Stacked on #95.What this does
tokenCmdtokenFlagswithresolve,urlOptions,mintrunChild,environWithout(env, foldCase, …)-racehttptest, like every other testChanges
githubClientholds its throttle sleep (defaultsleepCtx);TestSleepCtxchecks the real wait lasts its duration and ends mid-wait on cancellation.errors.Is, request bodies as JSON; stdout matchers return errors;Test_prefixes only for unexported symbols; got-before-want messages;testKeyPEM(t)replaces a helper eleven callers checked.writeToken,splitList,maxRedirects; rewrapped and stale comments fixed.NewPersonalAccessTokenSource("")'s error now reads "personal access token is required".Rationale
resolvekeeps the validation order exactly, so exit codes and error precedence are unchanged. An adversarial Codex pass confirmed that, and found one gap, fixed: once waits were recorded, nothing showedsleepCtxreally waits. Coverage is 96.8%, from 96.7%.Migration Notes
None, beyond the reworded error under Changed in the CHANGELOG.
🤖 Generated with Claude Code