From dcb459787aa134a4543ba64072d8a3c1d44bb653 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Wed, 29 Apr 2020 17:05:46 -0400 Subject: [PATCH] tests/provider: Introduce shared disappears TestCheckFunc, refactor Backup testing to implement and verify (#12864) * tests/provider: Introduce shared disappears TestCheckFunc, refactor Backup testing to implement and verify A common problem in Terraform Providers is the need to verify that a Terraform Resource will successfully remove itself from the Terraform state when externally deleted. In an effort to test this functionality, the Terraform AWS Provider has implemented testing, commonly named `_disappears` in each resource's acceptance testing, where each test includes a `TestCheckFunc` that manually reimplements the deletion functionality of the resource. This change proposes the introduction of a shared `TestCheckFunc` that can prevent the necessity of manually reimplementing the resource deletion code in a `TestCheckFunc` for each resource. If acceptable, a GitHub issue will be created outlining a refactoring and documentation proposal for the provider codebase. It is worth noting that in the future, the Terraform Plugin SDK's acceptance testing framework could introduce native functionality for this type of common testing, but this shared `TestCheckFunc` is an effort to reduce a current development pain point. Output from acceptance testing: ``` --- PASS: TestAccAwsBackupVault_disappears (13.75s) --- PASS: TestAccAwsBackupPlan_disappears (18.15s) --- PASS: TestAccAwsBackupSelection_disappears (24.02s) ``` * tests/provider: Remove extra colon in testAccCheckResourceDisappears error message Reference: https://github.com/terraform-providers/terraform-provider-aws/pull/12864#pullrequestreview-394967599 --- aws/provider_test.go | 16 ++++++++++++++++ aws/resource_aws_backup_plan_test.go | 16 +--------------- aws/resource_aws_backup_selection_test.go | 21 ++++----------------- aws/resource_aws_backup_vault_test.go | 14 +------------- 4 files changed, 22 insertions(+), 45 deletions(-) diff --git a/aws/provider_test.go b/aws/provider_test.go index 84a84f10218..5c76f99e72e 100644 --- a/aws/provider_test.go +++ b/aws/provider_test.go @@ -587,6 +587,22 @@ func testAccAwsRegionProviderFunc(region string, providers *[]*schema.Provider) } } +func testAccCheckResourceDisappears(provider *schema.Provider, resource *schema.Resource, resourceName string) resource.TestCheckFunc { + return func(s *terraform.State) error { + resourceState, ok := s.RootModule().Resources[resourceName] + + if !ok { + return fmt.Errorf("resource not found: %s", resourceName) + } + + if resourceState.Primary.ID == "" { + return fmt.Errorf("resource ID missing: %s", resourceName) + } + + return resource.Delete(resource.Data(resourceState.Primary), provider.Meta()) + } +} + func testAccCheckWithProviders(f func(*terraform.State, *schema.Provider) error, providers *[]*schema.Provider) resource.TestCheckFunc { return func(s *terraform.State) error { numberOfProviders := len(*providers) diff --git a/aws/resource_aws_backup_plan_test.go b/aws/resource_aws_backup_plan_test.go index 85c46131250..028771c64a5 100644 --- a/aws/resource_aws_backup_plan_test.go +++ b/aws/resource_aws_backup_plan_test.go @@ -465,7 +465,7 @@ func TestAccAwsBackupPlan_disappears(t *testing.T) { Config: testAccAwsBackupPlanConfig_basic(rName), Check: resource.ComposeTestCheckFunc( testAccCheckAwsBackupPlanExists(resourceName, &plan, &ruleNameMap), - testAccCheckAwsBackupPlanDisappears(&plan), + testAccCheckResourceDisappears(testAccProvider, resourceAwsBackupPlan(), resourceName), ), ExpectNonEmptyPlan: true, }, @@ -496,20 +496,6 @@ func testAccCheckAwsBackupPlanDestroy(s *terraform.State) error { return nil } -func testAccCheckAwsBackupPlanDisappears(backupPlan *backup.GetBackupPlanOutput) resource.TestCheckFunc { - return func(s *terraform.State) error { - conn := testAccProvider.Meta().(*AWSClient).backupconn - - input := &backup.DeleteBackupPlanInput{ - BackupPlanId: backupPlan.BackupPlanId, - } - - _, err := conn.DeleteBackupPlan(input) - - return err - } -} - func testAccCheckAwsBackupPlanExists(name string, plan *backup.GetBackupPlanOutput, ruleNameMap *map[string]string) resource.TestCheckFunc { return func(s *terraform.State) error { conn := testAccProvider.Meta().(*AWSClient).backupconn diff --git a/aws/resource_aws_backup_selection_test.go b/aws/resource_aws_backup_selection_test.go index 3963decd041..647c83f05fa 100644 --- a/aws/resource_aws_backup_selection_test.go +++ b/aws/resource_aws_backup_selection_test.go @@ -39,6 +39,8 @@ func TestAccAwsBackupSelection_basic(t *testing.T) { func TestAccAwsBackupSelection_disappears(t *testing.T) { var selection1 backup.GetBackupSelectionOutput rInt := acctest.RandInt() + resourceName := "aws_backup_selection.test" + resource.ParallelTest(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t); testAccPreCheckAWSBackup(t) }, Providers: testAccProviders, @@ -47,8 +49,8 @@ func TestAccAwsBackupSelection_disappears(t *testing.T) { { Config: testAccBackupSelectionConfigBasic(rInt), Check: resource.ComposeTestCheckFunc( - testAccCheckAwsBackupSelectionExists("aws_backup_selection.test", &selection1), - testAccCheckAwsBackupSelectionDisappears(&selection1), + testAccCheckAwsBackupSelectionExists(resourceName, &selection1), + testAccCheckResourceDisappears(testAccProvider, resourceAwsBackupSelection(), resourceName), ), ExpectNonEmptyPlan: true, }, @@ -190,21 +192,6 @@ func testAccCheckAwsBackupSelectionExists(name string, selection *backup.GetBack } } -func testAccCheckAwsBackupSelectionDisappears(selection *backup.GetBackupSelectionOutput) resource.TestCheckFunc { - return func(s *terraform.State) error { - conn := testAccProvider.Meta().(*AWSClient).backupconn - - input := &backup.DeleteBackupSelectionInput{ - BackupPlanId: selection.BackupPlanId, - SelectionId: selection.SelectionId, - } - - _, err := conn.DeleteBackupSelection(input) - - return err - } -} - func testAccCheckAwsBackupSelectionRecreated(t *testing.T, before, after *backup.GetBackupSelectionOutput) resource.TestCheckFunc { return func(s *terraform.State) error { diff --git a/aws/resource_aws_backup_vault_test.go b/aws/resource_aws_backup_vault_test.go index 13e319c3965..5ce6282c93f 100644 --- a/aws/resource_aws_backup_vault_test.go +++ b/aws/resource_aws_backup_vault_test.go @@ -124,7 +124,7 @@ func TestAccAwsBackupVault_disappears(t *testing.T) { Config: testAccBackupVaultConfig(rInt), Check: resource.ComposeTestCheckFunc( testAccCheckAwsBackupVaultExists(resourceName, &vault), - testAccCheckAwsBackupVaultDisappears(&vault), + testAccCheckResourceDisappears(testAccProvider, resourceAwsBackupVault(), resourceName), ), ExpectNonEmptyPlan: true, }, @@ -177,18 +177,6 @@ func testAccCheckAwsBackupVaultExists(name string, vault *backup.DescribeBackupV } } -func testAccCheckAwsBackupVaultDisappears(vault *backup.DescribeBackupVaultOutput) resource.TestCheckFunc { - return func(s *terraform.State) error { - conn := testAccProvider.Meta().(*AWSClient).backupconn - params := &backup.DeleteBackupVaultInput{ - BackupVaultName: vault.BackupVaultName, - } - _, err := conn.DeleteBackupVault(params) - - return err - } -} - func testAccPreCheckAWSBackup(t *testing.T) { conn := testAccProvider.Meta().(*AWSClient).backupconn