Add testify linter and address fixes (#1334)

While reviewing #1330 we had a discussion about the ability to generally prevent the use of `requires` within `Eventually` calls to prevent future flakes, and that led us to evaluate the `testifylint`-er to give us some cautionary guidelines. It did _not_ resolve the requires-within-eventually issue, but we may try to tackle that with some custom AST in the future. Signed-off-by: Jordan Keister <[email protected]>
operator-framework · Oct 4, 2024 · 2d4c000 · 2d4c000
1 parent 814bf63
commit 2d4c000
Show file tree

Hide file tree

Showing 10 changed files with 106 additions and 122 deletions.
diff --git a/.golangci.yaml b/.golangci.yaml
@@ -30,6 +30,7 @@ linters:
   - nonamedreturns
   - prealloc
   - stylecheck
+  - testifylint
   - tparallel
   - unconvert
   - unparam

diff --git a/internal/action/helm_test.go b/internal/action/helm_test.go
@@ -8,6 +8,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
 	"helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/release"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -130,12 +131,12 @@ func TestActionClientFor(t *testing.T) {
 
 	// Test the successful case
 	actionClient, err := acg.ActionClientFor(ctx, obj)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	assert.NotNil(t, actionClient)
 	assert.IsType(t, &ActionClient{}, actionClient)
 
 	// Test the error case
 	actionClient, err = acg.ActionClientFor(ctx, obj)
-	assert.Error(t, err)
+	require.Error(t, err)
 	assert.Nil(t, actionClient)
 }
diff --git a/internal/catalogmetadata/cache/cache_test.go b/internal/catalogmetadata/cache/cache_test.go
@@ -79,26 +79,26 @@ func TestFilesystemCachePutAndGet(t *testing.T) {
 
 	t.Log("Get empty v1 cache")
 	actualFSGet, err := c.Get(catalogName, resolvedRef1)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	assert.Nil(t, actualFSGet)
 
 	t.Log("Put v1 content into cache")
 	actualFSPut, err := c.Put(catalogName, resolvedRef1, defaultContent(), nil)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	require.NotNil(t, actualFSPut)
-	assert.NoError(t, equalFilesystems(defaultFS(), actualFSPut))
+	require.NoError(t, equalFilesystems(defaultFS(), actualFSPut))
 
 	t.Log("Get v1 content from cache")
 	actualFSGet, err = c.Get(catalogName, resolvedRef1)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	require.NotNil(t, actualFSGet)
 	assert.NoError(t, equalFilesystems(defaultFS(), actualFSPut))
 	assert.NoError(t, equalFilesystems(actualFSPut, actualFSGet))
 
 	t.Log("Put v1 error into cache")
 	actualFSPut, err = c.Put(catalogName, resolvedRef1, nil, errors.New("fake put error"))
 	// Errors do not override previously successfully populated cache
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	require.NotNil(t, actualFSPut)
 	assert.NoError(t, equalFilesystems(defaultFS(), actualFSPut))
 	assert.NoError(t, equalFilesystems(actualFSPut, actualFSGet))
@@ -115,13 +115,13 @@ func TestFilesystemCachePutAndGet(t *testing.T) {
 
 	t.Log("Put v2 content into cache")
 	actualFSPut, err = c.Put(catalogName, resolvedRef2, defaultContent(), nil)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	require.NotNil(t, actualFSPut)
-	assert.NoError(t, equalFilesystems(defaultFS(), actualFSPut))
+	require.NoError(t, equalFilesystems(defaultFS(), actualFSPut))
 
 	t.Log("Get v2 content from cache")
 	actualFSGet, err = c.Get(catalogName, resolvedRef2)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	require.NotNil(t, actualFSGet)
 	assert.NoError(t, equalFilesystems(defaultFS(), actualFSPut))
 	assert.NoError(t, equalFilesystems(actualFSPut, actualFSGet))
@@ -130,7 +130,7 @@ func TestFilesystemCachePutAndGet(t *testing.T) {
 	// Cache should be empty and no error because
 	// Put with a new version overrides the old version
 	actualFSGet, err = c.Get(catalogName, resolvedRef1)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	assert.Nil(t, actualFSGet)
 }
 

diff --git a/internal/catalogmetadata/client/client_test.go b/internal/catalogmetadata/client/client_test.go
@@ -11,6 +11,7 @@ import (
 	"testing/fstest"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	catalogd "github.com/operator-framework/catalogd/api/core/v1alpha1"
@@ -77,7 +78,7 @@ func TestClientGetPackage(t *testing.T) {
 			pkgName: "pkg-missing",
 			cache:   &fakeCache{getFS: testFS},
 			assert: func(t *testing.T, fbc *declcfg.DeclarativeConfig, err error) {
-				assert.NoError(t, err)
+				require.NoError(t, err)
 				assert.Equal(t, &declcfg.DeclarativeConfig{}, fbc)
 			},
 		},
@@ -89,7 +90,7 @@ func TestClientGetPackage(t *testing.T) {
 				"invalid-pkg-present/olm.package/invalid-pkg-present.json": &fstest.MapFile{Data: []byte(`{"schema": "olm.package","name": 12345}`)},
 			}},
 			assert: func(t *testing.T, fbc *declcfg.DeclarativeConfig, err error) {
-				assert.ErrorContains(t, err, `error loading package "invalid-pkg-present"`)
+				require.ErrorContains(t, err, `error loading package "invalid-pkg-present"`)
 				assert.Nil(t, fbc)
 			},
 		},
@@ -99,7 +100,7 @@ func TestClientGetPackage(t *testing.T) {
 			pkgName: "pkg-present",
 			cache:   &fakeCache{getFS: testFS},
 			assert: func(t *testing.T, fbc *declcfg.DeclarativeConfig, err error) {
-				assert.NoError(t, err)
+				require.NoError(t, err)
 				assert.Equal(t, &declcfg.DeclarativeConfig{Packages: []declcfg.Package{{Schema: declcfg.SchemaPackage, Name: "pkg-present"}}}, fbc)
 			},
 		},
@@ -111,7 +112,7 @@ func TestClientGetPackage(t *testing.T) {
 				return testFS, nil
 			}},
 			assert: func(t *testing.T, fbc *declcfg.DeclarativeConfig, err error) {
-				assert.NoError(t, err)
+				require.NoError(t, err)
 				assert.Equal(t, &declcfg.DeclarativeConfig{Packages: []declcfg.Package{{Schema: declcfg.SchemaPackage, Name: "pkg-present"}}}, fbc)
 			},
 		},
