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

plancheck: ExpectEmptyPlan Does Not Account for Output Changes #222

Closed
bflad opened this issue Nov 22, 2023 · 3 comments · Fixed by #232
Closed

plancheck: ExpectEmptyPlan Does Not Account for Output Changes #222

bflad opened this issue Nov 22, 2023 · 3 comments · Fixed by #232
Assignees
Labels
bug Something isn't working
Milestone

Comments

@bflad
Copy link
Contributor

bflad commented Nov 22, 2023

terraform-plugin-testing version

v1.5.1

Relevant provider source code

resource.UnitTest(t, resource.TestCase{
  Steps: []resource.TestStep{
    {
      Config: `output "test" { value = "one" }`,
    },
    {
      Config: `output "test" { value = "two" }`,
      ConfigPlanChecks: resource.ConfigPlanChecks{
        PreApply: []plancheck.PlanCheck{
          plancheck.ExpectEmptyPlan(),
        },
      },
    },
  },
})

Expected Behavior

Given the plan check naming and that in the human-readable plan output that the output change would show, that this assertion would fail the test.

Actual Behavior

Assertion passes and test passes.

Steps to Reproduce

go test

References

@bflad bflad added the bug Something isn't working label Nov 22, 2023
@bflad
Copy link
Contributor Author

bflad commented Nov 22, 2023

Given the similar resource changes only checking of ExpectNonEmptyPlan, I would also presume this applies to that plan check as well in the inverse way.

@bflad
Copy link
Contributor Author

bflad commented Nov 22, 2023

So this is actually very nuanced. The testing logic currently always calls a refresh command before plan. That refresh will update the output value so the plan views it as a no-op action. The explicit refresh does not need to explicitly be there as it can be done on the plan. Further down the rabbit hole we go!

bflad added a commit that referenced this issue Nov 29, 2023
…e state shimming when possible (#223)

Reference: hashicorp/terraform-plugin-sdk#515
Reference: #194
Reference: #222
Reference: https://developer.hashicorp.com/terraform/cli/commands/refresh

This change replaces the usage of explicit refresh commands by instead calling the next command with the relevant refresh flags. Separating the refresh from plan was previously done with the idea that the testing logic may introduce further assertion hooks based on that explicit refresh. Over time though, the Terraform recommendations for refreshing and planning have solidified around always recommending to use a plan with behavior changing flags as necessary. Even the Terraform CLI implementation of the refresh command itself has been shifted to call a plan with special flags.

This should reduce testing times decently as it means that the testing logic will skip spinning up and tearing down any relevant providers two times per `TestStep` and whatever time would be associated with Terraform CLI loading for those commands. Comparing the testing performed across this Go module:

Prior: `TF_ACC=1 go test -count=1 ./...  111.07s user 59.37s system 233% cpu 1:12.99 total`
Now: `TF_ACC=1 go test -count=1 ./...  90.81s user 48.25s system 229% cpu 1:00.54 total`

The separated refresh also prevented `PreApply` plan checks from catching certain situations because the refreshed state could cause the plan to be missing changes.
@bflad bflad added this to the v1.6.0 milestone Nov 29, 2023
@bflad bflad self-assigned this Nov 29, 2023
bflad added a commit that referenced this issue Nov 29, 2023
…for output changes in addition to resource changes

Reference: #222

Previously with new unit tests:

```
--- FAIL: Test_ExpectNonEmptyPlan_OutputChanges_None (0.90s)
    expect_non_empty_plan_test.go:17: Step 2/2 error: Pre-apply plan check(s) failed:
        expected a non-empty plan, but got an empty plan
--- FAIL: Test_ExpectEmptyPlan_OutputChanges_Error (1.00s)
    expect_empty_plan_test.go:41: Step 2/2, expected an error but got none
```
bflad added a commit that referenced this issue Nov 30, 2023
…for output changes in addition to resource changes (#232)

Reference: #222

Previously with new unit tests:

```
--- FAIL: Test_ExpectNonEmptyPlan_OutputChanges_None (0.90s)
    expect_non_empty_plan_test.go:17: Step 2/2 error: Pre-apply plan check(s) failed:
        expected a non-empty plan, but got an empty plan
--- FAIL: Test_ExpectEmptyPlan_OutputChanges_Error (1.00s)
    expect_empty_plan_test.go:41: Step 2/2, expected an error but got none
```
bflad added a commit that referenced this issue Nov 30, 2023
…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 added a commit that referenced this issue Dec 1, 2023
…ut changes (#234)

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.
Copy link

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, 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 Dec 31, 2023
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
1 participant