From 1610bf6efc9d34641e84cd1bbb5229b9a04e436d Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Thu, 30 Nov 2023 10:20:46 -0500 Subject: [PATCH 1/3] helper/resource: Ensure TestStep.ExpectNonEmptyPlan accounts for output changes Reference: https://github.com/hashicorp/terraform-plugin-testing/issues/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. --- helper/resource/testing_new.go | 14 +- helper/resource/testing_new_config.go | 12 +- helper/resource/testing_new_config_test.go | 272 +++++++++++++++++++ helper/resource/testing_new_refresh_state.go | 2 +- 4 files changed, 295 insertions(+), 5 deletions(-) diff --git a/helper/resource/testing_new.go b/helper/resource/testing_new.go index 2134abb9d..2464e3cc7 100644 --- 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,7 +477,7 @@ 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 { @@ -484,6 +485,17 @@ func planIsEmpty(plan *tfjson.Plan) bool { } } } + + 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 index ef080807c..0c4bbd1d1 100644 --- a/helper/resource/testing_new_config.go +++ b/helper/resource/testing_new_config.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" + "github.com/hashicorp/go-version" "github.com/hashicorp/terraform-exec/tfexec" tfjson "github.com/hashicorp/terraform-json" "github.com/mitchellh/go-testing-interface" @@ -20,6 +21,11 @@ import ( "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 = version.Must(version.NewVersion("0.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 index 33f816c4a..f6b9411ee 100644 --- 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 index 86073b165..5c0e38758 100644 --- 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 From 71f04d01cc45738cd251fa939d84f3018b5c3dcc Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Thu, 30 Nov 2023 10:23:36 -0500 Subject: [PATCH 2/3] Update CHANGELOG for #234 --- .changes/unreleased/BUG FIXES-20231130-102326.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changes/unreleased/BUG FIXES-20231130-102326.yaml diff --git a/.changes/unreleased/BUG FIXES-20231130-102326.yaml b/.changes/unreleased/BUG FIXES-20231130-102326.yaml new file mode 100644 index 000000000..d8b094384 --- /dev/null +++ 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" From 03103044038f6fdd0bbd6f197507e9899e606e87 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Fri, 1 Dec 2023 16:04:18 -0500 Subject: [PATCH 3/3] helper/resource: Updates from PR review --- .../unreleased/NOTES-20231201-160257.yaml | 10 ++++ helper/resource/testing_new_config.go | 4 +- .../testing_new_refresh_state_test.go | 57 +++++++++++++++++++ 3 files changed, 69 insertions(+), 2 deletions(-) create mode 100644 .changes/unreleased/NOTES-20231201-160257.yaml diff --git a/.changes/unreleased/NOTES-20231201-160257.yaml b/.changes/unreleased/NOTES-20231201-160257.yaml new file mode 100644 index 000000000..69282c3b0 --- /dev/null +++ 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_config.go b/helper/resource/testing_new_config.go index 0c4bbd1d1..78119b55d 100644 --- a/helper/resource/testing_new_config.go +++ b/helper/resource/testing_new_config.go @@ -8,7 +8,6 @@ import ( "errors" "fmt" - "github.com/hashicorp/go-version" "github.com/hashicorp/terraform-exec/tfexec" tfjson "github.com/hashicorp/terraform-json" "github.com/mitchellh/go-testing-interface" @@ -16,6 +15,7 @@ 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" @@ -24,7 +24,7 @@ import ( // 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 = version.Must(version.NewVersion("0.14.0")) +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() diff --git a/helper/resource/testing_new_refresh_state_test.go b/helper/resource/testing_new_refresh_state_test.go index c9996c54a..8ff9930df 100644 --- a/helper/resource/testing_new_refresh_state_test.go +++ b/helper/resource/testing_new_refresh_state_test.go @@ -149,3 +149,60 @@ func Test_RefreshPlanChecks_PostRefresh_Errors(t *testing.T) { }, }) } + +func Test_RefreshState_ExpectNonEmptyPlan(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() + }`, + ExpectNonEmptyPlan: false, // intentional + ExpectError: regexp.MustCompile("After applying this test step, the plan was not empty."), + }, + { + RefreshState: true, + ExpectNonEmptyPlan: true, + }, + }, + }) +} + +func Test_RefreshState_NonEmptyPlan_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() + }`, + ExpectNonEmptyPlan: false, // intentional + ExpectError: regexp.MustCompile("After applying this test step, the plan was not empty."), + }, + { + RefreshState: true, + ExpectNonEmptyPlan: false, // intentional + ExpectError: regexp.MustCompile("After refreshing state during this test step, a followup plan was not empty."), + }, + }, + }) +}