address linter issues (#985)

* address linter issues

Signed-off-by: Xavier Coulon <[email protected]>

* fix test failures

Signed-off-by: Xavier Coulon <[email protected]>

---------

Signed-off-by: Xavier Coulon <[email protected]>
Co-authored-by: Francisc Munteanu <[email protected]>

.golangci.yml

@@ -31,4 +31,9 @@ linters-settings:
     # if it's called for subdir of a project it can't find external interfaces. All text editor integrations
     # with golangci-lint call it on a directory with the changed file.
     check-exported: true
+  revive:
+    rules:
+      - name: dot-imports
+        disabled: true # we allow for dot-imports
+
 

controllers/deactivation/deactivation_controller_test.go

@@ -116,7 +116,7 @@ func TestReconcile(t *testing.T) {
 			// then
 			require.NoError(t, err)
 			require.False(t, res.Requeue, "requeue should not be set")
-			require.True(t, res.RequeueAfter == 0, "requeueAfter should not be set")
+			require.Equal(t, time.Duration(0), res.RequeueAfter, "requeueAfter should not be set")
 			assertThatUserSignupDeactivated(t, cl, username, false)
 		})
 
@@ -130,7 +130,7 @@ func TestReconcile(t *testing.T) {
 			// then
 			require.NoError(t, err)
 			require.False(t, res.Requeue, "requeue should not be set")
-			require.True(t, res.RequeueAfter == 0, "requeueAfter should not be set")
+			require.Equal(t, time.Duration(0), res.RequeueAfter, "requeueAfter should not be set")
 			assertThatUserSignupDeactivated(t, cl, username, false)
 		})
 
@@ -152,7 +152,7 @@ func TestReconcile(t *testing.T) {
 			// then
 			require.NoError(t, err)
 			require.False(t, res.Requeue, "requeue should not be set")
-			require.True(t, res.RequeueAfter == 0, "requeueAfter should not be set")
+			require.Equal(t, time.Duration(0), res.RequeueAfter, "requeueAfter should not be set")
 			assertThatUserSignupDeactivated(t, cl, username, false)
 		})
 
@@ -264,7 +264,7 @@ func TestReconcile(t *testing.T) {
 							// then
 							require.NoError(t, err)
 							require.False(t, res.Requeue, "requeue should not be set")
-							require.True(t, res.RequeueAfter == 0, "requeueAfter should not be set")
+							require.Equal(t, time.Duration(0), res.RequeueAfter, "requeueAfter should not be set")
 						})
 					})
 				})
@@ -284,7 +284,7 @@ func TestReconcile(t *testing.T) {
 			// then
 			require.NoError(t, err)
 			require.False(t, res.Requeue, "requeue should not be set")
-			require.True(t, res.RequeueAfter == 0, "requeue should not be set")
+			require.Equal(t, time.Duration(0), res.RequeueAfter, "requeue should not be set")
 			assertThatUserSignupDeactivated(t, cl, username, true)
 			AssertMetricsCounterEquals(t, 1, metrics.UserSignupAutoDeactivatedTotal)
 		})
@@ -396,7 +396,7 @@ func TestReconcile(t *testing.T) {
 			require.Error(t, err)
 			require.Contains(t, err.Error(), "usersignup get error")
 			require.False(t, res.Requeue, "requeue should not be set")
-			require.True(t, res.RequeueAfter == 0, "requeueAfter should not be set")
+			require.Equal(t, time.Duration(0), res.RequeueAfter, "requeueAfter should not be set")
 		})
 
 		// cannot update UserSignup
@@ -429,7 +429,7 @@ func TestReconcile(t *testing.T) {
 			require.Error(t, err)
 			require.Contains(t, err.Error(), "usersignup update error")
 			require.False(t, res.Requeue, "requeue should not be set")
-			require.True(t, res.RequeueAfter == 0, "requeueAfter should not be set")
+			require.Equal(t, time.Duration(0), res.RequeueAfter, "requeueAfter should not be set")
 			assertThatUserSignupDeactivated(t, cl, username, false)
 		})
 	})

