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

Document, Standardize, and Potentially Lint for Parent Resource Disappears Test Naming #16591

Closed
9 tasks
bflad opened this issue Dec 4, 2020 · 3 comments · Fixed by #16597
Closed
9 tasks
Assignees
Labels
documentation Introduces or discusses updates to documentation. linter Pertains to changes to or issues with the various linters. provider Pertains to the provider itself, rather than any interaction with AWS. technical-debt Addresses areas of the codebase that need refactoring or redesign. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Milestone

Comments

@bflad
Copy link
Contributor

bflad commented Dec 4, 2020

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

We currently document disappears testing for the simplest case (the resource itself) in the Contributing Guide, however there is not documentation for how to handle disappears testing designed for children resources where the test is meant to verify that removing the parent resource occurs in a recreation plan rather than an error, e.g. for verifying VPC deletion for a Route 53 Zone Association

func TestAccAWSRoute53ZoneAssociation_disappears_VPC(t *testing.T) {
	resourceName := "aws_route53_zone_association.foobar"
	vpcResourceName := "aws_vpc.bar"

	resource.ParallelTest(t, resource.TestCase{
		PreCheck:     func() { testAccPreCheck(t) },
		ErrorCheck:   testAccErrorCheckSkipRoute53(t),
		Providers:    testAccProviders,
		CheckDestroy: testAccCheckRoute53ZoneAssociationDestroy,
		Steps: []resource.TestStep{
			{
				Config: testAccRoute53ZoneAssociationConfig,
				Check: resource.ComposeTestCheckFunc(
					testAccCheckRoute53ZoneAssociationExists(resourceName),
					testAccCheckResourceDisappears(testAccProvider, resourceAwsVpc(), vpcResourceName),
				),
				ExpectNonEmptyPlan: true,
			},
		},
	})
}

There are also two current styles for naming these tests, where the first should be preferred due to higher prevalence:

