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.

Sign up for GitHub

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

tests/ec2/ami: Rename tests #17849

Closed
wants to merge 2 commits into from
Closed

tests/ec2/ami: Rename tests #17849

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ed57509
tests/ec2/ami: Rename tests
YakDriver Feb 26, 2021
6e76421
tests/ec2/ami: Lower case after underscore
YakDriver Feb 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions aws/data_source_aws_ami_ids_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import (
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
)

func TestAccDataSourceAwsAmiIds_basic(t *testing.T) {
func TestAccEC2AMIIDsDataSource_basic(t *testing.T) {
Copy link
Contributor

@bflad bflad Mar 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What determines whether test naming should include a fully capitalized initialism? Should we enforce the standard?

I've personally leaned towards only capital first letters in test naming after the resource type underscores, even for initialisms, e.g. TestAccEc2AmiIdsDataSource_basic

Copy link
Member Author

@YakDriver YakDriver Mar 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike above, the Go convention is clear on this point:

Words in names that are initialisms or acronyms (e.g. "URL" or "NATO") have a consistent case. For example, "URL" should appear as "URL" or "url" (as in "urlPony", or "URLPony"), never as "Url".

Although the AWS SDK for Go breaks this rule a lot, I have seen efforts to change course and some of the newer APIs follow this rule occassionally. My vote is that we stick with convention and win the respect of Go coders naming-convention followers.

Copy link
Contributor

@bflad bflad Mar 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to enforce this, we will need to manually enumerate all initialisms and maintain the list going forward. For example, what does sesv2 convert into?

I would prefer to see a proposal on that before really going down this route since it affects all code and testing contributions, otherwise it will be easy to escape out of the desired convention. I'm guessing it would need to be a text file with a list or a Go file with a slice of strings, but we will need to figure out how to ensure it is usable by tooling that needs it and ensure that all contributors know how to add to it and when that is necessary.

For what it is worth, not using initialisms allows you to easily convert snake_case to PascalCase and lowercase to Titlecase without additional logic to also bake in the conversions. Not saying that needs to be an end-all goal, but I'm not sure if folks are really chasing after this project's code for "correctness" in this sense. More likely folks are probably interested in functionality that has readable code and works.

Copy link
Member Author

@YakDriver YakDriver Mar 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm specifically limiting the scope of changes to test names. We have non-breaking freedom with test names. However, fixing things in test names but not code introduces a level of inconsistency. For instance, removing "AWS" and "Aws" from test names but not all the resource names. Despite this issue, which I think is small, I love the idea of having one area where we have clean, Go convention adhereing names.

Following Go's conventions, the "V" would either be the next word and capitalized or an initial and capitalized ("Greengrass" is the way it is because AWS treats it as one word):

package testing name example
apigatewayv2 APIGatewayV2 func TestAccAPIGatewayV2GreatTest_basic
cloudhsmv2 CloudHSMV2 func TestAccCloudHSMV2GreatTest_basic
elbv2 ELBV2 func TestAccELBV2GreatTest_basic
greengrassv2 GreengrassV2 func TestAccGreengrassV2GreatTest_basic
kinesisanalyticsv2 KinesisAnalyticsV2 func TestAccKinesisAnalyticsV2GreatTest_basic
lexmodelsv2 LexModelsV2 func TestAccLexModelsV2GreatTest_basic
lexruntimev2 LexRuntimeV2 func TestAccLexRuntimeV2GreatTest_basic
sesv2 SESV2 func TestAccSESV2GreatTest_basic
wafv2 WAFV2 func TestAccWAFV2GreatTest_basic

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the <service>V2 approach in general. However, it doesn't look right after initialisms, such as WAFV2. For straight up consistency, I'd go with <service>V2. For ease of readability or aesthetics, I'd go with <full word service>V2 and <initialism service>v2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can extract the list of initialisms from the SDK packages at service/<service package>/service.go files. The constant ServiceID could be checked for all-caps. There are some special cases, such as ACM PCA, where there's a space.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also somewhat relates to another cleanup issue I'd like to open around standardizing service and resource naming in, e.g. error messages

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
Expand All @@ -22,7 +22,7 @@ func TestAccDataSourceAwsAmiIds_basic(t *testing.T) {
})
}

