Merge pull request red-hat-storage#1306 from priyanka19-98/fix-unit-tests

Fixed usage of global mock variables in unit-tests

Author: jarrpa

CONTRIBUTING.md

@@ -119,7 +119,7 @@ make unit-test
 
 *It is of special note that many of the mock objects in the [StorageCluster
 tests](./pkg/controller/storagecluster/storagecluster_controller_test.go) are
-reused in many other test cases. They may be useful in developing your own.*
+reused in many other test cases. Please avoid using the global mock variables directly, instead use the mock variables by creating DeepCopy() to prevent modification of global mock variables. They may be useful in developing your own.*
 
 ### Certificate of Origin
 

controllers/storagecluster/cephcluster_test.go

@@ -72,7 +72,7 @@ func TestEnsureCephCluster(t *testing.T) {
 
 		reconciler := createFakeStorageClusterReconciler(t)
 
-		expected := newCephCluster(mockStorageCluster, "", 3, reconciler.serverVersion, nil, log)
+		expected := newCephCluster(mockStorageCluster.DeepCopy(), "", 3, reconciler.serverVersion, nil, log)
 		expected.ObjectMeta.SelfLink = "/api/v1/namespaces/ceph/secrets/pvc-ceph-client-key"
 		expected.Status.State = c.cephClusterState
 
@@ -158,7 +158,7 @@ func TestCephClusterMonTimeout(t *testing.T) {
 		mockStorageCluster.DeepCopyInto(sc)
 		sc.Status.Images.Ceph = &api.ComponentImageStatus{}
 
-		reconciler := createFakeStorageClusterReconciler(t, mockCephCluster)
+		reconciler := createFakeStorageClusterReconciler(t, mockCephCluster.DeepCopy())
 
 		reconciler.platform = &Platform{
 			platform: c.platform,

controllers/storagecluster/initialization_reconciler_test.go

@@ -5,20 +5,14 @@ import (
 	"os"
 	"testing"
 
-	snapapi "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1"
 	nbv1 "github.com/noobaa/noobaa-operator/v2/pkg/apis/noobaa/v1alpha1"
 	configv1 "github.com/openshift/api/config/v1"
 	routev1 "github.com/openshift/api/route/v1"
-	openshiftv1 "github.com/openshift/api/template/v1"
 	api "github.com/openshift/ocs-operator/api/v1"
-	monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
 	cephv1 "github.com/rook/rook/pkg/apis/ceph.rook.io/v1"
 	"github.com/stretchr/testify/assert"
 	appsv1 "k8s.io/api/apps/v1"
-	batchv1 "k8s.io/api/batch/v1"
-	corev1 "k8s.io/api/core/v1"
 	storagev1 "k8s.io/api/storage/v1"
-	extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
@@ -163,8 +157,8 @@ func createFakeInitializationStorageClusterReconciler(t *testing.T, obj ...runti
 func createFakeInitializationStorageClusterReconcilerWithPlatform(t *testing.T,
 	platform *Platform,
 	obj ...runtime.Object) StorageClusterReconciler {
-	scheme := createFakeInitializationScheme(t, obj...)
-	obj = append(obj, mockNodeList)
+	scheme := createFakeScheme(t)
+	obj = append(obj, mockNodeList.DeepCopy())
 	client := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(obj...).Build()
 	if platform == nil {
 		platform = &Platform{platform: configv1.NonePlatformType}
@@ -178,52 +172,3 @@ func createFakeInitializationStorageClusterReconcilerWithPlatform(t *testing.T,
 		platform:      platform,
 	}
 }
-
-func createFakeInitializationScheme(t *testing.T, obj ...runtime.Object) *runtime.Scheme {
-	registerObjs := obj
-	registerObjs = append(registerObjs) //nolint:staticcheck
-	api.SchemeBuilder.Register(registerObjs...)
-	scheme, err := api.SchemeBuilder.Build()
-	if err != nil {
-		assert.Fail(t, "unable to build scheme")
-	}
-	err = corev1.AddToScheme(scheme)
-	if err != nil {
-		assert.Fail(t, "failed to add corev1 scheme")
-	}
-	err = cephv1.AddToScheme(scheme)
-	if err != nil {
-		assert.Fail(t, "failed to add cephv1 scheme")
-	}
-	err = storagev1.AddToScheme(scheme)
-	if err != nil {
-		assert.Fail(t, "failed to add storagev1 scheme")
-	}
-	err = openshiftv1.AddToScheme(scheme)
-	if err != nil {
-		assert.Fail(t, "failed to add openshiftv1 scheme")
-	}
-	err = snapapi.AddToScheme(scheme)
-	if err != nil {
-		assert.Fail(t, "failed to add volume-snapshot scheme")
-	}
-	err = monitoringv1.AddToScheme(scheme)
-	if err != nil {
-		assert.Fail(t, "failed to add monitoringv1 scheme")
-	}
-	err = extv1.AddToScheme(scheme)
-	if err != nil {
-		assert.Fail(t, "failed to add extv1 scheme")
-	}
-	err = routev1.AddToScheme(scheme)
-	if err != nil {
-		assert.Fail(t, "failed to add routev1 scheme")
-	}
-
-	err = batchv1.AddToScheme(scheme)
-	if err != nil {
-		assert.Fail(t, "failed to add batchv1 scheme")
-	}
-
-	return scheme
-}

controllers/storagecluster/noobaa_system_reconciler_test.go

@@ -8,7 +8,6 @@ import (
 
 	"github.com/noobaa/noobaa-operator/v2/pkg/apis/noobaa/v1alpha1"
 	nbv1 "github.com/noobaa/noobaa-operator/v2/pkg/apis/noobaa/v1alpha1"
-	openshiftv1 "github.com/openshift/api/template/v1"
 	v1 "github.com/openshift/ocs-operator/api/v1"
 	cephv1 "github.com/rook/rook/pkg/apis/ceph.rook.io/v1"
 	"github.com/stretchr/testify/assert"
@@ -76,20 +75,20 @@ func TestEnsureNooBaaSystem(t *testing.T) {
 		{
 			label:          "case 1", //ensure create logic
 			namespacedName: namespacedName,
-			sc:             sc,
-			noobaa:         noobaa,
+			sc:             *sc.DeepCopy(),
+			noobaa:         *noobaa.DeepCopy(),
 			isCreate:       true,
 		},
 		{
 			label:          "case 2", //ensure update logic
 			namespacedName: namespacedName,
-			sc:             sc,
-			noobaa:         noobaa,
+			sc:             *sc.DeepCopy(),
+			noobaa:         *noobaa.DeepCopy(),
 		},
 		{
 			label:          "case 3", //equal, no update
 			namespacedName: namespacedName,
-			sc:             sc,
+			sc:             *sc.DeepCopy(),
 			noobaa: v1alpha1.NooBaa{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      namespacedName.Name,
@@ -399,20 +398,7 @@ func assertNoobaaResource(t *testing.T, reconciler StorageClusterReconciler) {
 func getReconciler(t *testing.T, objs ...runtime.Object) StorageClusterReconciler {
 	registerObjs := []runtime.Object{&v1.StorageCluster{}}
 	registerObjs = append(registerObjs, objs...)
-	v1.SchemeBuilder.Register(registerObjs...)
-
-	scheme, err := v1.SchemeBuilder.Build()
-	if err != nil {
-		assert.Fail(t, "unable to build scheme")
-	}
-	err = cephv1.AddToScheme(scheme)
-	if err != nil {
-		assert.Fail(t, "failed to add rookCephv1 scheme")
-	}
-	err = openshiftv1.AddToScheme(scheme)
-	if err != nil {
-		assert.Fail(t, "failed to add openshiftv1 scheme")
-	}
+	scheme := createFakeScheme(t)
 	client := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(registerObjs...).Build()
 
 	return StorageClusterReconciler{

controllers/storagecluster/placement_test.go

@@ -116,7 +116,7 @@ func TestGetPlacement(t *testing.T) {
 	}{
 		{
 			label:          "Case 1: Defaults are preserved i.e no placement and no label selector",
-			storageCluster: mockStorageCluster,
+			storageCluster: mockStorageCluster.DeepCopy(),
 			placements:     rookCephv1.PlacementSpec{},
 			labelSelector:  nil,
 			expectedPlacements: rookCephv1.PlacementSpec{
@@ -132,7 +132,7 @@ func TestGetPlacement(t *testing.T) {
 		},
 		{
 			label:          "Case 2: The configured Placements override the defaults",
-			storageCluster: mockStorageCluster,
+			storageCluster: mockStorageCluster.DeepCopy(),
 			placements:     emptyPlacements,
 			labelSelector:  nil,
 			expectedPlacements: rookCephv1.PlacementSpec{
@@ -147,7 +147,7 @@ func TestGetPlacement(t *testing.T) {
 		},
 		{
 			label:          "Case 3: LabelSelector to modify the default Placements correctly",
-			storageCluster: mockStorageCluster,
+			storageCluster: mockStorageCluster.DeepCopy(),
 			placements:     rookCephv1.PlacementSpec{},
 			labelSelector:  &workerLabelSelector,
 			expectedPlacements: rookCephv1.PlacementSpec{
@@ -163,7 +163,7 @@ func TestGetPlacement(t *testing.T) {
 		},
 		{
 			label:          "Case 4: LabelSelector modifies an empty NodeAffinity",
-			storageCluster: mockStorageCluster,
+			storageCluster: mockStorageCluster.DeepCopy(),
 			placements:     emptyPlacements,
 			labelSelector:  &workerLabelSelector,
 			expectedPlacements: rookCephv1.PlacementSpec{
@@ -178,7 +178,7 @@ func TestGetPlacement(t *testing.T) {
 		},
 		{
 			label:          "Case 5: LabelSelector modifies a configured NodeAffinity",
-			storageCluster: mockStorageCluster,
+			storageCluster: mockStorageCluster.DeepCopy(),
 			placements:     workerPlacements,
 			labelSelector:  &masterLabelSelector,
 			expectedPlacements: rookCephv1.PlacementSpec{
@@ -220,7 +220,7 @@ func TestGetPlacement(t *testing.T) {
 		},
 		{
 			label:          "Case 6: Empty LabelSelector sets no NodeAffinity",
-			storageCluster: mockStorageCluster,
+			storageCluster: mockStorageCluster.DeepCopy(),
 			placements:     rookCephv1.PlacementSpec{},
 			labelSelector:  &emptyLabelSelector,
 			expectedPlacements: rookCephv1.PlacementSpec{
@@ -234,7 +234,7 @@ func TestGetPlacement(t *testing.T) {
 		},
 		{
 			label:          "Case 7: Custom placement is applied without failure",
-			storageCluster: mockStorageCluster,
+			storageCluster: mockStorageCluster.DeepCopy(),
 			placements: rookCephv1.PlacementSpec{
 				"mon": customPlacement,
 			},
@@ -251,7 +251,7 @@ func TestGetPlacement(t *testing.T) {
 		},
 		{
 			label:          "Case 8: Custom placement is modified by labelSelector",
-			storageCluster: mockStorageCluster,
+			storageCluster: mockStorageCluster.DeepCopy(),
 			placements: rookCephv1.PlacementSpec{
 				"mon": customPlacement,
 			},
@@ -269,7 +269,7 @@ func TestGetPlacement(t *testing.T) {
 		},
 		{
 			label:          "Case 9: NodeTopologyMap modifies default mon placement",
-			storageCluster: mockStorageCluster,
+			storageCluster: mockStorageCluster.DeepCopy(),
 			placements:     rookCephv1.PlacementSpec{},
 			expectedPlacements: rookCephv1.PlacementSpec{
 				"all": {
@@ -308,7 +308,7 @@ func TestGetPlacement(t *testing.T) {
 		},
 		{
 			label:          "Case 10: skip podAntiAffinity in mon placement in case of stretched cluster",
-			storageCluster: mockStorageClusterWithArbiter,
+			storageCluster: mockStorageClusterWithArbiter.DeepCopy(),
 			placements:     rookCephv1.PlacementSpec{},
 			labelSelector:  nil,
 			expectedPlacements: rookCephv1.PlacementSpec{