Merge pull request hashicorp#40513 from hashicorp/td-acctest-checks-w…

…ith-expecterror Removes unused `Check`s when test step has `ExpectError`
opentofu · Dec 10, 2024 · c672825 · c672825
2 parents dbf6f42 + d0b7ed1
commit c672825
Show file tree

Hide file tree

Showing 10 changed files with 26 additions and 62 deletions.
diff --git a/.ci/semgrep/acctest/steps.yml b/.ci/semgrep/acctest/steps.yml
@@ -0,0 +1,15 @@
+rules:
+  - id: checks-with-expecterror
+    languages: [go]
+    message: Checks are ignored when ExpectError is set.
+    patterns:
+      - pattern-inside: "Steps: []resource.TestStep{ ... }"
+      - pattern: |
+          {
+            ...,
+            Check:       $CHECK,
+            ...,
+            ExpectError: $ERR,
+            ...,
+          }
+    severity: ERROR
diff --git a/internal/service/ec2/ec2_instance_test.go b/internal/service/ec2/ec2_instance_test.go
@@ -1373,12 +1373,7 @@ func TestAccEC2Instance_networkInstanceRemovingAllSecurityGroups(t *testing.T) {
 				},
 			},
 			{
-				Config: testAccInstanceConfig_networkVPCRemoveSecurityGroupIDs(rName),
-				Check: resource.ComposeTestCheckFunc(
-					testAccCheckInstanceExists(ctx, resourceName, &v),
-					resource.TestCheckResourceAttr(resourceName, "security_groups.#", "0"),
-					resource.TestCheckResourceAttr(resourceName, "vpc_security_group_ids.#", "1"),
-				),
+				Config:      testAccInstanceConfig_networkVPCRemoveSecurityGroupIDs(rName),
 				ExpectError: regexache.MustCompile(`VPC-based instances require at least one security group to be attached`),
 			},
 		},

diff --git a/internal/service/ec2/vpc_security_group_test.go b/internal/service/ec2/vpc_security_group_test.go
@@ -2653,8 +2653,6 @@ func TestAccVPCSecurityGroup_emrDependencyViolation(t *testing.T) {
 		t.Skip("skipping long-running test in short mode")
 	}
 
