Error with state upgrades after `terraform-plugin-testing@v1.6.0` update

[ewbankkit]

terraform-plugin-testing version

v1.6.0

Relevant provider source code

Terraform Schema

		resourceV0 := &schema.Resource{Schema: map[string]*schema.Schema{}}

		SchemaVersion: 1,
		StateUpgraders: []schema.StateUpgrader{
			{
				Type: resourceV0.CoreConfigSchema().ImpliedType(),
				Upgrade: func(_ context.Context, rawState map[string]interface{}, _ interface{}) (map[string]interface{}, error) {
					if v, ok := rawState["enable_default_standards"]; !ok || v == nil {
						rawState["enable_default_standards"] = "true"
					}

					return rawState, nil
				},
				Version: 0,
			},
		},
		Schema: map[string]*schema.Schema{
			"enable_default_standards": {
				Type:     schema.TypeBool,
				Optional: true,
				ForceNew: true,
				Default:  true,
			},
		},

Acceptance Test Case

func testAccAccount_migrateV0(t *testing.T) {
	ctx := acctest.Context(t)
	resourceName := "aws_securityhub_account.test"

	resource.Test(t, resource.TestCase{
		PreCheck:     func() { acctest.PreCheck(ctx, t) },
		ErrorCheck:   acctest.ErrorCheck(t, names.SecurityHubEndpointID),
		CheckDestroy: testAccCheckAccountDestroy(ctx),
		Steps: []resource.TestStep{
			{
				ExternalProviders: map[string]resource.ExternalProvider{
					"aws": {
						Source:            "hashicorp/aws",
						VersionConstraint: "4.59.0",
					},
				},
				Config: testAccAccountConfig_basic,
				Check: resource.ComposeTestCheckFunc(
					testAccCheckAccountExists(ctx, resourceName),
					resource.TestCheckNoResourceAttr(resourceName, "enable_default_standards"),
				),
			},
			{
				ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
				Config:                   testAccAccountConfig_basic,
				PlanOnly:                 true,
			},
		},
	})
}

Terraform Configuration Files

resource "aws_securityhub_account" "test" {}

Expected Behavior

With v1.5.1

% make testacc TESTARGS='-run=TestAccSecurityHub_serial/^Account$$/MigrateV0' PKG=securityhub
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/securityhub/... -v -count 1 -parallel 20  -run=TestAccSecurityHub_serial/^Account$/MigrateV0 -timeout 360m
=== RUN   TestAccSecurityHub_serial
=== PAUSE TestAccSecurityHub_serial
=== CONT  TestAccSecurityHub_serial
=== RUN   TestAccSecurityHub_serial/Account
=== RUN   TestAccSecurityHub_serial/Account/MigrateV0
--- PASS: TestAccSecurityHub_serial (66.17s)
    --- PASS: TestAccSecurityHub_serial/Account (66.17s)
        --- PASS: TestAccSecurityHub_serial/Account/MigrateV0 (66.17s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/securityhub	71.328s

Actual Behavior

With v1.6.0

% make testacc TESTARGS='-run=TestAccSecurityHub_serial/^Account$$/MigrateV0' PKG=securityhub
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/securityhub/... -v -count 1 -parallel 20  -run=TestAccSecurityHub_serial/^Account$/MigrateV0 -timeout 360m
=== RUN   TestAccSecurityHub_serial
=== PAUSE TestAccSecurityHub_serial
=== CONT  TestAccSecurityHub_serial
=== RUN   TestAccSecurityHub_serial/Account
=== RUN   TestAccSecurityHub_serial/Account/MigrateV0
    account_test.go:132: Step 2/2 error: After applying this test step, the plan was not empty.
        stdout:
        
        
        Terraform used the selected providers to generate the following execution
        plan. Resource actions are indicated with the following symbols:
          ~ update in-place
        
        Terraform will perform the following actions:
        
          # aws_securityhub_account.test will be updated in-place
          ~ resource "aws_securityhub_account" "test" {
              + auto_enable_controls     = true
                id                       = "346386234494"
                # (1 unchanged attribute hidden)
            }
        
        Plan: 0 to add, 1 to change, 0 to destroy.
    testing_new.go:80: Error retrieving state, there may be dangling resources: exit status 1
        Failed to marshal state to json: schema version 0 for aws_securityhub_account.test in state does not match version 1 from the provider
--- FAIL: TestAccSecurityHub_serial (45.22s)
    --- FAIL: TestAccSecurityHub_serial/Account (45.22s)
        --- FAIL: TestAccSecurityHub_serial/Account/MigrateV0 (45.22s)
FAIL
FAIL	github.com/hashicorp/terraform-provider-aws/internal/service/securityhub	51.015s
FAIL
make: *** [testacc] Error 1

[ewbankkit]

Verified that removing PlanOnly: true, from the 2nd test step (so that the state upgrader is run) resolves this particular problem.

[bflad]

We discussed this out of band a little bit and it feels like the test failure is potentially a better behavior. Essentially what is happening is that the testing prior was doing (during the second step with PlanOnly):

• terraform refresh
• terraform plan -refresh=false
• terraform plan

The state upgrade would get applied to the Terraform state as part of the explicit refresh command so the plan would show no differences (pass!). However for most practitioners, this wouldn't be a normal workflow in modern Terraform, as explicit refresh commands are discouraged. Now the testing is just running the plan commands, which correctly want to apply the state upgrader change(s) (fail!).

There are a few options for handling this type of new test failure:

• Add ExpectNonEmptyPlan: true since there are expected plan differences, which is unfortunate in the sense it gives no context what why there are expected plan differences without an additional code comment
• Remove PlanOnly: true and optionally add ConfigPlanChecks to verify the resource behavior

If this is a large enough compatibility issue, I think we would certainly be amenable to reverting the refresh command removal and saving it until the next major version, but I also think we should have some amount of leeway in having the testing framework trying to replicate true practitioner workflows and updating the testing logic to match.

[ewbankkit]

For the AWS Provider we probably have only at most a dozen test cases with that PlanOnly: true, pattern -- we'll open an issue in our own repo and address them.

[bflad]

This issue is remaining open to capture developer burden for the refresh change in v1.6.0 for state upgrade testing. The recommended path for developers is to replace PlanOnly: true from the final TestStep with ConfigPlanChecks containing a PreApply check of plancheck.ExpectEmptyPlan() instead. I will submit a changelog and release notes update to explicitly call that out since we now know about the behavior change. If we do not hear additional commentary in the next few weeks, this will get closed out. 👍