test: improve unit tests and add race condition detection (cloudnativ…

…e-pg#5936)

* Added `make test-race` target to the Makefile to enable running tests
with the Go race detector. This helps catch potential concurrency
issues.

* Added the missing `RunSpec` invocation in the
`internal/cmd/manager/instance/run` package.

* Marked relevant test suites with the `Ordered` modifier to ensure
proper execution order for dependent tests.

* Moved shared variables initialization in a `BeforeEach` block.

* Avoided disrupting the formatting of the test output by ensuring there
was no output to stdout during the tests.

* Renamed some imported packages to improve code clarity and
readability.

* Removed empty and unused test suites.

* Improved test suite setup and teardown with `DeferCleanup()` to handle
cleanup operations consistently and avoid manual `AfterSuite` calls.

* Used `SpecContext` injected parameter instead of `context.TODO()` when
a context is needed.

* Applied various style fixes for consistency and code quality.

Signed-off-by: Marco Nenciarini <[email protected]>
Signed-off-by: Marco Nenciarini <[email protected]>
Signed-off-by: Jaime Silvela <[email protected]>
Co-authored-by: Jaime Silvela <[email protected]>

Makefile

@@ -101,7 +101,13 @@ test: generate fmt vet manifests envtest ## Run tests.
 	source <(${ENVTEST} use -p env --bin-dir ${ENVTEST_ASSETS_DIR} ${ENVTEST_K8S_VERSION}) ;\
 	export KUBEBUILDER_CONTROLPLANE_STOP_TIMEOUT=60s ;\
 	export KUBEBUILDER_CONTROLPLANE_START_TIMEOUT=60s ;\
-	go test -coverpkg=./... --count=1 -coverprofile=cover.out ./api/... ./cmd/... ./internal/... ./pkg/... ./tests/utils ;
+	go test -coverpkg=./... -coverprofile=cover.out ./api/... ./cmd/... ./internal/... ./pkg/... ./tests/utils
+
+test-race: generate fmt vet manifests envtest ## Run tests enabling race detection.
+	mkdir -p ${ENVTEST_ASSETS_DIR} ;\
+	source <(${ENVTEST} use -p env --bin-dir ${ENVTEST_ASSETS_DIR} ${ENVTEST_K8S_VERSION}) ;\
+	go run github.com/onsi/ginkgo/v2/ginkgo -r -p --skip-package=e2e \
+	  --race --keep-going --fail-on-empty --randomize-all --randomize-suites
 
 e2e-test-kind: ## Run e2e tests locally using kind.
 	hack/e2e/run-e2e-kind.sh

api/v1/cluster_funcs_test.go

@@ -452,7 +452,7 @@ var _ = Describe("external cluster list", func() {
 	})
 })
 
