kubernetes-sigs/e2e-framework

Distinguish feature/assessment reason from name

negz opened this issue · 6 comments

negz commented

I'm working on porting Crossplane's nascent E2E tests to e2e-framework in crossplane/crossplane#4187. I find the features.Table approach more readable than the builder pattern, so most of my tests look like this:

func TestConfiguration(t *testing.T) {
	// Test that we can install a Configuration from a private repository using
	// a package pull secret.
	manifests := "test/e2e/manifests/pkg/configuration/private"
	private := features.Table{
		{
			Name: "ConfigurationIsCreated",
			Assessment: funcs.AllOf(
				funcs.ApplyResources(FieldManager, manifests, "*.yaml"),
				funcs.ResourcesCreatedWithin(1*time.Minute, manifests, "*.yaml"),
			),
		},
		// etc...
	}

	setup := funcs.ReadyToTestWithin(1*time.Minute, namespace)
	environment.Test(t,
		private.Build("PullFromPrivateRegistry").
			WithLabel("area", "pkg").
			WithLabel("size", "small").
			Setup(setup).Feature(),
	)

I've noticed that a lot of folks like to use descriptive prose to "name" their assessments and features. So more like:

func TestConfiguration(t *testing.T) {
	manifests := "test/e2e/manifests/pkg/configuration/private"
	private := features.Table{
		{
			Name: "The supplied Configuration should be created",
			Assessment: funcs.AllOf(
				funcs.ApplyResources(FieldManager, manifests, "*.yaml"),
				funcs.ResourcesCreatedWithin(1*time.Minute, manifests, "*.yaml"),
			),
		},
		// etc...
	}

	setup := funcs.ReadyToTestWithin(1*time.Minute, namespace)
	environment.Test(t,
		private.Build("Test that we can install a Configuration from a private repository using a package pull secret.").
			WithLabel("area", "pkg").
			WithLabel("size", "small").
			Setup(setup).Feature(),
	)

I like this in theory - I think it's a good idea to provide as much written context as possible for anyone reading the test code and/or debugging why the test has failed. In practice, I stick with PascalCase assessment and feature names, because they're actually t.Run subtest names. Go mangles them into Really_really_long/nested_snake_case_names which I find ugly and hard to read:

--- PASS: TestConfiguration (60.15s)
    --- PASS: TestConfiguration/Pull_a_Configuration_from_a_private_registry (30.07s)
        --- PASS: TestConfiguration/Pull_a_Configuration_from_a_private_registry/The_supplied_Configuration_should_be_created (5.03s)
        --- PASS: TestConfiguration/Pull_a_Configuration_from_a_private_registry/The_supplied_configuration_should_become_healthy,_and_active (5.01s)
        --- PASS: TestConfiguration/Pull_a_Configuration_from_a_private_registry/The_supplied_Configuration_should_be_deleted (5.03s)
    --- PASS: TestConfiguration/Pull_a_Configuration_that_depends_on_a_Provider (30.08s)
        --- PASS: TestConfiguration/Pull_a_Configuration_that_depends_on_a_Provider/The_supplied_Configuration_should_be_created (5.02s)
        --- PASS: TestConfiguration/Pull_a_Configuration_that_depends_on_a_Provider/The_supplied_Configuration_should_become_healthy,_and_active (5.01s)
        --- PASS: TestConfiguration/Pull_a_Configuration_that_depends_on_a_Provider/The_Provider_the_Configuration_depends_on_should_become_healthy,_and_active (5.01s)
        --- PASS: TestConfiguration/Pull_a_Configuration_that_depends_on_a_Provider/The_supplied_Configuration_should_be_deleted (5.01s)
        --- PASS: TestConfiguration/Pull_a_Configuration_that_depends_on_a_Provider/The_supplied_Provider_should_be_deleted_(providers_are_not_deleted_automatically) (5.01s)

What do you think about allowing folks to provide a feature or assessment's written context, distinct from the name of the feature or assessment? For example:

func TestConfiguration(t *testing.T) {
	manifests := "test/e2e/manifests/pkg/configuration/private"
	private := features.Table{
		{
			Name: "CreateConfiguration",
			Reason: "We should be able to install a Configuration from a private repository using a package pull secret",
			Assessment: funcs.AllOf(
				funcs.ApplyResources(FieldManager, manifests, "*.yaml"),
				funcs.ResourcesCreatedWithin(1*time.Minute, manifests, "*.yaml"),
			),
		},
		// etc...
	}

	setup := funcs.ReadyToTestWithin(1*time.Minute, namespace)
	environment.Test(t,
		private.Build("PullFromPrivateRegistry").
			Reason("Crossplane should support pulling packages from private repositories").
			WithLabel("area", "pkg").
			WithLabel("size", "small").
			Setup(setup).Feature(),
	)

I imagine the Reason field would be emitted via t.Log when a test started, and perhaps somehow included (regardless of test verbosity) if a test failed. I first saw a pattern like this in the go-cmp unit tests, and we adopted it a while back for Crossplane unit tests to good effect.

harshanarayana commented

@negz I think this would be really nice to have in place. Having a reason/description can always add more context to a test failure and the reason why certain test is being done. Let us see what @vladimirvivien @ShwethaKumbla @cpanato have to say about this.

diff --git a/pkg/env/env.go b/pkg/env/env.go
index 8055010..0f47301 100644
--- a/pkg/env/env.go
+++ b/pkg/env/env.go
@@ -441,6 +441,11 @@ func (e *testEnv) execFeature(ctx context.Context, t *testing.T, featName string
 
 		failed := false
 		for i, assess := range assessments {
+			if dAssess, ok := assess.(types.DescribableStep); ok {
+				if dAssess.Description() != "" {
+					klog.V(4).InfoS("Processing Assessment", "description", dAssess.Description())
+				}
+			}
 			assessName := assess.Name()
 			if assessName == "" {
 				assessName = fmt.Sprintf("Assessment-%d", i+1)
diff --git a/pkg/features/builder.go b/pkg/features/builder.go
index a6fa403..f5936f0 100644
--- a/pkg/features/builder.go
+++ b/pkg/features/builder.go
@@ -71,6 +71,11 @@ func (b *FeatureBuilder) Assess(desc string, fn Func) *FeatureBuilder {
 	return b.WithStep(desc, types.LevelAssess, fn)
 }
 
+func (b *FeatureBuilder) AssessWithDescription(name, description string, fn Func) *FeatureBuilder {
+	b.feat.steps = append(b.feat.steps, newStepWithDescription(name, description, types.LevelAssess, fn))
+	return b
+}
+
 // Feature returns a feature configured by builder.
 func (b *FeatureBuilder) Feature() types.Feature {
 	return b.feat
diff --git a/pkg/features/feature.go b/pkg/features/feature.go
index 99ba103..6b32261 100644
--- a/pkg/features/feature.go
+++ b/pkg/features/feature.go
@@ -53,9 +53,10 @@ func (f *defaultFeature) Steps() []types.Step {
 }
 
 type testStep struct {
-	name  string
-	level Level
-	fn    Func
+	description string
+	name        string
+	level       Level
+	fn          Func
 }
 
 func newStep(name string, level Level, fn Func) *testStep {
@@ -66,6 +67,15 @@ func newStep(name string, level Level, fn Func) *testStep {
 	}
 }
 
+func newStepWithDescription(name, description string, level Level, fn Func) *testStep {
+	return &testStep{
+		description: description,
+		name:        name,
+		level:       level,
+		fn:          fn,
+	}
+}
+
 func (s *testStep) Name() string {
 	return s.name
 }
@@ -78,6 +88,10 @@ func (s *testStep) Func() Func {
 	return s.fn
 }
 
+func (s *testStep) Description() string {
+	return s.description
+}
+
 func GetStepsByLevel(steps []types.Step, l types.Level) []types.Step {
 	if steps == nil {
 		return nil
diff --git a/pkg/features/table.go b/pkg/features/table.go
index d2afed9..242b6e9 100644
--- a/pkg/features/table.go
+++ b/pkg/features/table.go
@@ -23,8 +23,9 @@ import (
 // Table provides a structure for table-driven tests.
 // Each entry in the table represents an executable assessment.
 type Table []struct {
-	Name       string
-	Assessment Func
+	Name        string
+	Description string
+	Assessment  Func
 }
 
 // Build converts the defined test steps in the table
@@ -42,7 +43,7 @@ func (table Table) Build(featureName ...string) *FeatureBuilder {
 			test.Name = fmt.Sprintf("Assessment-%d", i)
 		}
 		if test.Assessment != nil {
-			f.Assess(test.Name, test.Assessment)
+			f.AssessWithDescription(test.Name, test.Description, test.Assessment)
 		}
 	}
 	return f
diff --git a/pkg/internal/types/types.go b/pkg/internal/types/types.go
index 657931f..41d7664 100644
--- a/pkg/internal/types/types.go
+++ b/pkg/internal/types/types.go
@@ -119,3 +119,10 @@ type Step interface {
 	// Func is the operation for the step
 	Func() StepFunc
 }
+
+type DescribableStep interface {
+	Step
+
+	// Description is the readable information indicating what the step is performing
+	Description() string
+}

I managed to make a few quick changes to see if we can really achieve what we really want in terms of being able to use a description without breaking the existing contracts so that we can continue the discussion further.

  • 👍1
harshanarayana commented 
❯ ./k --v=7 -test.v
=== RUN   TestTableDriven
=== RUN   TestTableDriven/Random_numbers
I0620 00:22:11.613167   72002 env.go:446] "Processing Assessment" description="This is a fun test"
=== RUN   TestTableDriven/Random_numbers/less_than_equal_64
    table_test.go:55: limit should be less than 64
=== RUN   TestTableDriven/Random_numbers/more_than_than_equal_128
=== RUN   TestTableDriven/Random_numbers/Assessment-2
=== RUN   TestTableDriven/Feature-2
=== RUN   TestTableDriven/Feature-2/A_simple_feature
    table_test.go:90: this is a great number
--- PASS: TestTableDriven (0.00s)
    --- PASS: TestTableDriven/Random_numbers (0.00s)
        --- PASS: TestTableDriven/Random_numbers/less_than_equal_64 (0.00s)
        --- PASS: TestTableDriven/Random_numbers/more_than_than_equal_128 (0.00s)
        --- PASS: TestTableDriven/Random_numbers/Assessment-2 (0.00s)
    --- PASS: TestTableDriven/Feature-2 (0.00s)
        --- PASS: TestTableDriven/Feature-2/A_simple_feature (0.00s)
PASS

I am yet to find out the best way to integrate this description into failure message.

negz commented

I am yet to find out the best way to integrate this description into failure message.

@harshanarayana Perhaps we don't need to. go test will automatically include all t.Log statements associated with a particular test if it fails, even when it's running without -test.v. So if you just keep the t.Log statement you've already added:

  • When running with -test.v it will be logged at the start of every test (regardless of pass or fail)
  • When running without -test.v It will be logged at the start of any test that fails.

I think that's good enough - if a test fails you'll see the log statement describing what the test is doing.

harshanarayana commented

@negz Valid point. I will switch to using t.Log instead of the klog for these descriptions so that they are managed by the go test accordingly based on failures/success and log levels. @vladimirvivien If you think this is a safe approach, I will open a PR for review.

  • 👍2
vladimirvivien commented

@negz and @harshanarayana This is beautiful ❤️
I really don't have much to add.

@negz Awesome to see someone using the Table construct.

harshanarayana commented

@vladimirvivien @negz I have opened a PR under #284 to discuss this further and get the changes done. PTAL when possible.

  • 👍1