controllers/deactivation/mapper_test.go

@@ -56,7 +56,7 @@ func TestUserSignupToMasterUserRecordMapper(t *testing.T) {
 		req := MapUserSignupToMasterUserRecord()(userSignup)
 
 		// then
-		require.Len(t, req, 0)
+		require.Empty(t, req)
 	})
 
 	t.Run("test non-UserSignup doesn't map", func(t *testing.T) {
@@ -79,7 +79,7 @@ func TestUserSignupToMasterUserRecordMapper(t *testing.T) {
 		req := MapUserSignupToMasterUserRecord()(mur)
 
 		// then
-		require.Len(t, req, 0)
+		require.Empty(t, req)
 	})
 
 }

controllers/masteruserrecord/mapper_test.go

@@ -54,7 +54,7 @@ func TestBannedUserToUserSignupMapper(t *testing.T) {
 		requests := MapSpaceToMasterUserRecord(client)(space)
 
 		// then
-		assert.Len(t, requests, 0)
+		assert.Empty(t, requests)
 	})
 
 	t.Run("when list fails", func(t *testing.T) {
@@ -68,7 +68,7 @@ func TestBannedUserToUserSignupMapper(t *testing.T) {
 		requests := MapSpaceToMasterUserRecord(client)(space)
 
 		// then
-		assert.Len(t, requests, 0)
+		assert.Empty(t, requests)
 	})
 }
 

controllers/masteruserrecord/masteruserrecord_controller_test.go

@@ -1164,9 +1164,9 @@ func TestSyncMurStatusWithUserAccountStatuses(t *testing.T) {
 		require.NoError(t, err)
 		assert.Equal(t, "userprovisioned", notification.Spec.Template)
 		assert.Contains(t, notification.Name, userAccount.Name+"-provisioned-")
-		assert.True(t, len(notification.Name) > len(userAccount.Name+"-provisioned-"))
+		assert.Greater(t, len(notification.Name), len(userAccount.Name+"-provisioned-"))
 		assert.Equal(t, mur.Namespace, notification.Namespace)
-		require.Equal(t, 1, len(notification.OwnerReferences))
+		require.Len(t, notification.OwnerReferences, 1)
 		assert.Equal(t, "MasterUserRecord", notification.OwnerReferences[0].Kind)
 		assert.Equal(t, mur.Name, notification.OwnerReferences[0].Name)
 		AssertThatCountersAndMetrics(t).

controllers/masteruserrecord/sync_test.go

@@ -683,7 +683,7 @@ func TestSynchronizeUserAccountFailed(t *testing.T) {
 			// then
 			require.Error(t, err)
 			assert.Contains(t, err.Error(), "unable to update MUR john")
-			assert.Len(t, provisionedMur.Status.UserAccounts, 0)
+			assert.Empty(t, provisionedMur.Status.UserAccounts)
 		})
 
 		t.Run("when the UserAccount was added", func(t *testing.T) {

controllers/notification/notification_controller_test.go

@@ -61,8 +61,8 @@ func TestNotificationSuccess(t *testing.T) {
 		// then
 		require.NoError(t, err)
 		assert.True(t, result.Requeue)
-		assert.True(t, result.RequeueAfter < cast.ToDuration("10s"))
-		assert.True(t, result.RequeueAfter > cast.ToDuration("1s"))
+		assert.Greater(t, cast.ToDuration("10s"), result.RequeueAfter)
+		assert.Greater(t, result.RequeueAfter, cast.ToDuration("1s"))
 		ntest.AssertThatNotification(t, notification.Name, cl).
 			HasConditions(sentCond())
 	})

controllers/nstemplatetier/nstemplatetier_controller_test.go

@@ -164,8 +164,7 @@ func TestReconcile(t *testing.T) {
 				// when
 				res, err := r.Reconcile(context.TODO(), req)
 				// then
-				require.Error(t, err)
-				assert.EqualError(t, err, "unable to insert a new entry in status.updates after NSTemplateTier changed: mock error")
+				require.EqualError(t, err, "unable to insert a new entry in status.updates after NSTemplateTier changed: mock error")
 				assert.Equal(t, reconcile.Result{}, res) // no explicit requeue
 			})
 		})

