diff --git a/pkg/env/action.go b/pkg/env/action.go index f1fdaafc..12488669 100644 --- a/pkg/env/action.go +++ b/pkg/env/action.go @@ -71,7 +71,7 @@ func (a *action) runWithT(ctx context.Context, cfg *envconf.Config, t *testing.T } // runWithFeature will run the action and inject a FeatureInfo object into the callback function. -func (a *action) runWithFeature(ctx context.Context, cfg *envconf.Config, fi types.FeatureInfo) (context.Context, error) { +func (a *action) runWithFeature(ctx context.Context, cfg *envconf.Config, fi types.Feature) (context.Context, error) { switch a.role { case roleBeforeFeature, roleAfterFeature: for _, f := range a.featureFuncs { diff --git a/pkg/env/env.go b/pkg/env/env.go index 04121b07..9723e4e7 100644 --- a/pkg/env/env.go +++ b/pkg/env/env.go @@ -192,7 +192,7 @@ func (e *testEnv) Test(t *testing.T, testFeatures ...types.Feature) { for _, feature := range testFeatures { // execute beforeFeature actions for _, action := range beforeFeatureActions { - if e.ctx, err = action.runWithFeature(e.ctx, e.cfg, featureInfoFromFeature(feature)); err != nil { + if e.ctx, err = action.runWithFeature(e.ctx, e.cfg, deepCopyFeature(feature)); err != nil { t.Fatalf("BeforeEachTest failure: %s", err) } } @@ -202,7 +202,7 @@ func (e *testEnv) Test(t *testing.T, testFeatures ...types.Feature) { // execute beforeFeature actions for _, action := range afterFeatureActions { - if e.ctx, err = action.runWithFeature(e.ctx, e.cfg, featureInfoFromFeature(feature)); err != nil { + if e.ctx, err = action.runWithFeature(e.ctx, e.cfg, deepCopyFeature(feature)); err != nil { t.Fatalf("BeforeEachTest failure: %s", err) } } @@ -367,12 +367,16 @@ func (e *testEnv) execFeature(ctx context.Context, t *testing.T, f types.Feature return ctx } -// featureInfoFromFeature just copies the values from the Feature but, as a new type, -// it has less risk of abuse and side-effects. Meant to solely be informational. -func featureInfoFromFeature(f types.Feature) types.FeatureInfo { - return types.FeatureInfo{ - Name: f.Name(), - Labels: f.Labels(), - Steps: f.Steps(), +// deepCopyFeature just copies the values from the Feature but creates 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()) + for k, v := range f.Labels() { + fcopy = fcopy.WithLabel(k, v) } + f.Steps() + for _, step := range f.Steps() { + fcopy = fcopy.WithStep(step.Name(), step.Level(), nil) + } + return fcopy.Feature() } diff --git a/pkg/env/env_test.go b/pkg/env/env_test.go index af5ca212..9fc0f232 100644 --- a/pkg/env/env_test.go +++ b/pkg/env/env_test.go @@ -22,7 +22,6 @@ import ( "sigs.k8s.io/e2e-framework/pkg/envconf" "sigs.k8s.io/e2e-framework/pkg/features" - "sigs.k8s.io/e2e-framework/pkg/internal/types" ) func TestEnv_New(t *testing.T) { @@ -398,10 +397,10 @@ func TestEnv_Test(t *testing.T) { setup: func(t *testing.T, ctx context.Context) []string { env := newTestEnv() val := []string{} - env.BeforeEachFeature(func(ctx context.Context, _ *envconf.Config, info types.FeatureInfo) (context.Context, error) { + env.BeforeEachFeature(func(ctx context.Context, _ *envconf.Config, info features.Feature) (context.Context, error) { val = append(val, "before-each-feature") return ctx, nil - }).AfterEachFeature(func(ctx context.Context, _ *envconf.Config, info types.FeatureInfo) (context.Context, error) { + }).AfterEachFeature(func(ctx context.Context, _ *envconf.Config, info features.Feature) (context.Context, error) { val = append(val, "after-each-feature") return ctx, nil }) @@ -417,6 +416,65 @@ func TestEnv_Test(t *testing.T) { return val }, }, + { + name: "before-and-after features unable to mutate feature", + ctx: context.TODO(), + expected: []string{ + "before-each-feature", + "test-feat-1", + "after-each-feature", + "before-each-feature", + "test-feat-2", + "after-each-feature", + }, + setup: func(t *testing.T, ctx context.Context) []string { + env := newTestEnv() + val := []string{} + env.BeforeEachFeature(func(ctx context.Context, _ *envconf.Config, info features.Feature) (context.Context, error) { + val = append(val, "before-each-feature") + t.Logf("%#v, len(steps)=%v step[0].Name: %v\n", info, len(info.Steps()), info.Steps()[0].Name()) + + if len(info.Steps()) == 0 { + t.Fatal("Expected more than 0 steps at start but found 0") + } + if info.Steps()[0].Func() != nil { + t.Fatal("Expected step functions to only be nil but found non-nil value") + } + + // Prior to fixing this logic, this would cause the test to fail/panic. + // Ensure nil'ing the value out doesn't mess up flow of function. + info.Steps()[0] = nil + + // Ensure changes aren't persisted to the afterEachFeature hook + labelMap := info.Labels() + labelMap["foo"] = "bar" + return ctx, nil + }).AfterEachFeature(func(ctx context.Context, _ *envconf.Config, info features.Feature) (context.Context, error) { + val = append(val, "after-each-feature") + t.Logf("%#v, len(steps)=%v\n", info, len(info.Steps())) + if info.Labels()["foo"] == "bar" { + t.Errorf("Expected label from previous feature hook to not include foo:bar") + } + if len(info.Steps()) == 0 { + t.Fatal("Expected more than 0 steps at start but found 0") + } + if info.Steps()[0].Func() != nil { + t.Fatal("Expected step functions to only be nil but found non-nil value") + } + return ctx, nil + }) + f1 := features.New("test-feat").Assess("assess", func(ctx context.Context, t *testing.T, _ *envconf.Config) context.Context { + val = append(val, "test-feat-1") + return ctx + }) + f2 := features.New("test-feat").Assess("assess", func(ctx context.Context, t *testing.T, _ *envconf.Config) context.Context { + val = append(val, "test-feat-2") + return ctx + }) + env.Test(t, f1.Feature(), f2.Feature()) + return val + }, + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { diff --git a/pkg/features/builder.go b/pkg/features/builder.go index 2fc4b17b..47f56cfb 100644 --- a/pkg/features/builder.go +++ b/pkg/features/builder.go @@ -38,6 +38,12 @@ func (b *FeatureBuilder) WithLabel(key, value string) *FeatureBuilder { return b } +// WithStep adds a new step that will be applied prior to feature test. +func (b *FeatureBuilder) WithStep(name string, level Level, fn Func) *FeatureBuilder { + b.feat.steps = append(b.feat.steps, newStep(name, level, fn)) + return b +} + // Setup adds a new setup step that will be applied prior to feature test. func (b *FeatureBuilder) Setup(fn Func) *FeatureBuilder { b.feat.steps = append(b.feat.steps, newStep(fmt.Sprintf("%s-setup", b.feat.name), types.LevelSetup, fn)) diff --git a/pkg/internal/types/types.go b/pkg/internal/types/types.go index 5452c9e9..b9ec14b3 100644 --- a/pkg/internal/types/types.go +++ b/pkg/internal/types/types.go @@ -33,7 +33,7 @@ type EnvFunc func(context.Context, *envconf.Config) (context.Context, error) // can be used to customized the behavior of the // environment. Changes to context are expected to surface // to caller. Meant for use with before/after feature hooks. -type FeatureEnvFunc func(context.Context, *envconf.Config, FeatureInfo) (context.Context, error) +type FeatureEnvFunc func(context.Context, *envconf.Config, Feature) (context.Context, error) // TestEnvFunc represents a user-defined operation that // can be used to customized the behavior of the @@ -90,12 +90,6 @@ type Feature interface { Steps() []Step } -type FeatureInfo struct { - Name string - Labels map[string]string - Steps []Step -} - type Level uint8 const (