From d91787694923f45247683a88e3641b03e6765008 Mon Sep 17 00:00:00 2001 From: Yunchi Luo Date: Sat, 27 Aug 2022 14:30:10 -0400 Subject: [PATCH] expand and clean up repospec_test --- api/internal/git/repospec_test.go | 314 +++++++++++++++++++++--------- 1 file changed, 217 insertions(+), 97 deletions(-) diff --git a/api/internal/git/repospec_test.go b/api/internal/git/repospec_test.go index 41e89ca31..384209cff 100644 --- a/api/internal/git/repospec_test.go +++ b/api/internal/git/repospec_test.go @@ -13,49 +13,50 @@ import ( "github.com/stretchr/testify/assert" ) -var orgRepos = []string{"someOrg/someRepo", "kubernetes/website"} - -var pathNames = []string{"README.md", "foo/krusty.txt", ""} - -var hrefArgs = []string{"someBranch", "master", "v0.1.0", ""} - -var hostNamesRawAndNormalized = [][]string{ - {"gh:", "gh:"}, - {"GH:", "gh:"}, - {"gitHub.com/", "https://github.com/"}, - {"github.com:", "https://github.com/"}, - {"http://github.com/", "https://github.com/"}, - {"https://github.com/", "https://github.com/"}, - {"hTTps://github.com/", "https://github.com/"}, - {"https://git-codecommit.us-east-2.amazonaws.com/", "https://git-codecommit.us-east-2.amazonaws.com/"}, - {"https://fabrikops2.visualstudio.com/", "https://fabrikops2.visualstudio.com/"}, - {"ssh://git.example.com:7999/", "ssh://git.example.com:7999/"}, - {"git::https://gitlab.com/", "https://gitlab.com/"}, - {"git::http://git.example.com/", "http://git.example.com/"}, - {"git::https://git.example.com/", "https://git.example.com/"}, - {"git@github.com:", "git@github.com:"}, - {"git@github.com/", "git@github.com:"}, -} - -func makeURL(hostFmt, orgRepo, path, href string) string { - if len(path) > 0 { - orgRepo = filepath.Join(orgRepo, path) +func TestNewRepoSpecFromUrl_Permute(t *testing.T) { + // Generate all many permutations of host, orgRepo, pathName, and ref. + // Not all combinations make sense, but the parsing is very permissive and + // we probably stil don't want to break backwards compatibility for things + // that are unintentionally supported. + var schemeAuthority = []struct{ raw, normalized string }{ + {"gh:", "gh:"}, + {"GH:", "gh:"}, + {"gitHub.com/", "https://github.com/"}, + {"github.com:", "https://github.com/"}, + {"http://github.com/", "https://github.com/"}, + {"https://github.com/", "https://github.com/"}, + {"hTTps://github.com/", "https://github.com/"}, + {"https://git-codecommit.us-east-2.amazonaws.com/", "https://git-codecommit.us-east-2.amazonaws.com/"}, + {"https://fabrikops2.visualstudio.com/", "https://fabrikops2.visualstudio.com/"}, + {"ssh://git.example.com:7999/", "ssh://git.example.com:7999/"}, + {"git::https://gitlab.com/", "https://gitlab.com/"}, + {"git::http://git.example.com/", "http://git.example.com/"}, + {"git::https://git.example.com/", "https://git.example.com/"}, + {"git@github.com:", "git@github.com:"}, + {"git@github.com/", "git@github.com:"}, + } + var orgRepos = []string{"someOrg/someRepo", "kubernetes/website"} + var pathNames = []string{"README.md", "foo/krusty.txt", ""} + var refArgs = []string{"someBranch", "master", "v0.1.0", ""} + + makeURL := func(hostFmt, orgRepo, path, ref string) string { + if len(path) > 0 { + orgRepo = filepath.Join(orgRepo, path) + } + url := hostFmt + orgRepo + if ref != "" { + url += refQuery + ref + } + return url } - url := hostFmt + orgRepo - if href != "" { - url += refQuery + href - } - return url -} -func TestNewRepoSpecFromUrl(t *testing.T) { var bad [][]string - for _, tuple := range hostNamesRawAndNormalized { - hostRaw := tuple[0] - hostSpec := tuple[1] + for _, v := range schemeAuthority { + hostRaw := v.raw + hostSpec := v.normalized for _, orgRepo := range orgRepos { for _, pathName := range pathNames { - for _, hrefArg := range hrefArgs { + for _, hrefArg := range refArgs { uri := makeURL(hostRaw, orgRepo, pathName, hrefArg) rs, err := NewRepoSpecFromURL(uri) if err != nil { @@ -89,113 +90,216 @@ func TestNewRepoSpecFromUrl(t *testing.T) { } } -var badData = [][]string{ - {"/tmp", "uri looks like abs path"}, - {"iauhsdiuashduas", "url lacks orgRepo"}, - {"htxxxtp://github.com/", "url lacks host"}, - {"ssh://git.example.com", "url lacks orgRepo"}, - {"git::___", "url lacks orgRepo"}, -} - func TestNewRepoSpecFromUrlErrors(t *testing.T) { - for _, tuple := range badData { - _, err := NewRepoSpecFromURL(tuple[0]) + var badData = []struct{ url, error string }{ + {"/tmp", "uri looks like abs path"}, + {"iauhsdiuashduas", "url lacks orgRepo"}, + {"htxxxtp://github.com/", "url lacks host"}, + {"ssh://git.example.com", "url lacks orgRepo"}, + {"git::___", "url lacks orgRepo"}, + } + + for _, testCase := range badData { + _, err := NewRepoSpecFromURL(testCase.url) if err == nil { t.Error("expected error") } - if !strings.Contains(err.Error(), tuple[1]) { + if !strings.Contains(err.Error(), testCase.error) { t.Errorf("unexpected error: %s", err) } } } -func TestNewRepoSpecFromUrl_CloneSpecs(t *testing.T) { - testcases := map[string]struct { +func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { + // A set of end to end parsing tests that smoke out obvious issues + // No tests for submodules and timeout as the expectations are hard-coded + // to the defaults for compactness. + testcases := []struct { + name string input string + repoSpec RepoSpec cloneSpec string absPath string - ref string }{ - "t1": { + { + name: "t1", input: "https://git-codecommit.us-east-2.amazonaws.com/someorg/somerepo/somedir", cloneSpec: "https://git-codecommit.us-east-2.amazonaws.com/someorg/somerepo", absPath: notCloned.Join("somedir"), - ref: "", + repoSpec: RepoSpec{ + Host: "https://git-codecommit.us-east-2.amazonaws.com/", + OrgRepo: "someorg/somerepo", + Path: "somedir", + GitSuffix: ".git", + }, }, - "t2": { + { + name: "t2", input: "https://git-codecommit.us-east-2.amazonaws.com/someorg/somerepo/somedir?ref=testbranch", cloneSpec: "https://git-codecommit.us-east-2.amazonaws.com/someorg/somerepo", absPath: notCloned.Join("somedir"), - ref: "testbranch", + repoSpec: RepoSpec{ + Host: "https://git-codecommit.us-east-2.amazonaws.com/", + OrgRepo: "someorg/somerepo", + Path: "somedir", + Ref: "testbranch", + GitSuffix: ".git", + }, }, - "t3": { + { + name: "t3", input: "https://fabrikops2.visualstudio.com/someorg/somerepo?ref=master", cloneSpec: "https://fabrikops2.visualstudio.com/someorg/somerepo", absPath: notCloned.String(), - ref: "master", + repoSpec: RepoSpec{ + Host: "https://fabrikops2.visualstudio.com/", + OrgRepo: "someorg/somerepo", + Ref: "master", + GitSuffix: ".git", + }, }, - "t4": { + { + name: "t4", input: "http://github.com/someorg/somerepo/somedir", cloneSpec: "https://github.com/someorg/somerepo.git", absPath: notCloned.Join("somedir"), - ref: "", + repoSpec: RepoSpec{ + Host: "https://github.com/", + OrgRepo: "someorg/somerepo", + Path: "somedir", + GitSuffix: ".git", + }, }, - "t5": { + { + name: "t5", input: "git@github.com:someorg/somerepo/somedir", cloneSpec: "git@github.com:someorg/somerepo.git", absPath: notCloned.Join("somedir"), - ref: "", + repoSpec: RepoSpec{ + Host: "git@github.com:", + OrgRepo: "someorg/somerepo", + Path: "somedir", + GitSuffix: ".git", + }, }, - "t6": { + { + name: "t6", input: "git@gitlab2.sqtools.ru:10022/infra/kubernetes/thanos-base.git?ref=v0.1.0", cloneSpec: "git@gitlab2.sqtools.ru:10022/infra/kubernetes/thanos-base.git", absPath: notCloned.String(), - ref: "v0.1.0", + repoSpec: RepoSpec{ + Host: "git@gitlab2.sqtools.ru:10022/", + OrgRepo: "infra/kubernetes/thanos-base", + Ref: "v0.1.0", + GitSuffix: ".git", + }, }, - "t7": { + { + name: "t7", input: "git@bitbucket.org:company/project.git//path?ref=branch", cloneSpec: "git@bitbucket.org:company/project.git", absPath: notCloned.Join("path"), - ref: "branch", + repoSpec: RepoSpec{ + Host: "git@bitbucket.org:company/", + OrgRepo: "project", + Path: "/path", + Ref: "branch", + GitSuffix: ".git", + }, }, - "t8": { + { + name: "t8", input: "https://itfs.mycompany.com/collection/project/_git/somerepos", cloneSpec: "https://itfs.mycompany.com/collection/project/_git/somerepos", absPath: notCloned.String(), - ref: "", + repoSpec: RepoSpec{ + Host: "https://itfs.mycompany.com/collection/project/_git/", + OrgRepo: "somerepos", + }, }, - "t9": { + { + name: "t9", input: "https://itfs.mycompany.com/collection/project/_git/somerepos?version=v1.0.0", cloneSpec: "https://itfs.mycompany.com/collection/project/_git/somerepos", absPath: notCloned.String(), - ref: "v1.0.0", + repoSpec: RepoSpec{ + Host: "https://itfs.mycompany.com/collection/project/_git/", + OrgRepo: "somerepos", + Ref: "v1.0.0", + }, }, - "t10": { + { + name: "t10", input: "https://itfs.mycompany.com/collection/project/_git/somerepos/somedir?version=v1.0.0", cloneSpec: "https://itfs.mycompany.com/collection/project/_git/somerepos", absPath: notCloned.Join("somedir"), - ref: "v1.0.0", + repoSpec: RepoSpec{ + Host: "https://itfs.mycompany.com/collection/project/_git/", + OrgRepo: "somerepos", + Path: "/somedir", + Ref: "v1.0.0", + }, }, - "t11": { + { + name: "t11", input: "git::https://itfs.mycompany.com/collection/project/_git/somerepos", cloneSpec: "https://itfs.mycompany.com/collection/project/_git/somerepos", absPath: notCloned.String(), - ref: "", + repoSpec: RepoSpec{ + Host: "https://itfs.mycompany.com/collection/project/_git/", + OrgRepo: "somerepos", + }, }, - "t12": { + { + name: "t12", input: "https://bitbucket.example.com/scm/project/repository.git", cloneSpec: "https://bitbucket.example.com/scm/project/repository.git", absPath: notCloned.String(), - ref: "", + repoSpec: RepoSpec{ + Host: "https://bitbucket.example.com/", + OrgRepo: "scm/project/repository", + GitSuffix: ".git", + }, + }, + { + name: "t13", + input: "ssh://git-codecommit.us-east-2.amazonaws.com/someorg/somerepo/somepath", + cloneSpec: "ssh://git-codecommit.us-east-2.amazonaws.com/someorg/somerepo", + absPath: notCloned.Join("somepath"), + repoSpec: RepoSpec{ + Host: "ssh://git-codecommit.us-east-2.amazonaws.com/", + OrgRepo: "someorg/somerepo", + Path: "somepath", + GitSuffix: ".git", + }, + }, + { + name: "t14", + input: "git@github.com/someorg/somerepo/somepath", + cloneSpec: "git@github.com:someorg/somerepo.git", + absPath: notCloned.Join("somepath"), + repoSpec: RepoSpec{ + Host: "git@github.com:", + OrgRepo: "someorg/somerepo", + Path: "somepath", + GitSuffix: ".git", + }, }, } - for tn, tc := range testcases { - t.Run(tn, func(t *testing.T) { + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { rs, err := NewRepoSpecFromURL(tc.input) assert.NoError(t, err) assert.Equal(t, tc.cloneSpec, rs.CloneSpec(), "cloneSpec mismatch") assert.Equal(t, tc.absPath, rs.AbsPath(), "absPath mismatch") - assert.Equal(t, tc.ref, rs.Ref, "ref mismatch") + // some values have defaults. Clear them here so test cases remain compact. + // This means submodules and timeout cannot be tested here. That's fine since + // they are tested in TestPeelQuery. + rs.raw = "" + rs.Dir = "" + rs.Submodules = false + rs.Timeout = 0 + assert.Equal(t, &tc.repoSpec, rs) }) } } @@ -231,14 +335,16 @@ func TestIsAzureHost(t *testing.T) { } func TestPeelQuery(t *testing.T) { - testcases := map[string]struct { + testcases := []struct { + name string input string path string ref string submodules bool timeout time.Duration }{ - "t1": { + { + name: "t1", // All empty. input: "somerepos", path: "somerepos", @@ -246,21 +352,24 @@ func TestPeelQuery(t *testing.T) { submodules: defaultSubmodules, timeout: defaultTimeout, }, - "t2": { + { + name: "t2", input: "somerepos?ref=v1.0.0", path: "somerepos", ref: "v1.0.0", submodules: defaultSubmodules, timeout: defaultTimeout, }, - "t3": { + { + name: "t3", input: "somerepos?version=master", path: "somerepos", ref: "master", submodules: defaultSubmodules, timeout: defaultTimeout, }, - "t4": { + { + name: "t4", // A ref value takes precedence over a version value. input: "somerepos?version=master&ref=v1.0.0", path: "somerepos", @@ -268,7 +377,8 @@ func TestPeelQuery(t *testing.T) { submodules: defaultSubmodules, timeout: defaultTimeout, }, - "t5": { + { + name: "t5", // Empty submodules value uses default. input: "somerepos?version=master&submodules=", path: "somerepos", @@ -276,7 +386,8 @@ func TestPeelQuery(t *testing.T) { submodules: defaultSubmodules, timeout: defaultTimeout, }, - "t6": { + { + name: "t6", // Malformed submodules value uses default. input: "somerepos?version=master&submodules=maybe", path: "somerepos", @@ -284,21 +395,24 @@ func TestPeelQuery(t *testing.T) { submodules: defaultSubmodules, timeout: defaultTimeout, }, - "t7": { + { + name: "t7", input: "somerepos?version=master&submodules=true", path: "somerepos", ref: "master", submodules: true, timeout: defaultTimeout, }, - "t8": { + { + name: "t8", input: "somerepos?version=master&submodules=false", path: "somerepos", ref: "master", submodules: false, timeout: defaultTimeout, }, - "t9": { + { + name: "t9", // Empty timeout value uses default. input: "somerepos?version=master&timeout=", path: "somerepos", @@ -306,7 +420,8 @@ func TestPeelQuery(t *testing.T) { submodules: defaultSubmodules, timeout: defaultTimeout, }, - "t10": { + { + name: "t10", // Malformed timeout value uses default. input: "somerepos?version=master&timeout=jiffy", path: "somerepos", @@ -314,7 +429,8 @@ func TestPeelQuery(t *testing.T) { submodules: defaultSubmodules, timeout: defaultTimeout, }, - "t11": { + { + name: "t11", // Zero timeout value uses default. input: "somerepos?version=master&timeout=0", path: "somerepos", @@ -322,28 +438,32 @@ func TestPeelQuery(t *testing.T) { submodules: defaultSubmodules, timeout: defaultTimeout, }, - "t12": { + { + name: "t12", input: "somerepos?version=master&timeout=0s", path: "somerepos", ref: "master", submodules: defaultSubmodules, timeout: defaultTimeout, }, - "t13": { + { + name: "t13", input: "somerepos?version=master&timeout=61", path: "somerepos", ref: "master", submodules: defaultSubmodules, timeout: 61 * time.Second, }, - "t14": { + { + name: "t14", input: "somerepos?version=master&timeout=1m1s", path: "somerepos", ref: "master", submodules: defaultSubmodules, timeout: 61 * time.Second, }, - "t15": { + { + name: "t15", input: "somerepos?version=master&submodules=false&timeout=1m1s", path: "somerepos", ref: "master", @@ -351,8 +471,8 @@ func TestPeelQuery(t *testing.T) { timeout: 61 * time.Second, }, } - for tn, tc := range testcases { - t.Run(tn, func(t *testing.T) { + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { path, ref, timeout, submodules := peelQuery(tc.input) assert.Equal(t, tc.path, path, "path mismatch") assert.Equal(t, tc.ref, ref, "ref mismatch")