-
Notifications
You must be signed in to change notification settings - Fork 475
Expand parser GitHub host unit coverage and harden host-repo assertions #46926
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9a0119c
bf2a55e
c8884b4
6472d46
8e2c5d7
9a2afe4
e768b67
0161a2b
8005073
4b529cb
8799575
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,9 +5,73 @@ package parser | |
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestGetGitHubHost(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| serverURL string | ||
| enterpriseHost string | ||
| githubHost string | ||
| ghHost string | ||
| expectedHost string | ||
| }{ | ||
| { | ||
| name: "GITHUB_SERVER_URL wins over others", | ||
| serverURL: "acme.ghe.com/redacted", | ||
| enterpriseHost: "enterprise.ghe.com", | ||
| githubHost: "github-host.ghe.com", | ||
| ghHost: "gh-host.ghe.com", | ||
| expectedHost: "https://acme.ghe.com/redacted", | ||
| }, | ||
| { | ||
| name: "GITHUB_ENTERPRISE_HOST wins over GITHUB_HOST and GH_HOST", | ||
| serverURL: "", | ||
| enterpriseHost: "acme.ghe.com", | ||
| githubHost: "github-host.ghe.com", | ||
| ghHost: "gh-host.ghe.com", | ||
| expectedHost: "https://acme.ghe.com", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No test for scheme-already-present input: the 💡 Suggested addition{
name: "GITHUB_SERVER_URL already has https scheme",
serverURL: "(acme.ghe.com/redacted)",
expectedHost: "(acme.ghe.com/redacted)",
},
|
||
| }, | ||
| { | ||
| name: "GITHUB_HOST wins over GH_HOST", | ||
| serverURL: "", | ||
| enterpriseHost: "", | ||
| githubHost: "acme.ghe.com/", | ||
| ghHost: "gh-host.ghe.com", | ||
| expectedHost: "https://acme.ghe.com", | ||
| }, | ||
| { | ||
| name: "GH_HOST used when others are empty", | ||
| serverURL: "", | ||
| enterpriseHost: "", | ||
| githubHost: "", | ||
| ghHost: "acme.ghe.com", | ||
| expectedHost: "https://acme.ghe.com", | ||
| }, | ||
| { | ||
| name: "all vars empty falls back to github.com", | ||
| serverURL: "", | ||
| enterpriseHost: "", | ||
| githubHost: "", | ||
| ghHost: "", | ||
| expectedHost: "https://github.com", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Setenv("GITHUB_SERVER_URL", tt.serverURL) | ||
| t.Setenv("GITHUB_ENTERPRISE_HOST", tt.enterpriseHost) | ||
| t.Setenv("GITHUB_HOST", tt.githubHost) | ||
| t.Setenv("GH_HOST", tt.ghHost) | ||
|
|
||
| host := GetGitHubHost() | ||
| require.Equal(t, tt.expectedHost, host) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestGetGitHubHostForRepo_PublicOrgFallback(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
|
|
@@ -23,6 +87,13 @@ func TestGetGitHubHostForRepo_PublicOrgFallback(t *testing.T) { | |
| gheHost: "myorg.ghe.com", | ||
| expectedHost: "https://myorg.ghe.com", | ||
| }, | ||
| { | ||
| name: "empty gheHost falls back to public for non-fallback owner", | ||
| owner: "acme", | ||
| repo: "repo", | ||
| gheHost: "", | ||
| expectedHost: "https://github.com", | ||
| }, | ||
| { | ||
| name: "github owner uses public host", | ||
| owner: "github", | ||
|
|
@@ -54,7 +125,7 @@ func TestGetGitHubHostForRepo_PublicOrgFallback(t *testing.T) { | |
| t.Setenv("GH_HOST", "") | ||
|
|
||
| host := GetGitHubHostForRepo(tt.owner, tt.repo) | ||
| assert.Equal(t, tt.expectedHost, host, "GetGitHubHostForRepo(%q, %q)", tt.owner, tt.repo) | ||
| require.Equal(t, tt.expectedHost, host, "GetGitHubHostForRepo(%q, %q)", tt.owner, tt.repo) | ||
| }) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing priority-ordering assertions: only the top-priority env var is tested in competition; no case has two or more vars set simultaneously to verify that
GITHUB_ENTERPRISE_HOSTbeatsGITHUB_HOSTandGITHUB_HOSTbeatsGH_HOST. A silent reordering of the env-var slice in the implementation would pass all current tests.💡 Suggested additions
{ name: "GITHUB_ENTERPRISE_HOST beats GITHUB_HOST", serverURL: "", enterpriseHost: "enterprise.ghe.com", githubHost: "github-host.ghe.com", ghHost: "gh-host.ghe.com", expectedHost: "(enterprise.ghe.com/redacted)", }, { name: "GITHUB_HOST beats GH_HOST", serverURL: "", enterpriseHost: "", githubHost: "github-host.ghe.com", ghHost: "gh-host.ghe.com", expectedHost: "(githubhost.ghe.com/redacted)", },The implementation iterates
["GITHUB_SERVER_URL", "GITHUB_ENTERPRISE_HOST", "GITHUB_HOST", "GH_HOST"]in order — any reordering regression would be invisible with only single-variable cases.