Skip to content

Commit

Permalink
Add testify linter and address fixes (#1334)
Browse files Browse the repository at this point in the history
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 <jordan@nimblewidget.com>
  • Loading branch information
grokspawn authored Oct 4, 2024
1 parent 814bf63 commit 2d4c000
Show file tree
Hide file tree
Showing 10 changed files with 106 additions and 122 deletions.
1 change: 1 addition & 0 deletions .golangci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ linters:
- nonamedreturns
- prealloc
- stylecheck
- testifylint
- tparallel
- unconvert
- unparam
Expand Down
5 changes: 3 additions & 2 deletions internal/action/helm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
}
18 changes: 9 additions & 9 deletions internal/catalogmetadata/cache/cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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))
Expand All @@ -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)
}

Expand Down
11 changes: 6 additions & 5 deletions internal/catalogmetadata/client/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
},
},
Expand All @@ -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)
},
},
Expand All @@ -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)
},
},
Expand All @@ -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)
},
},
Expand Down Expand Up @@ -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) {
Expand Down
3 changes: 2 additions & 1 deletion internal/catalogmetadata/filter/bundle_predicates_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)

Expand Down
6 changes: 3 additions & 3 deletions internal/catalogmetadata/filter/successors_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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))
Expand Down
2 changes: 1 addition & 1 deletion internal/controllers/clusterextension_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
8 changes: 4 additions & 4 deletions internal/resolve/catalog_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)
})
}
Expand Down Expand Up @@ -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)
Expand Down
20 changes: 10 additions & 10 deletions internal/rukpak/source/containers_image_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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))
}

Expand Down Expand Up @@ -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))
}

Expand All @@ -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))
}

Expand Down Expand Up @@ -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)
}

Expand Down
Loading

0 comments on commit 2d4c000

Please sign in to comment.