From cbf94307ef3c25cb0497923bb1af6356c9c20ff1 Mon Sep 17 00:00:00 2001
From: Patrick Ohly <patrick.ohly@intel.com>
Date: Wed, 24 Aug 2022 08:44:26 +0200
Subject: [PATCH] e2e framework: clean up instance after all other code ran

In contrast to ginkgo.AfterEach, ginkgo.DeferCleanup runs the callback in
first-in-last-out order. Using it makes the following test code work as
expected:

    f := framework.NewDefaultFramework("some test")

    ginkgo.AfterEach(func() {
        // do something with f.ClientSet
    })

Previously, f.ClientSet was already set to nil by the framework's cleanup code.
---
 test/e2e/framework/framework.go | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/test/e2e/framework/framework.go b/test/e2e/framework/framework.go
index d21ce41af75..d85a3664757 100644
--- a/test/e2e/framework/framework.go
+++ b/test/e2e/framework/framework.go
@@ -108,9 +108,6 @@ type Framework struct {
 	// the various afterEaches
 	afterEaches map[string]AfterEachActionFunc
 
-	// beforeEachStarted indicates that BeforeEach has started
-	beforeEachStarted bool
-
 	// configuration for framework's client
 	Options Options
 
@@ -149,8 +146,10 @@ func NewFrameworkWithCustomTimeouts(baseName string, timeouts *TimeoutContext) *
 	return f
 }
 
-// NewDefaultFramework makes a new framework and sets up a BeforeEach/AfterEach for
-// you (you can write additional before/after each functions).
+// NewDefaultFramework makes a new framework and sets up a BeforeEach which
+// initializes the framework instance. It cleans up with a DeferCleanup,
+// which runs last, so a AfterEach in the test still has a valid framework
+// instance.
 func NewDefaultFramework(baseName string) *Framework {
 	options := Options{
 		ClientQPS:   20,
@@ -184,14 +183,18 @@ func NewFramework(baseName string, options Options, client clientset.Interface)
 	})
 
 	ginkgo.BeforeEach(f.BeforeEach)
-	ginkgo.AfterEach(f.AfterEach)
 
 	return f
 }
 
 // BeforeEach gets a client and makes a namespace.
 func (f *Framework) BeforeEach() {
-	f.beforeEachStarted = true
+	// DeferCleanup, in constrast to AfterEach, triggers execution in
+	// first-in-last-out order. This ensures that the framework instance
+	// remains valid as long as possible.
+	//
+	// In addition, AfterEach will not be called if a test never gets here.
+	ginkgo.DeferCleanup(f.AfterEach)
 
 	// The fact that we need this feels like a bug in ginkgo.
 	// https://github.com/onsi/ginkgo/v2/issues/222
@@ -369,12 +372,6 @@ func (f *Framework) AddAfterEach(name string, fn AfterEachActionFunc) {
 
 // AfterEach deletes the namespace, after reading its events.
 func (f *Framework) AfterEach() {
-	// If BeforeEach never started AfterEach should be skipped.
-	// Currently some tests under e2e/storage have this condition.
-	if !f.beforeEachStarted {
-		return
-	}
-
 	RemoveCleanupAction(f.cleanupHandle)
 
 	// This should not happen. Given ClientSet is a public field a test must have updated it!