From 810dcd4ed5e7803eb1ad0d314fbefb38dc2a31b4 Mon Sep 17 00:00:00 2001
From: Sebastiaan van Stijn <github@gone.nl>
Date: Sat, 27 May 2023 13:58:02 +0200
Subject: [PATCH 01/10] use "tc" for test-cases

Mostly for my own sanity; just about every repository we have
started to converge to using "tc" as variable name for this, so
updating this repository as well to help reduce cognitive load.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
---
 osxkeychain/osxkeychain_darwin_test.go | 64 +++++++++++++-------------
 registryurl/parse_test.go              | 20 ++++----
 wincred/wincred_windows_test.go        | 64 +++++++++++++-------------
 3 files changed, 74 insertions(+), 74 deletions(-)

diff --git a/osxkeychain/osxkeychain_darwin_test.go b/osxkeychain/osxkeychain_darwin_test.go
index b5098ef..48792d6 100644
--- a/osxkeychain/osxkeychain_darwin_test.go
+++ b/osxkeychain/osxkeychain_darwin_test.go
@@ -75,26 +75,26 @@ func TestOSXKeychainHelperRetrieveAliases(t *testing.T) {
 
 	helper := Osxkeychain{}
 	defer func() {
-		for _, te := range tests {
-			helper.Delete(te.storeURL)
+		for _, tc := range tests {
+			helper.Delete(tc.storeURL)
 		}
 	}()
 
 	// Clean store before testing.
-	for _, te := range tests {
-		helper.Delete(te.storeURL)
+	for _, tc := range tests {
+		helper.Delete(tc.storeURL)
 	}
 
-	for _, te := range tests {
-		c := &credentials.Credentials{ServerURL: te.storeURL, Username: "hello", Secret: "world"}
+	for _, tc := range tests {
+		c := &credentials.Credentials{ServerURL: tc.storeURL, Username: "hello", Secret: "world"}
 		if err := helper.Add(c); err != nil {
-			t.Errorf("Error: failed to store secret for URL %q: %s", te.storeURL, err)
+			t.Errorf("Error: failed to store secret for URL %q: %s", tc.storeURL, err)
 			continue
 		}
-		if _, _, err := helper.Get(te.readURL); err != nil {
-			t.Errorf("Error: failed to read secret for URL %q using %q", te.storeURL, te.readURL)
+		if _, _, err := helper.Get(tc.readURL); err != nil {
+			t.Errorf("Error: failed to read secret for URL %q using %q", tc.storeURL, tc.readURL)
 		}
-		helper.Delete(te.storeURL)
+		helper.Delete(tc.storeURL)
 	}
 }
 
@@ -118,7 +118,7 @@ func TestOSXKeychainHelperRetrieveStrict(t *testing.T) {
 		{"https://foobar.docker.io:1234", "https://foobar.docker.io:5678"},
 
 		// non-matching ports TODO is this desired behavior? The other way round does work
-		//{"https://foobar.docker.io", "https://foobar.docker.io:5678"},
+		// {"https://foobar.docker.io", "https://foobar.docker.io:5678"},
 
 		// non-matching paths
 		{"https://foobar.docker.io:1234/one/two", "https://foobar.docker.io:1234/five/six"},
@@ -126,26 +126,26 @@ func TestOSXKeychainHelperRetrieveStrict(t *testing.T) {
 
 	helper := Osxkeychain{}
 	defer func() {
-		for _, te := range tests {
-			helper.Delete(te.storeURL)
+		for _, tc := range tests {
+			helper.Delete(tc.storeURL)
 		}
 	}()
 
 	// Clean store before testing.
-	for _, te := range tests {
-		helper.Delete(te.storeURL)
+	for _, tc := range tests {
+		helper.Delete(tc.storeURL)
 	}
 
-	for _, te := range tests {
-		c := &credentials.Credentials{ServerURL: te.storeURL, Username: "hello", Secret: "world"}
+	for _, tc := range tests {
+		c := &credentials.Credentials{ServerURL: tc.storeURL, Username: "hello", Secret: "world"}
 		if err := helper.Add(c); err != nil {
-			t.Errorf("Error: failed to store secret for URL %q: %s", te.storeURL, err)
+			t.Errorf("Error: failed to store secret for URL %q: %s", tc.storeURL, err)
 			continue
 		}
-		if _, _, err := helper.Get(te.readURL); err == nil {
-			t.Errorf("Error: managed to read secret for URL %q using %q, but should not be able to", te.storeURL, te.readURL)
+		if _, _, err := helper.Get(tc.readURL); err == nil {
+			t.Errorf("Error: managed to read secret for URL %q using %q, but should not be able to", tc.storeURL, tc.readURL)
 		}
-		helper.Delete(te.storeURL)
+		helper.Delete(tc.storeURL)
 	}
 }
 
@@ -167,39 +167,39 @@ func TestOSXKeychainHelperStoreRetrieve(t *testing.T) {
 
 	helper := Osxkeychain{}
 	defer func() {
-		for _, te := range tests {
-			helper.Delete(te.url)
+		for _, tc := range tests {
+			helper.Delete(tc.url)
 		}
 	}()
 
 	// Clean store before testing.
-	for _, te := range tests {
-		helper.Delete(te.url)
+	for _, tc := range tests {
+		helper.Delete(tc.url)
 	}
 
 	// Note that we don't delete between individual tests here, to verify that
 	// subsequent stores/overwrites don't affect storing / retrieving secrets.
-	for i, te := range tests {
+	for i, tc := range tests {
 		c := &credentials.Credentials{
-			ServerURL: te.url,
+			ServerURL: tc.url,
 			Username:  fmt.Sprintf("user-%d", i),
 			Secret:    fmt.Sprintf("secret-%d", i),
 		}
 
 		if err := helper.Add(c); err != nil {
-			t.Errorf("Error: failed to store secret for URL: %s: %s", te.url, err)
+			t.Errorf("Error: failed to store secret for URL: %s: %s", tc.url, err)
 			continue
 		}
-		user, secret, err := helper.Get(te.url)
+		user, secret, err := helper.Get(tc.url)
 		if err != nil {
-			t.Errorf("Error: failed to read secret for URL %q: %s", te.url, err)
+			t.Errorf("Error: failed to read secret for URL %q: %s", tc.url, err)
 			continue
 		}
 		if user != c.Username {
-			t.Errorf("Error: expected username %s, got username %s for URL: %s", c.Username, user, te.url)
+			t.Errorf("Error: expected username %s, got username %s for URL: %s", c.Username, user, tc.url)
 		}
 		if secret != c.Secret {
-			t.Errorf("Error: expected secret %s, got secret %s for URL: %s", c.Secret, secret, te.url)
+			t.Errorf("Error: expected secret %s, got secret %s for URL: %s", c.Secret, secret, tc.url)
 		}
 	}
 }
diff --git a/registryurl/parse_test.go b/registryurl/parse_test.go
index 89feaa7..03a0e03 100644
--- a/registryurl/parse_test.go
+++ b/registryurl/parse_test.go
@@ -24,23 +24,23 @@ func TestHelperParseURL(t *testing.T) {
 		{url: "ftp://foobar.docker.io:2376", err: errors.New("unsupported scheme: ftp")},
 	}
 
-	for _, te := range tests {
-		u, err := Parse(te.url)
+	for _, tc := range tests {
+		u, err := Parse(tc.url)
 
-		if te.err == nil && err != nil {
-			t.Errorf("Error: failed to parse URL %q: %s", te.url, err)
+		if tc.err == nil && err != nil {
+			t.Errorf("Error: failed to parse URL %q: %s", tc.url, err)
 			continue
 		}
-		if te.err != nil && err == nil {
-			t.Errorf("Error: expected error %q, got none when parsing URL %q", te.err, te.url)
+		if tc.err != nil && err == nil {
+			t.Errorf("Error: expected error %q, got none when parsing URL %q", tc.err, tc.url)
 			continue
 		}
-		if te.err != nil && err.Error() != te.err.Error() {
-			t.Errorf("Error: expected error %q, got %q when parsing URL %q", te.err, err, te.url)
+		if tc.err != nil && err.Error() != tc.err.Error() {
+			t.Errorf("Error: expected error %q, got %q when parsing URL %q", tc.err, err, tc.url)
 			continue
 		}
-		if u != nil && u.String() != te.expectedURL {
-			t.Errorf("Error: expected URL: %q, but got %q for URL: %q", te.expectedURL, u.String(), te.url)
+		if u != nil && u.String() != tc.expectedURL {
+			t.Errorf("Error: expected URL: %q, but got %q for URL: %q", tc.expectedURL, u.String(), tc.url)
 		}
 	}
 }
diff --git a/wincred/wincred_windows_test.go b/wincred/wincred_windows_test.go
index 68d293a..074b29c 100644
--- a/wincred/wincred_windows_test.go
+++ b/wincred/wincred_windows_test.go
@@ -106,26 +106,26 @@ func TestWinCredHelperRetrieveAliases(t *testing.T) {
 
 	helper := Wincred{}
 	defer func() {
-		for _, te := range tests {
-			helper.Delete(te.storeURL)
+		for _, tc := range tests {
+			helper.Delete(tc.storeURL)
 		}
 	}()
 
 	// Clean store before testing.
-	for _, te := range tests {
-		helper.Delete(te.storeURL)
+	for _, tc := range tests {
+		helper.Delete(tc.storeURL)
 	}
 
-	for _, te := range tests {
-		c := &credentials.Credentials{ServerURL: te.storeURL, Username: "hello", Secret: "world"}
+	for _, tc := range tests {
+		c := &credentials.Credentials{ServerURL: tc.storeURL, Username: "hello", Secret: "world"}
 		if err := helper.Add(c); err != nil {
-			t.Errorf("Error: failed to store secret for URL %q: %s", te.storeURL, err)
+			t.Errorf("Error: failed to store secret for URL %q: %s", tc.storeURL, err)
 			continue
 		}
-		if _, _, err := helper.Get(te.readURL); err != nil {
-			t.Errorf("Error: failed to read secret for URL %q using %q", te.storeURL, te.readURL)
+		if _, _, err := helper.Get(tc.readURL); err != nil {
+			t.Errorf("Error: failed to read secret for URL %q using %q", tc.storeURL, tc.readURL)
 		}
-		helper.Delete(te.storeURL)
+		helper.Delete(tc.storeURL)
 	}
 }
 
@@ -149,7 +149,7 @@ func TestWinCredHelperRetrieveStrict(t *testing.T) {
 		{"https://foobar.docker.io:1234", "https://foobar.docker.io:5678"},
 
 		// non-matching ports TODO is this desired behavior? The other way round does work
-		//{"https://foobar.docker.io", "https://foobar.docker.io:5678"},
+		// {"https://foobar.docker.io", "https://foobar.docker.io:5678"},
 
 		// non-matching paths
 		{"https://foobar.docker.io:1234/one/two", "https://foobar.docker.io:1234/five/six"},
@@ -157,26 +157,26 @@ func TestWinCredHelperRetrieveStrict(t *testing.T) {
 
 	helper := Wincred{}
 	defer func() {
-		for _, te := range tests {
-			helper.Delete(te.storeURL)
+		for _, tc := range tests {
+			helper.Delete(tc.storeURL)
 		}
 	}()
 
 	// Clean store before testing.
-	for _, te := range tests {
-		helper.Delete(te.storeURL)
+	for _, tc := range tests {
+		helper.Delete(tc.storeURL)
 	}
 
-	for _, te := range tests {
-		c := &credentials.Credentials{ServerURL: te.storeURL, Username: "hello", Secret: "world"}
+	for _, tc := range tests {
+		c := &credentials.Credentials{ServerURL: tc.storeURL, Username: "hello", Secret: "world"}
 		if err := helper.Add(c); err != nil {
-			t.Errorf("Error: failed to store secret for URL %q: %s", te.storeURL, err)
+			t.Errorf("Error: failed to store secret for URL %q: %s", tc.storeURL, err)
 			continue
 		}
-		if _, _, err := helper.Get(te.readURL); err == nil {
-			t.Errorf("Error: managed to read secret for URL %q using %q, but should not be able to", te.storeURL, te.readURL)
+		if _, _, err := helper.Get(tc.readURL); err == nil {
+			t.Errorf("Error: managed to read secret for URL %q using %q, but should not be able to", tc.storeURL, tc.readURL)
 		}
-		helper.Delete(te.storeURL)
+		helper.Delete(tc.storeURL)
 	}
 }
 
@@ -198,39 +198,39 @@ func TestWinCredHelperStoreRetrieve(t *testing.T) {
 
 	helper := Wincred{}
 	defer func() {
-		for _, te := range tests {
-			helper.Delete(te.url)
+		for _, tc := range tests {
+			helper.Delete(tc.url)
 		}
 	}()
 
 	// Clean store before testing.
-	for _, te := range tests {
-		helper.Delete(te.url)
+	for _, tc := range tests {
+		helper.Delete(tc.url)
 	}
 
 	// Note that we don't delete between individual tests here, to verify that
 	// subsequent stores/overwrites don't affect storing / retrieving secrets.
-	for i, te := range tests {
+	for i, tc := range tests {
 		c := &credentials.Credentials{
-			ServerURL: te.url,
+			ServerURL: tc.url,
 			Username:  fmt.Sprintf("user-%d", i),
 			Secret:    fmt.Sprintf("secret-%d", i),
 		}
 
 		if err := helper.Add(c); err != nil {
-			t.Errorf("Error: failed to store secret for URL: %s: %s", te.url, err)
+			t.Errorf("Error: failed to store secret for URL: %s: %s", tc.url, err)
 			continue
 		}
-		user, secret, err := helper.Get(te.url)
+		user, secret, err := helper.Get(tc.url)
 		if err != nil {
-			t.Errorf("Error: failed to read secret for URL %q: %s", te.url, err)
+			t.Errorf("Error: failed to read secret for URL %q: %s", tc.url, err)
 			continue
 		}
 		if user != c.Username {
-			t.Errorf("Error: expected username %s, got username %s for URL: %s", c.Username, user, te.url)
+			t.Errorf("Error: expected username %s, got username %s for URL: %s", c.Username, user, tc.url)
 		}
 		if secret != c.Secret {
-			t.Errorf("Error: expected secret %s, got secret %s for URL: %s", c.Secret, secret, te.url)
+			t.Errorf("Error: expected secret %s, got secret %s for URL: %s", c.Secret, secret, tc.url)
 		}
 	}
 }

From 4a8c2d1d81c0c0cd1a034f7a835bf99560cd12fb Mon Sep 17 00:00:00 2001
From: Sebastiaan van Stijn <github@gone.nl>
Date: Sat, 27 May 2023 14:38:28 +0200
Subject: [PATCH 02/10] registryurl: use sub-tests

Also reformat the test-table to easier see what fields are used for
each test in the table.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
---
 registryurl/parse_test.go | 77 ++++++++++++++++++++++++++-------------
 1 file changed, 52 insertions(+), 25 deletions(-)

diff --git a/registryurl/parse_test.go b/registryurl/parse_test.go
index 03a0e03..b5c161d 100644
--- a/registryurl/parse_test.go
+++ b/registryurl/parse_test.go
@@ -13,34 +13,61 @@ func TestHelperParseURL(t *testing.T) {
 		expectedURL string
 		err         error
 	}{
-		{url: "foobar.docker.io", expectedURL: "//foobar.docker.io"},
-		{url: "foobar.docker.io:2376", expectedURL: "//foobar.docker.io:2376"},
-		{url: "//foobar.docker.io:2376", expectedURL: "//foobar.docker.io:2376"},
-		{url: "http://foobar.docker.io:2376", expectedURL: "http://foobar.docker.io:2376"},
-		{url: "https://foobar.docker.io:2376", expectedURL: "https://foobar.docker.io:2376"},
-		{url: "https://foobar.docker.io:2376/some/path", expectedURL: "https://foobar.docker.io:2376/some/path"},
-		{url: "https://foobar.docker.io:2376/some/other/path?foo=bar", expectedURL: "https://foobar.docker.io:2376/some/other/path"},
-		{url: "/foobar.docker.io", err: errors.New("no hostname in URL")},
-		{url: "ftp://foobar.docker.io:2376", err: errors.New("unsupported scheme: ftp")},
+		{
+			url:         "foobar.docker.io",
+			expectedURL: "//foobar.docker.io",
+		},
+		{
+			url:         "foobar.docker.io:2376",
+			expectedURL: "//foobar.docker.io:2376",
+		},
+		{
+			url:         "//foobar.docker.io:2376",
+			expectedURL: "//foobar.docker.io:2376",
+		},
+		{
+			url:         "http://foobar.docker.io:2376",
+			expectedURL: "http://foobar.docker.io:2376",
+		},
+		{
+			url:         "https://foobar.docker.io:2376",
+			expectedURL: "https://foobar.docker.io:2376",
+		},
+		{
+			url:         "https://foobar.docker.io:2376/some/path",
+			expectedURL: "https://foobar.docker.io:2376/some/path",
+		},
+		{
+			url:         "https://foobar.docker.io:2376/some/other/path?foo=bar",
+			expectedURL: "https://foobar.docker.io:2376/some/other/path",
+		},
+		{
+			url: "/foobar.docker.io",
+			err: errors.New("no hostname in URL"),
+		},
+		{
+			url: "ftp://foobar.docker.io:2376",
+			err: errors.New("unsupported scheme: ftp"),
+		},
 	}
 
 	for _, tc := range tests {
-		u, err := Parse(tc.url)
+		tc := tc
+		t.Run(tc.url, func(t *testing.T) {
+			u, err := Parse(tc.url)
 
-		if tc.err == nil && err != nil {
-			t.Errorf("Error: failed to parse URL %q: %s", tc.url, err)
-			continue
-		}
-		if tc.err != nil && err == nil {
-			t.Errorf("Error: expected error %q, got none when parsing URL %q", tc.err, tc.url)
-			continue
-		}
-		if tc.err != nil && err.Error() != tc.err.Error() {
-			t.Errorf("Error: expected error %q, got %q when parsing URL %q", tc.err, err, tc.url)
-			continue
-		}
-		if u != nil && u.String() != tc.expectedURL {
-			t.Errorf("Error: expected URL: %q, but got %q for URL: %q", tc.expectedURL, u.String(), tc.url)
-		}
+			if tc.err == nil && err != nil {
+				t.Fatalf("Error: failed to parse URL %q: %s", tc.url, err)
+			}
+			if tc.err != nil && err == nil {
+				t.Fatalf("Error: expected error %q, got none when parsing URL %q", tc.err, tc.url)
+			}
+			if tc.err != nil && err.Error() != tc.err.Error() {
+				t.Fatalf("Error: expected error %q, got %q when parsing URL %q", tc.err, err, tc.url)
+			}
+			if u != nil && u.String() != tc.expectedURL {
+				t.Errorf("Error: expected URL: %q, but got %q for URL: %q", tc.expectedURL, u.String(), tc.url)
+			}
+		})
 	}
 }

From a7ff1c7d168260b646d20979d4d8ab04507ac2d5 Mon Sep 17 00:00:00 2001
From: Sebastiaan van Stijn <github@gone.nl>
Date: Sat, 27 May 2023 14:15:20 +0200
Subject: [PATCH 03/10] osxkeychain: don't use un-keyed literals in test-tables

Also moving the comments into the test-tables.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
---
 osxkeychain/osxkeychain_darwin_test.go | 74 +++++++++++++++++---------
 1 file changed, 49 insertions(+), 25 deletions(-)

diff --git a/osxkeychain/osxkeychain_darwin_test.go b/osxkeychain/osxkeychain_darwin_test.go
index 48792d6..f639496 100644
--- a/osxkeychain/osxkeychain_darwin_test.go
+++ b/osxkeychain/osxkeychain_darwin_test.go
@@ -60,17 +60,25 @@ func TestOSXKeychainHelper(t *testing.T) {
 // through variations on the URL
 func TestOSXKeychainHelperRetrieveAliases(t *testing.T) {
 	tests := []struct {
+		doc      string
 		storeURL string
 		readURL  string
 	}{
-		// stored with port, retrieved without
-		{"https://foobar.docker.io:2376", "https://foobar.docker.io"},
-
-		// stored as https, retrieved without scheme
-		{"https://foobar.docker.io:2376", "foobar.docker.io"},
-
-		// stored with path, retrieved without
-		{"https://foobar.docker.io:1234/one/two", "https://foobar.docker.io:1234"},
+		{
+			doc:      "stored with port, retrieved without",
+			storeURL: "https://foobar.docker.io:2376",
+			readURL:  "https://foobar.docker.io",
+		},
+		{
+			doc:      "stored as https, retrieved without scheme",
+			storeURL: "https://foobar.docker.io:2376",
+			readURL:  "foobar.docker.io",
+		},
+		{
+			doc:      "stored with path, retrieved without",
+			storeURL: "https://foobar.docker.io:1234/one/two",
+			readURL:  "https://foobar.docker.io:1234",
+		},
 	}
 
 	helper := Osxkeychain{}
@@ -102,26 +110,42 @@ func TestOSXKeychainHelperRetrieveAliases(t *testing.T) {
 // returned.
 func TestOSXKeychainHelperRetrieveStrict(t *testing.T) {
 	tests := []struct {
+		doc      string
 		storeURL string
 		readURL  string
 	}{
-		// stored as https, retrieved using http
-		{"https://foobar.docker.io:2376", "http://foobar.docker.io:2376"},
-
-		// stored as http, retrieved using https
-		{"http://foobar.docker.io:2376", "https://foobar.docker.io:2376"},
-
-		// same: stored as http, retrieved without a scheme specified (hence, using the default https://)
-		{"http://foobar.docker.io", "foobar.docker.io:5678"},
-
-		// non-matching ports
-		{"https://foobar.docker.io:1234", "https://foobar.docker.io:5678"},
-
-		// non-matching ports TODO is this desired behavior? The other way round does work
-		// {"https://foobar.docker.io", "https://foobar.docker.io:5678"},
-
-		// non-matching paths
-		{"https://foobar.docker.io:1234/one/two", "https://foobar.docker.io:1234/five/six"},
+		{
+			doc:      "stored as https, retrieved using http",
+			storeURL: "https://foobar.docker.io:2376",
+			readURL:  "http://foobar.docker.io:2376",
+		},
+		{
+			doc:      "stored as http, retrieved using https",
+			storeURL: "http://foobar.docker.io:2376",
+			readURL:  "https://foobar.docker.io:2376",
+		},
+		{
+			// stored as http, retrieved without a scheme specified (hence, using the default https://)
+			doc:      "stored as http, retrieved without scheme",
+			storeURL: "http://foobar.docker.io",
+			readURL:  "foobar.docker.io:5678",
+		},
+		{
+			doc:      "non-matching ports",
+			storeURL: "https://foobar.docker.io:1234",
+			readURL:  "https://foobar.docker.io:5678",
+		},
+		// TODO: is this desired behavior? The other way round does work
+		// {
+		// 	doc:      "non-matching ports (stored without port)",
+		// 	storeURL: "https://foobar.docker.io",
+		// 	readURL:  "https://foobar.docker.io:5678",
+		// },
+		{
+			doc:      "non-matching paths",
+			storeURL: "https://foobar.docker.io:1234/one/two",
+			readURL:  "https://foobar.docker.io:1234/five/six",
+		},
 	}
 
 	helper := Osxkeychain{}

From 8282d3336abcb682ceeb215601f1dbc54772d96d Mon Sep 17 00:00:00 2001
From: Sebastiaan van Stijn <github@gone.nl>
Date: Sat, 27 May 2023 15:16:36 +0200
Subject: [PATCH 04/10] osxkeychain: use t.Cleanup(), and don't ignore errors

Make sure we don't drop errors when cleaning up state before/after
tests.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
---
 osxkeychain/osxkeychain_darwin_test.go | 44 ++++++++++++++++++--------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/osxkeychain/osxkeychain_darwin_test.go b/osxkeychain/osxkeychain_darwin_test.go
index f639496..7735c1c 100644
--- a/osxkeychain/osxkeychain_darwin_test.go
+++ b/osxkeychain/osxkeychain_darwin_test.go
@@ -82,15 +82,19 @@ func TestOSXKeychainHelperRetrieveAliases(t *testing.T) {
 	}
 
 	helper := Osxkeychain{}
-	defer func() {
+	t.Cleanup(func() {
 		for _, tc := range tests {
-			helper.Delete(tc.storeURL)
+			if err := helper.Delete(tc.storeURL); err != nil && !credentials.IsErrCredentialsNotFound(err) {
+				t.Errorf("cleanup: failed to delete '%s': %v", tc.storeURL, err)
+			}
 		}
-	}()
+	})
 
 	// Clean store before testing.
 	for _, tc := range tests {
-		helper.Delete(tc.storeURL)
+		if err := helper.Delete(tc.storeURL); err != nil && !credentials.IsErrCredentialsNotFound(err) {
+			t.Errorf("prepare: failed to delete '%s': %v", tc.storeURL, err)
+		}
 	}
 
 	for _, tc := range tests {
@@ -102,7 +106,9 @@ func TestOSXKeychainHelperRetrieveAliases(t *testing.T) {
 		if _, _, err := helper.Get(tc.readURL); err != nil {
 			t.Errorf("Error: failed to read secret for URL %q using %q", tc.storeURL, tc.readURL)
 		}
-		helper.Delete(tc.storeURL)
+		if err := helper.Delete(tc.storeURL); err != nil {
+			t.Error(err)
+		}
 	}
 }
 
@@ -149,15 +155,19 @@ func TestOSXKeychainHelperRetrieveStrict(t *testing.T) {
 	}
 
 	helper := Osxkeychain{}
-	defer func() {
+	t.Cleanup(func() {
 		for _, tc := range tests {
-			helper.Delete(tc.storeURL)
+			if err := helper.Delete(tc.storeURL); err != nil && !credentials.IsErrCredentialsNotFound(err) {
+				t.Errorf("cleanup: failed to delete '%s': %v", tc.storeURL, err)
+			}
 		}
-	}()
+	})
 
 	// Clean store before testing.
 	for _, tc := range tests {
-		helper.Delete(tc.storeURL)
+		if err := helper.Delete(tc.storeURL); err != nil && !credentials.IsErrCredentialsNotFound(err) {
+			t.Errorf("prepare: failed to delete '%s': %v", tc.storeURL, err)
+		}
 	}
 
 	for _, tc := range tests {
@@ -169,7 +179,9 @@ func TestOSXKeychainHelperRetrieveStrict(t *testing.T) {
 		if _, _, err := helper.Get(tc.readURL); err == nil {
 			t.Errorf("Error: managed to read secret for URL %q using %q, but should not be able to", tc.storeURL, tc.readURL)
 		}
-		helper.Delete(tc.storeURL)
+		if err := helper.Delete(tc.storeURL); err != nil {
+			t.Error(err)
+		}
 	}
 }
 
@@ -190,15 +202,19 @@ func TestOSXKeychainHelperStoreRetrieve(t *testing.T) {
 	}
 
 	helper := Osxkeychain{}
-	defer func() {
+	t.Cleanup(func() {
 		for _, tc := range tests {
-			helper.Delete(tc.url)
+			if err := helper.Delete(tc.url); err != nil && !credentials.IsErrCredentialsNotFound(err) {
+				t.Errorf("cleanup: failed to delete '%s': %v", tc.url, err)
+			}
 		}
-	}()
+	})
 
 	// Clean store before testing.
 	for _, tc := range tests {
-		helper.Delete(tc.url)
+		if err := helper.Delete(tc.url); err != nil && !credentials.IsErrCredentialsNotFound(err) {
+			t.Errorf("prepare: failed to delete '%s': %v", tc.url, err)
+		}
 	}
 
 	// Note that we don't delete between individual tests here, to verify that

From 5c5b09e7f8115b7d5be5b9116d2f4cc5f78436c3 Mon Sep 17 00:00:00 2001
From: Sebastiaan van Stijn <github@gone.nl>
Date: Sat, 27 May 2023 15:38:04 +0200
Subject: [PATCH 05/10] osxkeychain: use sub-tests

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
---
 osxkeychain/osxkeychain_darwin_test.go | 89 ++++++++++++++------------
 1 file changed, 47 insertions(+), 42 deletions(-)

diff --git a/osxkeychain/osxkeychain_darwin_test.go b/osxkeychain/osxkeychain_darwin_test.go
index 7735c1c..425f314 100644
--- a/osxkeychain/osxkeychain_darwin_test.go
+++ b/osxkeychain/osxkeychain_darwin_test.go
@@ -98,17 +98,19 @@ func TestOSXKeychainHelperRetrieveAliases(t *testing.T) {
 	}
 
 	for _, tc := range tests {
-		c := &credentials.Credentials{ServerURL: tc.storeURL, Username: "hello", Secret: "world"}
-		if err := helper.Add(c); err != nil {
-			t.Errorf("Error: failed to store secret for URL %q: %s", tc.storeURL, err)
-			continue
-		}
-		if _, _, err := helper.Get(tc.readURL); err != nil {
-			t.Errorf("Error: failed to read secret for URL %q using %q", tc.storeURL, tc.readURL)
-		}
-		if err := helper.Delete(tc.storeURL); err != nil {
-			t.Error(err)
-		}
+		tc := tc
+		t.Run(tc.doc, func(t *testing.T) {
+			c := &credentials.Credentials{ServerURL: tc.storeURL, Username: "hello", Secret: "world"}
+			if err := helper.Add(c); err != nil {
+				t.Fatalf("Error: failed to store secret for URL %q: %s", tc.storeURL, err)
+			}
+			if _, _, err := helper.Get(tc.readURL); err != nil {
+				t.Errorf("Error: failed to read secret for URL %q using %q", tc.storeURL, tc.readURL)
+			}
+			if err := helper.Delete(tc.storeURL); err != nil {
+				t.Error(err)
+			}
+		})
 	}
 }
 
@@ -171,17 +173,19 @@ func TestOSXKeychainHelperRetrieveStrict(t *testing.T) {
 	}
 
 	for _, tc := range tests {
-		c := &credentials.Credentials{ServerURL: tc.storeURL, Username: "hello", Secret: "world"}
-		if err := helper.Add(c); err != nil {
-			t.Errorf("Error: failed to store secret for URL %q: %s", tc.storeURL, err)
-			continue
-		}
-		if _, _, err := helper.Get(tc.readURL); err == nil {
-			t.Errorf("Error: managed to read secret for URL %q using %q, but should not be able to", tc.storeURL, tc.readURL)
-		}
-		if err := helper.Delete(tc.storeURL); err != nil {
-			t.Error(err)
-		}
+		tc := tc
+		t.Run(tc.doc, func(t *testing.T) {
+			c := &credentials.Credentials{ServerURL: tc.storeURL, Username: "hello", Secret: "world"}
+			if err := helper.Add(c); err != nil {
+				t.Fatalf("Error: failed to store secret for URL %q: %s", tc.storeURL, err)
+			}
+			if _, _, err := helper.Get(tc.readURL); err == nil {
+				t.Errorf("Error: managed to read secret for URL %q using %q, but should not be able to", tc.storeURL, tc.readURL)
+			}
+			if err := helper.Delete(tc.storeURL); err != nil {
+				t.Error(err)
+			}
+		})
 	}
 }
 
@@ -220,27 +224,28 @@ func TestOSXKeychainHelperStoreRetrieve(t *testing.T) {
 	// Note that we don't delete between individual tests here, to verify that
 	// subsequent stores/overwrites don't affect storing / retrieving secrets.
 	for i, tc := range tests {
-		c := &credentials.Credentials{
-			ServerURL: tc.url,
-			Username:  fmt.Sprintf("user-%d", i),
-			Secret:    fmt.Sprintf("secret-%d", i),
-		}
+		tc := tc
+		t.Run(tc.url, func(t *testing.T) {
+			c := &credentials.Credentials{
+				ServerURL: tc.url,
+				Username:  fmt.Sprintf("user-%d", i),
+				Secret:    fmt.Sprintf("secret-%d", i),
+			}
 
-		if err := helper.Add(c); err != nil {
-			t.Errorf("Error: failed to store secret for URL: %s: %s", tc.url, err)
-			continue
-		}
-		user, secret, err := helper.Get(tc.url)
-		if err != nil {
-			t.Errorf("Error: failed to read secret for URL %q: %s", tc.url, err)
-			continue
-		}
-		if user != c.Username {
-			t.Errorf("Error: expected username %s, got username %s for URL: %s", c.Username, user, tc.url)
-		}
-		if secret != c.Secret {
-			t.Errorf("Error: expected secret %s, got secret %s for URL: %s", c.Secret, secret, tc.url)
-		}
+			if err := helper.Add(c); err != nil {
+				t.Fatalf("Error: failed to store secret for URL: %s: %s", tc.url, err)
+			}
+			user, secret, err := helper.Get(tc.url)
+			if err != nil {
+				t.Fatalf("Error: failed to read secret for URL %q: %s", tc.url, err)
+			}
+			if user != c.Username {
+				t.Errorf("Error: expected username %s, got username %s for URL: %s", c.Username, user, tc.url)
+			}
+			if secret != c.Secret {
+				t.Errorf("Error: expected secret %s, got secret %s for URL: %s", c.Secret, secret, tc.url)
+			}
+		})
 	}
 }
 

From ed91395f201a6a8d167e856ed3958f1901508934 Mon Sep 17 00:00:00 2001
From: Sebastiaan van Stijn <github@gone.nl>
Date: Sat, 27 May 2023 14:18:56 +0200
Subject: [PATCH 06/10] wincred: don't use un-keyed literals in test-tables

Also moving the comments into the test-tables.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
---
 wincred/wincred_windows_test.go | 74 ++++++++++++++++++++++-----------
 1 file changed, 49 insertions(+), 25 deletions(-)

diff --git a/wincred/wincred_windows_test.go b/wincred/wincred_windows_test.go
index 074b29c..27a12a9 100644
--- a/wincred/wincred_windows_test.go
+++ b/wincred/wincred_windows_test.go
@@ -91,17 +91,25 @@ func TestWinCredHelper(t *testing.T) {
 // through variations on the URL
 func TestWinCredHelperRetrieveAliases(t *testing.T) {
 	tests := []struct {
+		doc      string
 		storeURL string
 		readURL  string
 	}{
-		// stored with port, retrieved without
-		{"https://foobar.docker.io:2376", "https://foobar.docker.io"},
-
-		// stored as https, retrieved without scheme
-		{"https://foobar.docker.io", "foobar.docker.io"},
-
-		// stored with path, retrieved without
-		{"https://foobar.docker.io/one/two", "https://foobar.docker.io"},
+		{
+			doc:      "stored with port, retrieved without",
+			storeURL: "https://foobar.docker.io:2376",
+			readURL:  "https://foobar.docker.io",
+		},
+		{
+			doc:      "stored as https, retrieved without scheme",
+			storeURL: "https://foobar.docker.io",
+			readURL:  "foobar.docker.io",
+		},
+		{
+			doc:      "stored with path, retrieved without",
+			storeURL: "https://foobar.docker.io/one/two",
+			readURL:  "https://foobar.docker.io",
+		},
 	}
 
 	helper := Wincred{}
@@ -133,26 +141,42 @@ func TestWinCredHelperRetrieveAliases(t *testing.T) {
 // returned.
 func TestWinCredHelperRetrieveStrict(t *testing.T) {
 	tests := []struct {
+		doc      string
 		storeURL string
 		readURL  string
 	}{
-		// stored as https, retrieved using http
-		{"https://foobar.docker.io:2376", "http://foobar.docker.io:2376"},
-
-		// stored as http, retrieved using https
-		{"http://foobar.docker.io:2376", "https://foobar.docker.io:2376"},
-
-		// same: stored as http, retrieved without a scheme specified (hence, using the default https://)
-		{"http://foobar.docker.io", "foobar.docker.io:5678"},
-
-		// non-matching ports
-		{"https://foobar.docker.io:1234", "https://foobar.docker.io:5678"},
-
-		// non-matching ports TODO is this desired behavior? The other way round does work
-		// {"https://foobar.docker.io", "https://foobar.docker.io:5678"},
-
-		// non-matching paths
-		{"https://foobar.docker.io:1234/one/two", "https://foobar.docker.io:1234/five/six"},
+		{
+			doc:      "stored as https, retrieved using http",
+			storeURL: "https://foobar.docker.io:2376",
+			readURL:  "http://foobar.docker.io:2376",
+		},
+		{
+			doc:      "stored as http, retrieved using https",
+			storeURL: "http://foobar.docker.io:2376",
+			readURL:  "https://foobar.docker.io:2376",
+		},
+		{
+			// stored as http, retrieved without a scheme specified (hence, using the default https://)
+			doc:      "stored as http, retrieved without scheme",
+			storeURL: "http://foobar.docker.io",
+			readURL:  "foobar.docker.io:5678",
+		},
+		{
+			doc:      "non-matching ports",
+			storeURL: "https://foobar.docker.io:1234",
+			readURL:  "https://foobar.docker.io:5678",
+		},
+		// TODO: is this desired behavior? The other way round does work
+		// {
+		// 	doc:      "non-matching ports (stored without port)",
+		// 	storeURL: "https://foobar.docker.io",
+		// 	readURL:  "https://foobar.docker.io:5678",
+		// },
+		{
+			doc:      "non-matching paths",
+			storeURL: "https://foobar.docker.io:1234/one/two",
+			readURL:  "https://foobar.docker.io:1234/five/six",
+		},
 	}
 
 	helper := Wincred{}

From 814dbb3b5aa4367cdff63a8598aef9a4c9fb73ae Mon Sep 17 00:00:00 2001
From: Sebastiaan van Stijn <github@gone.nl>
Date: Sat, 27 May 2023 15:17:24 +0200
Subject: [PATCH 07/10] wincred: use t.Cleanup(), and don't ignore errors

Make sure we don't drop errors when cleaning up state before/after
tests.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
---
 wincred/wincred_windows_test.go | 44 ++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/wincred/wincred_windows_test.go b/wincred/wincred_windows_test.go
index 27a12a9..9b56c33 100644
--- a/wincred/wincred_windows_test.go
+++ b/wincred/wincred_windows_test.go
@@ -113,15 +113,19 @@ func TestWinCredHelperRetrieveAliases(t *testing.T) {
 	}
 
 	helper := Wincred{}
-	defer func() {
+	t.Cleanup(func() {
 		for _, tc := range tests {
-			helper.Delete(tc.storeURL)
+			if err := helper.Delete(tc.storeURL); err != nil && !credentials.IsErrCredentialsNotFound(err) {
+				t.Errorf("cleanup: failed to delete '%s': %v", tc.storeURL, err)
+			}
 		}
-	}()
+	})
 
 	// Clean store before testing.
 	for _, tc := range tests {
-		helper.Delete(tc.storeURL)
+		if err := helper.Delete(tc.storeURL); err != nil && !credentials.IsErrCredentialsNotFound(err) {
+			t.Errorf("prepare: failed to delete '%s': %v", tc.storeURL, err)
+		}
 	}
 
 	for _, tc := range tests {
@@ -133,7 +137,9 @@ func TestWinCredHelperRetrieveAliases(t *testing.T) {
 		if _, _, err := helper.Get(tc.readURL); err != nil {
 			t.Errorf("Error: failed to read secret for URL %q using %q", tc.storeURL, tc.readURL)
 		}
-		helper.Delete(tc.storeURL)
+		if err := helper.Delete(tc.storeURL); err != nil {
+			t.Error(err)
+		}
 	}
 }
 
@@ -180,15 +186,19 @@ func TestWinCredHelperRetrieveStrict(t *testing.T) {
 	}
 
 	helper := Wincred{}
-	defer func() {
+	t.Cleanup(func() {
 		for _, tc := range tests {
-			helper.Delete(tc.storeURL)
+			if err := helper.Delete(tc.storeURL); err != nil && !credentials.IsErrCredentialsNotFound(err) {
+				t.Errorf("cleanup: failed to delete '%s': %v", tc.storeURL, err)
+			}
 		}
-	}()
+	})
 
 	// Clean store before testing.
 	for _, tc := range tests {
-		helper.Delete(tc.storeURL)
+		if err := helper.Delete(tc.storeURL); err != nil && !credentials.IsErrCredentialsNotFound(err) {
+			t.Errorf("prepare: failed to delete '%s': %v", tc.storeURL, err)
+		}
 	}
 
 	for _, tc := range tests {
@@ -200,7 +210,9 @@ func TestWinCredHelperRetrieveStrict(t *testing.T) {
 		if _, _, err := helper.Get(tc.readURL); err == nil {
 			t.Errorf("Error: managed to read secret for URL %q using %q, but should not be able to", tc.storeURL, tc.readURL)
 		}
-		helper.Delete(tc.storeURL)
+		if err := helper.Delete(tc.storeURL); err != nil {
+			t.Error(err)
+		}
 	}
 }
 
@@ -221,15 +233,19 @@ func TestWinCredHelperStoreRetrieve(t *testing.T) {
 	}
 
 	helper := Wincred{}
-	defer func() {
+	t.Cleanup(func() {
 		for _, tc := range tests {
-			helper.Delete(tc.url)
+			if err := helper.Delete(tc.url); err != nil && !credentials.IsErrCredentialsNotFound(err) {
+				t.Errorf("cleanup: failed to delete '%s': %v", tc.url, err)
+			}
 		}
-	}()
+	})
 
 	// Clean store before testing.
 	for _, tc := range tests {
-		helper.Delete(tc.url)
+		if err := helper.Delete(tc.url); err != nil && !credentials.IsErrCredentialsNotFound(err) {
+			t.Errorf("prepare: failed to delete '%s': %v", tc.url, err)
+		}
 	}
 
 	// Note that we don't delete between individual tests here, to verify that

From 7a60d7011439d0ed5a9d359e2f806632300fc1cf Mon Sep 17 00:00:00 2001
From: Sebastiaan van Stijn <github@gone.nl>
Date: Sat, 27 May 2023 15:38:24 +0200
Subject: [PATCH 08/10] wincred: use sub-tests

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
---
 wincred/wincred_windows_test.go | 89 +++++++++++++++++----------------
 1 file changed, 47 insertions(+), 42 deletions(-)

diff --git a/wincred/wincred_windows_test.go b/wincred/wincred_windows_test.go
index 9b56c33..e6010fe 100644
--- a/wincred/wincred_windows_test.go
+++ b/wincred/wincred_windows_test.go
@@ -129,17 +129,19 @@ func TestWinCredHelperRetrieveAliases(t *testing.T) {
 	}
 
 	for _, tc := range tests {
-		c := &credentials.Credentials{ServerURL: tc.storeURL, Username: "hello", Secret: "world"}
-		if err := helper.Add(c); err != nil {
-			t.Errorf("Error: failed to store secret for URL %q: %s", tc.storeURL, err)
-			continue
-		}
-		if _, _, err := helper.Get(tc.readURL); err != nil {
-			t.Errorf("Error: failed to read secret for URL %q using %q", tc.storeURL, tc.readURL)
-		}
-		if err := helper.Delete(tc.storeURL); err != nil {
-			t.Error(err)
-		}
+		tc := tc
+		t.Run(tc.doc, func(t *testing.T) {
+			c := &credentials.Credentials{ServerURL: tc.storeURL, Username: "hello", Secret: "world"}
+			if err := helper.Add(c); err != nil {
+				t.Fatalf("Error: failed to store secret for URL %q: %s", tc.storeURL, err)
+			}
+			if _, _, err := helper.Get(tc.readURL); err != nil {
+				t.Errorf("Error: failed to read secret for URL %q using %q", tc.storeURL, tc.readURL)
+			}
+			if err := helper.Delete(tc.storeURL); err != nil {
+				t.Error(err)
+			}
+		})
 	}
 }
 
@@ -202,17 +204,19 @@ func TestWinCredHelperRetrieveStrict(t *testing.T) {
 	}
 
 	for _, tc := range tests {
-		c := &credentials.Credentials{ServerURL: tc.storeURL, Username: "hello", Secret: "world"}
-		if err := helper.Add(c); err != nil {
-			t.Errorf("Error: failed to store secret for URL %q: %s", tc.storeURL, err)
-			continue
-		}
-		if _, _, err := helper.Get(tc.readURL); err == nil {
-			t.Errorf("Error: managed to read secret for URL %q using %q, but should not be able to", tc.storeURL, tc.readURL)
-		}
-		if err := helper.Delete(tc.storeURL); err != nil {
-			t.Error(err)
-		}
+		tc := tc
+		t.Run(tc.doc, func(t *testing.T) {
+			c := &credentials.Credentials{ServerURL: tc.storeURL, Username: "hello", Secret: "world"}
+			if err := helper.Add(c); err != nil {
+				t.Fatalf("Error: failed to store secret for URL %q: %s", tc.storeURL, err)
+			}
+			if _, _, err := helper.Get(tc.readURL); err == nil {
+				t.Errorf("Error: managed to read secret for URL %q using %q, but should not be able to", tc.storeURL, tc.readURL)
+			}
+			if err := helper.Delete(tc.storeURL); err != nil {
+				t.Error(err)
+			}
+		})
 	}
 }
 
@@ -251,27 +255,28 @@ func TestWinCredHelperStoreRetrieve(t *testing.T) {
 	// Note that we don't delete between individual tests here, to verify that
 	// subsequent stores/overwrites don't affect storing / retrieving secrets.
 	for i, tc := range tests {
-		c := &credentials.Credentials{
-			ServerURL: tc.url,
-			Username:  fmt.Sprintf("user-%d", i),
-			Secret:    fmt.Sprintf("secret-%d", i),
-		}
+		tc := tc
+		t.Run(tc.url, func(t *testing.T) {
+			c := &credentials.Credentials{
+				ServerURL: tc.url,
+				Username:  fmt.Sprintf("user-%d", i),
+				Secret:    fmt.Sprintf("secret-%d", i),
+			}
 
-		if err := helper.Add(c); err != nil {
-			t.Errorf("Error: failed to store secret for URL: %s: %s", tc.url, err)
-			continue
-		}
-		user, secret, err := helper.Get(tc.url)
-		if err != nil {
-			t.Errorf("Error: failed to read secret for URL %q: %s", tc.url, err)
-			continue
-		}
-		if user != c.Username {
-			t.Errorf("Error: expected username %s, got username %s for URL: %s", c.Username, user, tc.url)
-		}
-		if secret != c.Secret {
-			t.Errorf("Error: expected secret %s, got secret %s for URL: %s", c.Secret, secret, tc.url)
-		}
+			if err := helper.Add(c); err != nil {
+				t.Fatalf("Error: failed to store secret for URL: %s: %s", tc.url, err)
+			}
+			user, secret, err := helper.Get(tc.url)
+			if err != nil {
+				t.Fatalf("Error: failed to read secret for URL %q: %s", tc.url, err)
+			}
+			if user != c.Username {
+				t.Errorf("Error: expected username %s, got username %s for URL: %s", c.Username, user, tc.url)
+			}
+			if secret != c.Secret {
+				t.Errorf("Error: expected secret %s, got secret %s for URL: %s", c.Secret, secret, tc.url)
+			}
+		})
 	}
 }
 

From c20f883316f183c48fef6c42a58bd52c0cb8b6b9 Mon Sep 17 00:00:00 2001
From: Sebastiaan van Stijn <github@gone.nl>
Date: Sat, 27 May 2023 15:48:28 +0200
Subject: [PATCH 09/10] client: don't use un-keyed literals in tests, and don't
 fail early

- don't fail tests early if there's more we can test
- don't user un-keyed literals in tests

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
---
 client/client_test.go | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/client/client_test.go b/client/client_test.go
index 693faaa..322d7f0 100644
--- a/client/client_test.go
+++ b/client/client_test.go
@@ -113,7 +113,7 @@ func TestStore(t *testing.T) {
 
 	for _, v := range valid {
 		if err := Store(mockProgramFn, &v); err != nil {
-			t.Fatal(err)
+			t.Error(err)
 		}
 	}
 
@@ -123,7 +123,7 @@ func TestStore(t *testing.T) {
 
 	for _, v := range invalid {
 		if err := Store(mockProgramFn, &v); err == nil {
-			t.Fatalf("Expected error for server %s, got nil", v.ServerURL)
+			t.Errorf("Expected error for server %s, got nil", v.ServerURL)
 		}
 	}
 }
@@ -152,10 +152,10 @@ func TestGet(t *testing.T) {
 		}
 
 		if c.Username != v.Username {
-			t.Fatalf("expected username `%s`, got %s", v.Username, c.Username)
+			t.Errorf("expected username `%s`, got %s", v.Username, c.Username)
 		}
 		if c.Secret != v.Secret {
-			t.Fatalf("expected secret `%s`, got %s", v.Secret, c.Secret)
+			t.Errorf("expected secret `%s`, got %s", v.Secret, c.Secret)
 		}
 	}
 
@@ -165,10 +165,17 @@ func TestGet(t *testing.T) {
 		serverURL string
 		err       string
 	}{
-		{missingCredsAddress, credentials.NewErrCredentialsNotFound().Error()},
-		{invalidServerAddress, "error getting credentials - err: exited 1, out: `program failed`"},
-		{"", fmt.Sprintf("error getting credentials - err: %s, out: `%s`",
-			missingServerURLErr.Error(), missingServerURLErr.Error())},
+		{
+			serverURL: missingCredsAddress,
+			err:       credentials.NewErrCredentialsNotFound().Error(),
+		},
+		{
+			serverURL: invalidServerAddress,
+			err:       "error getting credentials - err: exited 1, out: `program failed`",
+		},
+		{
+			err: fmt.Sprintf("error getting credentials - err: %s, out: `%s`", missingServerURLErr.Error(), missingServerURLErr.Error()),
+		},
 	}
 
 	for _, v := range invalid {
@@ -177,7 +184,7 @@ func TestGet(t *testing.T) {
 			t.Fatalf("Expected error for server %s, got nil", v.serverURL)
 		}
 		if err.Error() != v.err {
-			t.Fatalf("Expected error `%s`, got `%v`", v.err, err)
+			t.Errorf("Expected error `%s`, got `%v`", v.err, err)
 		}
 	}
 }
@@ -192,11 +199,11 @@ func ExampleErase() {
 
 func TestErase(t *testing.T) {
 	if err := Erase(mockProgramFn, validServerAddress); err != nil {
-		t.Fatal(err)
+		t.Error(err)
 	}
 
 	if err := Erase(mockProgramFn, invalidServerAddress); err == nil {
-		t.Fatalf("Expected error for server %s, got nil", invalidServerAddress)
+		t.Errorf("Expected error for server %s, got nil", invalidServerAddress)
 	}
 }
 
@@ -207,6 +214,6 @@ func TestList(t *testing.T) {
 	}
 
 	if username, exists := auths[validServerAddress]; !exists || username != validUsername {
-		t.Fatalf("auths[%s] returned %s, %t; expected %s, %t", validServerAddress, username, exists, validUsername, true)
+		t.Errorf("auths[%s] returned %s, %t; expected %s, %t", validServerAddress, username, exists, validUsername, true)
 	}
 }

From 14d46ffd7e6b10bfc9b0f345c65064d14ca5c912 Mon Sep 17 00:00:00 2001
From: Sebastiaan van Stijn <github@gone.nl>
Date: Sat, 27 May 2023 15:50:11 +0200
Subject: [PATCH 10/10] credentials: don't fail tests early, and use consts

- use consts for fixed values in tests
- don't fail tests early if there's more we can test

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
---
 credentials/credentials_test.go | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/credentials/credentials_test.go b/credentials/credentials_test.go
index e56281e..587d180 100644
--- a/credentials/credentials_test.go
+++ b/credentials/credentials_test.go
@@ -42,7 +42,7 @@ func (m *memoryStore) List() (map[string]string, error) {
 }
 
 func TestStore(t *testing.T) {
-	serverURL := "https://index.docker.io/v1/"
+	const serverURL = "https://index.docker.io/v1/"
 	creds := &Credentials{
 		ServerURL: serverURL,
 		Username:  "foo",
@@ -65,11 +65,11 @@ func TestStore(t *testing.T) {
 	}
 
 	if c.Username != "foo" {
-		t.Fatalf("expected username foo, got %s\n", c.Username)
+		t.Errorf("expected username foo, got %s\n", c.Username)
 	}
 
 	if c.Secret != "bar" {
-		t.Fatalf("expected username bar, got %s\n", c.Secret)
+		t.Errorf("expected username bar, got %s\n", c.Secret)
 	}
 }
 
@@ -89,7 +89,7 @@ func TestStoreMissingServerURL(t *testing.T) {
 	h := newMemoryStore()
 
 	if err := Store(h, in); IsCredentialsMissingServerURL(err) == false {
-		t.Fatal(err)
+		t.Error(err)
 	}
 }
 
@@ -109,12 +109,12 @@ func TestStoreMissingUsername(t *testing.T) {
 	h := newMemoryStore()
 
 	if err := Store(h, in); IsCredentialsMissingUsername(err) == false {
-		t.Fatal(err)
+		t.Error(err)
 	}
 }
 
 func TestGet(t *testing.T) {
-	serverURL := "https://index.docker.io/v1/"
+	const serverURL = "https://index.docker.io/v1/"
 	creds := &Credentials{
 		ServerURL: serverURL,
 		Username:  "foo",
@@ -147,16 +147,16 @@ func TestGet(t *testing.T) {
 	}
 
 	if c.Username != "foo" {
-		t.Fatalf("expected username foo, got %s\n", c.Username)
+		t.Errorf("expected username foo, got %s\n", c.Username)
 	}
 
 	if c.Secret != "bar" {
-		t.Fatalf("expected username bar, got %s\n", c.Secret)
+		t.Errorf("expected username bar, got %s\n", c.Secret)
 	}
 }
 
 func TestGetMissingServerURL(t *testing.T) {
-	serverURL := "https://index.docker.io/v1/"
+	const serverURL = "https://index.docker.io/v1/"
 	creds := &Credentials{
 		ServerURL: serverURL,
 		Username:  "foo",
@@ -177,12 +177,12 @@ func TestGetMissingServerURL(t *testing.T) {
 	w := new(bytes.Buffer)
 
 	if err := Get(h, buf, w); IsCredentialsMissingServerURL(err) == false {
-		t.Fatal(err)
+		t.Error(err)
 	}
 }
 
 func TestErase(t *testing.T) {
-	serverURL := "https://index.docker.io/v1/"
+	const serverURL = "https://index.docker.io/v1/"
 	creds := &Credentials{
 		ServerURL: serverURL,
 		Username:  "foo",
@@ -206,12 +206,12 @@ func TestErase(t *testing.T) {
 
 	w := new(bytes.Buffer)
 	if err := Get(h, buf, w); err == nil {
-		t.Fatal("expected error getting missing creds, got empty")
+		t.Error("expected error getting missing creds, got empty")
 	}
 }
 
 func TestEraseMissingServerURL(t *testing.T) {
-	serverURL := "https://index.docker.io/v1/"
+	const serverURL = "https://index.docker.io/v1/"
 	creds := &Credentials{
 		ServerURL: serverURL,
 		Username:  "foo",
@@ -244,6 +244,6 @@ func TestList(t *testing.T) {
 	}
 	// testing that there is an output
 	if out.Len() == 0 {
-		t.Fatalf("expected output in the writer, got %d", 0)
+		t.Error("expected output in the writer, got 0")
 	}
 }