Repository navigation
test: cover every reachable branch, drop a test that hit the network - #66
Merged
Merged
Conversation
resolveIssuer switched on reflect.Kind with a default arm returning "unsupported identifier type". Identifier is "~int64 | ~string", so the kind is only ever Int64 or String and that arm was dead. Collapse it, and fold the two duplicate zero-value checks into one "id == zero" — both forms are comparable. reflect stays, rather than fmt's %v, because %v dispatches to String() on a defined type and would let it rewrite its own issuer. createInstallationToken resolved its endpoint as a relative reference and handled the parse error. The reference is fmt.Sprintf over an int64 and always parses. url.URL.JoinPath builds the same URL with no error to handle, and appends to the base path rather than resolving against it, so an Enterprise /api/v3/ prefix survives by construction. Two guards that also cannot fire are kept, now with a comment saying why: json.Marshal on InstallationTokenOptions (only strings, int64s and pointers to them) and jwt.Token.SigningString (fixed header map and RegisteredClaims). Both stay so a future field or claim type that can fail is not silently sent as an empty body or signed as an empty JWT. No exported type, function, method or doc comment changes: verified by diffing "go doc -all" for both packages against main.
Test_createInstallationToken_ErrorPaths/"error parsing endpoint URL" pointed a client at example.com and asserted only that some error came back. The endpoint parsed fine. The error was a 405 from the real example.com, reached over the network on every CI run. Any error satisfies that assertion, so the test passed for the wrong reason and could not detect the thing it named: deleting the parse-error branch entirely leaves it green. An offline runner does not fail it either, it just supplies a connection error instead of the 405. Its sibling "error marshaling options" was named for a path it never exercised. It ran the success path. Replaced with tests that exercise what the names claim: - doCreateInstallationToken request failures, driven directly: a URL net/url rejects for a control byte, and a transport that fails the round trip. Neither opens a connection. - the endpoint path the client POSTs to, for root, /api/v3/ and nested base paths, pinning what JoinPath produces. - the request body for nil and populated options, asserting nil sends no body rather than a JSON "null". New coverage elsewhere: - WithInstallationExpirySkew, which had no test at all: both that it selects the early-refresh wrapper for a positive skew and delegates to oauth2.ReuseTokenSource otherwise, and that the window it configures is the one the cache refreshes on. - ReuseTokenSourceWithSkew with a zero-expiry token, the shape NewPersonalAccessTokenSource returns, which must be cached rather than re-minted on every call. Root package coverage 96.3% -> 99.2%. The two statements left are the unreachable guards documented in the previous commit. No test reaches the network now: the Example functions carry no "// Output:" comment, so they compile but never run.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #66 +/- ##
==========================================
+ Coverage 97.00% 99.31% +2.31%
==========================================
Files 4 4
Lines 300 294 -6
==========================================
+ Hits 291 292 +1
+ Misses 9 2 -7 ☔ 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
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
The root package sat at 96.3% coverage. Four of the missing statements cannot be reached by any input, so 100% was never available without changing the code. One problem was a test rather than a gap.
"error parsing endpoint URL"pointed a client at example.com and asserted only that some error came back. The endpoint parsed fine, and the 405 that satisfied it arrived over the network. This deletes the two branches no input reaches, documents the two guards that stay, and replaces that test. Root coverage is now 99.2%, webhook stays at 100%.Changes
resolveIssuerloses itsdefaultarm, because~int64 | ~stringmakesreflect.Kindonly everInt64orStringcreateInstallationTokenbuilds its endpoint withurl.URL.JoinPath, which has no error to handle and keeps an Enterprise/api/v3/prefixWithInstallationExpirySkew, and for a zero-expiry token inReuseTokenSourceWithSkewRationale
Deleting the parse-error branch leaves the old test green, which is how the dead code survived this long.
Function fields on
applicationTokenSourceandgithubClientwould let a test force the last two errors and reach 100.0%. That puts test-only machinery in production structs, so both guards stay instead, each commented with why it cannot fire.Migration Notes
None. Every exported type, function, method and doc comment is byte-identical to
main, verified by diffinggo doc -allfor both packages.🤖 Generated with Claude Code