refactor(e2e): make the teardown steps a type

registerTeardown took a cleanup func and an onPanic func, both injected only so
the test could watch them. The steps are a type now, the list is literal at the
call site, and register takes one argument.The ordering test registers inside a subtest, so it exercises testing's own
LIFO rather than a stand-in. The panic test still needs a stub: register
reports through assert.Fail, and a failing subtest fails its parent, so a real
one could not assert that the delete still ran.

Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>

Author: vyncint

e2e/internal/harness/harness.go

@@ -177,14 +177,12 @@ func (b *Builder) Start() *Env {
 		dumpNamespaces:   dumpNamespaces(b.cfg),
 	}
 
-	registerTeardown(t.Cleanup,
-		func(step string, v any) { assert.Fail(t, fmt.Sprintf("teardown step %q panicked: %v", step, v)) },
-		teardownSteps(
-			env.collectArtifacts,
-			func() { assert.NoError(t, c.Cleanup()) },
-			func() { stopCluster(t, cancel, startErr) },
-			func() { assert.NoError(t, common.DeleteTestCluster(b.cfg.cluster)) },
-		))
+	teardown{
+		{"artifacts", env.collectArtifacts},
+		{"kubeconfig", func() { assert.NoError(t, c.Cleanup()) }},
+		{"stop", func() { stopCluster(t, cancel, startErr) }},
+		{"delete", func() { assert.NoError(t, common.DeleteTestCluster(b.cfg.cluster)) }},
+	}.register(t)
 
 	setup.LoggingOperator(t, c, setup.LoggingOperatorOptionFunc(func(o *setup.LoggingOperatorOptions) {
 		o.Namespace = b.cfg.controlNamespace
@@ -303,35 +301,30 @@ func (p *pending) String() string {
 	return p.name
 }
 
-type teardownStep struct {
+type step struct {
 	name string
 	run  func()
 }
 
-// teardownSteps lists them in the order they have to run. This order is what
-// kept clusters from leaking before it moved here, so it is one list rather
-// than four Cleanup calls in reverse.
-func teardownSteps(artifacts, kubeconfig, stop, deleteCluster func()) []teardownStep {
-	return []teardownStep{
-		{"artifacts", artifacts},
-		{"kubeconfig", kubeconfig},
-		{"stop", stop},
-		{"delete", deleteCluster},
-	}
+type teardown []step
+
+// cleanupT is the part of testing.T register needs.
+type cleanupT interface {
+	Cleanup(func())
+	Errorf(format string, args ...any)
 }
 
-// registerTeardown hands the steps over back to front, because Cleanup is LIFO,
-// and isolates each one: Go abandons the remaining cleanups at the first panic,
-// which would strand the cluster.
-func registerTeardown(cleanup func(func()), onPanic func(step string, v any), steps []teardownStep) {
-	for _, step := range slices.Backward(steps) {
-		cleanup(func() {
+// register goes back to front because Cleanup is LIFO, and isolates each step:
+// Go abandons the rest at the first panic, which would strand the cluster.
+func (td teardown) register(t cleanupT) {
+	for _, s := range slices.Backward(td) {
+		t.Cleanup(func() {
 			defer func() {
 				if v := recover(); v != nil {
-					onPanic(step.name, v)
+					assert.Fail(t, fmt.Sprintf("teardown step %q panicked: %v", s.name, v))
 				}
 			}()
-			step.run()
+			s.run()
 		})
 	}
 }

e2e/internal/harness/harness_test.go

@@ -16,6 +16,7 @@ package harness
 
 import (
 	"errors"
+	"fmt"
 	"os"
 	"path/filepath"
 	"slices"
@@ -32,59 +33,53 @@ import (
 	"github.com/kube-logging/logging-operator/pkg/sdk/logging/api/v1beta1"
 )
 
-// cleanupRecorder stands in for testing.T's Cleanup, so the order the steps end
-// up running in can be asserted without a cluster.
-type cleanupRecorder struct {
-	registered []func()
-}
+// Registered in a subtest so the order comes from testing's own LIFO rather
+// than a stand-in for it.
+func TestTeardownRunsInDeclaredOrder(t *testing.T) {
+	var ran []string
+	record := func(name string) step { return step{name, func() { ran = append(ran, name) }} }
 
-func (c *cleanupRecorder) Cleanup(fn func()) {
-	c.registered = append(c.registered, fn)
-}
+	t.Run("teardown", func(t *testing.T) {
+		teardown{record("artifacts"), record("kubeconfig"), record("stop"), record("delete")}.register(t)
+	})
 
-// run does what testing does at the end of a test: LIFO.
-func (c *cleanupRecorder) run() {
-	for _, fn := range slices.Backward(c.registered) {
-		fn()
-	}
+	assert.Equal(t, []string{"artifacts", "kubeconfig", "stop", "delete"}, ran)
 }
 
-// The order here is what kept clusters from leaking: the log dump needs the
-// cache and the kubeconfig, both of which the later steps take away.
-func TestTeardownRunsInDeclaredOrder(t *testing.T) {
-	var ran []string
-	record := func(name string) func() { return func() { ran = append(ran, name) } }
+// A real subtest cannot be used here: register reports the panic, which would
+// fail the subtest and this test with it.
+type reportingT struct {
+	cleanups []func()
+	reported []string
+}
 
-	recorder := &cleanupRecorder{}
-	registerTeardown(recorder.Cleanup, failOnPanic(t), teardownSteps(
-		record("artifacts"), record("kubeconfig"), record("stop"), record("delete"),
-	))
-	recorder.run()
+func (r *reportingT) Cleanup(fn func()) { r.cleanups = append(r.cleanups, fn) }
+func (r *reportingT) Errorf(format string, args ...any) {
+	r.reported = append(r.reported, fmt.Sprintf(format, args...))
+}
 
-	assert.Equal(t, []string{"artifacts", "kubeconfig", "stop", "delete"}, ran)
+func (r *reportingT) runCleanups() {
+	for _, fn := range slices.Backward(r.cleanups) {
+		fn()
+	}
 }
 
 // Go abandons the remaining cleanups at the first panic, so without the recover
-// in registerTeardown a panicking log dump would leave the cluster running.
+// a panicking log dump would leave the cluster running.
 func TestTeardownDeletesTheClusterAfterAnEarlierPanic(t *testing.T) {
-	var ran, panicked []string
-	record := func(name string) func() { return func() { ran = append(ran, name) } }
-
-	recorder := &cleanupRecorder{}
-	registerTeardown(recorder.Cleanup,
-		func(step string, _ any) { panicked = append(panicked, step) },
-		teardownSteps(
-			func() { ran = append(ran, "artifacts"); panic("boom") },
-			record("kubeconfig"), record("stop"), record("delete"),
-		))
-	recorder.run()
+	var ran []string
+	record := func(name string) step { return step{name, func() { ran = append(ran, name) }} }
 
-	assert.Equal(t, []string{"artifacts", "kubeconfig", "stop", "delete"}, ran)
-	assert.Equal(t, []string{"artifacts"}, panicked, "the panic is reported, not swallowed")
-}
+	recorder := &reportingT{}
+	teardown{
+		{"artifacts", func() { ran = append(ran, "artifacts"); panic("boom") }},
+		record("kubeconfig"), record("stop"), record("delete"),
+	}.register(recorder)
+	recorder.runCleanups()
 
-func failOnPanic(t *testing.T) func(string, any) {
-	return func(step string, v any) { assert.Fail(t, "unexpected panic", "%s: %v", step, v) }
+	assert.Equal(t, []string{"artifacts", "kubeconfig", "stop", "delete"}, ran)
+	assert.Len(t, recorder.reported, 1, "the panic is reported, not swallowed")
+	assert.Contains(t, recorder.reported[0], `"artifacts" panicked`)
 }
 
 func TestBuildScheme(t *testing.T) {
@@ -133,8 +128,6 @@ func TestDumpNamespaces(t *testing.T) {
 			config: config{controlNamespace: "infra", namespaces: []string{"tenant"}},
 			want:   []string{"infra", "tenant", "default"},
 		},
-		// A suite that runs in default, or names it explicitly, would otherwise
-		// have stern dump it twice.
 		"repeats are dropped": {
 			config: config{controlNamespace: "default", namespaces: []string{"tenant", "tenant"}},
 			want:   []string{"default", "tenant"},
@@ -152,12 +145,8 @@ func TestDumpNamespaces(t *testing.T) {
 	}
 }
 
-// The cap is what the suites spend by hand today; the deadline is what stops
-// one wait taking the whole package budget with it.
 func TestWaitBudgetStaysUnderTheCapAndTheDeadline(t *testing.T) {
-	env := &Env{T: t}
-
-	budget := env.waitBudget()
+	budget := (&Env{T: t}).waitBudget()
 
 	assert.Positive(t, budget)
 	assert.LessOrEqual(t, budget, waitBudget)
@@ -174,7 +163,6 @@ func TestArtifactPath(t *testing.T) {
 	require.NoError(t, err)
 
 	assert.Equal(t, filepath.Join(root, "build/_test", "cluster-TestSomething.log"), path)
-	// The directory each suite used to build in its own init().
 	info, err := os.Stat(filepath.Dir(path))
 	require.NoError(t, err)
 	assert.True(t, info.IsDir())