diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ce50b1..eb84acc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## Unreleased +- add rule `AZT007`: a `TestAcc` name must not carry `Acc` again as a word of its own (`_storageAccBehindFireWall`); `Account` and `Access` are fine; report only, no fix ([#72](https://github.com/katbyte/azproviderlint/pull/72)) - add rule `AZT004`: test functions that run an acceptance test but are not named `TestAcc`, so acceptance runs never pick them up; report only, no fix ([#67](https://github.com/katbyte/azproviderlint/pull/67)) - add rule `AZT003`: test functions named `TestAcc` that never run an acceptance test, so are scheduled as one without touching Azure; report only, no fix ([#64](https://github.com/katbyte/azproviderlint/pull/64)) diff --git a/README.md b/README.md index 96b4f68..930959c 100644 --- a/README.md +++ b/README.md @@ -105,6 +105,7 @@ Rules are named `AZ`, a category letter, and a number. The letters follow [tfpro | [AZT001](checks/AZT/AZT001_acceptance_test_external_package) | acceptance tests must use a _test package | | [AZT002](checks/AZT/AZT002_credentials_from_environment) | acceptance tests must not read credentials from the environment | | [AZT003](checks/AZT/AZT003_misnamed_acceptance_test) | TestAcc functions must run an acceptance test | +| [AZT007](checks/AZT/AZT007_stray_acc_in_test_name) | acceptance test names must not carry a second Acc | | [AZT004](checks/AZT/AZT004_acceptance_test_not_named_test_acc) | acceptance tests must be named TestAcc | ### AZN - Naming Conventions diff --git a/checks/AZT/AZT.go b/checks/AZT/AZT.go index 9f29075..10a670e 100644 --- a/checks/AZT/AZT.go +++ b/checks/AZT/AZT.go @@ -8,6 +8,7 @@ import ( AZT002 "github.com/katbyte/azproviderlint/checks/AZT/AZT002_credentials_from_environment" AZT003 "github.com/katbyte/azproviderlint/checks/AZT/AZT003_misnamed_acceptance_test" AZT004 "github.com/katbyte/azproviderlint/checks/AZT/AZT004_acceptance_test_not_named_test_acc" + AZT007 "github.com/katbyte/azproviderlint/checks/AZT/AZT007_stray_acc_in_test_name" ) // Checks contains all AZT (acceptance testing) analyzers. @@ -15,5 +16,6 @@ var Checks = []*analysis.Analyzer{ AZT001.Analyzer, AZT002.Analyzer, AZT003.Analyzer, + AZT007.Analyzer, AZT004.Analyzer, } diff --git a/checks/AZT/AZT007_stray_acc_in_test_name/AZT007.go b/checks/AZT/AZT007_stray_acc_in_test_name/AZT007.go new file mode 100644 index 0000000..4c9d04b --- /dev/null +++ b/checks/AZT/AZT007_stray_acc_in_test_name/AZT007.go @@ -0,0 +1,52 @@ +// Package AZT007 defines an analyzer that reports acceptance test names carrying a second +// Acc after the TestAcc prefix. +package AZT007 + +import ( + "go/ast" + "regexp" + "strings" + + "golang.org/x/tools/go/analysis" +) + +// Analyzer checks that a TestAcc name does not contain Acc again as a word of its own: +// TestAccThing_storageAccBehindFirewall. Acc is the marker that makes a function an +// acceptance test, so a second one reads as a mistake, and as an abbreviation of account it +// is unclear. Account and Access are whole words and are left alone. +var Analyzer = &analysis.Analyzer{ + Name: "AZT007", + Doc: "check that acceptance test names do not carry a second Acc after the TestAcc prefix", + URL: "https://github.com/katbyte/azproviderlint/blob/main/checks/AZT/AZT007_stray_acc_in_test_name/README.md", + Run: run, +} + +// strayAcc matches Acc as a whole word inside a camel-case name: followed by an upper-case +// letter, an underscore or the end, so Account and Access are not matched. +var strayAcc = regexp.MustCompile(`Acc([A-Z_]|$)`) + +func run(pass *analysis.Pass) (any, error) { + for _, file := range pass.Files { + if !strings.HasSuffix(pass.Fset.Position(file.Pos()).Filename, "_test.go") { + continue + } + + for _, decl := range file.Decls { + fd, ok := decl.(*ast.FuncDecl) + if !ok || fd.Recv != nil { + continue + } + + rest, ok := strings.CutPrefix(fd.Name.Name, "TestAcc") + if !ok { + rest, ok = strings.CutPrefix(fd.Name.Name, "testAcc") + } + + if ok && strayAcc.MatchString(rest) { + pass.Reportf(fd.Name.Pos(), "%s has a second Acc in its name, spell the word out so it is not read as another acceptance marker", fd.Name.Name) + } + } + } + + return nil, nil +} diff --git a/checks/AZT/AZT007_stray_acc_in_test_name/AZT007_test.go b/checks/AZT/AZT007_stray_acc_in_test_name/AZT007_test.go new file mode 100644 index 0000000..6ff532a --- /dev/null +++ b/checks/AZT/AZT007_stray_acc_in_test_name/AZT007_test.go @@ -0,0 +1,16 @@ +package AZT007 + +import ( + "path/filepath" + "runtime" + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +func TestAZT007(t *testing.T) { + t.Parallel() + + _, filename, _, _ := runtime.Caller(0) + analysistest.Run(t, filepath.Join(filepath.Dir(filename), "testdata"), Analyzer, "azt007") +} diff --git a/checks/AZT/AZT007_stray_acc_in_test_name/README.md b/checks/AZT/AZT007_stray_acc_in_test_name/README.md new file mode 100644 index 0000000..6bab0d8 --- /dev/null +++ b/checks/AZT/AZT007_stray_acc_in_test_name/README.md @@ -0,0 +1,37 @@ +# AZT007 - acceptance test names must not carry a second Acc + +AZT007 reports a `TestAcc` or `testAcc` function whose name has `Acc` in it again as a word of its own, as in `TestAccThing_storageAccBehindFireWall`. + +`Acc` is the marker that makes a function an acceptance test. A second one reads as a mistake, and as an abbreviation of account it is unclear. Write the word out. + +## Flagged Code + +```go +func TestAccMsSqlServerExtendedAuditingPolicy_storageAccBehindFireWall(t *testing.T) { +``` + +## Passing Code + +```go +func TestAccMsSqlServerExtendedAuditingPolicy_storageAccountBehindFirewall(t *testing.T) { +``` + +## What counts + +`Acc` counts when it is a whole camel-case word: followed by an upper-case letter, an underscore, or the end of the name. `Account`, `Access` and `accelerated` are left alone, and so is the `Acc` of the prefix itself. + +Only `TestAcc` and lowercase `testAcc` functions in `_test.go` files are checked. A plain `Test` name with `Acc` in the wrong place, like `TestWebAppAccActiveSlot_basic`, is [AZT004](../AZT004_acceptance_test_not_named_test_acc)'s report when it runs an acceptance test. + +## No fix + +AZT007 only reports: whether `Acc` meant account, access or something else is the author's call. + +## Ignoring Reports + +Put `//azignore:AZT007 - ` at the end of the line, or on the line above it. The reason is required. + +```go +func TestAccThing_storageAccBehindFireWall(t *testing.T) { //azignore:AZT007 - +``` + +Under golangci-lint, `//nolint:azproviderlint` in the same place also works, but it silences every azproviderlint check on that line. diff --git a/checks/AZT/AZT007_stray_acc_in_test_name/testdata/src/azt007/thing.go b/checks/AZT/AZT007_stray_acc_in_test_name/testdata/src/azt007/thing.go new file mode 100644 index 0000000..f4796a5 --- /dev/null +++ b/checks/AZT/AZT007_stray_acc_in_test_name/testdata/src/azt007/thing.go @@ -0,0 +1,6 @@ +package azt007 + +import "testing" + +// Should NOT be flagged: go test only runs functions from _test.go files +func TestAccThing_storageAccNotATestFile(t *testing.T) {} diff --git a/checks/AZT/AZT007_stray_acc_in_test_name/testdata/src/azt007/thing_test.go b/checks/AZT/AZT007_stray_acc_in_test_name/testdata/src/azt007/thing_test.go new file mode 100644 index 0000000..1d1787c --- /dev/null +++ b/checks/AZT/AZT007_stray_acc_in_test_name/testdata/src/azt007/thing_test.go @@ -0,0 +1,23 @@ +package azt007_test + +import "testing" + +// Should be flagged: Acc as an abbreviation inside the step name, in a test or a sequential helper +func TestAccThing_storageAccBehindFireWall(t *testing.T) {} // want `TestAccThing_storageAccBehindFireWall has a second Acc in its name, spell the word out so it is not read as another acceptance marker` +func testAccThing_storageAccBehindFireWall(t *testing.T) {} // want `testAccThing_storageAccBehindFireWall has a second Acc in its name, spell the word out so it is not read as another acceptance marker` +func TestAccThing_withAcc(t *testing.T) {} // want `TestAccThing_withAcc has a second Acc in its name, spell the word out so it is not read as another acceptance marker` +func TestAccAcc_basic(t *testing.T) {} // want `TestAccAcc_basic has a second Acc in its name, spell the word out so it is not read as another acceptance marker` + +// Should NOT be flagged: whole words that start with Acc +func TestAccThing_storageAccountBehindFirewall(t *testing.T) {} +func TestAccThing_publicAccess(t *testing.T) {} +func TestAccAccount_basic(t *testing.T) {} +func TestAccThing_accelerated(t *testing.T) {} + +// Should NOT be flagged: not acceptance test names, AZT004 covers those that should be +func TestWebAppAccActiveSlot_basic(t *testing.T) {} +func TestThing_storageAcc(t *testing.T) {} + +type ThingResource struct{} + +func (r ThingResource) TestAccThing_storageAccMethod(t *testing.T) {}