From 3c162af45fa6a874b510015534835b74c30cc60b Mon Sep 17 00:00:00 2001
From: Patrick Ohly <patrick.ohly@intel.com>
Date: Mon, 17 Oct 2022 10:27:14 +0200
Subject: [PATCH] e2e: skip AllNodesReady when the test skipped framework
 initialization

This addresses a problem caused by
https://github.com/kubernetes/kubernetes/pull/112043: because the AfterEach
which invokes AllNodesReady always runs, including tests that skipped early,
those tests ran into a nil pointer access. This increased the size of log
files. The tests still worked.
---
 test/e2e/framework/framework.go      | 15 +++++++++++++--
 test/e2e/framework/node/init/init.go |  6 ++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/test/e2e/framework/framework.go b/test/e2e/framework/framework.go
index e214f7b2117..3f64ab39129 100644
--- a/test/e2e/framework/framework.go
+++ b/test/e2e/framework/framework.go
@@ -66,12 +66,21 @@ var (
 	// with gingko.BeforeEach/AfterEach/DeferCleanup.
 	//
 	// When a test runs, functions will be invoked in this order:
+	// - BeforeEaches defined by tests before f.NewDefaultFramework
+	//   in the order in which they were defined (first-in-first-out)
 	// - f.BeforeEach
-	// - all BeforeEaches in the order in which they were defined (first-in-first-out)
+	// - BeforeEaches defined by tests after f.NewDefaultFramework
 	// - It callback
 	// - all AfterEaches in the order in which they were defined
 	// - all DeferCleanups with the order reversed (first-in-last-out)
 	// - f.AfterEach
+	//
+	// Because a test might skip test execution in a BeforeEach that runs
+	// before f.BeforeEach, AfterEach callbacks that depend on the
+	// framework instance must check whether it was initialized. They can
+	// do that by checking f.ClientSet for nil. DeferCleanup callbacks
+	// don't need to do this because they get defined when the test
+	// runs.
 	NewFrameworkExtensions []func(f *Framework)
 )
 
@@ -344,7 +353,9 @@ func (f *Framework) AfterEach() {
 			}
 		}
 
-		// Paranoia-- prevent reuse!
+		// Unsetting this is relevant for a following test that uses
+		// the same instance because it might not reach f.BeforeEach
+		// when some other BeforeEach skips the test first.
 		f.Namespace = nil
 		f.clientConfig = nil
 		f.ClientSet = nil
diff --git a/test/e2e/framework/node/init/init.go b/test/e2e/framework/node/init/init.go
index b582283b756..f5fc28aef4e 100644
--- a/test/e2e/framework/node/init/init.go
+++ b/test/e2e/framework/node/init/init.go
@@ -30,6 +30,12 @@ func init() {
 	framework.NewFrameworkExtensions = append(framework.NewFrameworkExtensions,
 		func(f *framework.Framework) {
 			ginkgo.AfterEach(func() {
+				if f.ClientSet == nil {
+					// Test didn't reach f.BeforeEach, most
+					// likely because the test got
+					// skipped. Nothing to check...
+					return
+				}
 				e2enode.AllNodesReady(f.ClientSet, 3*time.Minute)
 			})
 		},