helper/resource: Ensure TestStep.ExpectNonEmptyPlan accounts for output changes #234
Conversation
…ut changes 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.
|
To be honest, I would also be fine only adding the missing testing here and leaving the existing |
There was a problem hiding this comment.
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.
Good call -- adding a second note entry along the lines of:
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.There was a problem hiding this comment.
Is it possible to add a test for this 😃
There was a problem hiding this comment.
Added positive and negative tests for RefreshState: true and ExpectNonEmptyPlan 👍
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."),
},
},
})
}
| // 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")) |
There was a problem hiding this comment.
| var expectNonEmptyPlanOutputChangesMinTFVersion = version.Must(version.NewVersion("0.14.0")) | |
| var expectNonEmptyPlanOutputChangesMinTFVersion = tfversion.Version0_14_0 |
Nit
There was a problem hiding this comment.
Good call -- sorry couldn't accept suggestion directly as imports also needed updates 👍
| return true | ||
| } | ||
|
|
||
| for _, change := range plan.OutputChanges { |
There was a problem hiding this comment.
Is this a potentially breaking change for the reasons described in Austin's comment?
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Reference: #222
This is a followup to similar
plancheckimplementation 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.ExpectNonEmptyPlanpre-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. IdeallyTestStep.ExpectNonEmptyPlanwill 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.New unit test failure prior to implementation update: