Conversation
…ubAPIURL Add internal/proxy/derive_api_url_test.go with two new test functions that cover branches left untested by the existing TestDeriveAPIFromServerURL and TestDeriveGitHubAPIURL tests: TestDeriveAPIFromServerURL_ExtendedCoverage: - www.github.com (alternate alias handled in the same switch case) - GITHUB.COM (uppercase hostname normalised by strings.ToLower) - github.com URL with path component (path is stripped via Hostname()) - http:// GHEC tenant (non-HTTPS scheme preserved in output) - deeply nested GHEC subdomain (*.ghe.com multi-level) - uppercase .GHE.COM (normalised before HasSuffix check) - bare ghe.com treated as GHES (HasSuffix requires a leading dot) - http:// GHES server (non-HTTPS scheme preserved) - GHES URL with path (path ignored, only Host used) - http:// with empty host (parsed.Host == → return ) - file:///some/path (no network host → return ) TestDeriveGitHubAPIURL_ExtendedCoverage: - GITHUB_SERVER_URL set to an unparseable URL → propagates - GITHUB_SERVER_URL set to http:// (empty host) → propagates - GITHUB_API_URL explicitly set to "" falls through to GITHUB_SERVER_URL - GITHUB_SERVER_URL=https://www.github.com derives DefaultGitHubAPIBase Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds additional table-driven tests in internal/proxy to improve coverage of URL-derivation logic used by the proxy when resolving the upstream GitHub API base URL from environment variables.
Changes:
- Add extended-coverage tests for
deriveAPIFromServerURL(www alias, case normalization, scheme preservation, path stripping, and empty-host edge cases). - Add extended-coverage tests for
DeriveGitHubAPIURLto validate env var fallthrough and propagation of unusableGITHUB_SERVER_URLvalues.
Show a summary per file
| File | Description |
|---|---|
| internal/proxy/derive_api_url_test.go | New tests covering previously untested branches/edge cases for deriving GitHub API base URLs. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
| // - GITHUB_SERVER_URL set to an invalid URL (deriveAPIFromServerURL returns "") → function returns "" | ||
| // - GITHUB_SERVER_URL set to a URL with empty host → function returns "" | ||
| // - GITHUB_API_URL set to empty string explicitly → falls through to GITHUB_SERVER_URL | ||
| // - Neither var set (already tested) just to confirm baseline |
There was a problem hiding this comment.
The doc comment lists "Neither var set (already tested) just to confirm baseline", but this test function doesn't include a case where both env vars are unset. Either add that case here or remove/update the bullet to avoid misleading future readers.
Suggested change
| // - Neither var set (already tested) just to confirm baseline |
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.
Test Coverage Improvement:
deriveAPIFromServerURL/DeriveGitHubAPIURLFunction Analyzed
internal/proxyinternal/proxy/proxy.goderiveAPIFromServerURL(unexported),DeriveGitHubAPIURL(exported).ghe.com, GHES fallback), hostname normalisation, scheme preservation, and port handlingWhy These Functions?
Static analysis showed that although
TestDeriveAPIFromServerURLandTestDeriveGitHubAPIURLexist inproxy_test.go, several real branches were left untested:www.github.comalias is explicitly in the switch condition alongsidegithub.combut was never exercised by any teststrings.ToLower) were not testedparsed.Host == ""edge cases ((redacted)(redacted) were not testedghe.comitself (without leading dot) was not tested — it falls to the GHES branch, not GHEC*.ghe.comsubdomains were not testedDeriveGitHubAPIURLhad no test for whenGITHUB_SERVER_URLis an unusable URL, propagating""Tests Added
New file:
internal/proxy/derive_api_url_test.goTestDeriveAPIFromServerURL_ExtendedCoverage(12 cases)www.github.comreturnsDefaultGitHubAPIBasewww.github.com/(trailing slash) returnsDefaultGitHubAPIBaseGITHUB.COM(uppercase) normalised toDefaultGitHubAPIBasehttps://github.com/org/repo(path component) returnsDefaultGitHubAPIBase(mycompany.ghe.com/redacted) (HTTP GHEC) preserves(redacted) scheme(deep.nested.tenant.ghe.com/redacted)→copilot-api.prefix(mycompany.ghe.com/redacted)(uppercase GHEC) normalised correctly(ghe.com/redacted)(bare, no dot) treated as GHES →/api/v3(github.mycompany.com/redacted) (HTTP GHES) preserves(redacted) scheme(redacted) (empty host) returns""`(redacted) (no network host) returns""`TestDeriveGitHubAPIURL_ExtendedCoverage(4 cases)GITHUB_SERVER_URL=not-a-valid-url→""propagatedGITHUB_SERVER_URL=(redacted) (empty host) →""` propagatedGITHUB_API_URL=""explicitly set falls through toGITHUB_SERVER_URLGITHUB_SERVER_URL=https://www.github.com→DefaultGitHubAPIBaseTest Methodology
All expectations were validated by running an equivalent Go function locally (to confirm URL parsing behaviour) before committing:
Test Execution
Tests follow existing project conventions:
package proxy(internal, accessing unexportedderiveAPIFromServerURL)t.Run()testify/assertwitht.Cleanup()for env var restorationGenerated by Test Coverage Improver