controllers/socialevent/socialevent_controller_test.go

@@ -103,8 +103,7 @@ func TestReconcileSocialEvent(t *testing.T) {
 			_, err := ctrl.Reconcile(context.TODO(), requestFor(event))
 
 			// then
-			require.Error(t, err)
-			assert.EqualError(t, err, "unable to get the 'notfound' UserTier: mock error")
+			require.EqualError(t, err, "unable to get the 'notfound' UserTier: mock error")
 			// check the social event status
 			socialeventtest.AssertThatSocialEvent(t, test.HostOperatorNs, event.Name, hostClient).
 				HasConditions(toolchainv1alpha1.Condition{

controllers/spacebindingcleanup/spacebinding_cleanup_controller_test.go

@@ -162,7 +162,7 @@ func TestDeleteSpaceBindingRequest(t *testing.T) {
 		res, err := reconciler.Reconcile(context.TODO(), requestFor(sbLaraAdmin))
 
 		// then
-		require.Equal(t, res.RequeueAfter, 10*time.Second)
+		require.Equal(t, 10*time.Second, res.RequeueAfter)
 		require.NoError(t, err)
 		sbrtestcommon.AssertThatSpaceBindingRequest(t, sbr.GetNamespace(), sbr.GetName(), member1.Client).DoesNotExist() // spacebindingrequest was deleted
 	})

controllers/spacerequest/spacerequest_controller_test.go

@@ -494,7 +494,7 @@ func TestCreateSpaceRequest(t *testing.T) {
 				})
 				require.NoError(t, err)
 				require.Len(t, secrets.Items, 1)
-				require.Equal(t, secrets.Items[0].Generation, int64(1))
+				require.Equal(t, int64(1), secrets.Items[0].Generation)
 			})
 		})
 

controllers/toolchainconfig/sync_test.go

