[env] Do not share config between tests

pkg/env/env.go

@@ -29,6 +29,7 @@ import (
 
 	klog "k8s.io/klog/v2"
 
+	"sigs.k8s.io/e2e-framework/klient"
 	"sigs.k8s.io/e2e-framework/pkg/envconf"
 	"sigs.k8s.io/e2e-framework/pkg/featuregate"
 	"sigs.k8s.io/e2e-framework/pkg/features"
@@ -116,6 +117,21 @@ func newTestEnvWithParallel() *testEnv {
 	}
 }
 
+type ctxName string
+
+// newChildTestEnv returns a child testEnv based on the one passed as an argument.
+// The child env inherits the context and actions from the parent and
+// creates a deep copy of the config so that it can be mutated without
+// affecting the parent's.
+func newChildTestEnv(e *testEnv) *testEnv {
+	childCtx := context.WithValue(e.ctx, ctxName("parent"), fmt.Sprintf("%s", e.ctx))
+	return &testEnv{
+		ctx:     childCtx,
+		cfg:     e.deepCopyConfig(),
+		actions: append([]action{}, e.actions...),
+	}
+}
+
 // WithContext returns a new environment with the context set to ctx.
 // Argument ctx cannot be nil
 func (e *testEnv) WithContext(ctx context.Context) types.Environment {
@@ -241,7 +257,8 @@ func (e *testEnv) processFeatureActions(ctx context.Context, t *testing.T, featu
 // In case if the parallel run of test features are enabled, this function will invoke the processTestFeature
 // as a go-routine to get them to run in parallel
 func (e *testEnv) processTests(ctx context.Context, t *testing.T, enableParallelRun bool, testFeatures ...types.Feature) context.Context {
-	if e.cfg.DryRunMode() {
+	dedicatedTestEnv := newChildTestEnv(e)
+	if dedicatedTestEnv.cfg.DryRunMode() {
 		klog.V(2).Info("e2e-framework is being run in dry-run mode. This will skip all the before/after step functions configured around your test assessments and features")
 	}
 	if ctx == nil {
@@ -251,19 +268,20 @@ func (e *testEnv) processTests(ctx context.Context, t *testing.T, enableParallel
 		t.Log("No test testFeatures provided, skipping test")
 		return ctx
 	}
-	beforeTestActions := e.getBeforeTestActions()
-	afterTestActions := e.getAfterTestActions()
+	beforeTestActions := dedicatedTestEnv.getBeforeTestActions()
+	afterTestActions := dedicatedTestEnv.getAfterTestActions()
 
-	runInParallel := e.cfg.ParallelTestEnabled() && enableParallelRun
+	runInParallel := dedicatedTestEnv.cfg.ParallelTestEnabled() && enableParallelRun
 
 	if runInParallel {
 		klog.V(4).Info("Running test features in parallel")
 	}
 
-	ctx = e.processTestActions(ctx, t, beforeTestActions)
+	ctx = dedicatedTestEnv.processTestActions(ctx, t, beforeTestActions)
 
 	var wg sync.WaitGroup
 	for i, feature := range testFeatures {
+		featureTestEnv := newChildTestEnv(dedicatedTestEnv)
 		featureCopy := feature
 		featName := feature.Name()
 		if featName == "" {
@@ -273,21 +291,21 @@ func (e *testEnv) processTests(ctx context.Context, t *testing.T, enableParallel
 			wg.Add(1)
 			go func(ctx context.Context, w *sync.WaitGroup, featName string, f types.Feature) {
 				defer w.Done()
-				_ = e.processTestFeature(ctx, t, featName, f)
+				_ = featureTestEnv.processTestFeature(ctx, t, featName, f)
 			}(ctx, &wg, featName, featureCopy)
 		} else {
-			ctx = e.processTestFeature(ctx, t, featName, featureCopy)
+			ctx = featureTestEnv.processTestFeature(ctx, t, featName, featureCopy)
 			// In case if the feature under test has failed, skip reset of the features
 			// that are part of the same test
-			if e.cfg.FailFast() && t.Failed() {
+			if featureTestEnv.cfg.FailFast() && t.Failed() {
 				break
 			}
 		}
 	}
 	if runInParallel {
 		wg.Wait()
 	}
-	return e.processTestActions(ctx, t, afterTestActions)
+	return dedicatedTestEnv.processTestActions(ctx, t, afterTestActions)
 }
 
 // TestInParallel executes a series a feature tests from within a
@@ -572,7 +590,54 @@ func (e *testEnv) requireProcessing(kind, testName string, requiredRegexp, skipR
 	return skip, message
 }
 
-// deepCopyFeature just copies the values from the Feature but creates a deep
+// deepCopyConfig just copies the values from the Config to create a deep
+// copy to avoid mutation when we just want an informational copy.
+func (e *testEnv) deepCopyConfig() *envconf.Config {
+	// Basic copy which takes care of all the basic types (str, bool...)
+	configCopy := *e.cfg
+
+	// Manually setting fields that are struct types
+	if client := e.cfg.GetClient(); client != nil {
+		// Need to recreate the underlying client because client.Resource is not thread safe
+		// Panic on error because this should never happen since the client was built once already
+		clientCopy, err := klient.New(client.RESTConfig())
+		if err != nil {
+			panic(err)
+		}
+		configCopy.WithClient(clientCopy)
+	}
+	if e.cfg.AssessmentRegex() != nil {
+		configCopy.WithAssessmentRegex(e.cfg.AssessmentRegex().String())
+	}
+	if e.cfg.FeatureRegex() != nil {
+		configCopy.WithFeatureRegex(e.cfg.FeatureRegex().String())
+	}
+	if e.cfg.SkipAssessmentRegex() != nil {
+		configCopy.WithSkipAssessmentRegex(e.cfg.SkipAssessmentRegex().String())
+	}
+	if e.cfg.SkipFeatureRegex() != nil {
+		configCopy.WithSkipFeatureRegex(e.cfg.SkipFeatureRegex().String())
+	}
+
+	labels := make(map[string][]string, len(e.cfg.Labels()))
+	for k, vals := range e.cfg.Labels() {
+		copyVals := make([]string, len(vals))
+		copyVals = append(copyVals, vals...)
+		labels[k] = copyVals
+	}
+	configCopy.WithLabels(labels)
+
+	skipLabels := make(map[string][]string, len(e.cfg.SkipLabels()))
+	for k, vals := range e.cfg.SkipLabels() {
+		copyVals := make([]string, len(vals))
+		copyVals = append(copyVals, vals...)
+		skipLabels[k] = copyVals
+	}
+	configCopy.WithSkipLabels(e.cfg.SkipLabels())
+	return &configCopy
+}
+
+// deepCopyFeature just copies the values from the Feature to create a deep
 // copy to avoid mutation when we just want an informational copy.
 func deepCopyFeature(f types.Feature) types.Feature {
 	fcopy := features.New(f.Name())

pkg/env/env_test.go

@@ -726,6 +726,89 @@ func TestTestEnv_TestInParallel(t *testing.T) {
 	}
 }
 
+// Create a dedicated env that can be used to test the parallel execution of tests and features to make sure
+// they don't share the same config object but they inherit the one from the parent env.
+// Meaning that each test inherit the global testEnv and each feature inherit the testEnv of the test.
+// This env is used for the 3 tests below.
+var envTForChildCfgParallelTesting = NewWithConfig(envconf.New().WithParallelTestEnabled().WithNamespace("child-cfg"))
+
+func TestTestEnv_ChildCfgInParallel(t *testing.T) {
+	envTForChildCfgParallelTesting.BeforeEachTest(func(ctx context.Context, config *envconf.Config, t *testing.T) (context.Context, error) {
+		t.Logf("Running before each test for test %s", t.Name())
+		t.Parallel()
+
+		// Check that namespace was inherited from the global config
+		if config.Namespace() != "child-cfg" {
+			t.Errorf("Expected namespace to be %s but got %s", t.Name(), config.Namespace())
+		}
+		config.WithNamespace(t.Name())
+		return ctx, nil
+	})
+
+	envTForChildCfgParallelTesting.AfterEachTest(func(ctx context.Context, config *envconf.Config, t *testing.T) (context.Context, error) {
+		t.Logf("Running after each test for test %s", t.Name())
+		if config.Namespace() != t.Name() {
+			t.Errorf("Expected namespace to be %s but got %s", t.Name(), config.Namespace())
+		}
+		return ctx, nil
+	})
+
+	envTForChildCfgParallelTesting.BeforeEachFeature(func(ctx context.Context, config *envconf.Config, t *testing.T, feature types.Feature) (context.Context, error) {
+		t.Logf("Running before each feature for feature %s", feature.Name())
+
+		// Check that namespace was inherited from the test
+		if config.Namespace() != t.Name() {
+			t.Errorf("Namespace in feature was not inherited from the test. Expected %s but got %s", t.Name(), config.Namespace())
+		}
+		config.WithNamespace(feature.Name())
+		return ctx, nil
+	})
+
+	envTForChildCfgParallelTesting.AfterEachFeature(func(ctx context.Context, config *envconf.Config, t *testing.T, feature types.Feature) (context.Context, error) {
+		t.Logf("Running after each feature for feature %s", feature.Name())
+		if config.Namespace() != feature.Name() {
+			t.Errorf("Expected namespace in feature to be %s but got %s", feature.Name(), config.Namespace())
+		}
+		return ctx, nil
+	})
+}
+
+func TestTestEnv_ChildCfgInParallelOne(t *testing.T) {
+	// Here, each feature sleeps for a different amount of time. This is done to ensure that both features can
+	// run their BeforeEachFeature that modify the config.Namespace BEFORE they run their AfterEachFeature that
+	// checks the config.Namespace. If the deepcopy did not work, the config.Namespace from 1 feature would be
+	// overwritten by the other feature.
+	f1 := features.New("test-parallel-feature1").
+		Assess("sleep more", func(ctx context.Context, t *testing.T, config *envconf.Config) context.Context {
+			t.Log("sleeping more to be sure feature 2 finishes first")
+			time.Sleep(5 * time.Second)
+			return ctx
+		})
+
+	f2 := features.New("test-parallel-feature2").
+		Assess("sleep less", func(ctx context.Context, t *testing.T, config *envconf.Config) context.Context {
+			t.Log("sleeping less to be sure feature 1 starts")
+			time.Sleep(1 * time.Second)
+			return ctx
+		})
+
+	_ = envTForChildCfgParallelTesting.TestInParallel(t, f1.Feature(), f2.Feature())
+}
+
+func TestTestEnv_ChildCfgInParallelTwo(t *testing.T) {
+	// Here, the only feature just sleeps a bit to make sure both tests will have run their BeforeEachTest
+	// before either runs their AfterEachTest. If the deepcopy did not work, the config.Namespace from 1 test
+	// would be overwritten by the other test.
+	f1 := features.New("test-parallel-feature1").
+		Assess("sleep less", func(ctx context.Context, t *testing.T, config *envconf.Config) context.Context {
+			t.Log("sleeping less to be sure test 1 starts")
+			time.Sleep(1 * time.Second)
+			return ctx
+		})
+
+	_ = envTForChildCfgParallelTesting.Test(t, f1.Feature())
+}
+
 // TestTParallelMultipleFeaturesInParallel runs multple features in parallel with a dedicated Parallel environment,
 // just to check there are no race conditions with this setting
 func TestTParallelMultipleFeaturesInParallel(t *testing.T) {

pkg/envconf/config.go

@@ -107,14 +107,15 @@ func (c *Config) WithClient(client klient.Client) *Config {
 	return c
 }
 
+// GetClient returns the client for the environment
+func (c *Config) GetClient() klient.Client {
+	return c.client
+}
+
 // NewClient is a constructor function that returns a previously
 // created klient.Client or create a new one based on configuration
 // previously set. Will return an error if unable to do so.
 func (c *Config) NewClient() (klient.Client, error) {
-	if c.client != nil {
-		return c.client, nil
-	}
-
 	client, err := klient.NewWithKubeConfigFile(c.kubeconfig)
 	if err != nil {
 		return nil, fmt.Errorf("envconfig: client failed: %w", err)
@@ -130,16 +131,11 @@ func (c *Config) NewClient() (klient.Client, error) {
 // are confident in the configuration or call NewClient() to ensure its
 // safe creation.
 func (c *Config) Client() klient.Client {
-	if c.client != nil {
-		return c.client
-	}
-
-	client, err := klient.NewWithKubeConfigFile(c.kubeconfig)
+	client, err := c.NewClient()
 	if err != nil {
-		panic(fmt.Errorf("envconfig: client failed: %w", err).Error())
+		panic(err)
 	}
-	c.client = client
-	return c.client
+	return client
 }
 
 // WithNamespace updates the environment namespace value