Merge pull request #71 from johnSchnake/featureInfoNotInternal

Rework the FeatureInfo wiring

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 {

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()
 }

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) {

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))

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 (