func TestAccDataSourceAwsAmiIds_sorted(t *testing.T) {
func TestAccEC2AMIIDsDataSource_sorted(t *testing.T) {
resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
Expand Down
10 changes: 5 additions & 5 deletions aws/data_source_aws_ami_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
)

func TestAccAWSAmiDataSource_natInstance(t *testing.T) {
func TestAccEC2AMIDataSource_natInstance(t *testing.T) {
resourceName := "data.aws_ami.nat_ami"
resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Expand Down Expand Up @@ -60,7 +60,7 @@ func TestAccAWSAmiDataSource_natInstance(t *testing.T) {
})
}

func TestAccAWSAmiDataSource_windowsInstance(t *testing.T) {
func TestAccEC2AMIDataSource_windowsInstance(t *testing.T) {
resourceName := "data.aws_ami.windows_ami"
resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Expand Down Expand Up @@ -103,7 +103,7 @@ func TestAccAWSAmiDataSource_windowsInstance(t *testing.T) {
})
}

func TestAccAWSAmiDataSource_instanceStore(t *testing.T) {
func TestAccEC2AMIDataSource_instanceStore(t *testing.T) {
resourceName := "data.aws_ami.amzn-ami-minimal-hvm-instance-store"
resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Expand Down Expand Up @@ -142,7 +142,7 @@ func TestAccAWSAmiDataSource_instanceStore(t *testing.T) {
})
}

func TestAccAWSAmiDataSource_localNameFilter(t *testing.T) {
func TestAccEC2AMIDataSource_localNameFilter(t *testing.T) {
resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
Expand All @@ -158,7 +158,7 @@ func TestAccAWSAmiDataSource_localNameFilter(t *testing.T) {
})
}

func TestAccAWSAmiDataSource_Gp3BlockDevice(t *testing.T) {
func TestAccEC2AMIDataSource_gp3BlockDevice(t *testing.T) {
resourceName := "aws_ami.test"
datasourceName := "data.aws_ami.test"
rName := acctest.RandomWithPrefix("tf-acc-test")
Expand Down
8 changes: 4 additions & 4 deletions aws/resource_aws_ami_copy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
)

func TestAccAWSAMICopy_basic(t *testing.T) {
func TestAccEC2AMICopy_basic(t *testing.T) {
var image ec2.Image
rName := acctest.RandomWithPrefix("tf-acc-test")
resourceName := "aws_ami_copy.test"
Expand All @@ -39,7 +39,7 @@ func TestAccAWSAMICopy_basic(t *testing.T) {
})
}

func TestAccAWSAMICopy_Description(t *testing.T) {
func TestAccEC2AMICopy_description(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current guidance for writing acceptance tests uses lowercase for "meta" tests like _basic and _disappears, but uppercase for attribute-based and other tests. Not sure if we should be following the documentation or updating the documentation to include more explicit guidance.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While Go conventions do allow for capitalized words after the underscore, they apply to types and methods on types. Drawing the analogy to what we're doing, it would be reasonable to apply the same logic. However, I do not think we should have capitalized words after underscores for one simple reason: a badly followed convention is worse than no convention at all. Currently, the capitalized word after an underscore is as likely to tell you nothing about the test as something. As a result, a clear, followable convention would be better. I vote for updating the documentation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the capitalized word after an underscore is as likely to tell you nothing about the test as something.

Can you please expand on this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, there are around about 1256 basic and disappears tests, which use the underscore-lowercase "meta" test convention. There are a further 1904 tests that use underscore-lowercase but many do not seem like "meta" tests:

TestAccAWSAPIGatewayUsagePlan_description()
TestAccAWSAPIGatewayMethod_customrequestvalidator()
TestAccAWSAPIGatewayIntegration_integrationType()
TestAccAWSEBSSnapshot_tags()
TestAccAWSEBSVolume_updateSize()
TestAccAWSEIP_instance()
TestAccAWSGlueTrigger_actions_notify()
TestAccAWSInstance_outpost()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should enforce whatever standard we choose. In this case, I think we should pick a standard that improves understandability/readability first, and then update whatever documentation and Semgrep rules to enforce it

var image ec2.Image
rName := acctest.RandomWithPrefix("tf-acc-test")
resourceName := "aws_ami_copy.test"
Expand Down Expand Up @@ -67,7 +67,7 @@ func TestAccAWSAMICopy_Description(t *testing.T) {
})
}

func TestAccAWSAMICopy_EnaSupport(t *testing.T) {
func TestAccEC2AMICopy_enaSupport(t *testing.T) {
var image ec2.Image
rName := acctest.RandomWithPrefix("tf-acc-test")
resourceName := "aws_ami_copy.test"
Expand All @@ -88,7 +88,7 @@ func TestAccAWSAMICopy_EnaSupport(t *testing.T) {
})
}

func TestAccAWSAMICopy_tags(t *testing.T) {
func TestAccEC2AMICopy_tags(t *testing.T) {
var ami ec2.Image
resourceName := "aws_ami_copy.test"
rName := acctest.RandomWithPrefix("tf-acc-test")
Expand Down
4 changes: 2 additions & 2 deletions aws/resource_aws_ami_from_instance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
)

func TestAccAWSAMIFromInstance_basic(t *testing.T) {
func TestAccEC2AMIFromInstance_basic(t *testing.T) {
var image ec2.Image
rName := acctest.RandomWithPrefix("tf-acc-test")
resourceName := "aws_ami_from_instance.test"
Expand All @@ -39,7 +39,7 @@ func TestAccAWSAMIFromInstance_basic(t *testing.T) {
})
}

func TestAccAWSAMIFromInstance_tags(t *testing.T) {
func TestAccEC2AMIFromInstance_tags(t *testing.T) {
var image ec2.Image
rName := acctest.RandomWithPrefix("tf-acc-test")
resourceName := "aws_ami_from_instance.test"
Expand Down
8 changes: 4 additions & 4 deletions aws/resource_aws_ami_launch_permission_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
)

func TestAccAWSAMILaunchPermission_basic(t *testing.T) {
func TestAccEC2AMILaunchPermission_basic(t *testing.T) {
resourceName := "aws_ami_launch_permission.test"
rName := acctest.RandomWithPrefix("tf-acc-test")

Expand All @@ -36,7 +36,7 @@ func TestAccAWSAMILaunchPermission_basic(t *testing.T) {
})
}

func TestAccAWSAMILaunchPermission_Disappears_LaunchPermission(t *testing.T) {
func TestAccEC2AMILaunchPermission_disappears_launchPermission(t *testing.T) {
resourceName := "aws_ami_launch_permission.test"
rName := acctest.RandomWithPrefix("tf-acc-test")

Expand All @@ -59,7 +59,7 @@ func TestAccAWSAMILaunchPermission_Disappears_LaunchPermission(t *testing.T) {

// Bug reference: https://github.com/hashicorp/terraform-provider-aws/issues/6222
// Images with <group>all</group> will not have <userId> and can cause a panic
func TestAccAWSAMILaunchPermission_Disappears_LaunchPermission_Public(t *testing.T) {
func TestAccEC2AMILaunchPermission_disappears_launchPermission_public(t *testing.T) {
resourceName := "aws_ami_launch_permission.test"
rName := acctest.RandomWithPrefix("tf-acc-test")

Expand All @@ -81,7 +81,7 @@ func TestAccAWSAMILaunchPermission_Disappears_LaunchPermission_Public(t *testing
})
}

func TestAccAWSAMILaunchPermission_Disappears_AMI(t *testing.T) {
func TestAccEC2AMILaunchPermission_disappears_ami(t *testing.T) {
imageID := ""
resourceName := "aws_ami_launch_permission.test"
rName := acctest.RandomWithPrefix("tf-acc-test")
Expand Down
12 changes: 6 additions & 6 deletions aws/resource_aws_ami_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
)

func TestAccAWSAMI_basic(t *testing.T) {
func TestAccEC2AMI_basic(t *testing.T) {
var ami ec2.Image
resourceName := "aws_ami.test"
snapshotResourceName := "aws_ebs_snapshot.test"
Expand Down Expand Up @@ -72,7 +72,7 @@ func TestAccAWSAMI_basic(t *testing.T) {
})
}

func TestAccAWSAMI_description(t *testing.T) {
func TestAccEC2AMI_description(t *testing.T) {
var ami ec2.Image
resourceName := "aws_ami.test"
snapshotResourceName := "aws_ebs_snapshot.test"
Expand Down Expand Up @@ -157,7 +157,7 @@ func TestAccAWSAMI_description(t *testing.T) {
})
}

func TestAccAWSAMI_disappears(t *testing.T) {
func TestAccEC2AMI_disappears(t *testing.T) {
var ami ec2.Image
resourceName := "aws_ami.test"
rName := acctest.RandomWithPrefix("tf-acc-test")
Expand All @@ -179,7 +179,7 @@ func TestAccAWSAMI_disappears(t *testing.T) {
})
}

func TestAccAWSAMI_EphemeralBlockDevices(t *testing.T) {
func TestAccEC2AMI_ephemeralBlockDevices(t *testing.T) {
var ami ec2.Image
resourceName := "aws_ami.test"
snapshotResourceName := "aws_ebs_snapshot.test"
Expand Down Expand Up @@ -239,7 +239,7 @@ func TestAccAWSAMI_EphemeralBlockDevices(t *testing.T) {
})
}

func TestAccAWSAMI_Gp3BlockDevice(t *testing.T) {
func TestAccEC2AMI_gp3BlockDevice(t *testing.T) {
var ami ec2.Image
resourceName := "aws_ami.test"
snapshotResourceName := "aws_ebs_snapshot.test"
Expand Down Expand Up @@ -301,7 +301,7 @@ func TestAccAWSAMI_Gp3BlockDevice(t *testing.T) {
})
}

func TestAccAWSAMI_tags(t *testing.T) {
func TestAccEC2AMI_tags(t *testing.T) {
var ami ec2.Image
resourceName := "aws_ami.test"
rName := acctest.RandomWithPrefix("tf-acc-test")
Expand Down