helper/resource: Ensure TestStep.ExpectNonEmptyPlan accounts for output changes #234
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, 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 { | ||
|
Contributor
There was a problem hiding this comment. Is this a potentially breaking change for the reasons described in Austin's comment? |
||
| if !change.Actions.NoOp() { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| return true | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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() | ||
|
|
||
|
|
||
|
Member
There was a problem hiding this comment. Is it possible to add a test for this 😃
Contributor
Author
There was a problem hiding this comment. Added positive and negative tests for 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."),
},
},
})
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 | |
| } | ||
| } | ||
|
|
||
| if !planIsEmpty(plan, wd.GetHelper().TerraformVersion()) && !step.ExpectNonEmptyPlan { | ||
| var stdout string | ||
| err = runProviderCommand(ctx, t, func() error { | ||
| var err error | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this PR also changes the "default" behavior (
TestStep.ExpectNonEmptyPlan = false) of non-empty plan's throwing an error by also checking output changes.Should we describe that (potentially non-obvious) behavior change in a separate changelog?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call -- adding a second note entry along the lines of: