From d0b7ed129c9824db333a1b0b3c9de73006e57c48 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Tue, 10 Dec 2024 14:26:55 -0800 Subject: [PATCH] Removes unused `Check`s when test step has `ExpectError` --- .ci/semgrep/acctest/steps.yml | 15 +++++++++++++++ internal/service/ec2/ec2_instance_test.go | 7 +------ internal/service/ec2/vpc_security_group_test.go | 14 +------------- .../service/ecr/pull_through_cache_rule_test.go | 8 ++------ .../ecr/repository_creation_template_test.go | 8 ++------ .../service/elasticache/replication_group_test.go | 7 +------ internal/service/elasticsearch/domain_test.go | 9 +-------- internal/service/iot/role_alias_test.go | 5 +---- internal/service/opensearch/domain_test.go | 8 +------- internal/service/s3/bucket_metric_test.go | 7 +------ 10 files changed, 26 insertions(+), 62 deletions(-) create mode 100644 .ci/semgrep/acctest/steps.yml diff --git a/.ci/semgrep/acctest/steps.yml b/.ci/semgrep/acctest/steps.yml new file mode 100644 index 00000000000..29daba47e8c --- /dev/null +++ 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 index 2740afb15fb..1484628a54a 100644 --- 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 index 62c6d45bffc..950cd2a5fd9 100644 --- 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 index ca216eb8a8f..0fae1126993 100644 --- 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 index e9c9d50c203..ddc462f2aaf 100644 --- 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 index e812f105ee4..e0a4b3a3e03 100644 --- 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 index c5d0bfa95d4..d9cb55b5e7c 100644 --- 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 index f31b10c1261..958883779d5 100644 --- 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 index 46de6b161f2..8764130e3cf 100644 --- 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 index 9e6d533b8ec..45c4937b1a3 100644 --- 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`), }, },