$ ag --stats 'func TestAcc.*_disappears_' aws/*_test.go
...
29 matches
25 files contained matches
...
$ ag --stats 'func TestAcc.*_[^_]+Disappears' aws/*_test.go
...
9 matches
9 files contained matches
...

We can document the preferred style and example implementation in the Contributing Guide. We may also be able to create a semgrep rule that reports these.

Affected Resource(s)

  • aws/resource_aws_backup_selection_test.go
  • aws/resource_aws_emr_instance_group_test.go
  • aws/resource_aws_lambda_event_source_mapping_test.go
  • aws/resource_aws_launch_configuration_test.go
  • aws/resource_aws_msk_scram_secret_association_test.go
  • aws/resource_aws_network_acl_rule_test.go
  • aws/resource_aws_s3_access_point_test.go
  • aws/resource_aws_s3_bucket_public_access_block_test.go
  • aws/resource_aws_wafv2_web_acl_logging_configuration_test.go

References

@bflad bflad added documentation Introduces or discusses updates to documentation. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. provider Pertains to the provider itself, rather than any interaction with AWS. linter Pertains to changes to or issues with the various linters. labels Dec 4, 2020
@bflad
Copy link
Contributor Author

bflad commented Dec 4, 2020

Example semgrep rule:

  - id: acceptance-test-naming-parent-disappears
    languages: [go]
    message: Prefer naming acceptance tests with _disappears_Parent suffix
    paths:
      include:
        - 'aws/*_test.go'
    patterns:
      - pattern: func $FUNCNAME(t *testing.T) { ... }
      - metavariable-regex:
          metavariable: "$FUNCNAME"
          regex: "^TestAcc[^_]+_[a-zA-Z]+[dD]isappears$"
    severity: WARNING
running 1 rules...
aws/resource_aws_backup_selection_test.go
severity:warning rule:acceptance-test-naming-parent-disappears: Prefer naming acceptance tests with _disappears_Parent suffix
62:func TestAccAwsBackupSelection_backupPlanDisappears(t *testing.T) {
63:	var selection1 backup.GetBackupSelectionOutput
64:	resourceName := "aws_backup_selection.test"
65:	backupPlanResourceName := "aws_backup_plan.test"
66:	rName := acctest.RandomWithPrefix("tf-acc-test")
67:
68:	resource.ParallelTest(t, resource.TestCase{
69:		PreCheck:     func() { testAccPreCheck(t); testAccPreCheckAWSBackup(t) },
70:		Providers:    testAccProviders,
71:		CheckDestroy: testAccCheckAwsBackupSelectionDestroy,
72:		Steps: []resource.TestStep{
73:			{
74:				Config: testAccBackupSelectionConfigBasic(rName),
75:				Check: resource.ComposeTestCheckFunc(
76:					testAccCheckAwsBackupSelectionExists(resourceName, &selection1),
77:					testAccCheckResourceDisappears(testAccProvider, resourceAwsBackupSelection(), resourceName),
78:					testAccCheckResourceDisappears(testAccProvider, resourceAwsBackupPlan(), backupPlanResourceName),
79:				),
80:				ExpectNonEmptyPlan: true,
81:			},
82:		},
83:	})
84:}

aws/resource_aws_emr_instance_group_test.go
severity:warning rule:acceptance-test-naming-parent-disappears: Prefer naming acceptance tests with _disappears_Parent suffix
205:func TestAccAWSEMRInstanceGroup_EmrClusterDisappears(t *testing.T) {
206:	var cluster emr.Cluster
207:	var ig emr.InstanceGroup
208:	rInt := acctest.RandInt()
209:	emrClusterResourceName := "aws_emr_cluster.tf-test-cluster"
210:	resourceName := "aws_emr_instance_group.task"
211:
212:	resource.ParallelTest(t, resource.TestCase{
213:		PreCheck:     func() { testAccPreCheck(t) },
214:		Providers:    testAccProviders,
215:		CheckDestroy: testAccCheckAWSEmrInstanceGroupDestroy,
216:		Steps: []resource.TestStep{
217:			{
218:				Config: testAccAWSEmrInstanceGroupConfig_basic(rInt),
219:				Check: resource.ComposeTestCheckFunc(
220:					testAccCheckAWSEmrClusterExists(emrClusterResourceName, &cluster),
221:					testAccCheckAWSEmrInstanceGroupExists(resourceName, &ig),
222:					testAccCheckAWSEmrClusterDisappears(&cluster),
223:				),
224:				ExpectNonEmptyPlan: true,
225:			},
226:		},
227:	})
228:}

aws/resource_aws_lambda_event_source_mapping_test.go
severity:warning rule:acceptance-test-naming-parent-disappears: Prefer naming acceptance tests with _disappears_Parent suffix
277:func TestAccAWSLambdaEventSourceMapping_sqsDisappears(t *testing.T) {
278:	var conf lambda.EventSourceMappingConfiguration
279:
280:	rString := acctest.RandString(8)
281:	roleName := fmt.Sprintf("tf_acc_role_lambda_sqs_import_%s", rString)
282:	policyName := fmt.Sprintf("tf_acc_policy_lambda_sqs_import_%s", rString)
283:	attName := fmt.Sprintf("tf_acc_att_lambda_sqs_import_%s", rString)
284:	streamName := fmt.Sprintf("tf_acc_stream_lambda_sqs_import_%s", rString)
285:	funcName := fmt.Sprintf("tf_acc_lambda_sqs_import_%s", rString)
286:	uFuncName := fmt.Sprintf("tf_acc_lambda_sqs_import_updated_%s", rString)
287:
288:	resource.ParallelTest(t, resource.TestCase{
289:		PreCheck:     func() { testAccPreCheck(t) },
290:		Providers:    testAccProviders,
291:		CheckDestroy: testAccCheckLambdaEventSourceMappingDestroy,
292:		Steps: []resource.TestStep{
293:			{
294:				Config: testAccAWSLambdaEventSourceMappingConfig_sqs(roleName, policyName, attName, streamName, funcName, uFuncName),
295:				Check: resource.ComposeTestCheckFunc(
296:					testAccCheckAwsLambdaEventSourceMappingExists("aws_lambda_event_source_mapping.lambda_event_source_mapping_test", &conf),
297:					testAccCheckAWSLambdaEventSourceMappingDisappears(&conf),
298:				),
299:				ExpectNonEmptyPlan: true,
300:			},
301:		},
302:	})
303:}

aws/resource_aws_msk_scram_secret_association_test.go
severity:warning rule:acceptance-test-naming-parent-disappears: Prefer naming acceptance tests with _disappears_Parent suffix
112:func TestAccAwsMskScramSecretAssociation_clusterDisappears(t *testing.T) {
113:	rName := acctest.RandomWithPrefix("tf-acc-test")
114:	resourceName := "aws_msk_scram_secret_association.test"
115:	clusterResourceName := "aws_msk_cluster.test"
116:
117:	resource.ParallelTest(t, resource.TestCase{
118:		PreCheck:     func() { testAccPreCheck(t); testAccPreCheckAWSMsk(t) },
119:		Providers:    testAccProviders,
120:		CheckDestroy: testAccCheckMskScramSecretAssociationDestroy,
121:		Steps: []resource.TestStep{
122:			{
123:				Config: testAccMskScramSecretAssociation_basic(rName, 1),
124:				Check: resource.ComposeTestCheckFunc(
125:					testAccCheckMskScramSecretAssociationExists(resourceName),
126:					testAccCheckResourceDisappears(testAccProvider, resourceAwsMskCluster(), clusterResourceName),
127:				),
128:				ExpectNonEmptyPlan: true,
129:			},
130:		},
131:	})
132:}

aws/resource_aws_network_acl_rule_test.go
severity:warning rule:acceptance-test-naming-parent-disappears: Prefer naming acceptance tests with _disappears_Parent suffix
71:func TestAccAWSNetworkAclRule_ingressEgressSameNumberDisappears(t *testing.T) {
72:	resource.ParallelTest(t, resource.TestCase{
73:		PreCheck:     func() { testAccPreCheck(t) },
74:		Providers:    testAccProviders,
75:		CheckDestroy: testAccCheckAWSNetworkAclRuleDestroy,
76:		Steps: []resource.TestStep{
77:			{
78:				Config: testAccAWSNetworkAclRuleIngressEgressSameNumberMissing,
79:				Check: resource.ComposeTestCheckFunc(
80:					testAccCheckAWSNetworkAclRuleExists("aws_network_acl_rule.baz"),
81:					testAccCheckAWSNetworkAclRuleDelete("aws_network_acl_rule.baz"),
82:				),
83:				ExpectNonEmptyPlan: true,
84:			},
85:		},
86:	})
87:}

aws/resource_aws_s3_access_point_test.go
severity:warning rule:acceptance-test-naming-parent-disappears: Prefer naming acceptance tests with _disappears_Parent suffix
144:func TestAccAWSS3AccessPoint_bucketDisappears(t *testing.T) {
145:	var v s3control.GetAccessPointOutput
146:	bucketName := acctest.RandomWithPrefix("tf-acc-test")
147:	accessPointName := acctest.RandomWithPrefix("tf-acc-test")
148:	resourceName := "aws_s3_access_point.test"
149:	bucketResourceName := "aws_s3_bucket.test"
150:
151:	resource.ParallelTest(t, resource.TestCase{
152:		PreCheck:     func() { testAccPreCheck(t) },
153:		Providers:    testAccProviders,
154:		CheckDestroy: testAccCheckAWSS3AccessPointDestroy,
155:		Steps: []resource.TestStep{
156:			{
157:				Config: testAccAWSS3AccessPointConfig_basic(bucketName, accessPointName),
158:				Check: resource.ComposeTestCheckFunc(
159:					testAccCheckAWSS3AccessPointExists(resourceName, &v),
160:					testAccCheckAWSS3DestroyBucket(bucketResourceName),
161:				),
162:				ExpectNonEmptyPlan: true,
163:			},
164:		},
165:	})
166:}

aws/resource_aws_s3_bucket_public_access_block_test.go
severity:warning rule:acceptance-test-naming-parent-disappears: Prefer naming acceptance tests with _disappears_Parent suffix
70:func TestAccAWSS3BucketPublicAccessBlock_bucketDisappears(t *testing.T) {
71:	var config s3.PublicAccessBlockConfiguration
72:	name := fmt.Sprintf("tf-test-bucket-%d", acctest.RandInt())
73:	resourceName := "aws_s3_bucket_public_access_block.bucket"
74:	bucketResourceName := "aws_s3_bucket.bucket"
75:
76:	resource.ParallelTest(t, resource.TestCase{
77:		PreCheck:     func() { testAccPreCheck(t) },
78:		Providers:    testAccProviders,
79:		CheckDestroy: testAccCheckAWSS3BucketDestroy,
80:		Steps: []resource.TestStep{
81:			{
82:				Config: testAccAWSS3BucketPublicAccessBlockConfig(name, "false", "false", "false", "false"),
83:				Check: resource.ComposeTestCheckFunc(
84:					testAccCheckAWSS3BucketPublicAccessBlockExists(resourceName, &config),
85:					testAccCheckAWSS3DestroyBucket(bucketResourceName),
86:				),
87:				ExpectNonEmptyPlan: true,
88:			},
89:		},
90:	})
91:}

aws/resource_aws_wafv2_web_acl_logging_configuration_test.go
severity:warning rule:acceptance-test-naming-parent-disappears: Prefer naming acceptance tests with _disappears_Parent suffix
204:func TestAccAwsWafv2WebACLLoggingConfiguration_webACLDisappears(t *testing.T) {
205:	var v wafv2.LoggingConfiguration
206:	rName := acctest.RandomWithPrefix("tf-acc-test")
207:	resourceName := "aws_wafv2_web_acl_logging_configuration.test"
208:	webACLResourceName := "aws_wafv2_web_acl.test"
209:
210:	resource.ParallelTest(t, resource.TestCase{
211:		PreCheck:     func() { testAccPreCheck(t); testAccPreCheckAWSWafv2ScopeRegional(t) },
212:		Providers:    testAccProviders,
213:		CheckDestroy: testAccCheckAwsWafv2WebACLLoggingConfigurationDestroy,
214:		Steps: []resource.TestStep{
215:			{
216:				Config: testAccAwsWafv2WebACLLoggingConfiguration_basic(rName),
217:				Check: resource.ComposeTestCheckFunc(
218:					testAccCheckAwsWafv2WebACLLoggingConfigurationExists(resourceName, &v),
219:					testAccCheckResourceDisappears(testAccProvider, resourceAwsWafv2WebACL(), webACLResourceName),
220:				),
221:				ExpectNonEmptyPlan: true,
222:			},
223:		},
224:	})
225:}

@bflad bflad self-assigned this Dec 4, 2020
bflad added a commit that referenced this issue Dec 4, 2020
@ewbankkit ewbankkit added the technical-debt Addresses areas of the codebase that need refactoring or redesign. label Dec 8, 2020
@bflad bflad added this to the v3.21.0 milestone Dec 9, 2020
bflad added a commit that referenced this issue Dec 9, 2020
@ghost
Copy link

ghost commented Dec 11, 2020

This has been released in version 3.21.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

ewbankkit added a commit to ewbankkit/terraform-provider-aws that referenced this issue Dec 29, 2020
@ghost
Copy link

ghost commented Jan 9, 2021

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Jan 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. linter Pertains to changes to or issues with the various linters. provider Pertains to the provider itself, rather than any interaction with AWS. technical-debt Addresses areas of the codebase that need refactoring or redesign. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
2 participants