helper/resource: Ensure TestStep.ExpectNonEmptyPlan accounts for output changes (#234)

bflad · web-flow · commit 38ccdbdc131e · 2023-12-01T16:06:39.000-05:00
Reference: #222 This is a followup to similar `plancheck` implementation updates that ensure output changes are checked in the plan in addition to resource changes. The intention of this functionality is to catch any plan differences and has been documented in this manner for a long while. Since `TestStep.ExpectNonEmptyPlan` pre-dates this Go module, for extra compatibility for developers migrating, output checking is only enabled for Terraform 0.14 and later since prior versions will always show changes when outputs are present in the configuration. Ideally `TestStep.ExpectNonEmptyPlan` will be deprecated in preference of the newer plan checks since its abstraction is fairly ambiguous to when its being invoked, but this deprecation may come by nature of larger changes for a next major version.
diff --git a/.changes/unreleased/BUG FIXES-20231130-102326.yaml b/.changes/unreleased/BUG FIXES-20231130-102326.yaml
@@ -0,0 +1,6 @@
+kind: BUG FIXES
+body: 'helper/resource: Ensured `TestStep.ExpectNonEmptyPlan` accounts for output
+  changes with Terraform 0.14 and later'
+time: 2023-11-30T10:23:26.348382-05:00
+custom:
+  Issue: "234"
diff --git a/.changes/unreleased/NOTES-20231201-160257.yaml b/.changes/unreleased/NOTES-20231201-160257.yaml
@@ -0,0 +1,10 @@
+kind: NOTES
+body: 'helper/resource: Configuration based `TestStep` now include post-apply plan
+  checks for output changes in addition to resource changes. If this causes
+  unexpected new test failures, most `output` configuration blocks can be likely
+  be removed. Test steps involving resources and data sources should never need
+  to use `output` configuration blocks as plan and state checks support working
+  on resource and data source attributes values directly.'
+time: 2023-12-01T16:02:57.111641-05:00
+custom:
+  Issue: "234"
diff --git a/helper/resource/testing_new.go b/helper/resource/testing_new.go
@@ -13,6 +13,7 @@ import (
 	"strings"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/hashicorp/go-version"
 	tfjson "github.com/hashicorp/terraform-json"
 	"github.com/mitchellh/go-testing-interface"
 
@@ -476,14 +477,25 @@ func stateIsEmpty(state *terraform.State) bool {
 	return state.Empty() || !state.HasResources() //nolint:staticcheck // legacy usage
 }
 
-func planIsEmpty(plan *tfjson.Plan) bool {
+func planIsEmpty(plan *tfjson.Plan, tfVersion *version.Version) bool {
 	for _, rc := range plan.ResourceChanges {
 		for _, a := range rc.Change.Actions {
 			if a != tfjson.ActionNoop {
 				return false
 			}
 		}
 	}
+
+	if tfVersion.LessThan(expectNonEmptyPlanOutputChangesMinTFVersion) {
+		return true
+	}
+
+	for _, change := range plan.OutputChanges {
+		if !change.Actions.NoOp() {
+			return false
+		}
+	}
+
 	return true
 }
 
diff --git a/helper/resource/testing_new_config.go b/helper/resource/testing_new_config.go
@@ -15,11 +15,17 @@ import (
 	"github.com/hashicorp/terraform-plugin-testing/config"
 	"github.com/hashicorp/terraform-plugin-testing/internal/teststep"
 	"github.com/hashicorp/terraform-plugin-testing/terraform"
+	"github.com/hashicorp/terraform-plugin-testing/tfversion"
 
 	"github.com/hashicorp/terraform-plugin-testing/internal/logging"
 	"github.com/hashicorp/terraform-plugin-testing/internal/plugintest"
 )
 
+// expectNonEmptyPlanOutputChangesMinTFVersion is used to keep compatibility for
+// Terraform 0.12 and 0.13 after enabling ExpectNonEmptyPlan to check output
+// changes. Those older versions will always show outputs being created.
+var expectNonEmptyPlanOutputChangesMinTFVersion = tfversion.Version0_14_0
+
 func testStepNewConfig(ctx context.Context, t testing.T, c TestCase, wd *plugintest.WorkingDir, step TestStep, providers *providerFactories, stepIndex int, helper *plugintest.Helper) error {
 	t.Helper()
 
@@ -236,7 +242,7 @@ func testStepNewConfig(ctx context.Context, t testing.T, c TestCase, wd *plugint
 		}
 	}
 
-	if !planIsEmpty(plan) && !step.ExpectNonEmptyPlan {
+	if !planIsEmpty(plan, helper.TerraformVersion()) && !step.ExpectNonEmptyPlan {
 		var stdout string
 		err = runProviderCommand(ctx, t, func() error {
 			var err error
@@ -283,7 +289,7 @@ func testStepNewConfig(ctx context.Context, t testing.T, c TestCase, wd *plugint
 	}
 
 	// check if plan is empty
-	if !planIsEmpty(plan) && !step.ExpectNonEmptyPlan {
+	if !planIsEmpty(plan, helper.TerraformVersion()) && !step.ExpectNonEmptyPlan {
 		var stdout string
 		err = runProviderCommand(ctx, t, func() error {
 			var err error
@@ -294,7 +300,7 @@ func testStepNewConfig(ctx context.Context, t testing.T, c TestCase, wd *plugint
 			return fmt.Errorf("Error retrieving formatted second plan output: %w", err)
 		}
 		return fmt.Errorf("After applying this test step and performing a `terraform refresh`, the plan was not empty.\nstdout\n\n%s", stdout)
-	} else if step.ExpectNonEmptyPlan && planIsEmpty(plan) {
+	} else if step.ExpectNonEmptyPlan && planIsEmpty(plan, helper.TerraformVersion()) {
 		return errors.New("Expected a non-empty plan, but got an empty plan")
 	}
 
diff --git a/helper/resource/testing_new_config_test.go b/helper/resource/testing_new_config_test.go
@@ -65,6 +65,278 @@ func TestTest_TestStep_ExpectError_NewConfig(t *testing.T) {
 	})
 }
 
+func Test_ExpectNonEmptyPlan_EmptyPlanError(t *testing.T) {
+	t.Parallel()
+
+	UnitTest(t, TestCase{
+		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
+			tfversion.SkipBelow(tfversion.Version1_4_0),
+		},
+		ExternalProviders: map[string]ExternalProvider{
+			"terraform": {Source: "terraform.io/builtin/terraform"},
+		},
+		Steps: []TestStep{
+			{
+				Config:             `resource "terraform_data" "test" {}`,
+				ExpectNonEmptyPlan: true,
+				ExpectError:        regexp.MustCompile("Expected a non-empty plan, but got an empty plan"),
+			},
+		},
+	})
+}
+
+func Test_ExpectNonEmptyPlan_PreRefresh_ResourceChanges(t *testing.T) {
+	t.Parallel()
+
+	UnitTest(t, TestCase{
+		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
+			tfversion.SkipBelow(tfversion.Version1_4_0),
+		},
+		ExternalProviders: map[string]ExternalProvider{
+			"terraform": {Source: "terraform.io/builtin/terraform"},
+		},
+		Steps: []TestStep{
+			{
+				Config: `resource "terraform_data" "test" {
+					# Never recommended for real world configurations, but tests
+					# the intended behavior.
+					input = timestamp()
+				}`,
+				ConfigPlanChecks: ConfigPlanChecks{
+					// Verification of that the behavior is being caught pre
+					// refresh. We want to ensure ExpectNonEmptyPlan allows test
+					// to pass if pre refresh also has changes.
+					PostApplyPreRefresh: []plancheck.PlanCheck{
+						plancheck.ExpectResourceAction("terraform_data.test", plancheck.ResourceActionUpdate),
+					},
+				},
+				ExpectNonEmptyPlan: true,
+			},
+		},
+	})
+}
+
+func Test_ExpectNonEmptyPlan_PostRefresh_OutputChanges(t *testing.T) {
+	t.Parallel()
+
+	UnitTest(t, TestCase{
+		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
+			tfversion.SkipAbove(tfversion.Version0_14_0), // outputs before 0.14 always show as created
+		},
+		// Avoid our own validation that requires at least one provider config.
+		ExternalProviders: map[string]ExternalProvider{
+			"terraform": {Source: "terraform.io/builtin/terraform"},
+		},
+		Steps: []TestStep{
+			{
+				Config:             `output "test" { value = timestamp() }`,
+				ExpectNonEmptyPlan: false, // compatibility compromise for 0.12 and 0.13
+			},
+		},
+	})
+
+	UnitTest(t, TestCase{
+		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
+			tfversion.SkipBelow(tfversion.Version0_14_0), // outputs before 0.14 always show as created
+		},
+		// Avoid our own validation that requires at least one provider config.
+		ExternalProviders: map[string]ExternalProvider{
+			"terraform": {Source: "terraform.io/builtin/terraform"},
+		},
+		Steps: []TestStep{
+			{
+				Config:             `output "test" { value = timestamp() }`,
+				ExpectNonEmptyPlan: true,
+			},
+		},
+	})
+}
+
+func Test_ExpectNonEmptyPlan_PostRefresh_ResourceChanges(t *testing.T) {
+	t.Parallel()
+
+	UnitTest(t, TestCase{
+		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
+			tfversion.SkipBelow(tfversion.Version1_0_0), // ProtoV6ProviderFactories
+		},
+		ProtoV6ProviderFactories: map[string]func() (tfprotov6.ProviderServer, error){
+			"test": providerserver.NewProviderServer(testprovider.Provider{
+				Resources: map[string]testprovider.Resource{
+					"test_resource": {
+						CreateResponse: &resource.CreateResponse{
+							NewState: tftypes.NewValue(
+								tftypes.Object{
+									AttributeTypes: map[string]tftypes.Type{
+										"id": tftypes.String,
+									},
+								},
+								map[string]tftypes.Value{
+									"id": tftypes.NewValue(tftypes.String, "test"), // intentionally same
+								},
+							),
+						},
+						ReadResponse: &resource.ReadResponse{
+							NewState: tftypes.NewValue(
+								tftypes.Object{
+									AttributeTypes: map[string]tftypes.Type{
+										"id": tftypes.String,
+									},
+								},
+								map[string]tftypes.Value{
+									"id": tftypes.NewValue(tftypes.String, "not-test"), // intentionally different
+								},
+							),
+						},
+						SchemaResponse: &resource.SchemaResponse{
+							Schema: &tfprotov6.Schema{
+								Block: &tfprotov6.SchemaBlock{
+									Attributes: []*tfprotov6.SchemaAttribute{
+										{
+											Name:     "id",
+											Type:     tftypes.String,
+											Required: true,
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			}),
+		},
+		Steps: []TestStep{
+			{
+				Config: `resource "test_resource" "test" {
+					# Post create refresh intentionally changes configured value
+					# which is an errant resource implementation. Create should
+					# account for the correct post creation state, preventing an
+					# immediate difference next Terraform run for practitioners.
+					# This errant resource behavior verifies the expected
+					# behavior of ExpectNonEmptyPlan for post refresh planning.
+					id = "test"
+				}`,
+				ConfigPlanChecks: ConfigPlanChecks{
+					// Verification of that the behavior is being caught post
+					// refresh. We want to ensure ExpectNonEmptyPlan is being
+					// triggered after the pre refresh plan shows no changes.
+					PostApplyPreRefresh: []plancheck.PlanCheck{
+						plancheck.ExpectResourceAction("test_resource.test", plancheck.ResourceActionNoop),
+					},
+				},
+				ExpectNonEmptyPlan: true,
+			},
+		},
+	})
+}
+
+func Test_NonEmptyPlan_PreRefresh_Error(t *testing.T) {
+	t.Parallel()
+
+	UnitTest(t, TestCase{
+		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
+			tfversion.SkipBelow(tfversion.Version1_4_0),
+		},
+		ExternalProviders: map[string]ExternalProvider{
+			"terraform": {Source: "terraform.io/builtin/terraform"},
+		},
+		Steps: []TestStep{
+			{
+				Config: `resource "terraform_data" "test" {
+					# Never recommended for real world configurations, but tests
+					# the intended behavior.
+					input = timestamp()
+				}`,
+				ConfigPlanChecks: ConfigPlanChecks{
+					// Verification of that the behavior is being caught pre
+					// refresh.
+					PostApplyPreRefresh: []plancheck.PlanCheck{
+						plancheck.ExpectResourceAction("terraform_data.test", plancheck.ResourceActionUpdate),
+					},
+				},
+				ExpectNonEmptyPlan: false, // intentional
+				ExpectError:        regexp.MustCompile("After applying this test step, the plan was not empty."),
+			},
+		},
+	})
+}
+
+func Test_NonEmptyPlan_PostRefresh_Error(t *testing.T) {
+	t.Parallel()
+
+	UnitTest(t, TestCase{
+		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
+			tfversion.SkipBelow(tfversion.Version1_0_0), // ProtoV6ProviderFactories
+		},
+		ProtoV6ProviderFactories: map[string]func() (tfprotov6.ProviderServer, error){
+			"test": providerserver.NewProviderServer(testprovider.Provider{
+				Resources: map[string]testprovider.Resource{
+					"test_resource": {
+						CreateResponse: &resource.CreateResponse{
+							NewState: tftypes.NewValue(
+								tftypes.Object{
+									AttributeTypes: map[string]tftypes.Type{
+										"id": tftypes.String,
+									},
+								},
+								map[string]tftypes.Value{
+									"id": tftypes.NewValue(tftypes.String, "test"), // intentionally same
+								},
+							),
+						},
+						ReadResponse: &resource.ReadResponse{
+							NewState: tftypes.NewValue(
+								tftypes.Object{
+									AttributeTypes: map[string]tftypes.Type{
+										"id": tftypes.String,
+									},
+								},
+								map[string]tftypes.Value{
+									"id": tftypes.NewValue(tftypes.String, "not-test"), // intentionally different
+								},
+							),
+						},
+						SchemaResponse: &resource.SchemaResponse{
+							Schema: &tfprotov6.Schema{
+								Block: &tfprotov6.SchemaBlock{
+									Attributes: []*tfprotov6.SchemaAttribute{
+										{
+											Name:     "id",
+											Type:     tftypes.String,
+											Required: true,
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			}),
+		},
+		Steps: []TestStep{
+			{
+				Config: `resource "test_resource" "test" {
+					# Post create refresh intentionally changes configured value
+					# which is an errant resource implementation. Create should
+					# account for the correct post creation state, preventing an
+					# immediate difference next Terraform run for practitioners.
+					# This errant resource behavior verifies the expected
+					# behavior of ExpectNonEmptyPlan for post refresh planning.
+					id = "test"
+				}`,
+				ConfigPlanChecks: ConfigPlanChecks{
+					// Verification of that the behavior is being caught post
+					// refresh.
+					PostApplyPreRefresh: []plancheck.PlanCheck{
+						plancheck.ExpectResourceAction("test_resource.test", plancheck.ResourceActionNoop),
+					},
+				},
+				ExpectNonEmptyPlan: false, // intentional
+				ExpectError:        regexp.MustCompile("After applying this test step and performing a `terraform refresh`, the plan was not empty."),
+			},
+		},
+	})
+}
+
 func Test_ConfigPlanChecks_PreApply_Called(t *testing.T) {
 	t.Parallel()
 
diff --git a/helper/resource/testing_new_refresh_state.go b/helper/resource/testing_new_refresh_state.go
@@ -88,7 +88,7 @@ func testStepNewRefreshState(ctx context.Context, t testing.T, wd *plugintest.Wo
 		}
 	}
 
-	if !planIsEmpty(plan) && !step.ExpectNonEmptyPlan {
+	if !planIsEmpty(plan, wd.GetHelper().TerraformVersion()) && !step.ExpectNonEmptyPlan {
 		var stdout string
 		err = runProviderCommand(ctx, t, func() error {
 			var err error
diff --git a/helper/resource/testing_new_refresh_state_test.go b/helper/resource/testing_new_refresh_state_test.go

Original file line number	Diff line number	Diff line change
`@@ -88,7 +88,7 @@ func testStepNewRefreshState(ctx context.Context, t testing.T, wd *plugintest.Wo`
`88`	`88`	`}`
`89`	`89`	`}`
`90`	`90`
`91`		`- if !planIsEmpty(plan) && !step.ExpectNonEmptyPlan {`
	`91`	`+ if !planIsEmpty(plan, wd.GetHelper().TerraformVersion()) && !step.ExpectNonEmptyPlan {`
`92`	`92`	`var stdout string`
`93`	`93`	`err = runProviderCommand(ctx, t, func() error {`
`94`	`94`	`var err error`