@@ -170,7 +171,7 @@ func TestClientPopulateCache(t *testing.T) {
 				}, nil
 			},
 			assert: func(t *testing.T, fs fs.FS, err error) {
-				assert.NoError(t, err)
+				require.NoError(t, err)
 				assert.Equal(t, testFS, fs)
 			},
 			putFuncConstructor: func(t *testing.T) func(source string, errToCache error) (fs.FS, error) {

diff --git a/internal/catalogmetadata/filter/bundle_predicates_test.go b/internal/catalogmetadata/filter/bundle_predicates_test.go
@@ -6,6 +6,7 @@ import (
 
 	mmsemver "github.com/Masterminds/semver/v3"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"github.com/operator-framework/operator-registry/alpha/declcfg"
 	"github.com/operator-framework/operator-registry/alpha/property"
@@ -40,7 +41,7 @@ func TestInMastermindsSemverRange(t *testing.T) {
 	}
 
 	vRange, err := mmsemver.NewConstraint(">=1.0.0")
-	assert.NoError(t, err)
+	require.NoError(t, err)
 
 	f := filter.InMastermindsSemverRange(vRange)
 

diff --git a/internal/catalogmetadata/filter/successors_test.go b/internal/catalogmetadata/filter/successors_test.go
@@ -171,7 +171,7 @@ func TestSuccessorsPredicateWithForceSemverUpgradeConstraintsEnabled(t *testing.
 	} {
 		t.Run(tt.name, func(t *testing.T) {
 			successors, err := SuccessorsOf(tt.installedBundle, channelSet[testPackageName])
-			assert.NoError(t, err)
+			require.NoError(t, err)
 
 			allBundles := make([]declcfg.Bundle, 0, len(bundleSet))
 			for _, bundle := range bundleSet {
@@ -328,7 +328,7 @@ func TestSuccessorsPredicateWithForceSemverUpgradeConstraintsDisabled(t *testing
 	} {
 		t.Run(tt.name, func(t *testing.T) {
 			successors, err := SuccessorsOf(tt.installedBundle, channelSet[testPackageName])
-			assert.NoError(t, err)
+			require.NoError(t, err)
 
 			allBundles := make([]declcfg.Bundle, 0, len(bundleSet))
 			for _, bundle := range bundleSet {
@@ -380,7 +380,7 @@ func TestLegacySuccessor(t *testing.T) {
 	emptyBundle := declcfg.Bundle{}
 
 	f, err := legacySuccessor(installedBundle, fakeChannel)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 
 	assert.True(t, f(b2))
 	assert.False(t, f(b3))

diff --git a/internal/controllers/clusterextension_controller_test.go b/internal/controllers/clusterextension_controller_test.go
@@ -167,7 +167,7 @@ func TestClusterExtensionResolutionSuccessfulUnpackFails(t *testing.T) {
 
 			isTerminal := errors.Is(err, reconcile.TerminalError(nil))
 			assert.Equal(t, tc.expectTerminal, isTerminal, "expected terminal error: %v, got: %v", tc.expectTerminal, isTerminal)
-			assert.ErrorContains(t, err, tc.unpackErr.Error())
+			require.ErrorContains(t, err, tc.unpackErr.Error())
 
 			t.Log("By fetching updated cluster extension after reconcile")
 			require.NoError(t, cl.Get(ctx, extKey, clusterExtension))

diff --git a/internal/resolve/catalog_test.go b/internal/resolve/catalog_test.go
@@ -397,7 +397,7 @@ func TestPackageVariationsBetweenCatalogs(t *testing.T) {
 		gotBundle, gotVersion, gotDeprecation, err := r.Resolve(context.Background(), ce, nil)
 		require.Error(t, err)
 		// We will not make a decision on which catalog to use
-		assert.ErrorContains(t, err, "in multiple catalogs with the same priority [b c]")
+		require.ErrorContains(t, err, "in multiple catalogs with the same priority [b c]")
 		assert.Nil(t, gotBundle)
 		assert.Nil(t, gotVersion)
 		assert.Nil(t, gotDeprecation)
@@ -408,7 +408,7 @@ func TestPackageVariationsBetweenCatalogs(t *testing.T) {
 		gotBundle, gotVersion, gotDeprecation, err := r.Resolve(context.Background(), ce, nil)
 		require.Error(t, err)
 		// We will not make a decision on which catalog to use
-		assert.ErrorContains(t, err, "in multiple catalogs with the same priority [d f]")
+		require.ErrorContains(t, err, "in multiple catalogs with the same priority [d f]")
 		assert.Nil(t, gotBundle)
 		assert.Nil(t, gotVersion)
 		assert.Nil(t, gotDeprecation)
@@ -635,7 +635,7 @@ func TestCatalogWalker(t *testing.T) {
 			seenCatalogs = append(seenCatalogs, cat.Name)
 			return nil
 		}
-		assert.NoError(t, w(context.Background(), "", walkFunc))
+		require.NoError(t, w(context.Background(), "", walkFunc))
 		assert.Equal(t, []string{"a", "b"}, seenCatalogs)
 	})
 }
@@ -936,7 +936,7 @@ func TestMultiplePriority(t *testing.T) {
 	ce := buildFooClusterExtension(pkgName, []string{}, ">=1.0.0 <=1.0.1", ocv1alpha1.UpgradeConstraintPolicyCatalogProvided)
 	gotBundle, gotVersion, gotDeprecation, err := r.Resolve(context.Background(), ce, nil)
 	require.Error(t, err)
-	assert.ErrorContains(t, err, "in multiple catalogs with the same priority [a b c]")
+	require.ErrorContains(t, err, "in multiple catalogs with the same priority [a b c]")
 	assert.Nil(t, gotBundle)
 	assert.Nil(t, gotVersion)
 	assert.Nil(t, gotDeprecation)

diff --git a/internal/rukpak/source/containers_image_test.go b/internal/rukpak/source/containers_image_test.go
@@ -53,12 +53,12 @@ func TestUnpackValidInsecure(t *testing.T) {
 	result, err := unpacker.Unpack(context.Background(), bundleSource)
 	require.NoError(t, err)
 	require.NotNil(t, result)
-	assert.Equal(t, result.State, source.StateUnpacked)
+	assert.Equal(t, source.StateUnpacked, result.State)
 
 	require.NoDirExists(t, oldBundlePath)
 
 	unpackedFile, err := fs.ReadFile(result.Bundle, testFileName)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	// Ensure the unpacked file matches the source content
 	assert.Equal(t, []byte(testFileContents), unpackedFile)
 	assert.NoError(t, unpacker.Cleanup(context.Background(), bundleSource))
@@ -87,9 +87,9 @@ func TestUnpackValidUsesCache(t *testing.T) {
 
 	// Attempt to pull and unpack the image
 	result, err := unpacker.Unpack(context.Background(), bundleSource)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	require.NotNil(t, result)
-	assert.Equal(t, result.State, source.StateUnpacked)
+	assert.Equal(t, source.StateUnpacked, result.State)
 
 	// Make sure the original contents of the cache are still present. If the cached contents
 	// were not used, we would expect the original contents to be removed.
@@ -144,7 +144,7 @@ func TestUnpackNameOnlyImageReference(t *testing.T) {
 
 	// Attempt to pull and unpack the image
 	_, err := unpacker.Unpack(context.Background(), bundleSource)
-	assert.ErrorContains(t, err, "tag or digest is needed")
+	require.ErrorContains(t, err, "tag or digest is needed")
 	assert.ErrorIs(t, err, reconcile.TerminalError(nil))
 }
 
@@ -226,8 +226,8 @@ func TestUnpackInvalidNilImage(t *testing.T) {
 	// Attempt to unpack
 	result, err := unpacker.Unpack(context.Background(), bundleSource)
 	assert.Nil(t, result)
-	assert.ErrorContains(t, err, "nil image source")
-	assert.ErrorIs(t, err, reconcile.TerminalError(nil))
+	require.ErrorContains(t, err, "nil image source")
+	require.ErrorIs(t, err, reconcile.TerminalError(nil))
 	assert.NoDirExists(t, filepath.Join(unpacker.BaseCachePath, bundleSource.Name))
 }
 
@@ -245,8 +245,8 @@ func TestUnpackInvalidImageRef(t *testing.T) {
 	// Attempt to unpack
 	result, err := unpacker.Unpack(context.Background(), bundleSource)
 	assert.Nil(t, result)
-	assert.ErrorContains(t, err, "error parsing image reference")
-	assert.ErrorIs(t, err, reconcile.TerminalError(nil))
+	require.ErrorContains(t, err, "error parsing image reference")
+	require.ErrorIs(t, err, reconcile.TerminalError(nil))
 	assert.NoDirExists(t, filepath.Join(unpacker.BaseCachePath, bundleSource.Name))
 }
 
@@ -322,7 +322,7 @@ func TestCleanup(t *testing.T) {
 
 	// Clean up the bundle
 	err := unpacker.Cleanup(context.Background(), bundleSource)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	assert.NoDirExists(t, bundleDir)
 }