refactor: use t.Cleanup instead of defer (#402)

Using `testing.T.Cleanup` instead of `defer` where appropriate

Co-authored-by: Ken Sipe <kensipe@gmail.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>

pkg/env/env_test.go

@@ -9,9 +9,9 @@ import (
 
 func TestExpandWithMap(t *testing.T) {
 	os.Setenv("KUTTL_TEST_123", "hello")
-	defer func() {
+	t.Cleanup(func() {
 		os.Unsetenv("KUTTL_TEST_123")
-	}()
+	})
 	assert.Equal(t, "hello $  world", ExpandWithMap("$KUTTL_TEST_123 $$ $DOES_NOT_EXIST_1234 ${EXPAND_ME}", map[string]string{
 		"EXPAND_ME": "world",
 	}))

pkg/test/case.go

@@ -80,7 +80,7 @@ func (t *Case) DeleteNamespace(cl client.Client, ns *namespace) error {
 }
 
 // CreateNamespace creates a namespace in Kubernetes to use for a test.
-func (t *Case) CreateNamespace(cl client.Client, ns *namespace) error {
+func (t *Case) CreateNamespace(test *testing.T, cl client.Client, ns *namespace) error {
 	if !ns.AutoCreated {
 		t.Logger.Log("Skipping creation of user-supplied namespace:", ns.Name)
 		return nil
@@ -94,6 +94,14 @@ func (t *Case) CreateNamespace(cl client.Client, ns *namespace) error {
 		defer cancel()
 	}
 
+	if !t.SkipDelete {
+		test.Cleanup(func() {
+			if err := t.DeleteNamespace(cl, ns); err != nil {
+				test.Error(err)
+			}
+		})
+	}
+
 	return cl.Create(ctx, &corev1.Namespace{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: ns.Name,
@@ -290,6 +298,17 @@ func shortString(obj *corev1.ObjectReference) string {
 		fieldRef)
 }
 
+func runStep(test *testing.T, testCase *Case, testStep *Step, ns *namespace) []error {
+	if !testCase.SkipDelete {
+		test.Cleanup(func() {
+			if err := testStep.Clean(ns.Name); err != nil {
+				test.Error(err)
+			}
+		})
+	}
+	return testStep.Run(ns.Name)
+}
+
 // Run runs a test case including all of its steps.
 func (t *Case) Run(test *testing.T, tc *report.Testcase) {
 	ns := t.determineNamespace()
@@ -317,22 +336,12 @@ func (t *Case) Run(test *testing.T, tc *report.Testcase) {
 	}
 
 	for _, c := range clients {
-		if err := t.CreateNamespace(c, ns); err != nil {
+		if err := t.CreateNamespace(test, c, ns); err != nil {
 			tc.Failure = report.NewFailure(err.Error(), nil)
 			test.Fatal(err)
 		}
 	}
 
-	if !t.SkipDelete {
-		defer func() {
-			for _, c := range clients {
-				if err := t.DeleteNamespace(c, ns); err != nil {
-					test.Error(err)
-				}
-			}
-		}()
-	}
-
 	for _, testStep := range t.Steps {
 		testStep.Client = t.Client
 		if testStep.Kubeconfig != "" {
@@ -346,15 +355,7 @@ func (t *Case) Run(test *testing.T, tc *report.Testcase) {
 		tc.Assertions += len(testStep.Asserts)
 		tc.Assertions += len(testStep.Errors)
 
-		if !t.SkipDelete {
-			defer func(step *Step) {
-				if err := step.Clean(ns.Name); err != nil {
-					test.Error(err)
-				}
-			}(testStep)
-		}
-
-		if errs := testStep.Run(ns.Name); len(errs) > 0 {
+		if errs := runStep(test, t, testStep, ns); len(errs) > 0 {
 			caseErr := fmt.Errorf("failed in step %s", testStep.String())
 			tc.Failure = report.NewFailure(caseErr.Error(), errs)
 

pkg/test/case_integration_test.go

@@ -21,14 +21,22 @@ func TestMultiClusterCase(t *testing.T) {
 		t.Error(err)
 		return
 	}
-	defer testenv.Environment.Stop()
+	t.Cleanup(func() {
+		if err := testenv.Environment.Stop(); err != nil {
+			t.Error(err)
+		}
+	})
 
 	testenv2, err := testutils.StartTestEnvironment(testutils.APIServerDefaultArgs, false)
 	if err != nil {
 		t.Error(err)
 		return
 	}
-	defer testenv2.Environment.Stop()
+	t.Cleanup(func() {
+		if err := testenv2.Environment.Stop(); err != nil {
+			t.Error(err)
+		}
+	})
 
 	podSpec := map[string]interface{}{
 		"restartPolicy": "Never",
@@ -45,7 +53,9 @@ func TestMultiClusterCase(t *testing.T) {
 		t.Error(err)
 		return
 	}
-	defer os.Remove(tmpfile.Name())
+	t.Cleanup(func() {
+		os.Remove(tmpfile.Name())
+	})
 	if err := testutils.Kubeconfig(testenv2.Config, tmpfile); err != nil {
 		t.Error(err)
 		return

pkg/test/harness.go

@@ -351,7 +351,7 @@ func (h *Harness) DockerClient() (testutils.DockerClient, error) {
 // tests at dir.
 func (h *Harness) RunTests() {
 	// cleanup after running tests
-	defer h.Stop()
+	h.T.Cleanup(h.Stop)
 	h.T.Log("running tests")
 
 	testDirs := h.testPreProcessing()

pkg/test/harness_integration_test.go

@@ -63,7 +63,7 @@ func TestRunBackgroundCommands(t *testing.T) {
 	h.TestSuite.Commands = commands
 
 	h.Setup()
-	defer h.Stop()
+	t.Cleanup(h.Stop)
 
 	// setup creates bg processes
 	assert.Equal(t, 1, len(h.bgProcesses))

pkg/test/kind_integration_test.go

@@ -36,11 +36,11 @@ func TestAddContainers(t *testing.T) {
 		t.Fatalf("failed to start KIND cluster: %v", err)
 	}
 
-	defer func() {
+	t.Cleanup(func() {
 		if err := kind.Stop(); err != nil {
 			t.Fatalf("failed to stop KIND cluster: %v", err)
 		}
-	}()
+	})
 
 	docker, err := dockerClient.NewClientWithOpts(dockerClient.FromEnv)
 	if err != nil {

pkg/test/step_integration_test.go

@@ -347,9 +347,9 @@ func TestCheckedTypeAssertions(t *testing.T) {
 
 func TestApplyExpansion(t *testing.T) {
 	os.Setenv("TEST_FOO", "test")
-	defer func() {
+	t.Cleanup(func() {
 		os.Unsetenv("TEST_FOO")
-	}()
+	})
 
 	step := Step{Dir: "step_integration_test_data/assert_expand/"}
 	path := "step_integration_test_data/assert_expand/00-step1.yaml"
@@ -361,9 +361,9 @@ func TestApplyExpansion(t *testing.T) {
 
 func TestOverriddenKubeconfigPathResolution(t *testing.T) {
 	os.Setenv("SUBPATH", "test")
-	defer func() {
+	t.Cleanup(func() {
 		os.Unsetenv("SUBPATH")
-	}()
+	})
 	stepRelativePath := &Step{Dir: "step_integration_test_data/kubeconfig_path_resolution/"}
 	err := stepRelativePath.LoadYAML("step_integration_test_data/kubeconfig_path_resolution/00-step1.yaml")
 	assert.NoError(t, err)