-var _ = Describe("look up for secrets", func() {
+var _ = Describe("look up for secrets", Ordered, func() {
 	cluster := Cluster{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "clustername",
@@ -479,15 +479,19 @@ var _ = Describe("look up for secrets", func() {
 	It("retrieves client CA secret name", func() {
 		Expect(cluster.GetClientCASecretName()).To(Equal("clustername-ca"))
 	})
+
 	It("retrieves server CA secret name", func() {
 		Expect(cluster.GetServerCASecretName()).To(Equal("clustername-ca"))
 	})
+
 	It("retrieves replication secret name", func() {
 		Expect(cluster.GetReplicationSecretName()).To(Equal("clustername-replication"))
 	})
+
 	It("retrieves replication secret name", func() {
 		Expect(cluster.GetReplicationSecretName()).To(Equal("clustername-replication"))
 	})
+
 	It("retrieves all names needed to build a server CA certificate", func() {
 		names := cluster.GetClusterAltDNSNames()
 		Expect(names).To(HaveLen(12))

api/v1/scheduledbackup_funcs_test.go

@@ -26,9 +26,13 @@ import (
 )
 
 var _ = Describe("Scheduled backup", func() {
-	scheduledBackup := &ScheduledBackup{}
+	var scheduledBackup *ScheduledBackup
 	backupName := "test"
 
+	BeforeEach(func() {
+		scheduledBackup = &ScheduledBackup{}
+	})
+
 	It("properly creates a backup with no annotations", func() {
 		backup := scheduledBackup.CreateBackup("test")
 		Expect(backup).ToNot(BeNil())

internal/cmd/manager/walarchive/suite_test.go → internal/cmd/manager/instance/run/suite_test.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package walarchive
+package run
 
 import (
 	"testing"
@@ -23,7 +23,7 @@ import (
 	. "github.com/onsi/gomega"
 )
 
-func TestUtils(t *testing.T) {
+func TestSuite(t *testing.T) {
 	RegisterFailHandler(Fail)
-	RunSpecs(t, "walarchive test suite")
+	RunSpecs(t, "instance run test suite")
 }

internal/cmd/plugin/logs/cluster_logs_test.go

@@ -17,12 +17,11 @@ limitations under the License.
 package logs
 
 import (
-	"context"
 	"path"
 
-	v1 "k8s.io/api/core/v1"
-	v12 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	fake2 "k8s.io/client-go/kubernetes/fake"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	fakeClient "k8s.io/client-go/kubernetes/fake"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	apiv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1"
@@ -34,18 +33,18 @@ import (
 	. "github.com/onsi/gomega"
 )
 
-var _ = Describe("Get the logs", func() {
+var _ = Describe("Get the logs", Ordered, func() {
 	namespace := "default"
 	clusterName := "test-cluster"
-	pod := &v1.Pod{
-		ObjectMeta: v12.ObjectMeta{
+	pod := &corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
 			Namespace: namespace,
 			Name:      clusterName + "-1",
 		},
 	}
-	client := fake2.NewSimpleClientset(pod)
+	client := fakeClient.NewSimpleClientset(pod)
 	cluster := &apiv1.Cluster{
-		ObjectMeta: v12.ObjectMeta{
+		ObjectMeta: metav1.ObjectMeta{
 			Namespace: namespace,
 			Name:      clusterName,
 			Labels: map[string]string{
@@ -54,20 +53,24 @@ var _ = Describe("Get the logs", func() {
 		},
 		Spec: apiv1.ClusterSpec{},
 	}
-	cl := clusterLogs{
-		ctx:         context.TODO(),
-		clusterName: clusterName,
-		namespace:   namespace,
-		follow:      true,
-		timestamp:   true,
-		tailLines:   -1,
-		client:      client,
-	}
+	var cl clusterLogs
 	plugin.Client = fake.NewClientBuilder().
 		WithScheme(scheme.BuildWithAllKnownScheme()).
 		WithObjects(cluster).
 		Build()
 
+	BeforeEach(func(ctx SpecContext) {
+		cl = clusterLogs{
+			ctx:         ctx,
+			clusterName: clusterName,
+			namespace:   namespace,
+			follow:      true,
+			timestamp:   true,
+			tailLines:   -1,
+			client:      client,
+		}
+	})
+
 	It("should get a proper cluster", func() {
 		cluster, err := getCluster(cl)
 		Expect(err).ToNot(HaveOccurred())
@@ -95,18 +98,24 @@ var _ = Describe("Get the logs", func() {
 	})
 
 	It("should get the proper stream for logs", func() {
+		PauseOutputInterception()
 		err := followCluster(cl)
+		ResumeOutputInterception()
 		Expect(err).ToNot(HaveOccurred())
 	})
 
 	It("should save the logs to file", func() {
+		tempDir := GinkgoT().TempDir()
 		cl.outputFile = path.Join(tempDir, "test-file.logs")
+		PauseOutputInterception()
 		err := saveClusterLogs(cl)
+		ResumeOutputInterception()
 		Expect(err).ToNot(HaveOccurred())
 	})
 
 	It("should fail if can't write a file", func() {
-		cl.outputFile = "/this-does-not-exist/test-file.log"
+		tempDir := GinkgoT().TempDir()
+		cl.outputFile = path.Join(tempDir, "this-does-not-exist/test-file.log")
 		err := saveClusterLogs(cl)
 		Expect(err).To(HaveOccurred())
 	})

internal/cmd/plugin/logs/cluster_test.go

@@ -17,8 +17,8 @@ limitations under the License.
 package logs
 
 import (
-	v1 "k8s.io/api/core/v1"
-	v12 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	fakeClient "k8s.io/client-go/kubernetes/fake"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
@@ -35,14 +35,14 @@ var _ = Describe("Test the command", func() {
 	clusterName := "test-cluster"
 	namespace := "default"
 	var cluster *apiv1.Cluster
-	pod := &v1.Pod{
-		ObjectMeta: v12.ObjectMeta{
+	pod := &corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
 			Namespace: namespace,
 			Name:      clusterName + "-1",
 		},
 	}
 	cluster = &apiv1.Cluster{
-		ObjectMeta: v12.ObjectMeta{
+		ObjectMeta: metav1.ObjectMeta{
 			Namespace: namespace,
 			Name:      clusterName,
 			Labels: map[string]string{
@@ -62,14 +62,18 @@ var _ = Describe("Test the command", func() {
 	It("should not fail, with cluster name as argument", func() {
 		cmd := clusterCmd()
 		cmd.SetArgs([]string{clusterName})
+		PauseOutputInterception()
 		err := cmd.Execute()
+		ResumeOutputInterception()
 		Expect(err).ToNot(HaveOccurred())
 	})
 
 	It("could follow the logs", func() {
 		cmd := clusterCmd()
 		cmd.SetArgs([]string{clusterName, "-f"})
+		PauseOutputInterception()
 		err := cmd.Execute()
+		ResumeOutputInterception()
 		Expect(err).ToNot(HaveOccurred())
 	})
 })

internal/cmd/plugin/logs/suite_test.go

@@ -17,27 +17,13 @@ limitations under the License.
 package logs
 
 import (
-	"os"
 	"testing"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 )
 
-var tempDir string
-
 func TestPgbench(t *testing.T) {
 	RegisterFailHandler(Fail)
 	RunSpecs(t, "Logs Suite")
 }
-
-var _ = BeforeSuite(func() {
-	var err error
-	tempDir, err = os.MkdirTemp(os.TempDir(), "logs_")
-	Expect(err).ToNot(HaveOccurred())
-})
-
-var _ = AfterSuite(func() {
-	err := os.RemoveAll(tempDir)
-	Expect(err).ToNot(HaveOccurred())
-})

internal/cmd/plugin/suite_test.go

@@ -48,8 +48,6 @@ func TestPlugin(t *testing.T) {
 }
 
 var _ = BeforeSuite(func() {
-	By("bootstrapping test environment")
-
 	if os.Getenv("USE_EXISTING_CLUSTER") == "true" {
 		By("using existing config for test environment")
 		testEnv = &envtest.Environment{}
@@ -65,6 +63,12 @@ var _ = BeforeSuite(func() {
 	Expect(err).ToNot(HaveOccurred())
 	Expect(cfg).ToNot(BeNil())
 
+	DeferCleanup(func() {
+		By("tearing down the test environment")
+		err := testEnv.Stop()
+		Expect(err).ToNot(HaveOccurred())
+	})
+
 	err = apiv1.AddToScheme(scheme.Scheme)
 	Expect(err).NotTo(HaveOccurred())
 
@@ -74,9 +78,3 @@ var _ = BeforeSuite(func() {
 	Expect(err).ToNot(HaveOccurred())
 	Expect(k8sClient).ToNot(BeNil())
 })
-
-var _ = AfterSuite(func() {
-	By("tearing down the test environment")
-	err := testEnv.Stop()
-	Expect(err).ToNot(HaveOccurred())
-})

internal/controller/cluster_controller_test.go

@@ -17,7 +17,6 @@ limitations under the License.
 package controller
 
 import (
-	"context"
 	"time"
 
 	cnpgTypes "github.com/cloudnative-pg/machinery/pkg/types"
@@ -73,8 +72,7 @@ var _ = Describe("Updating target primary", func() {
 		env = buildTestEnvironment()
 	})
 
-	It("selects the new target primary right away", func() {
-		ctx := context.TODO()
+	It("selects the new target primary right away", func(ctx SpecContext) {
 		namespace := newFakeNamespace(env.client)
 		cluster := newFakeCNPGCluster(env.client, namespace)
 
@@ -132,8 +130,7 @@ var _ = Describe("Updating target primary", func() {
 		})
 	})
 
-	It("it should wait the failover delay to select the new target primary", func() {
-		ctx := context.TODO()
+	It("it should wait the failover delay to select the new target primary", func(ctx SpecContext) {
 		namespace := newFakeNamespace(env.client)
 		cluster := newFakeCNPGCluster(env.client, namespace, func(cluster *apiv1.Cluster) {
 			cluster.Spec.FailoverDelay = 2
@@ -210,8 +207,7 @@ var _ = Describe("Updating target primary", func() {
 		})
 	})
 
-	It("Issue #1783: ensure that the scale-down behaviour remain consistent", func() {
-		ctx := context.TODO()
+	It("Issue #1783: ensure that the scale-down behaviour remain consistent", func(ctx SpecContext) {
 		namespace := newFakeNamespace(env.client)
 		cluster := newFakeCNPGCluster(env.client, namespace, func(cluster *apiv1.Cluster) {
 			cluster.Spec.Instances = 2

internal/controller/cluster_create_test.go

@@ -822,14 +822,12 @@ var _ = Describe("createOrPatchClusterCredentialSecret", func() {
 		namespace  = "test-namespace"
 	)
 	var (
-		ctx      context.Context
 		proposed *corev1.Secret
 		cli      k8client.Client
 	)
 
 	BeforeEach(func() {
 		cli = fake.NewClientBuilder().WithScheme(schemeBuilder.BuildWithAllKnownScheme()).Build()
-		ctx = context.TODO()
 		const secretName = "test-secret"
 		proposed = &corev1.Secret{
 			ObjectMeta: metav1.ObjectMeta{
@@ -843,7 +841,7 @@ var _ = Describe("createOrPatchClusterCredentialSecret", func() {
 	})
 
 	Context("when the secret does not exist", func() {
-		It("should create the secret", func() {
+		It("should create the secret", func(ctx SpecContext) {
 			err := createOrPatchClusterCredentialSecret(ctx, cli, proposed)
 			Expect(err).NotTo(HaveOccurred())
 
@@ -857,7 +855,7 @@ var _ = Describe("createOrPatchClusterCredentialSecret", func() {
 	})
 
 	Context("when the secret exists and is owned by the cluster", func() {
-		BeforeEach(func() {
+		BeforeEach(func(ctx SpecContext) {
 			existingSecret := &corev1.Secret{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:        secretName,
@@ -878,7 +876,7 @@ var _ = Describe("createOrPatchClusterCredentialSecret", func() {
 			Expect(cli.Create(ctx, existingSecret)).To(Succeed())
 		})
 
-		It("should patch the secret if metadata differs", func() {
+		It("should patch the secret if metadata differs", func(ctx SpecContext) {
 			Expect(proposed.Labels).To(HaveKeyWithValue("test", "label"))
 			Expect(proposed.Annotations).To(HaveKeyWithValue("test", "annotation"))
 
@@ -892,7 +890,7 @@ var _ = Describe("createOrPatchClusterCredentialSecret", func() {
 			Expect(patchedSecret.Annotations).To(HaveKeyWithValue("test", "annotation"))
 		})
 
-		It("should not patch the secret if metadata is the same", func() {
+		It("should not patch the secret if metadata is the same", func(ctx SpecContext) {
 			var originalSecret corev1.Secret
 			err := cli.Get(ctx, types.NamespacedName{Name: secretName, Namespace: namespace}, &originalSecret)
 			Expect(err).NotTo(HaveOccurred())
@@ -913,7 +911,7 @@ var _ = Describe("createOrPatchClusterCredentialSecret", func() {
 	})
 
 	Context("when the secret exists but is not owned by the cluster", func() {
-		BeforeEach(func() {
+		BeforeEach(func(ctx SpecContext) {
 			existingSecret := &corev1.Secret{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      secretName,
@@ -923,7 +921,7 @@ var _ = Describe("createOrPatchClusterCredentialSecret", func() {
 			Expect(cli.Create(ctx, existingSecret)).To(Succeed())
 		})
 
-		It("should not modify the secret", func() {
+		It("should not modify the secret", func(ctx SpecContext) {
 			var originalSecret corev1.Secret
 			err := cli.Get(ctx, types.NamespacedName{Name: secretName, Namespace: namespace}, &originalSecret)
 			Expect(err).NotTo(HaveOccurred())