assorted test updates #3885

thaJeztah · 2023-04-30T13:49:22Z

follow-up to / depends on reference: rewrite test to use sub-tests, add benchmark #3883

codecov-commenter · 2023-04-30T13:51:51Z

Codecov Report

Patch coverage has no change and project coverage change: +0.13 🎉

Comparison is base (29b5e79) 56.63% compared to head (0c84c60) 56.76%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3885      +/-   ##
==========================================
+ Coverage   56.63%   56.76%   +0.13%     
==========================================
  Files         106      106              
  Lines       10674    10674              
==========================================
+ Hits         6045     6059      +14     
+ Misses       3955     3940      -15     
- Partials      674      675       +1

see 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

thaJeztah · 2023-05-02T20:58:53Z

@milosgajdos @corhere ptal

corhere · 2023-05-02T20:59:22Z

@squizzi PTAL

milosgajdos

If I read this PR correctly, it seems to be that most of these are renames, race condition fixes [in parallelised subtests] and a small refactoring. On the first pass it LGTM but I have one stylistic comment

milosgajdos · 2023-05-04T10:19:55Z

context/trace_test.go

+func checkContextForValues(ctx context.Context, t *testing.T, tests []valueTestCase) {
+	for _, tc := range tests {
+		tc := tc
+		t.Run(tc.key, func(t *testing.T) {


I am not sure I like the idea of calling a subtest from inside a private func. I like to be explicit about these thiings and call them inside the Test funcs. Also, we could parallelise this too 🤔

Yeah; looks like this function was written to make the test more DRY, but effectively this "helper" was running subtests. Changing it to be TestXX wouldn't work because it doesn't have the right signature, so I just pushed a commit that inlines what it does (at cost of duplicating a couple of lines of code)

milosgajdos · 2023-05-04T10:25:59Z

I am also assuming we've split these updates into dedicated commits per change so we can do some git archaeology in the future if it comes to it?

thaJeztah · 2023-05-04T11:13:52Z

I am also assuming we've split these updates into dedicated commits per change so we can do some git archaeology in the future if it comes to it?

Yes, both to keep history, to keep the changes in a bit more sizeable chunks, and to split them up a bit into per-package changes.

thaJeztah · 2023-05-04T11:14:06Z

Either way; updated; PTAL 🤗

squizzi

Just making sure some parallel changes weren't missed and a typo on a test name we might as well update; otherwise lgtm

squizzi · 2023-05-04T08:00:05Z

registry/api/errcode/errors_test.go

+			}
+
+			if ec.Message() != desc.Message {
+				t.Fatalf("ec.Message doesn't mtach desc.Message: %q != %q", ec.Message(), desc.Message)


Suggested change

t.Fatalf("ec.Message doesn't mtach desc.Message: %q != %q", ec.Message(), desc.Message)

t.Fatalf("ec.Message doesn't match desc.Message: %q != %q", ec.Message(), desc.Message)

squizzi · 2023-05-04T08:05:29Z

registry/api/v2/routes_test.go

-			t.Fatalf("route for name %q not found", testcase.RouteName)
-		}
+	for _, tc := range tests {
+		tc := tc


Do you intend to have checkTestRouter be parallel? Looks like t.Parallel() is missing.

squizzi · 2023-05-04T08:12:54Z

registry/storage/driver/middleware/cloudfront/s3filter_test.go

-		t.Run(name, func(t *testing.T) {
-			assertEqual(t, testCase.Expected, eligibleForS3(testCase.Context, awsIPs))
+	for _, tc := range tests {
+		tc := tc


Once again, seems like you're intending for this to be parallel? Just making sure you're not forgetting these if it's intended, if not disregard.

I think I did these mostly "in case" but I might as well go all the way and enable t.Parallel()

squizzi · 2023-05-04T08:13:12Z

registry/storage/driver/middleware/cloudfront/s3filter_test.go

-		t.Run(name, func(t *testing.T) {
-			assertEqual(t, testCase.Expected, eligibleForS3(testCase.Context, awsIPs))
+	for _, tc := range tests {
+		tc := tc


And again, seems like you're intending for this to be parallel? Just making sure you're not forgetting these if it's intended, if not disregard.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Also renamed a var that collided with a type, and marked a helper as t.Helper() Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

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 <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Also marked assertBlobUploadStateEquals as t.Helper() Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2023-05-09T12:43:38Z

@squizzi @milosgajdos I think I addressed all comments; ptal

thaJeztah mentioned this pull request Apr 30, 2023

reference: rewrite test to use sub-tests, add benchmark #3883

Merged

thaJeztah force-pushed the rename_table_tests branch from 0c84c60 to cda9591 Compare May 2, 2023 20:02

thaJeztah marked this pull request as ready for review May 2, 2023 20:03

thaJeztah added refactor status/2-code-review labels May 2, 2023

milosgajdos reviewed May 4, 2023

View reviewed changes

thaJeztah force-pushed the rename_table_tests branch from cda9591 to 87f631c Compare May 4, 2023 11:11

milosgajdos approved these changes May 4, 2023

View reviewed changes

milosgajdos requested a review from squizzi May 4, 2023 11:54

squizzi approved these changes May 4, 2023

View reviewed changes

thaJeztah added 14 commits May 9, 2023 13:54

reference: use consistent names for test-tables

fcbddfc

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

reference: TestParseAnyReference(): use sub-tests

fa1d14c

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

reference: TestNormalizedSplitHostname(): use sub-tests

2819bca

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

reference: TestMatch(): use sub-tests

f238f7d

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

context: use consistent names for test-tables

2829f7b

Also renamed a var that collided with a type, and marked a helper as t.Helper() Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

notifications: use consistent names for test tables

e9ac172

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

notifications: don't use un-keyed structs

57f9f31

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

registry/api/errorcode: TestErrorCodes: use sub-tests

f884a07

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

registry/auth/token: use consistent names for test-tables

5e67a40

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

registry/auth/token: TestAudienceList_Unmarshal: t.Parallel()

9266220

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

registry/api/v2: use consistent names for test-tables

2444c32

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

registry/api/v2: checkTestRouter(): use sub-tests, t.Parallel()

f4acd98

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

registry/handlers: use consistent names for test-tables

85028a8

Also marked assertBlobUploadStateEquals as t.Helper() Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah added 2 commits May 9, 2023 14:00

cloudfront: rename vars that collided with type

5301ae1

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

cloudfront: use consistent names for test-tables, t.Parallel()

f03d966

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the rename_table_tests branch from 8a1f366 to f03d966 Compare May 9, 2023 12:40

thaJeztah mentioned this pull request May 9, 2023

update golangci-lint to v1.52, fix linting issues #3906

Merged

milosgajdos merged commit c9b844e into distribution:main May 9, 2023

	t.Fatalf("ec.Message doesn't mtach desc.Message: %q != %q", ec.Message(), desc.Message)
	t.Fatalf("ec.Message doesn't match desc.Message: %q != %q", ec.Message(), desc.Message)

assorted test updates #3885

assorted test updates #3885

Uh oh!

Conversation

thaJeztah commented Apr 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Apr 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thaJeztah commented May 2, 2023

Uh oh!

corhere commented May 2, 2023

Uh oh!

milosgajdos left a comment

Choose a reason for hiding this comment

Uh oh!

milosgajdos May 4, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah May 4, 2023

Choose a reason for hiding this comment

Uh oh!

milosgajdos commented May 4, 2023

Uh oh!

thaJeztah commented May 4, 2023

Uh oh!

thaJeztah commented May 4, 2023

Uh oh!

squizzi left a comment

Choose a reason for hiding this comment

Uh oh!

squizzi May 4, 2023

Choose a reason for hiding this comment

Uh oh!

squizzi May 4, 2023

Choose a reason for hiding this comment

Uh oh!

squizzi May 4, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah May 9, 2023

Choose a reason for hiding this comment

Uh oh!

squizzi May 4, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented May 9, 2023

Uh oh!

Uh oh!

thaJeztah commented Apr 30, 2023 •

edited

Loading

codecov-commenter commented Apr 30, 2023 •

edited

Loading