tests/ec2/ami: Rename tests #17849
tests/ec2/ami: Rename tests #17849
Conversation
ghost
added
size/S
| @@ -39,7 +39,7 @@ func TestAccAWSAMICopy_basic(t *testing.T) { | |||
| }) | |||
| } | |||
|
|
|||
| func TestAccAWSAMICopy_Description(t *testing.T) { | |||
| func TestAccEC2AMICopy_description(t *testing.T) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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
| @@ -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) { | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This also somewhat relates to another cleanup issue I'd like to open around standardizing service and resource naming in, e.g. error messages
There was a problem hiding this comment.
my view on topics covered:
(1) dropping Aws/AWS -- YES 🥳 .
(2) initialisms -- also leaning towards 🐫 Case (e.g. WafV2RuleGroup) as it seems initialisms are not uniformly defined w/in AWS and in the real-world. also maintaining a list of initialisms can prove tricky/subjective and easy to miss adding new ones to the docs when introduced in PRs (e.g. maintaining our list of Environment variables)
(3) test suffixes (e.g. _Basic, ContentHandling) -- maintain 🐫 Case; imo, case doesn't map to semantics too intuitively and guidance from our documentation can be easily overlooked (hmm though a linter can enforce using lowercase for our "meta" tests 🤔 ; seems to add more complexity tho); no preference on whether case after underscore is lower (_contentHandling) vs. upper (_ContentHandling)
|
One other comment on standardization: Where do we want to move |
@bflad suggests, and I heartily agree, that we should move "DataSource" to the end of the first clause, i.e., just before the underscore. Most of the time, the default should be to run both the resource and data source when changing either since there can be dependencies. This fascilitates that. We should be able to still run only one or the other with |
|
Re: @anGie44
I don't think that not capitalizing acronyms and initialisms will get rid of all lists. For example, if we decide that There are about 50 initialisms and acronyms in common AWS use. We only use some of those in the AWS provider. I don't think it's an unreasonable burden to maintain a list. We will need to maintain a list anyway to fix mixed case as well. Also, by adhereing to Go's well-established convention, we don't have to explain special, non-conforming Go naming rules. We certainly can and should explain that we follow convention but Go developers will expect it. Many contributors have been trying to adhere for years. Probably some of them forget that we're special. Acronyms and initialism: |
I think you're right here 👍 i thought it was a bit rare in golang projects at first but totally overlooked what current linters already do 😅 e.g. https://github.com/golang/lint/blob/master/lint.go#L767-L809. Which then makes me wonder, since we already get those for free from a linter like golint which could be enabled in our So let's say we go with the idea of having a list of initialisms/acronyms we see fit, i still have a couple Q's and perhaps I'm alluding to an additional PR where our conventions are documented to help the team but also contributors to this tech-debt issue:
p.s. thank you for taking this on @YakDriver and taking this naming convo from the abstract to more tangible examples! |
|
@anGie44 Thanks, Angie! 🙏 🙏 It would be awesome to just use industry-standard linters! Maybe as a first step we could just lint test files and then eventually move to the resource and data source files. I like the idea of just feeding an iinitialism/acronym list to the linter that the community can help maintain!
The rules are the same. "Capitalisms" (my word for abbreviations, initialisms, acronyms, etc.) should be either all caps or all lowercase. vpcCount := 0
namedVPC := "vpc"We have a few complexities here that maybe are not exactly handled in the convention though, e.g.,
Somewhere in the repo, where we can feed them to the linter(s).
Right now, I would expect to handle "WAF" the same as "EBS", "VPC", or "EC2".
I think we start with tests. It's beyond the scope of this effort but we can turn on the the enforcement just for tests to get going. I have a script that can fix acceptance test names to avoid this taking much engineering time.
As I'm not sure which direction we're going or if we'll even make any initialism changes, I haven't spent much time on enforcement. I'd be happy to collaborate if you have ideas! |
|
This functionality has been released in v3.64.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. Thank you! |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Community Note
Relates #17847
Output from acceptance testing: