diff --git a/pkg/env/env.go b/pkg/env/env.go index 3b1e4838..103ec5c5 100644 --- a/pkg/env/env.go +++ b/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,13 +291,13 @@ 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 } } @@ -287,7 +305,7 @@ func (e *testEnv) processTests(ctx context.Context, t *testing.T, enableParallel 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()) diff --git a/pkg/env/env_test.go b/pkg/env/env_test.go index d0a024d3..4e4d7aee 100644 --- a/pkg/env/env_test.go +++ b/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) { diff --git a/pkg/envconf/config.go b/pkg/envconf/config.go index f2267ae2..b3edf66d 100644 --- a/pkg/envconf/config.go +++ b/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