From fcbddfc6ae2a4ea45bd180b2085bc591aeb0cc37 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 30 Apr 2023 14:48:09 +0200 Subject: [PATCH 01/16] reference: use consistent names for test-tables Signed-off-by: Sebastiaan van Stijn --- reference/normalize_test.go | 64 ++++++------ reference/reference_test.go | 190 ++++++++++++++++++------------------ reference/regexp_test.go | 16 +-- 3 files changed, 135 insertions(+), 135 deletions(-) diff --git a/reference/normalize_test.go b/reference/normalize_test.go index 22803672..ef3944be 100644 --- a/reference/normalize_test.go +++ b/reference/normalize_test.go @@ -146,7 +146,7 @@ func TestParseRepositoryInfo(t *testing.T) { RemoteName, FamiliarName, FullName, AmbiguousName, Domain string } - tcases := []tcase{ + tests := []tcase{ { RemoteName: "fooo/bar", FamiliarName: "fooo/bar", @@ -261,7 +261,7 @@ func TestParseRepositoryInfo(t *testing.T) { }, } - for i, tc := range tcases { + for i, tc := range tests { tc := tc refStrings := []string{tc.FamiliarName, tc.FullName} if tc.AmbiguousName != "" { @@ -367,7 +367,7 @@ func equalReference(r1, r2 Reference) bool { func TestParseAnyReference(t *testing.T) { t.Parallel() - tcases := []struct { + tests := []struct { Reference string Equivalent string Expected Reference @@ -432,22 +432,22 @@ func TestParseAnyReference(t *testing.T) { }, } - for _, tcase := range tcases { + for _, tc := range tests { var ref Reference var err error - ref, err = ParseAnyReference(tcase.Reference) + ref, err = ParseAnyReference(tc.Reference) if err != nil { - t.Fatalf("Error parsing reference %s: %v", tcase.Reference, err) + t.Fatalf("Error parsing reference %s: %v", tc.Reference, err) } - if ref.String() != tcase.Equivalent { - t.Fatalf("Unexpected string: %s, expected %s", ref.String(), tcase.Equivalent) + if ref.String() != tc.Equivalent { + t.Fatalf("Unexpected string: %s, expected %s", ref.String(), tc.Equivalent) } - expected := tcase.Expected + expected := tc.Expected if expected == nil { - expected, err = Parse(tcase.Equivalent) + expected, err = Parse(tc.Equivalent) if err != nil { - t.Fatalf("Error parsing reference %s: %v", tcase.Equivalent, err) + t.Fatalf("Error parsing reference %s: %v", tc.Equivalent, err) } } if !equalReference(ref, expected) { @@ -458,7 +458,7 @@ func TestParseAnyReference(t *testing.T) { func TestNormalizedSplitHostname(t *testing.T) { t.Parallel() - testcases := []struct { + tests := []struct { input string domain string name string @@ -519,22 +519,22 @@ func TestNormalizedSplitHostname(t *testing.T) { name: "library/foo/bar", }, } - for _, testcase := range testcases { + for _, tc := range tests { failf := func(format string, v ...interface{}) { - t.Logf(strconv.Quote(testcase.input)+": "+format, v...) + t.Logf(strconv.Quote(tc.input)+": "+format, v...) t.Fail() } - named, err := ParseNormalizedNamed(testcase.input) + named, err := ParseNormalizedNamed(tc.input) if err != nil { failf("error parsing name: %s", err) } domain, name := SplitHostname(named) - if domain != testcase.domain { - failf("unexpected domain: got %q, expected %q", domain, testcase.domain) + if domain != tc.domain { + failf("unexpected domain: got %q, expected %q", domain, tc.domain) } - if name != testcase.name { - failf("unexpected name: got %q, expected %q", name, testcase.name) + if name != tc.name { + failf("unexpected name: got %q, expected %q", name, tc.name) } } } @@ -553,7 +553,7 @@ func TestMatchError(t *testing.T) { func TestMatch(t *testing.T) { t.Parallel() - matchCases := []struct { + tests := []struct { reference string pattern string expected bool @@ -604,24 +604,24 @@ func TestMatch(t *testing.T) { expected: true, }, } - for _, c := range matchCases { - named, err := ParseAnyReference(c.reference) + for _, tc := range tests { + named, err := ParseAnyReference(tc.reference) if err != nil { t.Fatal(err) } - actual, err := FamiliarMatch(c.pattern, named) + actual, err := FamiliarMatch(tc.pattern, named) if err != nil { t.Fatal(err) } - if actual != c.expected { - t.Fatalf("expected %s match %s to be %v, was %v", c.reference, c.pattern, c.expected, actual) + if actual != tc.expected { + t.Fatalf("expected %s match %s to be %v, was %v", tc.reference, tc.pattern, tc.expected, actual) } } } func TestParseDockerRef(t *testing.T) { t.Parallel() - testcases := []struct { + tests := []struct { name string input string expected string @@ -682,17 +682,17 @@ func TestParseDockerRef(t *testing.T) { expected: "gcr.io/library/busybox@sha256:e6693c20186f837fc393390135d8a598a96a833917917789d63766cab6c59582", }, } - for _, test := range testcases { - test := test - t.Run(test.name, func(t *testing.T) { + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { t.Parallel() - normalized, err := ParseDockerRef(test.input) + normalized, err := ParseDockerRef(tc.input) if err != nil { t.Fatal(err) } output := normalized.String() - if output != test.expected { - t.Fatalf("expected %q to be parsed as %v, got %v", test.input, test.expected, output) + if output != tc.expected { + t.Fatalf("expected %q to be parsed as %v, got %v", tc.input, tc.expected, output) } _, err = Parse(output) if err != nil { diff --git a/reference/reference_test.go b/reference/reference_test.go index 5d6878cf..d5a897c7 100644 --- a/reference/reference_test.go +++ b/reference/reference_test.go @@ -12,9 +12,9 @@ import ( func TestReferenceParse(t *testing.T) { t.Parallel() - // referenceTestcases is a unified set of testcases for + // tests is a unified set of testcases for // testing the parsing of references - referenceTestcases := []struct { + tests := []struct { // input is the repository name or name component testcase input string // err is the error expected from Parse, or nil @@ -267,43 +267,43 @@ func TestReferenceParse(t *testing.T) { err: ErrReferenceInvalidFormat, }, } - for _, testcase := range referenceTestcases { - testcase := testcase - t.Run(testcase.input, func(t *testing.T) { + for _, tc := range tests { + tc := tc + t.Run(tc.input, func(t *testing.T) { t.Parallel() - repo, err := Parse(testcase.input) - if testcase.err != nil { + repo, err := Parse(tc.input) + if tc.err != nil { if err == nil { - t.Errorf("missing expected error: %v", testcase.err) - } else if testcase.err != err { - t.Errorf("mismatched error: got %v, expected %v", err, testcase.err) + t.Errorf("missing expected error: %v", tc.err) + } else if tc.err != err { + t.Errorf("mismatched error: got %v, expected %v", err, tc.err) } return } else if err != nil { t.Errorf("unexpected parse error: %v", err) return } - if repo.String() != testcase.input { - t.Errorf("mismatched repo: got %q, expected %q", repo.String(), testcase.input) + if repo.String() != tc.input { + t.Errorf("mismatched repo: got %q, expected %q", repo.String(), tc.input) } if named, ok := repo.(Named); ok { - if named.Name() != testcase.repository { - t.Errorf("unexpected repository: got %q, expected %q", named.Name(), testcase.repository) + if named.Name() != tc.repository { + t.Errorf("unexpected repository: got %q, expected %q", named.Name(), tc.repository) } domain, _ := SplitHostname(named) - if domain != testcase.domain { - t.Errorf("unexpected domain: got %q, expected %q", domain, testcase.domain) + if domain != tc.domain { + t.Errorf("unexpected domain: got %q, expected %q", domain, tc.domain) } - } else if testcase.repository != "" || testcase.domain != "" { + } else if tc.repository != "" || tc.domain != "" { t.Errorf("expected named type, got %T", repo) } tagged, ok := repo.(Tagged) - if testcase.tag != "" { + if tc.tag != "" { if ok { - if tagged.Tag() != testcase.tag { - t.Errorf("unexpected tag: got %q, expected %q", tagged.Tag(), testcase.tag) + if tagged.Tag() != tc.tag { + t.Errorf("unexpected tag: got %q, expected %q", tagged.Tag(), tc.tag) } } else { t.Errorf("expected tagged type, got %T", repo) @@ -313,10 +313,10 @@ func TestReferenceParse(t *testing.T) { } digested, ok := repo.(Digested) - if testcase.digest != "" { + if tc.digest != "" { if ok { - if digested.Digest().String() != testcase.digest { - t.Errorf("unexpected digest: got %q, expected %q", digested.Digest().String(), testcase.digest) + if digested.Digest().String() != tc.digest { + t.Errorf("unexpected digest: got %q, expected %q", digested.Digest().String(), tc.digest) } } else { t.Errorf("expected digested type, got %T", repo) @@ -332,7 +332,7 @@ func TestReferenceParse(t *testing.T) { // should succeed are covered by TestSplitHostname, below. func TestWithNameFailure(t *testing.T) { t.Parallel() - testcases := []struct { + tests := []struct { input string err error }{ @@ -361,13 +361,13 @@ func TestWithNameFailure(t *testing.T) { err: ErrReferenceInvalidFormat, }, } - for _, testcase := range testcases { - testcase := testcase - t.Run(testcase.input, func(t *testing.T) { + for _, tc := range tests { + tc := tc + t.Run(tc.input, func(t *testing.T) { t.Parallel() - _, err := WithName(testcase.input) + _, err := WithName(tc.input) if err == nil { - t.Errorf("no error parsing name. expected: %s", testcase.err) + t.Errorf("no error parsing name. expected: %s", tc.err) } }) } @@ -375,7 +375,7 @@ func TestWithNameFailure(t *testing.T) { func TestSplitHostname(t *testing.T) { t.Parallel() - testcases := []struct { + tests := []struct { input string domain string name string @@ -411,20 +411,20 @@ func TestSplitHostname(t *testing.T) { name: "foo", }, } - for _, testcase := range testcases { - testcase := testcase - t.Run(testcase.input, func(t *testing.T) { + for _, tc := range tests { + tc := tc + t.Run(tc.input, func(t *testing.T) { t.Parallel() - named, err := WithName(testcase.input) + named, err := WithName(tc.input) if err != nil { t.Errorf("error parsing name: %s", err) } domain, name := SplitHostname(named) - if domain != testcase.domain { - t.Errorf("unexpected domain: got %q, expected %q", domain, testcase.domain) + if domain != tc.domain { + t.Errorf("unexpected domain: got %q, expected %q", domain, tc.domain) } - if name != testcase.name { - t.Errorf("unexpected name: got %q, expected %q", name, testcase.name) + if name != tc.name { + t.Errorf("unexpected name: got %q, expected %q", name, tc.name) } }) } @@ -437,7 +437,7 @@ type serializationType struct { func TestSerialization(t *testing.T) { t.Parallel() - testcases := []struct { + tests := []struct { description string input string name string @@ -467,13 +467,13 @@ func TestSerialization(t *testing.T) { digest: "sha256:1234567890098765432112345667890098765432112345667890098765432112", }, } - for _, testcase := range testcases { - testcase := testcase - t.Run(testcase.description, func(t *testing.T) { + for _, tc := range tests { + tc := tc + t.Run(tc.description, func(t *testing.T) { t.Parallel() m := map[string]string{ - "Description": testcase.description, - "Field": testcase.input, + "Description": tc.description, + "Field": tc.input, } b, err := json.Marshal(m) if err != nil { @@ -482,37 +482,37 @@ func TestSerialization(t *testing.T) { st := serializationType{} if err := json.Unmarshal(b, &st); err != nil { - if testcase.err == nil { + if tc.err == nil { t.Errorf("error unmarshalling: %v", err) } - if err != testcase.err { - t.Errorf("wrong error, expected %v, got %v", testcase.err, err) + if err != tc.err { + t.Errorf("wrong error, expected %v, got %v", tc.err, err) } return - } else if testcase.err != nil { - t.Errorf("expected error unmarshalling: %v", testcase.err) + } else if tc.err != nil { + t.Errorf("expected error unmarshalling: %v", tc.err) } - if st.Description != testcase.description { - t.Errorf("wrong description, expected %q, got %q", testcase.description, st.Description) + if st.Description != tc.description { + t.Errorf("wrong description, expected %q, got %q", tc.description, st.Description) } ref := st.Field.Reference() if named, ok := ref.(Named); ok { - if named.Name() != testcase.name { - t.Errorf("unexpected repository: got %q, expected %q", named.Name(), testcase.name) + if named.Name() != tc.name { + t.Errorf("unexpected repository: got %q, expected %q", named.Name(), tc.name) } - } else if testcase.name != "" { + } else if tc.name != "" { t.Errorf("expected named type, got %T", ref) } tagged, ok := ref.(Tagged) - if testcase.tag != "" { + if tc.tag != "" { if ok { - if tagged.Tag() != testcase.tag { - t.Errorf("unexpected tag: got %q, expected %q", tagged.Tag(), testcase.tag) + if tagged.Tag() != tc.tag { + t.Errorf("unexpected tag: got %q, expected %q", tagged.Tag(), tc.tag) } } else { t.Errorf("expected tagged type, got %T", ref) @@ -522,10 +522,10 @@ func TestSerialization(t *testing.T) { } digested, ok := ref.(Digested) - if testcase.digest != "" { + if tc.digest != "" { if ok { - if digested.Digest().String() != testcase.digest { - t.Errorf("unexpected digest: got %q, expected %q", digested.Digest().String(), testcase.digest) + if digested.Digest().String() != tc.digest { + t.Errorf("unexpected digest: got %q, expected %q", digested.Digest().String(), tc.digest) } } else { t.Errorf("expected digested type, got %T", ref) @@ -535,7 +535,7 @@ func TestSerialization(t *testing.T) { } st = serializationType{ - Description: testcase.description, + Description: tc.description, Field: AsField(ref), } @@ -560,7 +560,7 @@ func TestSerialization(t *testing.T) { func TestWithTag(t *testing.T) { t.Parallel() - testcases := []struct { + tests := []struct { name string digest digest.Digest tag string @@ -593,28 +593,28 @@ func TestWithTag(t *testing.T) { combined: "test.com:8000/foo:TAG5@sha256:1234567890098765432112345667890098765", }, } - for _, testcase := range testcases { - testcase := testcase - t.Run(testcase.combined, func(t *testing.T) { + for _, tc := range tests { + tc := tc + t.Run(tc.combined, func(t *testing.T) { t.Parallel() - named, err := WithName(testcase.name) + named, err := WithName(tc.name) if err != nil { t.Errorf("error parsing name: %s", err) } - if testcase.digest != "" { - canonical, err := WithDigest(named, testcase.digest) + if tc.digest != "" { + canonical, err := WithDigest(named, tc.digest) if err != nil { t.Errorf("error adding digest") } named = canonical } - tagged, err := WithTag(named, testcase.tag) + tagged, err := WithTag(named, tc.tag) if err != nil { t.Errorf("WithTag failed: %s", err) } - if tagged.String() != testcase.combined { - t.Errorf("unexpected: got %q, expected %q", tagged.String(), testcase.combined) + if tagged.String() != tc.combined { + t.Errorf("unexpected: got %q, expected %q", tagged.String(), tc.combined) } }) } @@ -622,7 +622,7 @@ func TestWithTag(t *testing.T) { func TestWithDigest(t *testing.T) { t.Parallel() - testcases := []struct { + tests := []struct { name string digest digest.Digest tag string @@ -650,27 +650,27 @@ func TestWithDigest(t *testing.T) { combined: "test.com:8000/foo:latest@sha256:1234567890098765432112345667890098765", }, } - for _, testcase := range testcases { - testcase := testcase - t.Run(testcase.combined, func(t *testing.T) { + for _, tc := range tests { + tc := tc + t.Run(tc.combined, func(t *testing.T) { t.Parallel() - named, err := WithName(testcase.name) + named, err := WithName(tc.name) if err != nil { t.Errorf("error parsing name: %s", err) } - if testcase.tag != "" { - tagged, err := WithTag(named, testcase.tag) + if tc.tag != "" { + tagged, err := WithTag(named, tc.tag) if err != nil { t.Errorf("error adding tag") } named = tagged } - digested, err := WithDigest(named, testcase.digest) + digested, err := WithDigest(named, tc.digest) if err != nil { t.Errorf("WithDigest failed: %s", err) } - if digested.String() != testcase.combined { - t.Errorf("unexpected: got %q, expected %q", digested.String(), testcase.combined) + if digested.String() != tc.combined { + t.Errorf("unexpected: got %q, expected %q", digested.String(), tc.combined) } }) } @@ -678,7 +678,7 @@ func TestWithDigest(t *testing.T) { func TestParseNamed(t *testing.T) { t.Parallel() - testcases := []struct { + tests := []struct { input string domain string name string @@ -721,30 +721,30 @@ func TestParseNamed(t *testing.T) { err: ErrNameNotCanonical, }, } - for _, testcase := range testcases { - testcase := testcase - t.Run(testcase.input, func(t *testing.T) { + for _, tc := range tests { + tc := tc + t.Run(tc.input, func(t *testing.T) { t.Parallel() - named, err := ParseNamed(testcase.input) - if err != nil && testcase.err == nil { + named, err := ParseNamed(tc.input) + if err != nil && tc.err == nil { t.Errorf("error parsing name: %s", err) return - } else if err == nil && testcase.err != nil { - t.Errorf("parsing succeeded: expected error %v", testcase.err) + } else if err == nil && tc.err != nil { + t.Errorf("parsing succeeded: expected error %v", tc.err) return - } else if err != testcase.err { - t.Errorf("unexpected error %v, expected %v", err, testcase.err) + } else if err != tc.err { + t.Errorf("unexpected error %v, expected %v", err, tc.err) return } else if err != nil { return } domain, name := SplitHostname(named) - if domain != testcase.domain { - t.Errorf("unexpected domain: got %q, expected %q", domain, testcase.domain) + if domain != tc.domain { + t.Errorf("unexpected domain: got %q, expected %q", domain, tc.domain) } - if name != testcase.name { - t.Errorf("unexpected name: got %q, expected %q", name, testcase.name) + if name != tc.name { + t.Errorf("unexpected name: got %q, expected %q", name, tc.name) } }) } diff --git a/reference/regexp_test.go b/reference/regexp_test.go index 44b33f01..ca4680d3 100644 --- a/reference/regexp_test.go +++ b/reference/regexp_test.go @@ -36,7 +36,7 @@ func checkRegexp(t *testing.T, r *regexp.Regexp, m regexpMatch) { func TestDomainRegexp(t *testing.T) { t.Parallel() - hostcases := []struct { + tests := []struct { input string match bool }{ @@ -162,7 +162,7 @@ func TestDomainRegexp(t *testing.T) { }, } r := regexp.MustCompile(`^` + DomainRegexp.String() + `$`) - for _, tc := range hostcases { + for _, tc := range tests { tc := tc t.Run(tc.input, func(t *testing.T) { t.Parallel() @@ -181,7 +181,7 @@ func TestFullNameRegexp(t *testing.T) { anchoredNameRegexp, anchoredNameRegexp.NumSubexp()) } - testcases := []regexpMatch{ + tests := []regexpMatch{ { input: "", match: false, @@ -465,7 +465,7 @@ func TestFullNameRegexp(t *testing.T) { match: false, }, } - for _, tc := range testcases { + for _, tc := range tests { tc := tc t.Run(tc.input, func(t *testing.T) { t.Parallel() @@ -481,7 +481,7 @@ func TestReferenceRegexp(t *testing.T) { ReferenceRegexp, ReferenceRegexp.NumSubexp()) } - testcases := []regexpMatch{ + tests := []regexpMatch{ { input: "registry.com:8080/myapp:tag", match: true, @@ -540,7 +540,7 @@ func TestReferenceRegexp(t *testing.T) { }, } - for _, tc := range testcases { + for _, tc := range tests { tc := tc t.Run(tc.input, func(t *testing.T) { t.Parallel() @@ -551,7 +551,7 @@ func TestReferenceRegexp(t *testing.T) { func TestIdentifierRegexp(t *testing.T) { t.Parallel() - fullCases := []struct { + tests := []struct { input string match bool }{ @@ -576,7 +576,7 @@ func TestIdentifierRegexp(t *testing.T) { match: false, }, } - for _, tc := range fullCases { + for _, tc := range tests { tc := tc t.Run(tc.input, func(t *testing.T) { t.Parallel() From fa1d14c5132c044140da59cfcbdb326e5c898cb4 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 30 Apr 2023 14:58:45 +0200 Subject: [PATCH 02/16] reference: TestParseAnyReference(): use sub-tests Signed-off-by: Sebastiaan van Stijn --- reference/normalize_test.go | 40 ++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/reference/normalize_test.go b/reference/normalize_test.go index ef3944be..72b1b92a 100644 --- a/reference/normalize_test.go +++ b/reference/normalize_test.go @@ -433,26 +433,30 @@ func TestParseAnyReference(t *testing.T) { } for _, tc := range tests { - var ref Reference - var err error - ref, err = ParseAnyReference(tc.Reference) - if err != nil { - t.Fatalf("Error parsing reference %s: %v", tc.Reference, err) - } - if ref.String() != tc.Equivalent { - t.Fatalf("Unexpected string: %s, expected %s", ref.String(), tc.Equivalent) - } - - expected := tc.Expected - if expected == nil { - expected, err = Parse(tc.Equivalent) + tc := tc + t.Run(tc.Reference, func(t *testing.T) { + t.Parallel() + var ref Reference + var err error + ref, err = ParseAnyReference(tc.Reference) if err != nil { - t.Fatalf("Error parsing reference %s: %v", tc.Equivalent, err) + t.Fatalf("Error parsing reference %s: %v", tc.Reference, err) } - } - if !equalReference(ref, expected) { - t.Errorf("Unexpected reference %#v, expected %#v", ref, expected) - } + if ref.String() != tc.Equivalent { + t.Fatalf("Unexpected string: %s, expected %s", ref.String(), tc.Equivalent) + } + + expected := tc.Expected + if expected == nil { + expected, err = Parse(tc.Equivalent) + if err != nil { + t.Fatalf("Error parsing reference %s: %v", tc.Equivalent, err) + } + } + if !equalReference(ref, expected) { + t.Errorf("Unexpected reference %#v, expected %#v", ref, expected) + } + }) } } From 2819bca991a22e384e7b4e4b127025cebec87cfb Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 30 Apr 2023 15:00:51 +0200 Subject: [PATCH 03/16] reference: TestNormalizedSplitHostname(): use sub-tests Signed-off-by: Sebastiaan van Stijn --- reference/normalize_test.go | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/reference/normalize_test.go b/reference/normalize_test.go index 72b1b92a..acfe5ec5 100644 --- a/reference/normalize_test.go +++ b/reference/normalize_test.go @@ -524,22 +524,21 @@ func TestNormalizedSplitHostname(t *testing.T) { }, } for _, tc := range tests { - failf := func(format string, v ...interface{}) { - t.Logf(strconv.Quote(tc.input)+": "+format, v...) - t.Fail() - } - - named, err := ParseNormalizedNamed(tc.input) - if err != nil { - failf("error parsing name: %s", err) - } - domain, name := SplitHostname(named) - if domain != tc.domain { - failf("unexpected domain: got %q, expected %q", domain, tc.domain) - } - if name != tc.name { - failf("unexpected name: got %q, expected %q", name, tc.name) - } + tc := tc + t.Run(tc.input, func(t *testing.T) { + t.Parallel() + named, err := ParseNormalizedNamed(tc.input) + if err != nil { + t.Errorf("error parsing name: %s", err) + } + domain, name := SplitHostname(named) + if domain != tc.domain { + t.Errorf("unexpected domain: got %q, expected %q", domain, tc.domain) + } + if name != tc.name { + t.Errorf("unexpected name: got %q, expected %q", name, tc.name) + } + }) } } From f238f7dcaa9133ce208b44737fed5e0b985cc310 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 30 Apr 2023 15:03:30 +0200 Subject: [PATCH 04/16] reference: TestMatch(): use sub-tests Signed-off-by: Sebastiaan van Stijn --- reference/normalize_test.go | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/reference/normalize_test.go b/reference/normalize_test.go index acfe5ec5..0932d06f 100644 --- a/reference/normalize_test.go +++ b/reference/normalize_test.go @@ -608,17 +608,21 @@ func TestMatch(t *testing.T) { }, } for _, tc := range tests { - named, err := ParseAnyReference(tc.reference) - if err != nil { - t.Fatal(err) - } - actual, err := FamiliarMatch(tc.pattern, named) - if err != nil { - t.Fatal(err) - } - if actual != tc.expected { - t.Fatalf("expected %s match %s to be %v, was %v", tc.reference, tc.pattern, tc.expected, actual) - } + tc := tc + t.Run(tc.reference, func(t *testing.T) { + t.Parallel() + named, err := ParseAnyReference(tc.reference) + if err != nil { + t.Fatal(err) + } + actual, err := FamiliarMatch(tc.pattern, named) + if err != nil { + t.Fatal(err) + } + if actual != tc.expected { + t.Fatalf("expected %s match %s to be %v, was %v", tc.reference, tc.pattern, tc.expected, actual) + } + }) } } From 2829f7b7d5417580d7036521c4c55c598d7bd564 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 30 Apr 2023 14:43:13 +0200 Subject: [PATCH 05/16] context: use consistent names for test-tables Also renamed a var that collided with a type, and marked a helper as t.Helper() Signed-off-by: Sebastiaan van Stijn --- context/http_test.go | 20 ++++++++++---------- context/trace_test.go | 19 ++++++++++--------- 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/context/http_test.go b/context/http_test.go index 69682966..5c741d59 100644 --- a/context/http_test.go +++ b/context/http_test.go @@ -22,7 +22,7 @@ func TestWithRequest(t *testing.T) { req.Header.Set("User-Agent", "test/0.1") ctx := WithRequest(Background(), &req) - for _, testcase := range []struct { + for _, tc := range []struct { key string expected interface{} }{ @@ -61,18 +61,18 @@ func TestWithRequest(t *testing.T) { key: "http.request.startedat", }, } { - v := ctx.Value(testcase.key) + v := ctx.Value(tc.key) if v == nil { - t.Fatalf("value not found for %q", testcase.key) + t.Fatalf("value not found for %q", tc.key) } - if testcase.expected != nil && v != testcase.expected { - t.Fatalf("%s: %v != %v", testcase.key, v, testcase.expected) + if tc.expected != nil && v != tc.expected { + t.Fatalf("%s: %v != %v", tc.key, v, tc.expected) } // Key specific checks! - switch testcase.key { + switch tc.key { case "http.request.id": if _, ok := v.(string); !ok { t.Fatalf("request id not a string: %v", v) @@ -195,7 +195,7 @@ func TestWithVars(t *testing.T) { } ctx := WithVars(Background(), &req) - for _, testcase := range []struct { + for _, tc := range []struct { key string expected interface{} }{ @@ -212,10 +212,10 @@ func TestWithVars(t *testing.T) { expected: "qwer", }, } { - v := ctx.Value(testcase.key) + v := ctx.Value(tc.key) - if !reflect.DeepEqual(v, testcase.expected) { - t.Fatalf("%q: %v != %v", testcase.key, v, testcase.expected) + if !reflect.DeepEqual(v, tc.expected) { + t.Fatalf("%q: %v != %v", tc.key, v, tc.expected) } } } diff --git a/context/trace_test.go b/context/trace_test.go index f973f9a4..2fcf1e65 100644 --- a/context/trace_test.go +++ b/context/trace_test.go @@ -41,7 +41,7 @@ func TestWithTrace(t *testing.T) { expected: f.Name(), })) - traced := func() { + tracedFn := func() { parentID := ctx.Value("trace.id") // ensure the parent trace id is correct. pc, _, _, _ := runtime.Caller(0) // get current caller. @@ -57,7 +57,7 @@ func TestWithTrace(t *testing.T) { expected: parentID, })) } - traced() + tracedFn() time.Sleep(time.Second) } @@ -68,18 +68,19 @@ type valueTestCase struct { notnilorempty bool // just check not empty/not nil } -func checkContextForValues(ctx context.Context, t *testing.T, values []valueTestCase) { - for _, testcase := range values { - v := ctx.Value(testcase.key) - if testcase.notnilorempty { +func checkContextForValues(ctx context.Context, t *testing.T, tests []valueTestCase) { + t.Helper() + for _, tc := range tests { + v := ctx.Value(tc.key) + if tc.notnilorempty { if v == nil || v == "" { - t.Fatalf("value was nil or empty for %q: %#v", testcase.key, v) + t.Fatalf("value was nil or empty for %q: %#v", tc.key, v) } continue } - if v != testcase.expected { - t.Fatalf("unexpected value for key %q: %v != %v", testcase.key, v, testcase.expected) + if v != tc.expected { + t.Fatalf("unexpected value for key %q: %v != %v", tc.key, v, tc.expected) } } } From f4be328bff4cc94435d70ea69e6c17ac2f05c20d Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 30 Apr 2023 15:41:20 +0200 Subject: [PATCH 06/16] context: TestWithTrace(): inline checkContextForValues, use sub-tests checkContextForValues was effectively running sub-tests, but disguided as a helper (to DRY). While it helped some duplication, rewriting it to run subtests within a helper also was a bit confusing, so just inline what it does. While updating, also run tests with t.Parallel() Signed-off-by: Sebastiaan van Stijn --- context/trace_test.go | 61 +++++++++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), 22 deletions(-) diff --git a/context/trace_test.go b/context/trace_test.go index 2fcf1e65..4ee530bf 100644 --- a/context/trace_test.go +++ b/context/trace_test.go @@ -1,7 +1,6 @@ package context import ( - "context" "runtime" "testing" "time" @@ -9,6 +8,7 @@ import ( // TestWithTrace ensures that tracing has the expected values in the context. func TestWithTrace(t *testing.T) { + t.Parallel() pc, file, _, _ := runtime.Caller(0) // get current caller. f := runtime.FuncForPC(pc) @@ -36,10 +36,27 @@ func TestWithTrace(t *testing.T) { ctx, done := WithTrace(Background()) defer done("this will be emitted at end of test") - checkContextForValues(ctx, t, append(base, valueTestCase{ + tests := append(base, valueTestCase{ key: "trace.func", expected: f.Name(), - })) + }) + for _, tc := range tests { + tc := tc + t.Run(tc.key, func(t *testing.T) { + t.Parallel() + v := ctx.Value(tc.key) + if tc.notnilorempty { + if v == nil || v == "" { + t.Fatalf("value was nil or empty: %#v", v) + } + return + } + + if v != tc.expected { + t.Fatalf("unexpected value: %v != %v", v, tc.expected) + } + }) + } tracedFn := func() { parentID := ctx.Value("trace.id") // ensure the parent trace id is correct. @@ -49,13 +66,30 @@ func TestWithTrace(t *testing.T) { ctx, done := WithTrace(ctx) defer done("this should be subordinate to the other trace") time.Sleep(time.Second) - checkContextForValues(ctx, t, append(base, valueTestCase{ + tests := append(base, valueTestCase{ key: "trace.func", expected: f.Name(), }, valueTestCase{ key: "trace.parent.id", expected: parentID, - })) + }) + for _, tc := range tests { + tc := tc + t.Run(tc.key, func(t *testing.T) { + t.Parallel() + v := ctx.Value(tc.key) + if tc.notnilorempty { + if v == nil || v == "" { + t.Fatalf("value was nil or empty: %#v", v) + } + return + } + + if v != tc.expected { + t.Fatalf("unexpected value: %v != %v", v, tc.expected) + } + }) + } } tracedFn() @@ -67,20 +101,3 @@ type valueTestCase struct { expected interface{} notnilorempty bool // just check not empty/not nil } - -func checkContextForValues(ctx context.Context, t *testing.T, tests []valueTestCase) { - t.Helper() - for _, tc := range tests { - v := ctx.Value(tc.key) - if tc.notnilorempty { - if v == nil || v == "" { - t.Fatalf("value was nil or empty for %q: %#v", tc.key, v) - } - continue - } - - if v != tc.expected { - t.Fatalf("unexpected value for key %q: %v != %v", tc.key, v, tc.expected) - } - } -} From e9ac1728e6ebd6ba90bf3b31bdff7422d178f4de Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 30 Apr 2023 14:40:44 +0200 Subject: [PATCH 07/16] notifications: use consistent names for test tables Signed-off-by: Sebastiaan van Stijn --- notifications/sinks_test.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/notifications/sinks_test.go b/notifications/sinks_test.go index b1e51d5b..5347bb03 100644 --- a/notifications/sinks_test.go +++ b/notifications/sinks_test.go @@ -73,7 +73,7 @@ func TestIgnoredSink(t *testing.T) { expected events.Event } - cases := []testcase{ + tests := []testcase{ {nil, nil, blob}, {[]string{"other"}, []string{"other"}, blob}, {[]string{"blob", "manifest"}, []string{"other"}, nil}, @@ -81,22 +81,22 @@ func TestIgnoredSink(t *testing.T) { {[]string{"other"}, []string{"pull", "push"}, nil}, } - for _, c := range cases { + for _, tc := range tests { ts := &testSink{} - s := newIgnoredSink(ts, c.ignoreMediaTypes, c.ignoreActions) + s := newIgnoredSink(ts, tc.ignoreMediaTypes, tc.ignoreActions) if err := s.Write(blob); err != nil { t.Fatalf("error writing event: %v", err) } ts.mu.Lock() - if !reflect.DeepEqual(ts.event, c.expected) { - t.Fatalf("unexpected event: %#v != %#v", ts.event, c.expected) + if !reflect.DeepEqual(ts.event, tc.expected) { + t.Fatalf("unexpected event: %#v != %#v", ts.event, tc.expected) } ts.mu.Unlock() } - cases = []testcase{ + tests = []testcase{ {nil, nil, manifest}, {[]string{"other"}, []string{"other"}, manifest}, {[]string{"blob"}, []string{"other"}, manifest}, @@ -105,17 +105,17 @@ func TestIgnoredSink(t *testing.T) { {[]string{"other"}, []string{"pull", "push"}, nil}, } - for _, c := range cases { + for _, tc := range tests { ts := &testSink{} - s := newIgnoredSink(ts, c.ignoreMediaTypes, c.ignoreActions) + s := newIgnoredSink(ts, tc.ignoreMediaTypes, tc.ignoreActions) if err := s.Write(manifest); err != nil { t.Fatalf("error writing event: %v", err) } ts.mu.Lock() - if !reflect.DeepEqual(ts.event, c.expected) { - t.Fatalf("unexpected event: %#v != %#v", ts.event, c.expected) + if !reflect.DeepEqual(ts.event, tc.expected) { + t.Fatalf("unexpected event: %#v != %#v", ts.event, tc.expected) } ts.mu.Unlock() } From 57f9f31af92f556c3a5b7e4e435c3a06a8fa077c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 30 Apr 2023 14:41:57 +0200 Subject: [PATCH 08/16] notifications: don't use un-keyed structs Signed-off-by: Sebastiaan van Stijn --- notifications/sinks_test.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/notifications/sinks_test.go b/notifications/sinks_test.go index 5347bb03..de3eae3f 100644 --- a/notifications/sinks_test.go +++ b/notifications/sinks_test.go @@ -74,11 +74,11 @@ func TestIgnoredSink(t *testing.T) { } tests := []testcase{ - {nil, nil, blob}, - {[]string{"other"}, []string{"other"}, blob}, - {[]string{"blob", "manifest"}, []string{"other"}, nil}, - {[]string{"other"}, []string{"pull"}, blob}, - {[]string{"other"}, []string{"pull", "push"}, nil}, + {expected: blob}, + {ignoreMediaTypes: []string{"other"}, ignoreActions: []string{"other"}, expected: blob}, + {ignoreMediaTypes: []string{"blob", "manifest"}, ignoreActions: []string{"other"}}, + {ignoreMediaTypes: []string{"other"}, ignoreActions: []string{"pull"}, expected: blob}, + {ignoreMediaTypes: []string{"other"}, ignoreActions: []string{"pull", "push"}}, } for _, tc := range tests { @@ -97,12 +97,12 @@ func TestIgnoredSink(t *testing.T) { } tests = []testcase{ - {nil, nil, manifest}, - {[]string{"other"}, []string{"other"}, manifest}, - {[]string{"blob"}, []string{"other"}, manifest}, - {[]string{"blob", "manifest"}, []string{"other"}, nil}, - {[]string{"other"}, []string{"push"}, manifest}, - {[]string{"other"}, []string{"pull", "push"}, nil}, + {expected: manifest}, + {ignoreMediaTypes: []string{"other"}, ignoreActions: []string{"other"}, expected: manifest}, + {ignoreMediaTypes: []string{"blob"}, ignoreActions: []string{"other"}, expected: manifest}, + {ignoreMediaTypes: []string{"blob", "manifest"}, ignoreActions: []string{"other"}}, + {ignoreMediaTypes: []string{"other"}, ignoreActions: []string{"push"}, expected: manifest}, + {ignoreMediaTypes: []string{"other"}, ignoreActions: []string{"pull", "push"}}, } for _, tc := range tests { From f884a079dff8222766b5b373140ad2255bbdf2b8 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 30 Apr 2023 15:23:44 +0200 Subject: [PATCH 09/16] registry/api/errorcode: TestErrorCodes: use sub-tests Signed-off-by: Sebastiaan van Stijn --- registry/api/errcode/errors_test.go | 78 +++++++++++++++-------------- 1 file changed, 41 insertions(+), 37 deletions(-) diff --git a/registry/api/errcode/errors_test.go b/registry/api/errcode/errors_test.go index 89ee9f3d..6a7eb7d2 100644 --- a/registry/api/errcode/errors_test.go +++ b/registry/api/errcode/errors_test.go @@ -34,57 +34,61 @@ var ErrorCodeTest3 = Register("test.errors", ErrorDescriptor{ // TestErrorCodes ensures that error code format, mappings and // marshaling/unmarshaling. round trips are stable. func TestErrorCodes(t *testing.T) { + t.Parallel() if len(errorCodeToDescriptors) == 0 { t.Fatal("errors aren't loaded!") } for ec, desc := range errorCodeToDescriptors { - if ec != desc.Code { - t.Fatalf("error code in descriptor isn't correct, %q != %q", ec, desc.Code) - } + t.Run(ec.String(), func(t *testing.T) { + t.Parallel() + if ec != desc.Code { + t.Fatalf("error code in descriptor isn't correct, %q != %q", ec, desc.Code) + } - if idToDescriptors[desc.Value].Code != ec { - t.Fatalf("error code in idToDesc isn't correct, %q != %q", idToDescriptors[desc.Value].Code, ec) - } + if idToDescriptors[desc.Value].Code != ec { + t.Fatalf("error code in idToDesc isn't correct, %q != %q", idToDescriptors[desc.Value].Code, ec) + } - if ec.Message() != desc.Message { - t.Fatalf("ec.Message doesn't mtach desc.Message: %q != %q", ec.Message(), desc.Message) - } + if ec.Message() != desc.Message { + t.Fatalf("ec.Message doesn't match desc.Message: %q != %q", ec.Message(), desc.Message) + } - // Test (de)serializing the ErrorCode - p, err := json.Marshal(ec) - if err != nil { - t.Fatalf("couldn't marshal ec %v: %v", ec, err) - } + // Test (de)serializing the ErrorCode + p, err := json.Marshal(ec) + if err != nil { + t.Fatalf("couldn't marshal ec %v: %v", ec, err) + } - if len(p) <= 0 { - t.Fatalf("expected content in marshaled before for error code %v", ec) - } + if len(p) <= 0 { + t.Fatalf("expected content in marshaled before for error code %v", ec) + } - // First, unmarshal to interface and ensure we have a string. - var ecUnspecified interface{} - if err := json.Unmarshal(p, &ecUnspecified); err != nil { - t.Fatalf("error unmarshaling error code %v: %v", ec, err) - } + // First, unmarshal to interface and ensure we have a string. + var ecUnspecified interface{} + if err := json.Unmarshal(p, &ecUnspecified); err != nil { + t.Fatalf("error unmarshaling error code %v: %v", ec, err) + } - if _, ok := ecUnspecified.(string); !ok { - t.Fatalf("expected a string for error code %v on unmarshal got a %T", ec, ecUnspecified) - } + if _, ok := ecUnspecified.(string); !ok { + t.Fatalf("expected a string for error code %v on unmarshal got a %T", ec, ecUnspecified) + } - // Now, unmarshal with the error code type and ensure they are equal - var ecUnmarshaled ErrorCode - if err := json.Unmarshal(p, &ecUnmarshaled); err != nil { - t.Fatalf("error unmarshaling error code %v: %v", ec, err) - } + // Now, unmarshal with the error code type and ensure they are equal + var ecUnmarshaled ErrorCode + if err := json.Unmarshal(p, &ecUnmarshaled); err != nil { + t.Fatalf("error unmarshaling error code %v: %v", ec, err) + } - if ecUnmarshaled != ec { - t.Fatalf("unexpected error code during error code marshal/unmarshal: %v != %v", ecUnmarshaled, ec) - } + if ecUnmarshaled != ec { + t.Fatalf("unexpected error code during error code marshal/unmarshal: %v != %v", ecUnmarshaled, ec) + } - expectedErrorString := strings.ToLower(strings.Replace(ec.Descriptor().Value, "_", " ", -1)) - if ec.Error() != expectedErrorString { - t.Fatalf("unexpected return from %v.Error(): %q != %q", ec, ec.Error(), expectedErrorString) - } + expectedErrorString := strings.ToLower(strings.Replace(ec.Descriptor().Value, "_", " ", -1)) + if ec.Error() != expectedErrorString { + t.Fatalf("unexpected return from %v.Error(): %q != %q", ec, ec.Error(), expectedErrorString) + } + }) } } From 5e67a40e0865c881f1d4878b429a5d88724630bf Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 30 Apr 2023 15:05:51 +0200 Subject: [PATCH 10/16] registry/auth/token: use consistent names for test-tables Signed-off-by: Sebastiaan van Stijn --- registry/auth/token/types_test.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/registry/auth/token/types_test.go b/registry/auth/token/types_test.go index 5e547761..f8678600 100644 --- a/registry/auth/token/types_test.go +++ b/registry/auth/token/types_test.go @@ -7,7 +7,7 @@ import ( func TestAudienceList_Unmarshal(t *testing.T) { t.Run("OK", func(t *testing.T) { - testCases := []struct { + tests := []struct { value string expected AudienceList }{ @@ -25,18 +25,17 @@ func TestAudienceList_Unmarshal(t *testing.T) { }, } - for _, testCase := range testCases { - testCase := testCase - + for _, tc := range tests { + tc := tc t.Run("", func(t *testing.T) { var actual AudienceList - err := json.Unmarshal([]byte(testCase.value), &actual) + err := json.Unmarshal([]byte(tc.value), &actual) if err != nil { t.Fatal(err) } - assertStringListEqual(t, testCase.expected, actual) + assertStringListEqual(t, tc.expected, actual) }) } }) From 9266220c2a3c8dd4202c542472bde57594efef41 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 30 Apr 2023 15:09:41 +0200 Subject: [PATCH 11/16] registry/auth/token: TestAudienceList_Unmarshal: t.Parallel() Signed-off-by: Sebastiaan van Stijn --- registry/auth/token/types_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/registry/auth/token/types_test.go b/registry/auth/token/types_test.go index f8678600..5e45c048 100644 --- a/registry/auth/token/types_test.go +++ b/registry/auth/token/types_test.go @@ -6,6 +6,7 @@ import ( ) func TestAudienceList_Unmarshal(t *testing.T) { + t.Parallel() t.Run("OK", func(t *testing.T) { tests := []struct { value string @@ -28,6 +29,7 @@ func TestAudienceList_Unmarshal(t *testing.T) { for _, tc := range tests { tc := tc t.Run("", func(t *testing.T) { + t.Parallel() var actual AudienceList err := json.Unmarshal([]byte(tc.value), &actual) @@ -41,6 +43,7 @@ func TestAudienceList_Unmarshal(t *testing.T) { }) t.Run("Error", func(t *testing.T) { + t.Parallel() var actual AudienceList err := json.Unmarshal([]byte("1234"), &actual) From 2444c3282de621dc7c2f5d58667565dc032862ff Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 30 Apr 2023 15:14:30 +0200 Subject: [PATCH 12/16] registry/api/v2: use consistent names for test-tables Signed-off-by: Sebastiaan van Stijn --- registry/api/v2/routes_test.go | 60 +++++++++++++++++----------------- registry/api/v2/urls_test.go | 54 +++++++++++++++--------------- 2 files changed, 57 insertions(+), 57 deletions(-) diff --git a/registry/api/v2/routes_test.go b/registry/api/v2/routes_test.go index b91fc48f..efea869c 100644 --- a/registry/api/v2/routes_test.go +++ b/registry/api/v2/routes_test.go @@ -29,7 +29,7 @@ type routeTestCase struct { // // This may go away as the application structure comes together. func TestRouter(t *testing.T) { - testCases := []routeTestCase{ + tests := []routeTestCase{ { RouteName: RouteNameBase, RequestURI: "/v2/", @@ -172,12 +172,12 @@ func TestRouter(t *testing.T) { }, } - checkTestRouter(t, testCases, "", true) - checkTestRouter(t, testCases, "/prefix/", true) + checkTestRouter(t, tests, "", true) + checkTestRouter(t, tests, "/prefix/", true) } func TestRouterWithPathTraversals(t *testing.T) { - testCases := []routeTestCase{ + tests := []routeTestCase{ { RouteName: RouteNameBlobUploadChunk, RequestURI: "/v2/foo/../../blobs/uploads/D95306FA-FAD3-4E36-8D41-CF1C93EF8286", @@ -194,12 +194,12 @@ func TestRouterWithPathTraversals(t *testing.T) { }, }, } - checkTestRouter(t, testCases, "", false) + checkTestRouter(t, tests, "", false) } func TestRouterWithBadCharacters(t *testing.T) { if testing.Short() { - testCases := []routeTestCase{ + tests := []routeTestCase{ { RouteName: RouteNameBlobUploadChunk, RequestURI: "/v2/foo/blobs/uploads/不95306FA-FAD3-4E36-8D41-CF1C93EF8286", @@ -212,26 +212,26 @@ func TestRouterWithBadCharacters(t *testing.T) { StatusCode: http.StatusNotFound, }, } - checkTestRouter(t, testCases, "", true) + checkTestRouter(t, tests, "", true) } else { // in the long version we're going to fuzz the router // with random UTF8 characters not in the 128 bit ASCII range. // These are not valid characters for the router and we expect // 404s on every test. rand.Seed(time.Now().UTC().UnixNano()) - testCases := make([]routeTestCase, 1000) - for idx := range testCases { - testCases[idx] = routeTestCase{ + tests := make([]routeTestCase, 1000) + for idx := range tests { + tests[idx] = routeTestCase{ RouteName: RouteNameTags, RequestURI: fmt.Sprintf("/v2/%v/%v/tags/list", randomString(10), randomString(10)), StatusCode: http.StatusNotFound, } } - checkTestRouter(t, testCases, "", true) + checkTestRouter(t, tests, "", true) } } -func checkTestRouter(t *testing.T, testCases []routeTestCase, prefix string, deeplyEqual bool) { +func checkTestRouter(t *testing.T, tests []routeTestCase, prefix string, deeplyEqual bool) { router := RouterWithPrefix(prefix) testHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -252,37 +252,37 @@ func checkTestRouter(t *testing.T, testCases []routeTestCase, prefix string, dee // Startup test server server := httptest.NewServer(router) - for _, testcase := range testCases { - testcase.RequestURI = strings.TrimSuffix(prefix, "/") + testcase.RequestURI + for _, tc := range tests { + tc.RequestURI = strings.TrimSuffix(prefix, "/") + tc.RequestURI // Register the endpoint - route := router.GetRoute(testcase.RouteName) + route := router.GetRoute(tc.RouteName) if route == nil { - t.Fatalf("route for name %q not found", testcase.RouteName) + t.Fatalf("route for name %q not found", tc.RouteName) } route.Handler(testHandler) - u := server.URL + testcase.RequestURI + u := server.URL + tc.RequestURI resp, err := http.Get(u) if err != nil { t.Fatalf("error issuing get request: %v", err) } - if testcase.StatusCode == 0 { + if tc.StatusCode == 0 { // Override default, zero-value - testcase.StatusCode = http.StatusOK + tc.StatusCode = http.StatusOK } - if testcase.ExpectedURI == "" { + if tc.ExpectedURI == "" { // Override default, zero-value - testcase.ExpectedURI = testcase.RequestURI + tc.ExpectedURI = tc.RequestURI } - if resp.StatusCode != testcase.StatusCode { + if resp.StatusCode != tc.StatusCode { t.Fatalf("unexpected status for %s: %v %v", u, resp.Status, resp.StatusCode) } - if testcase.StatusCode != http.StatusOK { + if tc.StatusCode != http.StatusOK { resp.Body.Close() // We don't care about json response. continue @@ -297,20 +297,20 @@ func checkTestRouter(t *testing.T, testCases []routeTestCase, prefix string, dee // Needs to be set out of band actualRouteInfo.StatusCode = resp.StatusCode - if actualRouteInfo.RequestURI != testcase.ExpectedURI { - t.Fatalf("URI %v incorrectly parsed, expected %v", actualRouteInfo.RequestURI, testcase.ExpectedURI) + if actualRouteInfo.RequestURI != tc.ExpectedURI { + t.Fatalf("URI %v incorrectly parsed, expected %v", actualRouteInfo.RequestURI, tc.ExpectedURI) } - if actualRouteInfo.RouteName != testcase.RouteName { - t.Fatalf("incorrect route %q matched, expected %q", actualRouteInfo.RouteName, testcase.RouteName) + if actualRouteInfo.RouteName != tc.RouteName { + t.Fatalf("incorrect route %q matched, expected %q", actualRouteInfo.RouteName, tc.RouteName) } // when testing deep equality, the actualRouteInfo has an empty ExpectedURI, we don't want // that to make the comparison fail. We're otherwise done with the testcase so empty the // testcase.ExpectedURI - testcase.ExpectedURI = "" - if deeplyEqual && !reflect.DeepEqual(actualRouteInfo, testcase) { - t.Fatalf("actual does not equal expected: %#v != %#v", actualRouteInfo, testcase) + tc.ExpectedURI = "" + if deeplyEqual && !reflect.DeepEqual(actualRouteInfo, tc) { + t.Fatalf("actual does not equal expected: %#v != %#v", actualRouteInfo, tc) } resp.Body.Close() diff --git a/registry/api/v2/urls_test.go b/registry/api/v2/urls_test.go index 66fb4fd0..0ccf3518 100644 --- a/registry/api/v2/urls_test.go +++ b/registry/api/v2/urls_test.go @@ -138,23 +138,23 @@ func TestURLBuilder(t *testing.T) { t.Fatalf("unexpected error creating urlbuilder: %v", err) } - for _, testCase := range makeURLBuilderTestCases(urlBuilder) { - url, err := testCase.build() - expectedErr := testCase.expectedErr + for _, tc := range makeURLBuilderTestCases(urlBuilder) { + buildURL, err := tc.build() + expectedErr := tc.expectedErr if !reflect.DeepEqual(expectedErr, err) { - t.Fatalf("%s: Expecting %v but got error %v", testCase.description, expectedErr, err) + t.Fatalf("%s: Expecting %v but got error %v", tc.description, expectedErr, err) } if expectedErr != nil { continue } - expectedURL := testCase.expectedPath + expectedURL := tc.expectedPath if !relative { expectedURL = root + expectedURL } - if url != expectedURL { - t.Fatalf("%s: %q != %q", testCase.description, url, expectedURL) + if buildURL != expectedURL { + t.Fatalf("%s: %q != %q", tc.description, buildURL, expectedURL) } } } @@ -178,22 +178,22 @@ func TestURLBuilderWithPrefix(t *testing.T) { t.Fatalf("unexpected error creating urlbuilder: %v", err) } - for _, testCase := range makeURLBuilderTestCases(urlBuilder) { - url, err := testCase.build() - expectedErr := testCase.expectedErr + for _, tc := range makeURLBuilderTestCases(urlBuilder) { + buildURL, err := tc.build() + expectedErr := tc.expectedErr if !reflect.DeepEqual(expectedErr, err) { - t.Fatalf("%s: Expecting %v but got error %v", testCase.description, expectedErr, err) + t.Fatalf("%s: Expecting %v but got error %v", tc.description, expectedErr, err) } if expectedErr != nil { continue } - expectedURL := testCase.expectedPath + expectedURL := tc.expectedPath if !relative { expectedURL = root[0:len(root)-1] + expectedURL } - if url != expectedURL { - t.Fatalf("%s: %q != %q", testCase.description, url, expectedURL) + if buildURL != expectedURL { + t.Fatalf("%s: %q != %q", tc.description, buildURL, expectedURL) } } } @@ -424,23 +424,23 @@ func TestBuilderFromRequest(t *testing.T) { builder = NewURLBuilderFromRequest(tr.request, relative) } - for _, testCase := range makeURLBuilderTestCases(builder) { - buildURL, err := testCase.build() - expectedErr := testCase.expectedErr + for _, tc := range makeURLBuilderTestCases(builder) { + buildURL, err := tc.build() + expectedErr := tc.expectedErr if !reflect.DeepEqual(expectedErr, err) { - t.Fatalf("%s: Expecting %v but got error %v", testCase.description, expectedErr, err) + t.Fatalf("%s: Expecting %v but got error %v", tc.description, expectedErr, err) } if expectedErr != nil { continue } - expectedURL := testCase.expectedPath + expectedURL := tc.expectedPath if !relative { expectedURL = tr.base + expectedURL } if buildURL != expectedURL { - t.Errorf("[relative=%t, request=%q, case=%q]: %q != %q", relative, tr.name, testCase.description, buildURL, expectedURL) + t.Errorf("[relative=%t, request=%q, case=%q]: %q != %q", relative, tr.name, tc.description, buildURL, expectedURL) } } } @@ -497,11 +497,11 @@ func TestBuilderFromRequestWithPrefix(t *testing.T) { builder = NewURLBuilderFromRequest(tr.request, false) } - for _, testCase := range makeURLBuilderTestCases(builder) { - buildURL, err := testCase.build() - expectedErr := testCase.expectedErr + for _, tc := range makeURLBuilderTestCases(builder) { + buildURL, err := tc.build() + expectedErr := tc.expectedErr if !reflect.DeepEqual(expectedErr, err) { - t.Fatalf("%s: Expecting %v but got error %v", testCase.description, expectedErr, err) + t.Fatalf("%s: Expecting %v but got error %v", tc.description, expectedErr, err) } if expectedErr != nil { continue @@ -510,7 +510,7 @@ func TestBuilderFromRequestWithPrefix(t *testing.T) { var expectedURL string proto, ok := tr.request.Header["X-Forwarded-Proto"] if !ok { - expectedURL = testCase.expectedPath + expectedURL = tc.expectedPath if !relative { expectedURL = tr.base[0:len(tr.base)-1] + expectedURL } @@ -520,7 +520,7 @@ func TestBuilderFromRequestWithPrefix(t *testing.T) { t.Fatal(err) } urlBase.Scheme = proto[0] - expectedURL = testCase.expectedPath + expectedURL = tc.expectedPath if !relative { expectedURL = urlBase.String()[0:len(urlBase.String())-1] + expectedURL } @@ -528,7 +528,7 @@ func TestBuilderFromRequestWithPrefix(t *testing.T) { } if buildURL != expectedURL { - t.Fatalf("%s: %q != %q", testCase.description, buildURL, expectedURL) + t.Fatalf("%s: %q != %q", tc.description, buildURL, expectedURL) } } } From f4acd98865ec9efd23581ddd3f365ca8cd7d8411 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 30 Apr 2023 15:38:30 +0200 Subject: [PATCH 13/16] registry/api/v2: checkTestRouter(): use sub-tests, t.Parallel() Signed-off-by: Sebastiaan van Stijn --- registry/api/v2/routes_test.go | 116 ++++++++++++++++++--------------- 1 file changed, 62 insertions(+), 54 deletions(-) diff --git a/registry/api/v2/routes_test.go b/registry/api/v2/routes_test.go index efea869c..bdd0ef48 100644 --- a/registry/api/v2/routes_test.go +++ b/registry/api/v2/routes_test.go @@ -29,6 +29,7 @@ type routeTestCase struct { // // This may go away as the application structure comes together. func TestRouter(t *testing.T) { + t.Parallel() tests := []routeTestCase{ { RouteName: RouteNameBase, @@ -177,6 +178,7 @@ func TestRouter(t *testing.T) { } func TestRouterWithPathTraversals(t *testing.T) { + t.Parallel() tests := []routeTestCase{ { RouteName: RouteNameBlobUploadChunk, @@ -198,6 +200,7 @@ func TestRouterWithPathTraversals(t *testing.T) { } func TestRouterWithBadCharacters(t *testing.T) { + t.Parallel() if testing.Short() { tests := []routeTestCase{ { @@ -253,67 +256,72 @@ func checkTestRouter(t *testing.T, tests []routeTestCase, prefix string, deeplyE server := httptest.NewServer(router) for _, tc := range tests { - tc.RequestURI = strings.TrimSuffix(prefix, "/") + tc.RequestURI - // Register the endpoint - route := router.GetRoute(tc.RouteName) - if route == nil { - t.Fatalf("route for name %q not found", tc.RouteName) - } + tc := tc + requestURI := strings.TrimSuffix(prefix, "/") + tc.RequestURI + t.Run("("+tc.RouteName+")"+requestURI, func(t *testing.T) { + t.Parallel() + tc.RequestURI = requestURI + // Register the endpoint + route := router.GetRoute(tc.RouteName) + if route == nil { + t.Fatalf("route for name %q not found", tc.RouteName) + } - route.Handler(testHandler) + route.Handler(testHandler) - u := server.URL + tc.RequestURI + u := server.URL + tc.RequestURI - resp, err := http.Get(u) - if err != nil { - t.Fatalf("error issuing get request: %v", err) - } + resp, err := http.Get(u) + if err != nil { + t.Fatalf("error issuing get request: %v", err) + } - if tc.StatusCode == 0 { - // Override default, zero-value - tc.StatusCode = http.StatusOK - } - if tc.ExpectedURI == "" { - // Override default, zero-value - tc.ExpectedURI = tc.RequestURI - } + if tc.StatusCode == 0 { + // Override default, zero-value + tc.StatusCode = http.StatusOK + } + if tc.ExpectedURI == "" { + // Override default, zero-value + tc.ExpectedURI = tc.RequestURI + } - if resp.StatusCode != tc.StatusCode { - t.Fatalf("unexpected status for %s: %v %v", u, resp.Status, resp.StatusCode) - } + if resp.StatusCode != tc.StatusCode { + t.Fatalf("unexpected status for %s: %v %v", u, resp.Status, resp.StatusCode) + } + + if tc.StatusCode != http.StatusOK { + resp.Body.Close() + // We don't care about json response. + return + } + + dec := json.NewDecoder(resp.Body) + + var actualRouteInfo routeTestCase + if err := dec.Decode(&actualRouteInfo); err != nil { + t.Fatalf("error reading json response: %v", err) + } + // Needs to be set out of band + actualRouteInfo.StatusCode = resp.StatusCode + + if actualRouteInfo.RequestURI != tc.ExpectedURI { + t.Fatalf("URI %v incorrectly parsed, expected %v", actualRouteInfo.RequestURI, tc.ExpectedURI) + } + + if actualRouteInfo.RouteName != tc.RouteName { + t.Fatalf("incorrect route %q matched, expected %q", actualRouteInfo.RouteName, tc.RouteName) + } + + // when testing deep equality, the actualRouteInfo has an empty ExpectedURI, we don't want + // that to make the comparison fail. We're otherwise done with the testcase so empty the + // testcase.ExpectedURI + tc.ExpectedURI = "" + if deeplyEqual && !reflect.DeepEqual(actualRouteInfo, tc) { + t.Fatalf("actual does not equal expected: %#v != %#v", actualRouteInfo, tc) + } - if tc.StatusCode != http.StatusOK { resp.Body.Close() - // We don't care about json response. - continue - } - - dec := json.NewDecoder(resp.Body) - - var actualRouteInfo routeTestCase - if err := dec.Decode(&actualRouteInfo); err != nil { - t.Fatalf("error reading json response: %v", err) - } - // Needs to be set out of band - actualRouteInfo.StatusCode = resp.StatusCode - - if actualRouteInfo.RequestURI != tc.ExpectedURI { - t.Fatalf("URI %v incorrectly parsed, expected %v", actualRouteInfo.RequestURI, tc.ExpectedURI) - } - - if actualRouteInfo.RouteName != tc.RouteName { - t.Fatalf("incorrect route %q matched, expected %q", actualRouteInfo.RouteName, tc.RouteName) - } - - // when testing deep equality, the actualRouteInfo has an empty ExpectedURI, we don't want - // that to make the comparison fail. We're otherwise done with the testcase so empty the - // testcase.ExpectedURI - tc.ExpectedURI = "" - if deeplyEqual && !reflect.DeepEqual(actualRouteInfo, tc) { - t.Fatalf("actual does not equal expected: %#v != %#v", actualRouteInfo, tc) - } - - resp.Body.Close() + }) } } From 85028a801b54cb8b432130e13605aa962bea53db Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 30 Apr 2023 15:21:48 +0200 Subject: [PATCH 14/16] registry/handlers: use consistent names for test-tables Also marked assertBlobUploadStateEquals as t.Helper() Signed-off-by: Sebastiaan van Stijn --- registry/handlers/hmac_test.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/registry/handlers/hmac_test.go b/registry/handlers/hmac_test.go index 366c7279..ce9af25d 100644 --- a/registry/handlers/hmac_test.go +++ b/registry/handlers/hmac_test.go @@ -45,8 +45,8 @@ var secrets = []string{ func TestLayerUploadTokens(t *testing.T) { secret := hmacKey("supersecret") - for _, testcase := range blobUploadStates { - token, err := secret.packUploadState(testcase) + for _, tc := range blobUploadStates { + token, err := secret.packUploadState(tc) if err != nil { t.Fatal(err) } @@ -56,7 +56,7 @@ func TestLayerUploadTokens(t *testing.T) { t.Fatal(err) } - assertBlobUploadStateEquals(t, testcase, lus) + assertBlobUploadStateEquals(t, tc, lus) } } @@ -68,8 +68,8 @@ func TestHMACValidation(t *testing.T) { secret2 := hmacKey(secret) badSecret := hmacKey("DifferentSecret") - for _, testcase := range blobUploadStates { - token, err := secret1.packUploadState(testcase) + for _, tc := range blobUploadStates { + token, err := secret1.packUploadState(tc) if err != nil { t.Fatal(err) } @@ -79,7 +79,7 @@ func TestHMACValidation(t *testing.T) { t.Fatal(err) } - assertBlobUploadStateEquals(t, testcase, lus) + assertBlobUploadStateEquals(t, tc, lus) _, err = badSecret.unpackUploadState(token) if err == nil { @@ -105,6 +105,7 @@ func TestHMACValidation(t *testing.T) { } func assertBlobUploadStateEquals(t *testing.T, expected blobUploadState, received blobUploadState) { + t.Helper() if expected.Name != received.Name { t.Fatalf("Expected Name=%q, Received Name=%q", expected.Name, received.Name) } From 5301ae14bf344c60909825568808380e21c92adb Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 30 Apr 2023 15:27:42 +0200 Subject: [PATCH 15/16] cloudfront: rename vars that collided with type Signed-off-by: Sebastiaan van Stijn --- .../middleware/cloudfront/s3filter_test.go | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/registry/storage/driver/middleware/cloudfront/s3filter_test.go b/registry/storage/driver/middleware/cloudfront/s3filter_test.go index 3b0331b7..f38af490 100644 --- a/registry/storage/driver/middleware/cloudfront/s3filter_test.go +++ b/registry/storage/driver/middleware/cloudfront/s3filter_test.go @@ -8,12 +8,11 @@ import ( "net" "net/http" "net/http/httptest" + "reflect" // used as a replacement for testify "testing" "time" dcontext "github.com/distribution/distribution/v3/context" - - "reflect" // used as a replacement for testify ) // Rather than pull in all of testify @@ -262,7 +261,7 @@ func TestUpdateCalledRegularly(t *testing.T) { } func TestEligibleForS3(t *testing.T) { - awsIPs := &awsIPs{ + ips := &awsIPs{ ipv4: []net.IPNet{{ IP: net.ParseIP("192.168.1.1"), Mask: net.IPv4Mask(255, 255, 255, 0), @@ -291,13 +290,13 @@ func TestEligibleForS3(t *testing.T) { name := fmt.Sprintf("Client IP = %v", testCase.Context.Value("http.request.ip")) t.Run(name, func(t *testing.T) { - assertEqual(t, testCase.Expected, eligibleForS3(testCase.Context, awsIPs)) + assertEqual(t, testCase.Expected, eligibleForS3(testCase.Context, ips)) }) } } func TestEligibleForS3WithAWSIPNotInitialized(t *testing.T) { - awsIPs := &awsIPs{ + ips := &awsIPs{ ipv4: []net.IPNet{{ IP: net.ParseIP("192.168.1.1"), Mask: net.IPv4Mask(255, 255, 255, 0), @@ -326,7 +325,7 @@ func TestEligibleForS3WithAWSIPNotInitialized(t *testing.T) { name := fmt.Sprintf("Client IP = %v", testCase.Context.Value("http.request.ip")) t.Run(name, func(t *testing.T) { - assertEqual(t, testCase.Expected, eligibleForS3(testCase.Context, awsIPs)) + assertEqual(t, testCase.Expected, eligibleForS3(testCase.Context, ips)) }) } } @@ -364,8 +363,8 @@ func BenchmarkContainsRandom(b *testing.B) { numNetworksPerType := 1000 // keep in sync with the above // intentionally skip constructor when creating awsIPs, to avoid updater routine. // This benchmark is only concerned with contains() performance. - awsIPs := awsIPs{} - populateRandomNetworks(b, &awsIPs, numNetworksPerType, numNetworksPerType) + ips := awsIPs{} + populateRandomNetworks(b, &ips, numNetworksPerType, numNetworksPerType) ipv4 := make([][]byte, b.N) ipv6 := make([][]byte, b.N) @@ -377,13 +376,13 @@ func BenchmarkContainsRandom(b *testing.B) { } b.ResetTimer() for i := 0; i < b.N; i++ { - awsIPs.contains(ipv4[i]) - awsIPs.contains(ipv6[i]) + ips.contains(ipv4[i]) + ips.contains(ipv6[i]) } } func BenchmarkContainsProd(b *testing.B) { - awsIPs := newAWSIPs(defaultIPRangesURL, defaultUpdateFrequency, nil) + ips := newAWSIPs(defaultIPRangesURL, defaultUpdateFrequency, nil) ipv4 := make([][]byte, b.N) ipv6 := make([][]byte, b.N) for i := 0; i < b.N; i++ { @@ -394,7 +393,7 @@ func BenchmarkContainsProd(b *testing.B) { } b.ResetTimer() for i := 0; i < b.N; i++ { - awsIPs.contains(ipv4[i]) - awsIPs.contains(ipv6[i]) + ips.contains(ipv4[i]) + ips.contains(ipv6[i]) } } From f03d966ef7a9c2620af5a42b0c11e3bdafa2d421 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 30 Apr 2023 15:30:32 +0200 Subject: [PATCH 16/16] cloudfront: use consistent names for test-tables, t.Parallel() Signed-off-by: Sebastiaan van Stijn --- .../middleware/cloudfront/s3filter_test.go | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/registry/storage/driver/middleware/cloudfront/s3filter_test.go b/registry/storage/driver/middleware/cloudfront/s3filter_test.go index f38af490..81ef6aa8 100644 --- a/registry/storage/driver/middleware/cloudfront/s3filter_test.go +++ b/registry/storage/driver/middleware/cloudfront/s3filter_test.go @@ -261,6 +261,7 @@ func TestUpdateCalledRegularly(t *testing.T) { } func TestEligibleForS3(t *testing.T) { + t.Parallel() ips := &awsIPs{ ipv4: []net.IPNet{{ IP: net.ParseIP("192.168.1.1"), @@ -277,7 +278,7 @@ func TestEligibleForS3(t *testing.T) { return dcontext.WithRequest(empty, req) } - cases := []struct { + tests := []struct { Context context.Context Expected bool }{ @@ -286,16 +287,17 @@ func TestEligibleForS3(t *testing.T) { {Context: makeContext("192.168.0.2"), Expected: false}, } - for _, testCase := range cases { - name := fmt.Sprintf("Client IP = %v", - testCase.Context.Value("http.request.ip")) - t.Run(name, func(t *testing.T) { - assertEqual(t, testCase.Expected, eligibleForS3(testCase.Context, ips)) + for _, tc := range tests { + tc := tc + t.Run(fmt.Sprintf("Client IP = %v", tc.Context.Value("http.request.ip")), func(t *testing.T) { + t.Parallel() + assertEqual(t, tc.Expected, eligibleForS3(tc.Context, ips)) }) } } func TestEligibleForS3WithAWSIPNotInitialized(t *testing.T) { + t.Parallel() ips := &awsIPs{ ipv4: []net.IPNet{{ IP: net.ParseIP("192.168.1.1"), @@ -312,7 +314,7 @@ func TestEligibleForS3WithAWSIPNotInitialized(t *testing.T) { return dcontext.WithRequest(empty, req) } - cases := []struct { + tests := []struct { Context context.Context Expected bool }{ @@ -321,11 +323,11 @@ func TestEligibleForS3WithAWSIPNotInitialized(t *testing.T) { {Context: makeContext("192.168.0.2"), Expected: false}, } - for _, testCase := range cases { - name := fmt.Sprintf("Client IP = %v", - testCase.Context.Value("http.request.ip")) - t.Run(name, func(t *testing.T) { - assertEqual(t, testCase.Expected, eligibleForS3(testCase.Context, ips)) + for _, tc := range tests { + tc := tc + t.Run(fmt.Sprintf("Client IP = %v", tc.Context.Value("http.request.ip")), func(t *testing.T) { + t.Parallel() + assertEqual(t, tc.Expected, eligibleForS3(tc.Context, ips)) }) } }