-	var group awstypes.SecurityGroup
-	resourceName := "aws_security_group.allow_access"
 	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
 
 	resource.ParallelTest(t, resource.TestCase{
@@ -2664,17 +2662,7 @@ func TestAccVPCSecurityGroup_emrDependencyViolation(t *testing.T) {
 		CheckDestroy:             testAccCheckSecurityGroupDestroy(ctx),
 		Steps: []resource.TestStep{
 			{
-				Config: testAccVPCSecurityGroupConfig_emrLinkedRulesDestroy(rName),
-				Check: resource.ComposeAggregateTestCheckFunc(
-					testAccCheckSecurityGroupExists(ctx, resourceName, &group),
-					acctest.MatchResourceAttrRegionalARN(ctx, resourceName, names.AttrARN, "ec2", regexache.MustCompile(`security-group/.+$`)),
-					resource.TestCheckResourceAttr(resourceName, "egress.#", "1"),
-					resource.TestCheckResourceAttr(resourceName, "ingress.#", "1"),
-					acctest.CheckResourceAttrAccountID(ctx, resourceName, names.AttrOwnerID),
-					resource.TestCheckResourceAttr(resourceName, "revoke_rules_on_delete", acctest.CtTrue),
-					resource.TestCheckResourceAttr(resourceName, acctest.CtTagsPercent, "1"),
-					resource.TestCheckResourceAttrPair(resourceName, names.AttrVPCID, "aws_vpc.test", names.AttrID),
-				),
+				Config:      testAccVPCSecurityGroupConfig_emrLinkedRulesDestroy(rName),
 				ExpectError: regexache.MustCompile("unexpected state"),
 			},
 		},

diff --git a/internal/service/ecr/pull_through_cache_rule_test.go b/internal/service/ecr/pull_through_cache_rule_test.go
@@ -105,7 +105,6 @@ func TestAccECRPullThroughCacheRule_disappears(t *testing.T) {
 func TestAccECRPullThroughCacheRule_failWhenAlreadyExists(t *testing.T) {
 	ctx := acctest.Context(t)
 	repositoryPrefix := "tf-test-" + sdkacctest.RandString(8)
-	resourceName := "aws_ecr_pull_through_cache_rule.test"
 
 	if acctest.Partition() == "aws-us-gov" {
 		t.Skip("ECR Pull Through Cache Rule is not supported in GovCloud partition")
@@ -118,10 +117,7 @@ func TestAccECRPullThroughCacheRule_failWhenAlreadyExists(t *testing.T) {
 		CheckDestroy:             testAccCheckPullThroughCacheRuleDestroy(ctx),
 		Steps: []resource.TestStep{
 			{
-				Config: testAccPullThroughCacheRuleConfig_failWhenAlreadyExist(repositoryPrefix),
-				Check: resource.ComposeTestCheckFunc(
-					testAccCheckPullThroughCacheRuleExists(ctx, resourceName),
-				),
+				Config:      testAccPullThroughCacheRuleConfig_failWhenAlreadyExists(repositoryPrefix),
 				ExpectError: regexache.MustCompile(`PullThroughCacheRuleAlreadyExistsException`),
 			},
 		},
@@ -222,7 +218,7 @@ resource "aws_ecr_pull_through_cache_rule" "test" {
 `, repositoryPrefix)
 }
 
-func testAccPullThroughCacheRuleConfig_failWhenAlreadyExist(repositoryPrefix string) string {
+func testAccPullThroughCacheRuleConfig_failWhenAlreadyExists(repositoryPrefix string) string {
 	return fmt.Sprintf(`
 resource "aws_ecr_pull_through_cache_rule" "test" {
   ecr_repository_prefix = %[1]q

diff --git a/internal/service/ecr/repository_creation_template_test.go b/internal/service/ecr/repository_creation_template_test.go
@@ -87,7 +87,6 @@ func TestAccECRRepositoryCreationTemplate_disappears(t *testing.T) {
 func TestAccECRRepositoryCreationTemplate_failWhenAlreadyExists(t *testing.T) {
 	ctx := acctest.Context(t)
 	repositoryPrefix := "tf-test-" + sdkacctest.RandString(8)
-	resourceName := "aws_ecr_repository_creation_template.test"
 
 	resource.ParallelTest(t, resource.TestCase{
 		PreCheck:                 func() { acctest.PreCheck(ctx, t) },
@@ -96,10 +95,7 @@ func TestAccECRRepositoryCreationTemplate_failWhenAlreadyExists(t *testing.T) {
 		CheckDestroy:             testAccCheckRepositoryCreationTemplateDestroy(ctx),
 		Steps: []resource.TestStep{
 			{
-				Config: testAccRepositoryCreationTemplateConfig_failWhenAlreadyExist(repositoryPrefix),
-				Check: resource.ComposeTestCheckFunc(
-					testAccCheckRepositoryCreationTemplateExists(ctx, resourceName),
-				),
+				Config:      testAccRepositoryCreationTemplateConfig_failWhenAlreadyExists(repositoryPrefix),
 				ExpectError: regexache.MustCompile(`TemplateAlreadyExistsException`),
 			},
 		},
@@ -246,7 +242,7 @@ resource "aws_ecr_repository_creation_template" "test" {
 `, repositoryPrefix)
 }
 
-func testAccRepositoryCreationTemplateConfig_failWhenAlreadyExist(repositoryPrefix string) string {
+func testAccRepositoryCreationTemplateConfig_failWhenAlreadyExists(repositoryPrefix string) string {
 	return fmt.Sprintf(`
 resource "aws_ecr_repository_creation_template" "test" {
   prefix = %[1]q

diff --git a/internal/service/elasticache/replication_group_test.go b/internal/service/elasticache/replication_group_test.go
@@ -366,12 +366,7 @@ func TestAccElastiCacheReplicationGroup_Engine_RedisToValkey(t *testing.T) {
 				),
 			},
 			{
-				Config: testAccReplicationGroupConfig_basic_engine(rName, "valkey"),
-				Check: resource.ComposeAggregateTestCheckFunc(
-					testAccCheckReplicationGroupExists(ctx, resourceName, &v2),
-					testAccCheckReplicationGroupNotRecreated(&v1, &v2),
-					resource.TestCheckResourceAttr(resourceName, names.AttrEngine, "valkey"),
-				),
+				Config:      testAccReplicationGroupConfig_basic_engine(rName, "valkey"),
 				ExpectError: regexache.MustCompile("must explicitly set 'engine_version' attribute"),
 			},
 			{

diff --git a/internal/service/elasticsearch/domain_test.go b/internal/service/elasticsearch/domain_test.go
@@ -352,9 +352,7 @@ func TestAccElasticsearchDomain_duplicate(t *testing.T) {
 		t.Skip("skipping long-running test in short mode")
 	}
 
-	var domain awstypes.ElasticsearchDomainStatus
 	rName := testAccRandomDomainName()
-	resourceName := "aws_elasticsearch_domain.test"
 
 	resource.ParallelTest(t, resource.TestCase{
 		PreCheck:                 func() { acctest.PreCheck(ctx, t); testAccPreCheckIAMServiceLinkedRole(ctx, t) },
@@ -388,12 +386,7 @@ func TestAccElasticsearchDomain_duplicate(t *testing.T) {
 						t.Fatal(err)
 					}
 				},
-				Config: testAccDomainConfig_basic(rName),
-				Check: resource.ComposeTestCheckFunc(
-					testAccCheckDomainExists(ctx, resourceName, &domain),
-					resource.TestCheckResourceAttr(
-						resourceName, "elasticsearch_version", "1.5"),
-				),
+				Config:      testAccDomainConfig_basic(rName),
 				ExpectError: regexache.MustCompile(`Elasticsearch Domain .+ already exists`),
 			},
 		},

diff --git a/internal/service/iot/role_alias_test.go b/internal/service/iot/role_alias_test.go
@@ -56,10 +56,7 @@ func TestAccIoTRoleAlias_basic(t *testing.T) {
 				Check:  resource.ComposeTestCheckFunc(testAccCheckRoleAliasExists(ctx, resourceName2)),
 			},
 			{
-				Config: testAccRoleAliasConfig_update3(alias, alias2),
-				Check: resource.ComposeTestCheckFunc(
-					testAccCheckRoleAliasExists(ctx, resourceName2),
-				),
+				Config:      testAccRoleAliasConfig_update3(alias, alias2),
 				ExpectError: regexache.MustCompile("Role alias .+? already exists for this account"),
 			},
 			{

diff --git a/internal/service/opensearch/domain_test.go b/internal/service/opensearch/domain_test.go
@@ -542,9 +542,7 @@ func TestAccOpenSearchDomain_duplicate(t *testing.T) {
 		t.Skip("skipping long-running test in short mode")
 	}
 
-	var domain awstypes.DomainStatus
 	rName := testAccRandomDomainName()
-	resourceName := "aws_opensearch_domain.test"
 
 	resource.ParallelTest(t, resource.TestCase{
 		PreCheck:                 func() { acctest.PreCheck(ctx, t); testAccPreCheckIAMServiceLinkedRole(ctx, t) },
@@ -578,11 +576,7 @@ func TestAccOpenSearchDomain_duplicate(t *testing.T) {
 						t.Fatal(err)
 					}
 				},
-				Config: testAccDomainConfig_basic(rName),
-				Check: resource.ComposeTestCheckFunc(
-					testAccCheckDomainExists(ctx, resourceName, &domain),
-					resource.TestCheckResourceAttrSet(resourceName, names.AttrEngineVersion),
-				),
+				Config:      testAccDomainConfig_basic(rName),
 				ExpectError: regexache.MustCompile(`OpenSearch Domain ".+" already exists`),
 			},
 		},

diff --git a/internal/service/s3/bucket_metric_test.go b/internal/service/s3/bucket_metric_test.go
@@ -57,9 +57,7 @@ func TestAccS3BucketMetric_basic(t *testing.T) {
 // Disallow Empty filter block
 func TestAccS3BucketMetric_withEmptyFilter(t *testing.T) {
 	ctx := acctest.Context(t)
-	var conf types.MetricsConfiguration
 	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
-	resourceName := "aws_s3_bucket_metric.test"
 	metricName := t.Name()
 
 	resource.ParallelTest(t, resource.TestCase{
@@ -69,10 +67,7 @@ func TestAccS3BucketMetric_withEmptyFilter(t *testing.T) {
 		CheckDestroy:             testAccCheckBucketMetricDestroy(ctx),
 		Steps: []resource.TestStep{
 			{
-				Config: testAccBucketMetricConfig_emptyFilter(rName, metricName),
-				Check: resource.ComposeTestCheckFunc(
-					testAccCheckBucketMetricsExistsConfig(ctx, resourceName, &conf),
-				),
+				Config:      testAccBucketMetricConfig_emptyFilter(rName, metricName),
 				ExpectError: regexache.MustCompile(`(?is)one of.*must be specified`),
 			},
 		},