Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Dec 1, 2023

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Nov 30, 2023

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.

New unit test failure prior to implementation update:

--- FAIL: Test_ExpectNonEmptyPlan_PostRefresh_OutputChanges (0.42s)
    /Users/bflad/src/github.com/hashicorp/terraform-plugin-testing/helper/resource/testing_new_config_test.go:122: Step 1/1 error: Expected a non-empty plan, but got an empty plan

…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.
@bflad bflad added the bug Something isn't working label Nov 30, 2023
@bflad bflad added this to the v1.6.0 milestone Nov 30, 2023
@bflad bflad requested a review from a team as a code owner November 30, 2023 15:22
@bflad
Copy link
Contributor Author

bflad commented Nov 30, 2023

To be honest, I would also be fine only adding the missing testing here and leaving the existing TestStep.ExpectNonEmptyPlan implementation. This change is being suggested for consistency between the two ways of checking plans.

Copy link
Member

@austinvalle austinvalle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I like updating both implementations to be consistent. I'm not sure how often output values are used in provider tests, but it seems a very reasonable bug fix to make now.

Just had two notes in the PR :shipit:

Copy link
Member

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?

Copy link
Contributor Author

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:

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to add a test for this 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var expectNonEmptyPlanOutputChangesMinTFVersion = version.Must(version.NewVersion("0.14.0"))
var expectNonEmptyPlanOutputChangesMinTFVersion = tfversion.Version0_14_0

Nit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call -- sorry couldn't accept suggestion directly as imports also needed updates 👍

return true
}

for _, change := range plan.OutputChanges {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a potentially breaking change for the reasons described in Austin's comment?

Copy link
Contributor

@bendbennett bendbennett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM just a couple of minor comments.

@bflad bflad merged commit 38ccdbd into main Dec 1, 2023
26 checks passed
@bflad bflad deleted the bflad/TestStep-ExpectNonEmptyPlan-output-changes branch December 1, 2023 21:06
Copy link

github-actions bot commented Jan 1, 2024

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants