Pass pre-destroy state to CheckDestroy as expected by the documentation

[enk21]

Currently, the CheckDestroy function is being passed the state after the destroy occurs. In most cases, the resources will be empty so the function can't verify that the resources have been disposed / handled as expected.

I wrote up a discuss post here with some more details.

According to the documentation, the CheckDestroy method ‘receives the last known Terraform state as input’.

The comment in the code says: 'CheckDestroy is called after the resource is finally destroyed to allow the tester to test that the resource is truly gone’.

The documentation for the CheckDestroy type states: the ‘TestCheckFunc is the callback type used with acceptance tests to check the state of a resource. The state passed in is the latest state known, or in the case of being after a destroy, it is the last known state when it was created’.

This PR will pass the pre-destroy state to the runPostTestDestroy function, which will pass it to the CheckDestroy function where the user can verify that the destroy worked as expected.

[hashicorp-cla]

All committers have signed the CLA.

[bflad]

Please note that I can confirm this is a notable regression in the binary testing framework by attempting to run the AWS Provider acceptance testing with v2.0.3 (as compared to our current v2.0.1), since errors are now being returned due to #581

New test failure (hidden prior to v2.0.3):

=== CONT  TestAccAWSSSMDocument_basic
TestAccAWSSSMDocument_basic: testing_new.go:68: Error running post-test destroy, there may be dangling resources: Default error in SSM Document Test
--- FAIL: TestAccAWSSSMDocument_basic (26.80s)

Some of our older style acceptance testing includes CheckDestroy logic that follows the below coding pattern (which presumably was ensuring that the pre-destroy state included at least one of the expected resource types and verified it was deleted in the API):

func testAccCheckAWSSSMDocumentDestroy(s *terraform.State) error {
	conn := testAccProvider.Meta().(*AWSClient).ssmconn

	for _, rs := range s.RootModule().Resources {
		if rs.Type != "aws_ssm_document" {
			continue
		}

		out, err := conn.DescribeDocument(&ssm.DescribeDocumentInput{
			Name: aws.String(rs.Primary.Attributes["name"]),
		})

		if err != nil {
			// InvalidDocument means it's gone, this is good
			if wserr, ok := err.(awserr.Error); ok && wserr.Code() == ssm.ErrCodeInvalidDocument {
				return nil
			}
			return err
		}

		if out != nil {
			return fmt.Errorf("Expected AWS SSM Document to be gone, but was still found")
		}

		return nil
	}

	return fmt.Errorf("Default error in SSM Document Test")
}

In the old testing framework, that error would not trigger. Many of our newer CheckDestroy functions return nil outside the loop and are hiding this issue.

[bflad]

The fix seems correct here as the legacy testing framework after all steps ran the final destroy and followup CheckDestroy using the state from the last step import/config apply:

terraform-plugin-sdk/helper/resource/testing.go

Lines 677 to 679 in 3f85e93

	state, err = testStepImportState(opts, state, step)
	} else {
	state, err = testStepConfig(opts, state, step)

terraform-plugin-sdk/helper/resource/testing.go

Lines 753 to 764 in 3f85e93

	lastStep := c.Steps[len(c.Steps)-1]
	destroyStep := TestStep{
	Config: lastStep.Config,
	Check: c.CheckDestroy,
	Destroy: true,
	PreventDiskCleanup: lastStep.PreventDiskCleanup,
	PreventPostDestroyRefresh: c.PreventPostDestroyRefresh,
	providers: providers,
	}
	log.Printf("[WARN] Test: Executing destroy step")
	state, err := testStep(opts, state, destroyStep)

I can confirm the above test is fixed with this:

--- PASS: TestAccAWSSSMDocument_basic (15.86s)

[kmoe]

LGTM, but please wait for additional review by @paddycarver, who last touched this code.

[paddycarver]

This looks good to me, and reasonable. Thanks @enk21 and @bflad for the investigation and bugfix!

[ghost]

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.