diff --git a/CHANGELOG.md b/CHANGELOG.md index aecd6f7..1ce50b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## Unreleased +- 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)) ## v0.11.0 (2026-09-28) diff --git a/README.md b/README.md index ba48d62..96b4f68 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 | +| [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 c400309..9f29075 100644 --- a/checks/AZT/AZT.go +++ b/checks/AZT/AZT.go @@ -7,6 +7,7 @@ import ( AZT001 "github.com/katbyte/azproviderlint/checks/AZT/AZT001_acceptance_test_external_package" 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" ) // Checks contains all AZT (acceptance testing) analyzers. @@ -14,4 +15,5 @@ var Checks = []*analysis.Analyzer{ AZT001.Analyzer, AZT002.Analyzer, AZT003.Analyzer, + AZT004.Analyzer, } diff --git a/checks/AZT/AZT003_misnamed_acceptance_test/AZT003.go b/checks/AZT/AZT003_misnamed_acceptance_test/AZT003.go index 527d593..622dfdd 100644 --- a/checks/AZT/AZT003_misnamed_acceptance_test/AZT003.go +++ b/checks/AZT/AZT003_misnamed_acceptance_test/AZT003.go @@ -4,122 +4,57 @@ package AZT003 import ( "go/ast" - "go/constant" "go/types" - "slices" "strings" "unicode" "golang.org/x/tools/go/analysis" - "github.com/katbyte/azproviderlint/lib/astx" + "github.com/katbyte/azproviderlint/lib/acctest" ) // Analyzer checks that every `TestAcc...` function reaches the acceptance test harness. // Acceptance runs select tests by that prefix, so a unit test carrying it is scheduled and // reported as an acceptance test although it never touches Azure. var Analyzer = &analysis.Analyzer{ - Name: "AZT003", - Doc: "check that test functions named TestAcc run an acceptance test", - URL: "https://github.com/katbyte/azproviderlint/blob/main/checks/AZT/AZT003_misnamed_acceptance_test/README.md", - Run: run, -} - -// The Terraform test harnesses; a provider's own internal/acceptance package counts too. -var harnessPackages = map[string]bool{ - "github.com/hashicorp/terraform-plugin-testing/helper/resource": true, - "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource": true, + Name: "AZT003", + Doc: "check that test functions named TestAcc run an acceptance test", + URL: "https://github.com/katbyte/azproviderlint/blob/main/checks/AZT/AZT003_misnamed_acceptance_test/README.md", + Requires: []*analysis.Analyzer{acctest.Analyzer}, + Run: run, } func run(pass *analysis.Pass) (any, error) { - decls := map[*types.Func]*ast.FuncDecl{} - var candidates []*types.Func + runs, ok := pass.ResultOf[acctest.Analyzer].(acctest.Funcs) + if !ok { + return nil, nil + } + for _, file := range pass.Files { - isTest := strings.HasSuffix(pass.Fset.Position(file.Pos()).Filename, "_test.go") + 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.Body == nil { + // the bare prefix, as test runners match it: TestAccountName is picked up too + if !ok || fd.Body == nil || fd.Recv != nil || !strings.HasPrefix(fd.Name.Name, "TestAcc") { continue } + fn, ok := pass.TypesInfo.Defs[fd.Name].(*types.Func) - if !ok { + if !ok || runs[fn] != nil { continue } - decls[fn] = fd - // the bare prefix, as test runners match it: TestAccountName is picked up too - if isTest && fd.Recv == nil && strings.HasPrefix(fd.Name.Name, "TestAcc") { - candidates = append(candidates, fn) - } - } - } - if len(candidates) == 0 { - return nil, nil - } - - // runs marks the functions that are acceptance tests on their own: they hand t to a - // harness or look at TF_ACC. mentions lists the same-package functions each one calls or - // passes around as a value (`t.Run("basic", testAccThing_basic)`). - runs := map[*types.Func]bool{} - mentions := map[*types.Func][]*types.Func{} - for fn, fd := range decls { - ast.Inspect(fd.Body, func(n ast.Node) bool { - switch n := n.(type) { - case *ast.CallExpr: - callee := astx.CalledFunc(pass, n) - if callee == nil || callee.Pkg() == nil { - break - } - if path := callee.Pkg().Path(); !harnessPackages[path] && !strings.HasSuffix(path, "/internal/acceptance") { - break - } - for _, arg := range n.Args { - ptr, ok := pass.TypesInfo.TypeOf(arg).(*types.Pointer) - if !ok { - continue - } - if named, ok := ptr.Elem().(*types.Named); ok && named.Obj().Name() == "T" && named.Obj().Pkg() != nil && named.Obj().Pkg().Path() == "testing" { - runs[fn] = true - } - } - case *ast.Ident: - if used, ok := pass.TypesInfo.Uses[n].(*types.Func); ok && used.Pkg() == pass.Pkg { - mentions[fn] = append(mentions[fn], used.Origin()) - } - } - // by value, so resource.EnvTfAcc and local constants count as well as the literal - if e, ok := n.(ast.Expr); ok { - if v := pass.TypesInfo.Types[e].Value; v != nil && v.Kind() == constant.String && constant.StringVal(v) == "TF_ACC" { - runs[fn] = true - } - } - return true - }) - } - // a function that mentions an acceptance test is one too - for changed := true; changed; { - changed = false - for fn, used := range mentions { - if !runs[fn] && slices.ContainsFunc(used, func(u *types.Func) bool { return runs[u] }) { - runs[fn] = true - changed = true + // Acc that starts a word (TestAccountName) cannot simply be dropped + rest := strings.TrimPrefix(fn.Name(), "TestAcc") + if rest != "" && unicode.IsLower(rune(rest[0])) { + pass.Reportf(fd.Name.Pos(), "%s does not run an acceptance test, rename it so it does not start with TestAcc and is not picked up as one", fn.Name()) + continue } - } - } - for _, fn := range candidates { - if runs[fn] { - continue - } - // Acc that starts a word (TestAccountName) cannot simply be dropped - rest := strings.TrimPrefix(fn.Name(), "TestAcc") - if rest != "" && unicode.IsLower(rune(rest[0])) { - pass.Reportf(decls[fn].Name.Pos(), - "%s does not run an acceptance test, rename it so it does not start with TestAcc and is not picked up as one", fn.Name()) - continue + pass.Reportf(fd.Name.Pos(), "%s does not run an acceptance test, name it Test%s so it is not picked up as one", fn.Name(), rest) } - pass.Reportf(decls[fn].Name.Pos(), - "%s does not run an acceptance test, name it Test%s so it is not picked up as one", fn.Name(), rest) } return nil, nil diff --git a/checks/AZT/AZT003_misnamed_acceptance_test/testdata/src/github.com/hashicorp/terraform-plugin-testing/helper/resource/resource.go b/checks/AZT/AZT003_misnamed_acceptance_test/testdata/src/github.com/hashicorp/terraform-plugin-testing/helper/resource/resource.go index 798dba2..2d71238 100644 --- a/checks/AZT/AZT003_misnamed_acceptance_test/testdata/src/github.com/hashicorp/terraform-plugin-testing/helper/resource/resource.go +++ b/checks/AZT/AZT003_misnamed_acceptance_test/testdata/src/github.com/hashicorp/terraform-plugin-testing/helper/resource/resource.go @@ -19,3 +19,6 @@ type TestCase struct { func Test(t T, c TestCase) {} func ParallelTest(t T, c TestCase) {} + +// UnitTest runs the case without TF_ACC: a unit test on the harness, not an acceptance test. +func UnitTest(t T, c TestCase) {} diff --git a/checks/AZT/AZT003_misnamed_acceptance_test/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt003/thing.go b/checks/AZT/AZT003_misnamed_acceptance_test/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt003/thing.go index c54709c..623663f 100644 --- a/checks/AZT/AZT003_misnamed_acceptance_test/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt003/thing.go +++ b/checks/AZT/AZT003_misnamed_acceptance_test/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt003/thing.go @@ -1,6 +1,9 @@ package azt003 -import "testing" +import ( + "os" + "testing" +) func BuildName(prefix string) string { return prefix + "-thing" @@ -10,3 +13,9 @@ func BuildName(prefix string) string { func TestAccNotInATestFile(t *testing.T) { _ = BuildName("a") } + +// Configured is production code that reads TF_ACC to pick a default; calling it does not make +// a test an acceptance test +func Configured() bool { + return os.Getenv("TF_ACC") != "" +} diff --git a/checks/AZT/AZT003_misnamed_acceptance_test/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt003/thing_resource_test.go b/checks/AZT/AZT003_misnamed_acceptance_test/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt003/thing_resource_test.go index 252e585..22f666f 100644 --- a/checks/AZT/AZT003_misnamed_acceptance_test/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt003/thing_resource_test.go +++ b/checks/AZT/AZT003_misnamed_acceptance_test/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt003/thing_resource_test.go @@ -156,6 +156,18 @@ func TestAccThing_randomName(t *testing.T) { // want `TestAccThing_randomName do } } +// Should be flagged: the harness's UnitTest runs without TF_ACC, so this is a unit test +func TestAccThing_unitHarness(t *testing.T) { // want `TestAccThing_unitHarness does not run an acceptance test, name it TestThing_unitHarness so it is not picked up as one` + resource.UnitTest(t, resource.TestCase{Steps: []resource.TestStep{{Config: "unit"}}}) +} + +// Should be flagged: production code reading TF_ACC does not make this an acceptance test +func TestAccThing_configured(t *testing.T) { // want `TestAccThing_configured does not run an acceptance test, name it TestThing_configured so it is not picked up as one` + if !azt003.Configured() { + t.Skip("not configured") + } +} + // Should be flagged: Acc starts a word here, but test runners still match the prefix func TestAccountName(t *testing.T) { // want `TestAccountName does not run an acceptance test, rename it so it does not start with TestAcc and is not picked up as one` if azt003.BuildName("a") != "a-thing" { diff --git a/checks/AZT/AZT004_acceptance_test_not_named_test_acc/AZT004.go b/checks/AZT/AZT004_acceptance_test_not_named_test_acc/AZT004.go new file mode 100644 index 0000000..589db88 --- /dev/null +++ b/checks/AZT/AZT004_acceptance_test_not_named_test_acc/AZT004.go @@ -0,0 +1,66 @@ +// Package AZT004 defines an analyzer that reports test functions that run an acceptance test +// but are not named TestAcc. +package AZT004 + +import ( + "go/ast" + "go/types" + "regexp" + "strings" + + "golang.org/x/tools/go/analysis" + + "github.com/katbyte/azproviderlint/lib/acctest" +) + +// Analyzer checks that every test function reaching the acceptance test harness is named +// `TestAcc...`. Acceptance runs select tests by that prefix, so one named otherwise is never +// run against Azure, however complete it is. +var Analyzer = &analysis.Analyzer{ + Name: "AZT004", + Doc: "check that test functions running an acceptance test are named TestAcc", + URL: "https://github.com/katbyte/azproviderlint/blob/main/checks/AZT/AZT004_acceptance_test_not_named_test_acc/README.md", + Requires: []*analysis.Analyzer{acctest.Analyzer}, + 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) { + runs, ok := pass.ResultOf[acctest.Analyzer].(acctest.Funcs) + if !ok { + return nil, nil + } + + 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) + // what go test runs: a top-level Test function; its signature is enforced by the compiler + if !ok || fd.Body == nil || fd.Recv != nil || !strings.HasPrefix(fd.Name.Name, "Test") || strings.HasPrefix(fd.Name.Name, "TestAcc") { + continue + } + + fn, ok := pass.TypesInfo.Defs[fd.Name].(*types.Func) + if !ok || runs[fn] == nil { + continue + } + + // Acc already sits somewhere in the name as its own word (TestWebAppAccActiveSlot_basic), + // so prefixing it would double up; Account and Access do not count + rest := strings.TrimPrefix(fn.Name(), "Test") + if strayAcc.MatchString(rest) { + pass.Reportf(fd.Name.Pos(), "%s runs an acceptance test, rename it to start with TestAcc so acceptance runs pick it up", fn.Name()) + continue + } + + pass.Reportf(fd.Name.Pos(), "%s runs an acceptance test, name it TestAcc%s so acceptance runs pick it up", fn.Name(), rest) + } + } + + return nil, nil +} diff --git a/checks/AZT/AZT004_acceptance_test_not_named_test_acc/AZT004_test.go b/checks/AZT/AZT004_acceptance_test_not_named_test_acc/AZT004_test.go new file mode 100644 index 0000000..722f987 --- /dev/null +++ b/checks/AZT/AZT004_acceptance_test_not_named_test_acc/AZT004_test.go @@ -0,0 +1,16 @@ +package AZT004 + +import ( + "path/filepath" + "runtime" + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +func TestAZT004(t *testing.T) { + t.Parallel() + + _, filename, _, _ := runtime.Caller(0) + analysistest.Run(t, filepath.Join(filepath.Dir(filename), "testdata"), Analyzer, "github.com/hashicorp/terraform-provider-azurerm/internal/services/azt004") +} diff --git a/checks/AZT/AZT004_acceptance_test_not_named_test_acc/README.md b/checks/AZT/AZT004_acceptance_test_not_named_test_acc/README.md new file mode 100644 index 0000000..fe1cff3 --- /dev/null +++ b/checks/AZT/AZT004_acceptance_test_not_named_test_acc/README.md @@ -0,0 +1,61 @@ +# AZT004 - acceptance tests must be named TestAcc + +AZT004 reports test functions that run an acceptance test but are not named `TestAcc...`. + +Acceptance runs pick tests by that prefix. A test named `TestThing_basic` that drives the acceptance framework is skipped by every acceptance run, so the resource it covers is never actually tested against Azure, however complete the test looks. + +A function counts as an acceptance test when it does one of these, itself or through another function in the same package: + +- hands `t` to the provider's `internal/acceptance` package (`acceptance.BuildTestData(t, ...)`, `data.ResourceTest(t, ...)`, `acceptance.RunTestsInSequence(t, ...)`) +- hands `t` to the Terraform test harness (`resource.Test`, `resource.ParallelTest`) +- looks at `TF_ACC` + +Only functions go test runs are checked: a top-level `func TestXxx(t *testing.T)` in a `_test.go` file. Lowercase helpers and methods are left alone, since no runner selects them by name. + +## Flagged Code + +```go +func TestThing_basic(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_thing", "test") + r := ThingResource{} + + data.ResourceTest(t, r, []acceptance.TestStep{ + { + Config: r.basic(data), + }, + data.ImportStep(), + }) +} +``` + +## Passing Code + +```go +func TestAccThing_basic(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_thing", "test") + r := ThingResource{} + + data.ResourceTest(t, r, []acceptance.TestStep{ + { + Config: r.basic(data), + }, + data.ImportStep(), + }) +} +``` + +## No fix + +AZT004 only reports. Rename the function so it starts with `TestAcc`. When `Acc` already sits elsewhere in the name (`TestWebAppAccActiveSlot_basic`), move it to the front rather than adding a second one. + +This is the mirror of [AZT003](../AZT003_misnamed_acceptance_test), which reports the opposite: a `TestAcc` name on a test that never runs an acceptance test. + +## Ignoring Reports + +Put `//azignore:AZT004 - ` at the end of the line, or on the line above it. The reason is required. + +```go +func TestThing_basic(t *testing.T) { //azignore:AZT004 - +``` + +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/AZT004_acceptance_test_not_named_test_acc/testdata/src/github.com/hashicorp/terraform-plugin-testing/helper/resource/resource.go b/checks/AZT/AZT004_acceptance_test_not_named_test_acc/testdata/src/github.com/hashicorp/terraform-plugin-testing/helper/resource/resource.go new file mode 100644 index 0000000..2d71238 --- /dev/null +++ b/checks/AZT/AZT004_acceptance_test_not_named_test_acc/testdata/src/github.com/hashicorp/terraform-plugin-testing/helper/resource/resource.go @@ -0,0 +1,24 @@ +// Package resource is a minimal stub of the terraform-plugin-testing harness. +package resource + +const EnvTfAcc = "TF_ACC" + +// T mirrors the harness's own testing interface, which *testing.T satisfies. +type T interface { + Fatal(args ...any) +} + +type TestStep struct { + Config string +} + +type TestCase struct { + Steps []TestStep +} + +func Test(t T, c TestCase) {} + +func ParallelTest(t T, c TestCase) {} + +// UnitTest runs the case without TF_ACC: a unit test on the harness, not an acceptance test. +func UnitTest(t T, c TestCase) {} diff --git a/checks/AZT/AZT004_acceptance_test_not_named_test_acc/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/acceptance/acceptance.go b/checks/AZT/AZT004_acceptance_test_not_named_test_acc/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/acceptance/acceptance.go new file mode 100644 index 0000000..dd2196e --- /dev/null +++ b/checks/AZT/AZT004_acceptance_test_not_named_test_acc/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/acceptance/acceptance.go @@ -0,0 +1,26 @@ +// Package acceptance is a minimal stub of the provider's acceptance test framework. +package acceptance + +import "testing" + +type TestStep struct { + Config string +} + +type TestData struct { + ResourceName string +} + +func BuildTestData(t *testing.T, resourceType, resourceLabel string) TestData { + return TestData{ResourceName: resourceType + "." + resourceLabel} +} + +func (td TestData) ResourceTest(t *testing.T, r any, steps []TestStep) {} + +func (td TestData) DataSourceTest(t *testing.T, steps []TestStep) {} + +func RunTestsInSequence(t *testing.T, tests map[string]map[string]func(t *testing.T)) {} + +func RandString(length int) string { + return "" +} diff --git a/checks/AZT/AZT004_acceptance_test_not_named_test_acc/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt004/thing.go b/checks/AZT/AZT004_acceptance_test_not_named_test_acc/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt004/thing.go new file mode 100644 index 0000000..6ab2407 --- /dev/null +++ b/checks/AZT/AZT004_acceptance_test_not_named_test_acc/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt004/thing.go @@ -0,0 +1,23 @@ +package azt004 + +import ( + "os" + "testing" + + "github.com/hashicorp/terraform-provider-azurerm/internal/acceptance" +) + +func BuildName(prefix string) string { + return prefix + "-thing" +} + +// Should NOT be flagged: go test only runs functions from _test.go files +func TestNotInATestFile(t *testing.T) { + acceptance.BuildTestData(t, "azurerm_thing", "test") +} + +// Configured is production code that reads TF_ACC to pick a default; calling it does not make +// a test an acceptance test +func Configured() bool { + return os.Getenv("TF_ACC") != "" +} diff --git a/checks/AZT/AZT004_acceptance_test_not_named_test_acc/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt004/thing_internal_test.go b/checks/AZT/AZT004_acceptance_test_not_named_test_acc/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt004/thing_internal_test.go new file mode 100644 index 0000000..3a6d4fd --- /dev/null +++ b/checks/AZT/AZT004_acceptance_test_not_named_test_acc/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt004/thing_internal_test.go @@ -0,0 +1,19 @@ +package azt004 + +import ( + "testing" + + "github.com/hashicorp/terraform-provider-azurerm/internal/acceptance" +) + +// Should be flagged: in-package test files are checked too +func TestBuildName_live(t *testing.T) { // want `TestBuildName_live runs an acceptance test, name it TestAccBuildName_live so acceptance runs pick it up` + acceptance.BuildTestData(t, "azurerm_thing", "test") +} + +// Should NOT be flagged: a unit test +func TestBuildName(t *testing.T) { + if BuildName("a") != "a-thing" { + t.Fatal("wrong name") + } +} diff --git a/checks/AZT/AZT004_acceptance_test_not_named_test_acc/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt004/thing_resource_test.go b/checks/AZT/AZT004_acceptance_test_not_named_test_acc/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt004/thing_resource_test.go new file mode 100644 index 0000000..878fc21 --- /dev/null +++ b/checks/AZT/AZT004_acceptance_test_not_named_test_acc/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt004/thing_resource_test.go @@ -0,0 +1,97 @@ +package azt004_test + +import ( + "os" + "testing" + + "github.com/hashicorp/terraform-plugin-testing/helper/resource" + "github.com/hashicorp/terraform-provider-azurerm/internal/acceptance" + "github.com/hashicorp/terraform-provider-azurerm/internal/services/azt004" +) + +type ThingResource struct{} + +// Should NOT be flagged: named as an acceptance test +func TestAccThing_basic(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_thing", "test") + data.ResourceTest(t, ThingResource{}, []acceptance.TestStep{{Config: "basic"}}) +} + +// Should NOT be flagged: a unit test +func TestThing_validation(t *testing.T) { + if azt004.BuildName("a") != "a-thing" { + t.Fatal("wrong name") + } +} + +// Should be flagged: runs through the acceptance framework under a plain Test name +func TestThing_basic(t *testing.T) { // want `TestThing_basic runs an acceptance test, name it TestAccThing_basic so acceptance runs pick it up` + data := acceptance.BuildTestData(t, "azurerm_thing", "test") + data.ResourceTest(t, ThingResource{}, []acceptance.TestStep{{Config: "basic"}}) +} + +func TestThingDataSource_basic(t *testing.T) { // want `TestThingDataSource_basic runs an acceptance test, name it TestAccThingDataSource_basic so acceptance runs pick it up` + data := acceptance.BuildTestData(t, "azurerm_thing", "test") + data.DataSourceTest(t, []acceptance.TestStep{{Config: "basic"}}) +} + +// Should be flagged: through the plugin testing harness directly +func TestThing_list(t *testing.T) { // want `TestThing_list runs an acceptance test, name it TestAccThing_list so acceptance runs pick it up` + resource.Test(t, resource.TestCase{Steps: []resource.TestStep{{Config: "list"}}}) +} + +// Should be flagged: the acceptance test is in a helper, called or handed over as a value +func TestThing_viaHelper(t *testing.T) { // want `TestThing_viaHelper runs an acceptance test, name it TestAccThing_viaHelper so acceptance runs pick it up` + testThing_helper(t) +} + +func TestThing_sequential(t *testing.T) { // want `TestThing_sequential runs an acceptance test, name it TestAccThing_sequential so acceptance runs pick it up` + acceptance.RunTestsInSequence(t, map[string]map[string]func(t *testing.T){ + "Resource": { + "basic": testThing_helper, + }, + }) +} + +// Should NOT be flagged: lowercase helpers are not run by go test +func testThing_helper(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_thing", "test") + data.ResourceTest(t, ThingResource{}, []acceptance.TestStep{{Config: "helper"}}) +} + +// Should be flagged: talks to Azure by hand behind a TF_ACC check +func TestThing_cliAuth(t *testing.T) { // want `TestThing_cliAuth runs an acceptance test, name it TestAccThing_cliAuth so acceptance runs pick it up` + if os.Getenv("TF_ACC") == "" { + t.Skip("TF_ACC not set") + } +} + +// Should be flagged: Acc is in the name, just not where the runner looks +func TestWebAppAccActiveSlot_basic(t *testing.T) { // want `TestWebAppAccActiveSlot_basic runs an acceptance test, rename it to start with TestAcc so acceptance runs pick it up` + data := acceptance.BuildTestData(t, "azurerm_thing", "test") + data.ResourceTest(t, ThingResource{}, []acceptance.TestStep{{Config: "basic"}}) +} + +// Should NOT be flagged: the harness's UnitTest runs without TF_ACC, so this is a unit test +func TestThing_unitHarness(t *testing.T) { + resource.UnitTest(t, resource.TestCase{Steps: []resource.TestStep{{Config: "unit"}}}) +} + +// Should be flagged, with the plain suggestion: Account is not a stray Acc +func TestStorageAccount_basic(t *testing.T) { // want `TestStorageAccount_basic runs an acceptance test, name it TestAccStorageAccount_basic so acceptance runs pick it up` + data := acceptance.BuildTestData(t, "azurerm_thing", "test") + data.ResourceTest(t, ThingResource{}, []acceptance.TestStep{{Config: "basic"}}) +} + +// Should NOT be flagged: production code reading TF_ACC does not make this an acceptance test +func TestThing_configured(t *testing.T) { + if !azt004.Configured() { + t.Skip("not configured") + } +} + +// Should NOT be flagged: a method is not a test function, so no runner selects it +func (r ThingResource) TestThing_receiver(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_thing", "test") + data.ResourceTest(t, r, []acceptance.TestStep{{Config: "method"}}) +} diff --git a/lib/acctest/acctest.go b/lib/acctest/acctest.go new file mode 100644 index 0000000..aa9b80d --- /dev/null +++ b/lib/acctest/acctest.go @@ -0,0 +1,122 @@ +// Package acctest finds the functions of a package that run an acceptance test, for the +// checks that compare that against how the function is named. +package acctest + +import ( + "go/ast" + "go/constant" + "go/types" + "reflect" + "slices" + "strings" + + "golang.org/x/tools/go/analysis" + + "github.com/katbyte/azproviderlint/lib/astx" +) + +// Analyzer computes Funcs for a package. A function runs an acceptance test when it hands +// `t` to the Terraform test harness or the provider's own internal/acceptance package, looks +// at TF_ACC from a test file, or calls or passes around another function of the package that +// does. The harness's UnitTest entry point is the exception: it runs without TF_ACC by +// design. Production code reading TF_ACC to pick a default does not count either, or every +// test touching the provider would. Packages without test files have nothing to find and +// yield an empty result. +var Analyzer = &analysis.Analyzer{ + Name: "acctest", + Doc: "find the functions that run an acceptance test", + Run: run, + ResultType: reflect.TypeFor[Funcs](), +} + +// Funcs maps every function in the package that runs an acceptance test to its declaration. +type Funcs map[*types.Func]*ast.FuncDecl + +// The Terraform test harnesses; a provider's own internal/acceptance package counts too. +var harnessPackages = map[string]bool{ + "github.com/hashicorp/terraform-plugin-testing/helper/resource": true, + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource": true, +} + +func run(pass *analysis.Pass) (any, error) { + hasTests := slices.ContainsFunc(pass.Files, func(f *ast.File) bool { + return strings.HasSuffix(pass.Fset.Position(f.Pos()).Filename, "_test.go") + }) + if !hasTests { + return Funcs{}, nil + } + + decls := map[*types.Func]*ast.FuncDecl{} + inTestFile := map[*types.Func]bool{} + for _, file := range pass.Files { + isTest := strings.HasSuffix(pass.Fset.Position(file.Pos()).Filename, "_test.go") + for _, decl := range file.Decls { + fd, ok := decl.(*ast.FuncDecl) + if !ok || fd.Body == nil { + continue + } + if fn, ok := pass.TypesInfo.Defs[fd.Name].(*types.Func); ok { + decls[fn] = fd + inTestFile[fn] = isTest + } + } + } + + // runs marks the functions that are acceptance tests on their own: they hand t to a + // harness or look at TF_ACC. mentions lists the same-package functions each one calls or + // passes around as a value (`t.Run("basic", testAccThing_basic)`). + runs := map[*types.Func]bool{} + mentions := map[*types.Func][]*types.Func{} + for fn, fd := range decls { + ast.Inspect(fd.Body, func(n ast.Node) bool { + switch n := n.(type) { + case *ast.CallExpr: + callee := astx.CalledFunc(pass, n) + if callee == nil || callee.Pkg() == nil { + break + } + if path := callee.Pkg().Path(); (!harnessPackages[path] && !strings.HasSuffix(path, "/internal/acceptance")) || callee.Name() == "UnitTest" { + break + } + for _, arg := range n.Args { + ptr, ok := pass.TypesInfo.TypeOf(arg).(*types.Pointer) + if !ok { + continue + } + if named, ok := ptr.Elem().(*types.Named); ok && named.Obj().Name() == "T" && named.Obj().Pkg() != nil && named.Obj().Pkg().Path() == "testing" { + runs[fn] = true + } + } + case *ast.Ident: + if used, ok := pass.TypesInfo.Uses[n].(*types.Func); ok && used.Pkg() == pass.Pkg { + mentions[fn] = append(mentions[fn], used.Origin()) + } + } + // by value, so resource.EnvTfAcc and local constants count as well as the literal + if e, ok := n.(ast.Expr); ok && inTestFile[fn] { + if v := pass.TypesInfo.Types[e].Value; v != nil && v.Kind() == constant.String && constant.StringVal(v) == "TF_ACC" { + runs[fn] = true + } + } + return true + }) + } + + // a function that mentions an acceptance test is one too + for changed := true; changed; { + changed = false + for fn, used := range mentions { + if !runs[fn] && slices.ContainsFunc(used, func(u *types.Func) bool { return runs[u] }) { + runs[fn] = true + changed = true + } + } + } + + found := Funcs{} + for fn := range runs { + found[fn] = decls[fn] + } + + return found, nil +}