From f51a63d98eabc7c23befc95b398a5535f877ebbc Mon Sep 17 00:00:00 2001 From: kt Date: Fri, 9 Oct 2026 18:32:47 -0700 Subject: [PATCH 1/2] add AZT008: acceptance tests must be in a file named after the resource's file --- CHANGELOG.md | 1 + README.md | 1 + checks/AZT/AZT.go | 2 + .../AZT008.go | 309 ++++++++++++++++++ .../AZT008_test.go | 22 ++ .../README.md | 73 +++++ .../internal/acceptance/acceptance.go | 20 ++ .../internal/sdk/sdk.go | 10 + .../services/azt008/flagged_resource.go | 7 + .../internal/services/azt008/flagged_test.go | 14 + .../services/azt008/lb_probe_resource.go | 7 + .../loadbalancer_probe_resource_test.go | 26 ++ .../internal/services/azt008/registration.go | 46 +++ .../services/azt008/thing_data_source.go | 7 + .../services/azt008/thing_data_source_test.go | 26 ++ .../services/azt008/thing_internal_test.go | 13 + .../services/azt008/thing_resource.go | 7 + .../thing_resource_identity_gen_test.go | 13 + .../services/azt008/thing_resource_test.go | 48 +++ .../services/azt008/typed_data_source.go | 10 + .../services/azt008/typed_data_source_test.go | 16 + .../services/azt008/typed_resource.go | 7 + .../services/azt008/typed_resource_test.go | 20 ++ .../internal/services/azt008/typed_test.go | 27 ++ .../services/azt008exact/registration.go | 11 + .../services/azt008exact/thing_resource.go | 7 + .../azt008exact/thing_resource_other_test.go | 13 + .../azt008exact/thing_resource_test.go | 15 + .../internal/services/azt008other/other.go | 8 + .../internal/tf/pluginsdk/pluginsdk.go | 4 + 30 files changed, 790 insertions(+) create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/AZT008.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/AZT008_test.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/README.md create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/acceptance/acceptance.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/sdk/sdk.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008/flagged_resource.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008/flagged_test.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008/lb_probe_resource.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008/loadbalancer_probe_resource_test.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008/registration.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008/thing_data_source.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008/thing_data_source_test.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008/thing_internal_test.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008/thing_resource.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008/thing_resource_identity_gen_test.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008/thing_resource_test.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008/typed_data_source.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008/typed_data_source_test.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008/typed_resource.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008/typed_resource_test.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008/typed_test.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008exact/registration.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008exact/thing_resource.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008exact/thing_resource_other_test.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008exact/thing_resource_test.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008other/other.go create mode 100644 checks/AZT/AZT008_test_file_not_named_after_resource/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/tf/pluginsdk/pluginsdk.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 88ac409..81e21e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## Unreleased +- add rule `AZT008`: an acceptance test must be in a test file named after the file declaring the resource or data source it builds test data for (`lb_probe_resource.go` pairs with `lb_probe_resource_test.go`); `suffix: false` stops accepting `lb_probe_resource__test.go`; report only, no fix - add rule `AZT006`: tests named for an update step must apply two different configs; one config, or the same config twice, never runs the update; report only, no fix ([#69](https://github.com/katbyte/azproviderlint/pull/69)) - 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)) diff --git a/README.md b/README.md index 1a009cf..052fc1e 100644 --- a/README.md +++ b/README.md @@ -108,6 +108,7 @@ Rules are named `AZ`, a category letter, and a number. The letters follow [tfpro | [AZT004](checks/AZT/AZT004_acceptance_test_not_named_test_acc) | acceptance tests must be named TestAcc | | [AZT006](checks/AZT/AZT006_update_test_single_config) | update tests must apply two different configs | | [AZT007](checks/AZT/AZT007_stray_acc_in_test_name) | acceptance test names must not carry a second Acc | +| [AZT008](checks/AZT/AZT008_test_file_not_named_after_resource) | acceptance tests must be in a file named after the resource's file | ### AZN - Naming Conventions diff --git a/checks/AZT/AZT.go b/checks/AZT/AZT.go index f5a6ca1..32c483f 100644 --- a/checks/AZT/AZT.go +++ b/checks/AZT/AZT.go @@ -10,6 +10,7 @@ import ( AZT004 "github.com/katbyte/azproviderlint/checks/AZT/AZT004_acceptance_test_not_named_test_acc" AZT006 "github.com/katbyte/azproviderlint/checks/AZT/AZT006_update_test_single_config" AZT007 "github.com/katbyte/azproviderlint/checks/AZT/AZT007_stray_acc_in_test_name" + AZT008 "github.com/katbyte/azproviderlint/checks/AZT/AZT008_test_file_not_named_after_resource" ) // Checks contains all AZT (acceptance testing) analyzers. @@ -20,4 +21,5 @@ var Checks = []*analysis.Analyzer{ AZT004.Analyzer, AZT006.Analyzer, AZT007.Analyzer, + AZT008.Analyzer, } diff --git a/checks/AZT/AZT008_test_file_not_named_after_resource/AZT008.go b/checks/AZT/AZT008_test_file_not_named_after_resource/AZT008.go new file mode 100644 index 0000000..a5a7118 --- /dev/null +++ b/checks/AZT/AZT008_test_file_not_named_after_resource/AZT008.go @@ -0,0 +1,309 @@ +// Package AZT008 defines an analyzer that reports acceptance tests in a file that is not named +// after the file declaring the resource or data source they test. +package AZT008 + +import ( + "flag" + "fmt" + "go/ast" + "go/constant" + "go/parser" + "go/token" + "go/types" + "os" + "path/filepath" + "slices" + "strconv" + "strings" + + "golang.org/x/tools/go/analysis" + + "github.com/katbyte/azproviderlint/lib/astx" +) + +// Analyzer checks that a test sits in a file named after the file declaring what it tests: +// tests for the resource declared in `foo_resource.go` belong in `foo_resource_test.go` or, +// unless the suffix option is off, `foo_resource__test.go`. What a test tests is what it +// builds test data for, the second argument of `acceptance.BuildTestData`. +var Analyzer = &analysis.Analyzer{ + Name: "AZT008", + Doc: "check that acceptance tests are in a file named after the file declaring the resource they test", + URL: "https://github.com/katbyte/azproviderlint/blob/main/checks/AZT/AZT008_test_file_not_named_after_resource/README.md", + Run: run, +} + +// suffix accepts `foo_resource__test.go` as well as `foo_resource_test.go`, so one +// resource's tests can be split across files. +var suffix = true + +func init() { + Analyzer.Flags.Init("AZT008", flag.ContinueOnError) + Analyzer.Flags.BoolVar(&suffix, "suffix", true, "also accept a test file with a part between the declaring file's name and _test.go (false requires exactly _test.go)") +} + +// What a registration method registers, as reports name it. +const ( + resource = "resource" + dataSource = "data source" +) + +var registers = map[string]string{ + "SupportedResources": resource, + "Resources": resource, + "FrameworkResources": resource, + "SupportedDataSources": dataSource, + "DataSources": dataSource, + "FrameworkDataSources": dataSource, +} + +func run(pass *analysis.Pass) (any, error) { + // the functions in test files that build test data, with what each passes as the type + type test struct { + file string + decl *ast.FuncDecl + built []ast.Expr + dataSource bool // also hands t to a data source harness + } + + var tests []test + dir := "" + for _, file := range pass.Files { + filename := pass.Fset.Position(file.Pos()).Filename + if !strings.HasSuffix(filename, "_test.go") { + continue + } + + for _, decl := range file.Decls { + fd, ok := decl.(*ast.FuncDecl) + if !ok || fd.Body == nil { + continue + } + + tt := test{file: filepath.Base(filename), decl: fd} + ast.Inspect(fd.Body, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + + fn := astx.CalledFunc(pass, call) + if fn == nil || fn.Pkg() == nil || !strings.HasSuffix(fn.Pkg().Path(), "/internal/acceptance") { + return true + } + + switch { + case fn.Name() == "BuildTestData" && len(call.Args) > 1: + tt.built = append(tt.built, call.Args[1]) + case strings.HasPrefix(fn.Name(), "DataSource"): + tt.dataSource = true + } + + return true + }) + + if len(tt.built) > 0 { + tests = append(tests, tt) + dir = filepath.Dir(filename) + } + } + } + + if len(tests) == 0 { + return nil, nil + } + + // Test files are a separate package from the one they test, so that package's own files are + // read from disk. Syntax is enough to tell what it registers: a registered value is a call + // to one of its functions, or a literal of one of its types that ResourceType names. + names, err := os.ReadDir(dir) + if err != nil { + return nil, err + } + + fset := token.NewFileSet() + funcFile, typeFile := map[string]string{}, map[string]string{} // the file declaring each function and type + strs := map[string]string{} // package-level string constants and variables + resourceType := map[string]ast.Expr{} // what each type's ResourceType method returns + var registrations []*ast.FuncDecl + for _, entry := range names { + if entry.IsDir() || !strings.HasSuffix(entry.Name(), ".go") || strings.HasSuffix(entry.Name(), "_test.go") { + continue + } + + f, err := parser.ParseFile(fset, filepath.Join(dir, entry.Name()), nil, parser.SkipObjectResolution) + if err != nil { + continue + } + + for _, decl := range f.Decls { + switch d := decl.(type) { + case *ast.GenDecl: + for _, spec := range d.Specs { + switch s := spec.(type) { + case *ast.TypeSpec: + typeFile[s.Name.Name] = entry.Name() + case *ast.ValueSpec: + for i, name := range s.Names { + if i >= len(s.Values) { + break + } + if lit, ok := s.Values[i].(*ast.BasicLit); ok && lit.Kind == token.STRING { + strs[name.Name], _ = strconv.Unquote(lit.Value) + } + } + } + } + case *ast.FuncDecl: + if d.Recv == nil { + funcFile[d.Name.Name] = entry.Name() + continue + } + if d.Body == nil { + continue + } + + if registers[d.Name.Name] != "" { + registrations = append(registrations, d) + } + + recv := d.Recv.List[0].Type + if star, ok := recv.(*ast.StarExpr); ok { + recv = star.X + } + if id, ok := recv.(*ast.Ident); ok && d.Name.Name == "ResourceType" && len(d.Body.List) == 1 { + if ret, ok := d.Body.List[0].(*ast.ReturnStmt); ok && len(ret.Results) == 1 { + resourceType[id.Name] = ret.Results[0] + } + } + } + } + } + + // a terraform type name: a string literal, or a package-level constant or variable + text := func(e ast.Expr) string { + switch e := e.(type) { + case *ast.BasicLit: + if s, err := strconv.Unquote(e.Value); err == nil && e.Kind == token.STRING { + return s + } + case *ast.Ident: + return strs[e.Name] + } + return "" + } + + type declared struct{ kind, name, file string } + + byName, byType := map[string]declared{}, map[string]declared{} + for _, registration := range registrations { + kind := registers[registration.Name.Name] + ast.Inspect(registration.Body, func(n ast.Node) bool { + var key, value ast.Expr + switch n := n.(type) { + case *ast.KeyValueExpr: // a map entry: the name, then a call to the declaring function + key, value = n.Key, n.Value + case *ast.AssignStmt: // the same assigned by index, as behind a feature flag + if index, ok := n.Lhs[0].(*ast.IndexExpr); ok && len(n.Rhs) == 1 { + key, value = index.Index, n.Rhs[0] + } + case *ast.CompositeLit: // a typed or framework element, named by its ResourceType method + if id, ok := n.Type.(*ast.Ident); ok && typeFile[id.Name] != "" && text(resourceType[id.Name]) != "" { + d := declared{kind, text(resourceType[id.Name]), typeFile[id.Name]} + byName[kind+" "+d.name], byType[id.Name] = d, d + } + } + + if call, ok := value.(*ast.CallExpr); ok { + if fn, ok := call.Fun.(*ast.Ident); ok && funcFile[fn.Name] != "" && text(key) != "" { + byName[kind+" "+text(key)] = declared{kind, text(key), funcFile[fn.Name]} + } + } + + return true + }) + } + + // A test is in the right file when the file is named after the declaring file of anything + // it builds test data for. Otherwise it is reported against the first, once per test file. + type misplaced struct { + declared + first *ast.FuncDecl + count int + } + + found := map[string]*misplaced{} + var order []string + own := strings.TrimSuffix(pass.Pkg.Path(), "_test") + for _, tt := range tests { + var want []declared + for _, arg := range tt.built { + // "azurerm_foo", or "data.azurerm_foo" for a data source. A test run as a data source + // test under the bare name may be either, so both files are right for it. + if v := pass.TypesInfo.Types[arg].Value; v != nil && v.Kind() == constant.String { + name, isData := strings.CutPrefix(constant.StringVal(v), "data.") + if !isData { + want = append(want, byName[resource+" "+name]) + } + if isData || tt.dataSource { + want = append(want, byName[dataSource+" "+name]) + } + + continue + } + + // FooResource{}.ResourceType(), on its own or inside a fmt.Sprintf("data.%s", ...) + ast.Inspect(arg, func(n ast.Node) bool { + sel, ok := n.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "ResourceType" { + return true + } + + t := pass.TypesInfo.TypeOf(sel.X) + if ptr, ok := t.(*types.Pointer); ok { + t = ptr.Elem() + } + if named, ok := types.Unalias(t).(*types.Named); ok && named.Obj().Pkg() != nil && named.Obj().Pkg().Path() == own { + want = append(want, byType[named.Obj().Name()]) + } + + return true + }) + } + + // nothing registered under that name in this package: not this check's to judge + want = slices.DeleteFunc(want, func(d declared) bool { return d.file == "" }) + right := slices.ContainsFunc(want, func(d declared) bool { + stem := strings.TrimSuffix(d.file, ".go") + return tt.file == stem+"_test.go" || (suffix && strings.HasPrefix(tt.file, stem+"_")) + }) + if len(want) == 0 || right { + continue + } + + key := tt.file + " " + want[0].kind + " " + want[0].name + if found[key] == nil { + found[key] = &misplaced{declared: want[0], first: tt.decl} + order = append(order, key) + } + + found[key].count++ + } + + for _, key := range order { + m := found[key] + stem := strings.TrimSuffix(m.file, ".go") + who, belongs := m.first.Name.Name+" builds", "it belongs" + if m.count > 1 { + who, belongs = fmt.Sprintf("%s and %d more in this file build", m.first.Name.Name, m.count-1), "they belong" + } + + where := stem + "_test.go" + if suffix { + where += " or " + stem + "__test.go" + } + + pass.Reportf(m.first.Name.Pos(), "%s test data for %s %q, declared in %s, so %s in %s", who, m.kind, m.name, m.file, belongs, where) + } + + return nil, nil +} diff --git a/checks/AZT/AZT008_test_file_not_named_after_resource/AZT008_test.go b/checks/AZT/AZT008_test_file_not_named_after_resource/AZT008_test.go new file mode 100644 index 0000000..e3ae710 --- /dev/null +++ b/checks/AZT/AZT008_test_file_not_named_after_resource/AZT008_test.go @@ -0,0 +1,22 @@ +package AZT008 + +import ( + "path/filepath" + "runtime" + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +func TestAZT008(t *testing.T) { + t.Parallel() + + _, filename, _, _ := runtime.Caller(0) + dir := filepath.Join(filepath.Dir(filename), "testdata") + + analysistest.Run(t, dir, Analyzer, "github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008") + + suffix = false + analysistest.Run(t, dir, Analyzer, "github.com/hashicorp/terraform-provider-azurerm/internal/services/azt008exact") + suffix = true +} diff --git a/checks/AZT/AZT008_test_file_not_named_after_resource/README.md b/checks/AZT/AZT008_test_file_not_named_after_resource/README.md new file mode 100644 index 0000000..aee2055 --- /dev/null +++ b/checks/AZT/AZT008_test_file_not_named_after_resource/README.md @@ -0,0 +1,73 @@ +# AZT008 - acceptance tests must be in a file named after the resource's file + +AZT008 reports an acceptance test that sits in a test file not named after the file declaring the resource or data source it tests. + +Tests for the resource declared in `lb_probe_resource.go` belong in `lb_probe_resource_test.go`. When they live in `loadbalancer_probe_resource_test.go` nobody reading the resource finds them, tooling that pairs a resource with its tests by file name misses them, and a test that drifted into another resource's file goes unnoticed. + +## Flagged Code + +```go +// lb_probe_resource.go declares the resource registered as "azurerm_lb_probe" + +// loadbalancer_probe_resource_test.go +func TestAccAzureRMLoadBalancerProbe_basic(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_lb_probe", "test") + // ... +} +``` + +```text +TestAccAzureRMLoadBalancerProbe_basic and 5 more in this file build test data for resource "azurerm_lb_probe", declared in lb_probe_resource.go, so they belong in lb_probe_resource_test.go or lb_probe_resource__test.go +``` + +## Passing Code + +```go +// lb_probe_resource_test.go, or lb_probe_resource_identity_gen_test.go +func TestAccLbProbe_basic(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_lb_probe", "test") + // ... +} +``` + +## What counts + +What a test tests is what it builds test data for: the second argument of `acceptance.BuildTestData`. Every function in a `_test.go` file that makes that call is checked, lowercase helpers included. + +- `"azurerm_foo"` is the resource and `"data.azurerm_foo"` the data source; a constant works as well as a literal +- `FooResource{}.ResourceType()` is whatever that type is registered as, also inside a `fmt.Sprintf("data.%s", ...)` + +The declaring file is the one holding the function or type that the service's registration registers under that name: `resourceFoo()` in `SupportedResources()`, `FooResource{}` in `Resources()`, and the data source and framework equivalents. Test files are a separate package, so the service package's own files are read from disk. + +A test file is right when its name is the declaring file's name with `_test` added, or with a part in between: `foo_resource_test.go`, `foo_resource_network_test.go`, `foo_resource_identity_gen_test.go`. The part has to come after the whole name, so `foo_network_resource_test.go` is reported. + +One report is made per test file and resource, on the first such test, with a count of the rest. + +Not reported: + +- a test that builds test data for several things, when its file is right for any one of them +- a test run through `DataSourceTest` under the bare resource name, in either the resource's or the data source's test file +- a name nothing in the test's own package registers, ephemeral resources included +- a type that is not known until the test runs, such as a loop variable + +## No fix + +AZT008 only reports. Rename the test file, or move the test to the file it belongs in. If the test is in the right file and builds test data for the wrong thing, correct the `BuildTestData` argument instead. + +## Options + +| Option | Default | Effect | +|---|---|---| +| `suffix` | true | also accept a part between the declaring file's name and `_test.go`, so one resource's tests can be split across files; false requires exactly `_test.go` | + +Set with `-AZT008.