From bd34dad623173b74bd0dc59b97a8a3e6cf21578a Mon Sep 17 00:00:00 2001
From: justinsb <justinsb@google.com>
Date: Thu, 1 Feb 2024 22:38:13 -0500
Subject: [PATCH] tests: Fix shutdown ordering (again)

We need to pass the manager context to the mgr.Start method, not just
the mgr.New method.

Also there may have been some variable shadowing going on, and
possibly some unnecessary waiting on a channel.

Hopefully this will now address timeouts - I started hitting it
locally and this cleaned it up immediately, even with
--test.count=10

Also remove the signal handler in the test; it's not standard and it
breaks immediately with --test.count=10
---
 config/tests/samples/create/harness.go | 32 ++++----------------------
 tests/e2e/unified_test.go              |  3 +--
 2 files changed, 6 insertions(+), 29 deletions(-)

diff --git a/config/tests/samples/create/harness.go b/config/tests/samples/create/harness.go
index 2829ae44c6..e07c11233f 100644
--- a/config/tests/samples/create/harness.go
+++ b/config/tests/samples/create/harness.go
@@ -76,19 +76,6 @@ type Harness struct {
 	kccConfig      kccmanager.Config
 }
 
-// ForSubtest returns a harness scoped to a subtest (created by t.Run).
-func (h *Harness) ForSubtest(t *testing.T) *Harness {
-	ctx, cancel := context.WithCancel(h.Ctx)
-	t.Cleanup(func() {
-		cancel()
-	})
-
-	subHarness := *h
-	subHarness.T = t
-	subHarness.Ctx = ctx
-	return &subHarness
-}
-
 type httpRoundTripperKeyType int
 
 // httpRoundTripperKey is the key value for http.RoundTripper in a context.Context
@@ -106,9 +93,9 @@ func NewHarnessWithManager(t *testing.T, ctx context.Context, mgr manager.Manage
 }
 
 func NewHarness(t *testing.T, ctx context.Context) *Harness {
-	ctx, cancel := context.WithCancel(ctx)
+	ctx, ctxCancel := context.WithCancel(ctx)
 	t.Cleanup(func() {
-		cancel()
+		ctxCancel()
 	})
 
 	log := log.FromContext(ctx)
@@ -339,9 +326,9 @@ func NewHarness(t *testing.T, ctx context.Context) *Harness {
 	// Create a context specifically for this, and register the test cleanup function
 	// after the envtest cleanup function (these run last-in, first-out).
 	// See https://github.com/kubernetes-sigs/controller-runtime/issues/1571#issuecomment-945535598
-	mgrContext, cancel := context.WithCancel(ctx)
+	mgrContext, mgrContextCancel := context.WithCancel(ctx)
 	t.Cleanup(func() {
-		cancel()
+		mgrContextCancel()
 	})
 	mgr, err := kccmanager.New(mgrContext, h.restConfig, kccConfig)
 	if err != nil {
@@ -360,22 +347,13 @@ func NewHarness(t *testing.T, ctx context.Context) *Harness {
 		t.Fatalf("error adding registration controller for deletion defender controllers: %v", err)
 	}
 	// Start the manager, Start(...) is a blocking operation so it needs to be done asynchronously.
-	errors := make(chan error)
 	go func() {
-		err := mgr.Start(ctx)
+		err := mgr.Start(mgrContext)
 		if err != nil {
 			t.Errorf("error from mgr.Start: %v", err)
 		}
-		errors <- err
 	}()
 
-	t.Cleanup(func() {
-		cancel() // because cleanups run last-in-first-out, we need to cancel again
-		if err := <-errors; err != nil {
-			t.Errorf("error from mgr.Start: %v", err)
-		}
-	})
-
 	return h
 }
 
diff --git a/tests/e2e/unified_test.go b/tests/e2e/unified_test.go
index 564b9b6191..4ad140ed0d 100644
--- a/tests/e2e/unified_test.go
+++ b/tests/e2e/unified_test.go
@@ -31,7 +31,6 @@ import (
 
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"sigs.k8s.io/controller-runtime/pkg/manager/signals"
 )
 
 func TestAllInSeries(t *testing.T) {
@@ -39,7 +38,7 @@ func TestAllInSeries(t *testing.T) {
 		t.Skip("RUN_E2E not set; skipping")
 	}
 
-	ctx := signals.SetupSignalHandler()
+	ctx := context.Background()
 	ctx, cancel := context.WithCancel(ctx)
 	t.Cleanup(func() {
 		cancel()