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" 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.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..78119b55d 100644 --- 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 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 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."), + }, + }, + }) +}