From 3a44c1b1da7f3e45fbcccf994f98b811d227e2db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Charles-Edouard=20Br=C3=A9t=C3=A9ch=C3=A9?= Date: Wed, 9 Nov 2022 16:09:26 +0100 Subject: [PATCH] refactor: use t.Cleanup instead of defer (#402) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Using `testing.T.Cleanup` instead of `defer` where appropriate Co-authored-by: Ken Sipe Signed-off-by: Charles-Edouard Brétéché --- pkg/env/env_test.go | 4 +-- pkg/test/case.go | 43 ++++++++++++++-------------- pkg/test/case_integration_test.go | 16 +++++++++-- pkg/test/harness.go | 2 +- pkg/test/harness_integration_test.go | 2 +- pkg/test/kind_integration_test.go | 4 +-- pkg/test/step_integration_test.go | 8 +++--- 7 files changed, 45 insertions(+), 34 deletions(-) diff --git a/pkg/env/env_test.go b/pkg/env/env_test.go index 5b62c0e1..5f489d53 100644 --- a/pkg/env/env_test.go +++ b/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", })) diff --git a/pkg/test/case.go b/pkg/test/case.go index 93330d80..e0a879c1 100644 --- a/pkg/test/case.go +++ b/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) diff --git a/pkg/test/case_integration_test.go b/pkg/test/case_integration_test.go index 231f49f7..8d9e1067 100644 --- a/pkg/test/case_integration_test.go +++ b/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 diff --git a/pkg/test/harness.go b/pkg/test/harness.go index f0d610dc..3d369e71 100644 --- a/pkg/test/harness.go +++ b/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() diff --git a/pkg/test/harness_integration_test.go b/pkg/test/harness_integration_test.go index 5f66838f..341f7b44 100644 --- a/pkg/test/harness_integration_test.go +++ b/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)) diff --git a/pkg/test/kind_integration_test.go b/pkg/test/kind_integration_test.go index 18b95b2d..ca1525db 100644 --- a/pkg/test/kind_integration_test.go +++ b/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 { diff --git a/pkg/test/step_integration_test.go b/pkg/test/step_integration_test.go index 706742e0..acb83c84 100644 --- a/pkg/test/step_integration_test.go +++ b/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)