@@ -66,7 +66,7 @@ func TestSyncMemberConfigs(t *testing.T) {
 		t.Run("sync to a member failed", func(t *testing.T) {
 			// given
 			memberCl := test.NewFakeClient(t)
-			memberCl.MockGet = func(_ context.Context, key types.NamespacedName, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error {
+			memberCl.MockGet = func(_ context.Context, _ types.NamespacedName, _ runtimeclient.Object, _ ...runtimeclient.GetOption) error {
 				return fmt.Errorf("client error")
 			}
 			toolchainConfig := commonconfig.NewToolchainConfigObjWithReset(t,
@@ -88,11 +88,11 @@ func TestSyncMemberConfigs(t *testing.T) {
 		t.Run("sync to multiple members failed", func(t *testing.T) {
 			// given
 			memberCl := test.NewFakeClient(t)
-			memberCl.MockGet = func(_ context.Context, key types.NamespacedName, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error {
+			memberCl.MockGet = func(_ context.Context, _ types.NamespacedName, _ runtimeclient.Object, _ ...runtimeclient.GetOption) error {
 				return fmt.Errorf("client error")
 			}
 			memberCl2 := test.NewFakeClient(t)
-			memberCl2.MockGet = func(_ context.Context, key types.NamespacedName, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error {
+			memberCl2.MockGet = func(_ context.Context, _ types.NamespacedName, _ runtimeclient.Object, _ ...runtimeclient.GetOption) error {
 				return fmt.Errorf("client2 error")
 			}
 			toolchainConfig := commonconfig.NewToolchainConfigObjWithReset(t,
@@ -196,7 +196,7 @@ func TestSyncMemberConfig(t *testing.T) {
 			// given
 			originalConfig := testconfig.NewMemberOperatorConfigObj(testconfig.MemberStatus().RefreshPeriod("10s"))
 			memberCl := test.NewFakeClient(t, originalConfig)
-			memberCl.MockUpdate = func(_ context.Context, obj runtimeclient.Object, opts ...runtimeclient.UpdateOption) error {
+			memberCl.MockUpdate = func(_ context.Context, _ runtimeclient.Object, _ ...runtimeclient.UpdateOption) error {
 				return fmt.Errorf("client update error")
 			}
 			memberCluster := NewMemberClusterWithClient(memberCl, "member1", corev1.ConditionTrue)

controllers/toolchainconfig/toolchainconfig_controller_test.go

@@ -321,7 +321,7 @@ func TestWrapErrorWithUpdateStatus(t *testing.T) {
 		// test
 		err := controller.WrapErrorWithStatusUpdate(ctx, config, statusUpdater, nil, "failed to load the latest configuration")
 
-		require.Nil(t, err)
+		require.NoError(t, err)
 	})
 
 	t.Run("status updated", func(t *testing.T) {

controllers/toolchainstatus/toolchainstatus_controller_test.go

@@ -88,7 +88,7 @@ func prepareReconcile(t *testing.T, requestName string, httpTestClient *fakeHTTP
 		HTTPClientImpl: httpTestClient,
 		Scheme:         s,
 		Namespace:      test.HostOperatorNs,
-		GetMembersFunc: func(conditions ...cluster.Condition) []*cluster.CachedToolchainCluster {
+		GetMembersFunc: func(_ ...cluster.Condition) []*cluster.CachedToolchainCluster {
 			clusters := make([]*cluster.CachedToolchainCluster, len(memberClusters))
 			for i, clusterName := range memberClusters {
 				clusters[i] = cachedToolchainCluster(fakeClient, clusterName, corev1.ConditionTrue, metav1.Now())
@@ -1786,7 +1786,7 @@ func TestRemoveSchemeFromURL(t *testing.T) {
 
 		// then
 		require.Equal(t, "stone-stg-host.qc0p.p1.openshiftapps.com", domain)
-		require.Nil(t, err)
+		require.NoError(t, err)
 
 	})
 
@@ -1796,7 +1796,7 @@ func TestRemoveSchemeFromURL(t *testing.T) {
 
 		// then
 		require.Equal(t, "", domain)
-		require.NotNil(t, err)
+		require.Error(t, err)
 
 	})
 

controllers/usersignup/status_updater_test.go

@@ -66,7 +66,7 @@ func TestUpdateHomeSpaceStatus(t *testing.T) {
 		// if the UserSignup would be updated, then the function would return an error because
 		// we didn't add the UserSignup to the FakeClient as part of the setup
 		// (there is nothing to be updated), thus we expect no error returned
-		require.Nil(t, err)
+		require.NoError(t, err)
 	})
 
 	t.Run("home space status updated", func(t *testing.T) {

controllers/usersignup/usersignup_controller_test.go

@@ -1597,7 +1597,7 @@ func TestUserSignupMUROrSpaceOrSpaceBindingCreateFails(t *testing.T) {
 				}),
 			))
 
-			fakeClient.MockCreate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.CreateOption) error {
+			fakeClient.MockCreate = func(ctx context.Context, obj runtimeclient.Object, _ ...runtimeclient.CreateOption) error {
 				switch obj.(type) {
 				case *toolchainv1alpha1.MasterUserRecord:
 					if testcase.testName == "create mur error" {
@@ -1705,7 +1705,7 @@ func TestUserSignupSetStatusApprovedByAdminFails(t *testing.T) {
 		}),
 	))
 
-	fakeClient.MockStatusUpdate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.UpdateOption) error {
+	fakeClient.MockStatusUpdate = func(ctx context.Context, obj runtimeclient.Object, _ ...runtimeclient.UpdateOption) error {
 		switch obj.(type) {
 		case *toolchainv1alpha1.UserSignup:
 			return errors.New("failed to update UserSignup status")
@@ -2286,7 +2286,7 @@ func TestUserSignupDeactivatedAfterMURCreated(t *testing.T) {
 		require.Len(t, notifications.Items, 1)
 		notification := notifications.Items[0]
 		require.Contains(t, notification.Name, "john-doe-deactivated-")
-		assert.True(t, len(notification.Name) > len("john-doe-deactivated-"))
+		assert.Greater(t, len(notification.Name), len("john-doe-deactivated-"))
 		require.Equal(t, userSignup.Spec.IdentityClaims.Sub, notification.Spec.Context["Sub"])
 		require.Equal(t, "https://registration.crt-placeholder.com", notification.Spec.Context["RegistrationURL"])
 		assert.Equal(t, "userdeactivated", notification.Spec.Template)

controllers/usersignupcleanup/usersignup_cleanup_controller_test.go

@@ -2,14 +2,14 @@ package usersignupcleanup
 
 import (
 	"context"
-	"errors"
 	"fmt"
-	commonconfig "github.com/codeready-toolchain/toolchain-common/pkg/configuration"
-	testconfig "github.com/codeready-toolchain/toolchain-common/pkg/test/config"
 	"os"
 	"testing"
 	"time"
 
+	commonconfig "github.com/codeready-toolchain/toolchain-common/pkg/configuration"
+	testconfig "github.com/codeready-toolchain/toolchain-common/pkg/test/config"
+
 	toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1"
 	"github.com/codeready-toolchain/host-operator/pkg/apis"
 	"github.com/codeready-toolchain/host-operator/pkg/metrics"
@@ -101,7 +101,7 @@ func TestUserCleanup(t *testing.T) {
 		require.Error(t, err)
 		require.True(t, apierrors.IsNotFound(err))
 		statusErr := &apierrors.StatusError{}
-		require.True(t, errors.As(err, &statusErr))
+		require.ErrorAs(t, err, &statusErr)
 		require.Equal(t, fmt.Sprintf("usersignups.toolchain.dev.openshift.com \"%s\" not found", key.Name), statusErr.Error())
 	})
 
@@ -125,7 +125,7 @@ func TestUserCleanup(t *testing.T) {
 		require.True(t, apierrors.IsNotFound(err))
 		require.IsType(t, &apierrors.StatusError{}, err)
 		statusErr := &apierrors.StatusError{}
-		require.True(t, errors.As(err, &statusErr))
+		require.ErrorAs(t, err, &statusErr)
 		require.Equal(t, fmt.Sprintf("usersignups.toolchain.dev.openshift.com \"%s\" not found", key.Name), statusErr.Error())
 
 		t.Run("deletion is not initiated twice", func(t *testing.T) {
@@ -150,8 +150,8 @@ func TestUserCleanup(t *testing.T) {
 		require.True(t, apierrors.IsNotFound(err))
 		assert.Errorf(t, err, "usersignups.toolchain.dev.openshift.com \"%s\" not found", key.Name)
 		// and verify the metrics
-		assert.Equal(t, float64(0), promtestutil.ToFloat64(metrics.UserSignupDeletedWithInitiatingVerificationTotal))    // unchanged
-		assert.Equal(t, float64(1), promtestutil.ToFloat64(metrics.UserSignupDeletedWithoutInitiatingVerificationTotal)) // incremented
+		assert.InDelta(t, float64(0), promtestutil.ToFloat64(metrics.UserSignupDeletedWithInitiatingVerificationTotal), 0.01)    // unchanged
+		assert.InDelta(t, float64(1), promtestutil.ToFloat64(metrics.UserSignupDeletedWithoutInitiatingVerificationTotal), 0.01) // incremented
 
 		t.Run("deletion is not initiated twice", func(t *testing.T) {
 			alreadyDeletedSignupIgnored(t, userSignup)
@@ -352,7 +352,7 @@ func TestUserCleanup(t *testing.T) {
 					require.True(t, apierrors.IsNotFound(err))
 					require.IsType(t, &apierrors.StatusError{}, err)
 					statusErr := &apierrors.StatusError{}
-					require.True(t, errors.As(err, &statusErr))
+					require.ErrorAs(t, err, &statusErr)
 					require.Equal(t, fmt.Sprintf("usersignups.toolchain.dev.openshift.com \"%s\" not found", key.Name), statusErr.Error())
 				} else {
 					// Confirm the UserSignup has not been deleted

pkg/counter/cache_test.go

@@ -14,9 +14,10 @@ import (
 	testconfig "github.com/codeready-toolchain/toolchain-common/pkg/test/config"
 	"github.com/codeready-toolchain/toolchain-common/pkg/test/masteruserrecord"
 	spacetest "github.com/codeready-toolchain/toolchain-common/pkg/test/space"
-	"k8s.io/apimachinery/pkg/runtime"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/runtime"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 )
@@ -506,7 +507,7 @@ func TestMultipleExecutionsInParallel(t *testing.T) {
 			defer waitForFinished.Done()
 			latch.Wait()
 			err := counter.Synchronize(context.TODO(), fakeClient, toolchainStatus)
-			require.NoError(t, err)
+			assert.NoError(t, err) // require must only be used in the goroutine running the test function (testifylint)
 		}()
 	}
 

pkg/pending/cache_test.go

@@ -125,7 +125,7 @@ func TestGetOldestSpacePendingTargetCluster(t *testing.T) {
 func approve(t *testing.T, cl *test.FakeClient, signup *toolchainv1alpha1.UserSignup) {
 	commonsignup.WithStateLabel("approved")(signup)
 	err := cl.Update(context.TODO(), signup)
-	require.NoError(t, err)
+	assert.NoError(t, err) //  approve contains assertions that must only be used in the goroutine running the test function (testifylint)
 }
 
 func assignCluster(t *testing.T, cl *test.FakeClient, sp *toolchainv1alpha1.Space) {
@@ -267,14 +267,14 @@ func TestGetOldestPendingApprovalWithMultipleUserSignupsInParallel(t *testing.T)
 			}
 			for _, signup := range allSingups {
 				err := cl.Create(ctx, signup)
-				require.NoError(t, err)
+				assert.NoError(t, err) // require must only be used in the goroutine running the test function (testifylint)
 			}
 
 			for _, pending := range []*toolchainv1alpha1.UserSignup{pending1, pending2} {
 				go func(toApprove *toolchainv1alpha1.UserSignup) {
 					defer waitForFinished.Done()
 					oldestPendingApproval := cache.getOldestPendingObject(ctx, test.HostOperatorNs)
-					require.NotNil(t, oldestPendingApproval)
+					assert.NotNil(t, oldestPendingApproval) // require must only be used in the goroutine running the test function (testifylint)
 					approve(t, cl, toApprove)
 				}(pending)
 			}

test/metric.go

@@ -9,9 +9,9 @@ import (
 )
 
 func AssertMetricsCounterEquals(t *testing.T, expected int, c prometheus.Counter) {
-	assert.Equal(t, float64(expected), promtestutil.ToFloat64(c))
+	assert.InDelta(t, float64(expected), promtestutil.ToFloat64(c), 0.01)
 }
 
 func AssertMetricsGaugeEquals(t *testing.T, expected int, g prometheus.Gauge, msgAndArgs ...interface{}) {
-	assert.Equal(t, float64(expected), promtestutil.ToFloat64(g), msgAndArgs...)
+	assert.InDelta(t, float64(expected), promtestutil.ToFloat64(g), 0.01, msgAndArgs...)
 }