From d57786d22e11ed2507033e998051992254a4493b Mon Sep 17 00:00:00 2001 From: kt Date: Fri, 9 Oct 2026 14:05:06 -0700 Subject: [PATCH 1/3] add AZT006: update tests must apply two different configs --- CHANGELOG.md | 1 + README.md | 1 + checks/AZT/AZT.go | 2 + .../AZT006.go | 88 ++++++++++++++ .../AZT006_test.go | 16 +++ .../README.md | 66 +++++++++++ .../internal/acceptance/acceptance.go | 24 ++++ .../services/azt006/thing_resource.go | 3 + .../services/azt006/thing_resource_test.go | 108 ++++++++++++++++++ 9 files changed, 309 insertions(+) create mode 100644 checks/AZT/AZT006_update_test_single_config/AZT006.go create mode 100644 checks/AZT/AZT006_update_test_single_config/AZT006_test.go create mode 100644 checks/AZT/AZT006_update_test_single_config/README.md create mode 100644 checks/AZT/AZT006_update_test_single_config/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/acceptance/acceptance.go create mode 100644 checks/AZT/AZT006_update_test_single_config/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt006/thing_resource.go create mode 100644 checks/AZT/AZT006_update_test_single_config/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt006/thing_resource_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index aecd6f7..127fb18 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## Unreleased +- 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 - 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..52a80e6 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 | +| [AZT006](checks/AZT/AZT006_update_test_single_config) | update tests must apply two different configs | ### AZN - Naming Conventions diff --git a/checks/AZT/AZT.go b/checks/AZT/AZT.go index c400309..f05cf1e 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" + AZT006 "github.com/katbyte/azproviderlint/checks/AZT/AZT006_update_test_single_config" ) // Checks contains all AZT (acceptance testing) analyzers. @@ -14,4 +15,5 @@ var Checks = []*analysis.Analyzer{ AZT001.Analyzer, AZT002.Analyzer, AZT003.Analyzer, + AZT006.Analyzer, } diff --git a/checks/AZT/AZT006_update_test_single_config/AZT006.go b/checks/AZT/AZT006_update_test_single_config/AZT006.go new file mode 100644 index 0000000..14dbb5c --- /dev/null +++ b/checks/AZT/AZT006_update_test_single_config/AZT006.go @@ -0,0 +1,88 @@ +// Package AZT006 defines an analyzer that reports update tests that do not apply two +// different configs. +package AZT006 + +import ( + "go/ast" + "go/types" + "strings" + + "golang.org/x/tools/go/analysis" +) + +// Analyzer checks that a test named for an update step applies at least two different +// configs. An update test with one config, or the same config twice, only proves the +// resource can be created, and the update path it is named for is never run. +var Analyzer = &analysis.Analyzer{ + Name: "AZT006", + Doc: "check that update tests apply two different configs", + URL: "https://github.com/katbyte/azproviderlint/blob/main/checks/AZT/AZT006_update_test_single_config/README.md", + Run: run, +} + +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.Body == nil || fd.Recv != nil || !isUpdateTest(fd.Name.Name) { + continue + } + + // the configs the test applies, by their source text: Config keys of TestStep literals + var configs []string + distinct := map[string]bool{} + ast.Inspect(fd.Body, func(n ast.Node) bool { + cl, ok := n.(*ast.CompositeLit) + if !ok { + return true + } + named, ok := types.Unalias(pass.TypesInfo.TypeOf(cl)).(*types.Named) + if !ok || named.Obj().Name() != "TestStep" { + return true + } + for _, elt := range cl.Elts { + kv, ok := elt.(*ast.KeyValueExpr) + if !ok { + continue + } + if key, ok := kv.Key.(*ast.Ident); ok && key.Name == "Config" { + text := types.ExprString(kv.Value) + configs = append(configs, text) + distinct[text] = true + } + } + return true + }) + + // no steps here: the test delegates to a helper, which is checked on its own + switch { + case len(configs) == 0 || len(distinct) >= 2: + case len(configs) == 1: + pass.Reportf(fd.Name.Pos(), "%s applies only one config, an update test needs a second, different config to apply", fd.Name.Name) + default: + pass.Reportf(fd.Name.Pos(), "%s applies the same config every time, an update test needs a second, different config to apply", fd.Name.Name) + } + } + } + + return nil, nil +} + +// isUpdateTest reports whether a test name is for an update step: a segment after the first +// underscore that starts with update, as in TestAccThing_update, TestAccThing_updateSku and +// testAccThing_list_update, but not TestAccThing_autoUpdate. +func isUpdateTest(name string) bool { + _, rest, ok := strings.Cut(name, "_") + if !ok || !(strings.HasPrefix(name, "TestAcc") || strings.HasPrefix(name, "testAcc")) { + return false + } + for _, seg := range strings.Split(rest, "_") { + if strings.HasPrefix(strings.ToLower(seg), "update") { + return true + } + } + return false +} diff --git a/checks/AZT/AZT006_update_test_single_config/AZT006_test.go b/checks/AZT/AZT006_update_test_single_config/AZT006_test.go new file mode 100644 index 0000000..9ff31f6 --- /dev/null +++ b/checks/AZT/AZT006_update_test_single_config/AZT006_test.go @@ -0,0 +1,16 @@ +package AZT006 + +import ( + "path/filepath" + "runtime" + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +func TestAZT006(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/azt006") +} diff --git a/checks/AZT/AZT006_update_test_single_config/README.md b/checks/AZT/AZT006_update_test_single_config/README.md new file mode 100644 index 0000000..3b3ec6f --- /dev/null +++ b/checks/AZT/AZT006_update_test_single_config/README.md @@ -0,0 +1,66 @@ +# AZT006 - update tests must apply two different configs + +AZT006 reports a test named for an update step that applies a single config, or the same config more than once. + +An update test exists to run the resource's Update path. With one config it only proves the resource can be created, and with the same config twice the second apply is a no-op plan. Either way the step the test is named for never runs, and the name says it did. + +## Flagged Code + +```go +func TestAccThing_update(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_thing", "test") + r := ThingResource{} + + data.ResourceTest(t, r, []acceptance.TestStep{ + { + Config: r.complete(data), + }, + data.ImportStep(), + }) +} +``` + +## Passing Code + +```go +func TestAccThing_update(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_thing", "test") + r := ThingResource{} + + data.ResourceTest(t, r, []acceptance.TestStep{ + { + Config: r.basic(data), + }, + data.ImportStep(), + { + Config: r.complete(data), + }, + data.ImportStep(), + }) +} +``` + +## What counts + +A test is an update test when a segment of its name after the first underscore starts with `update`: `TestAccThing_update`, `TestAccThing_updateSku`, `testAccThing_list_update`. `TestAccThing_autoUpdateEnabled` is a feature name, not the step, and is left alone. Lowercase `testAcc` helpers count, since sequential tests keep their steps there. + +The configs are the `Config` values of the `TestStep` literals in the function body, compared as written. The same helper with different arguments is a different config. + +Not reported: + +- a test with no config steps of its own, which delegates to a helper; the helper is checked instead +- files other than `_test.go` + +## No fix + +AZT006 only reports. Add a second step with a changed config, or rename the test if it was never meant to update. + +## Ignoring Reports + +Put `//azignore:AZT006 - ` at the end of the line, or on the line above it. The reason is required. + +```go +func TestAccThing_update(t *testing.T) { //azignore:AZT006 - +``` + +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/AZT006_update_test_single_config/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/acceptance/acceptance.go b/checks/AZT/AZT006_update_test_single_config/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/acceptance/acceptance.go new file mode 100644 index 0000000..8286792 --- /dev/null +++ b/checks/AZT/AZT006_update_test_single_config/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/acceptance/acceptance.go @@ -0,0 +1,24 @@ +// Package acceptance is a minimal stub of the provider's acceptance test framework. +package acceptance + +import "testing" + +// TestStep mirrors the harness step; azurerm aliases it from terraform-plugin-testing. +type TestStep struct { + Config string + ImportState bool +} + +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) ImportStep() TestStep { + return TestStep{ImportState: true} +} diff --git a/checks/AZT/AZT006_update_test_single_config/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt006/thing_resource.go b/checks/AZT/AZT006_update_test_single_config/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt006/thing_resource.go new file mode 100644 index 0000000..4a8a0cf --- /dev/null +++ b/checks/AZT/AZT006_update_test_single_config/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt006/thing_resource.go @@ -0,0 +1,3 @@ +package azt006 + +type ThingResource struct{} diff --git a/checks/AZT/AZT006_update_test_single_config/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt006/thing_resource_test.go b/checks/AZT/AZT006_update_test_single_config/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt006/thing_resource_test.go new file mode 100644 index 0000000..2cefe10 --- /dev/null +++ b/checks/AZT/AZT006_update_test_single_config/testdata/src/github.com/hashicorp/terraform-provider-azurerm/internal/services/azt006/thing_resource_test.go @@ -0,0 +1,108 @@ +package azt006_test + +import ( + "testing" + + "github.com/hashicorp/terraform-provider-azurerm/internal/acceptance" +) + +type ThingResource struct{} + +func (r ThingResource) basic(data acceptance.TestData) string { return "basic" } +func (r ThingResource) complete(data acceptance.TestData) string { return "complete" } +func (r ThingResource) sku(data acceptance.TestData, sku string) string { + return "sku " + sku +} + +// Should NOT be flagged: two different configs +func TestAccThing_update(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_thing", "test") + r := ThingResource{} + + data.ResourceTest(t, r, []acceptance.TestStep{ + {Config: r.basic(data)}, + data.ImportStep(), + {Config: r.complete(data)}, + data.ImportStep(), + }) +} + +// Should NOT be flagged: the same helper with different arguments is a different config +func TestAccThing_updateSku(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_thing", "test") + r := ThingResource{} + + data.ResourceTest(t, r, []acceptance.TestStep{ + {Config: r.sku(data, "Basic")}, + {Config: r.sku(data, "Standard")}, + }) +} + +// Should NOT be flagged: not an update test +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)}, + }) +} + +// Should NOT be flagged: update inside a word is a feature name, not the step +func TestAccThing_autoUpdateEnabled(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_thing", "test") + r := ThingResource{} + + data.ResourceTest(t, r, []acceptance.TestStep{ + {Config: r.basic(data)}, + }) +} + +// Should NOT be flagged: delegates to a helper, which is checked on its own +func TestAccThing_updateViaHelper(t *testing.T) { + testAccThing_updateWith(t, "Standard") +} + +func testAccThing_updateWith(t *testing.T, sku string) { + data := acceptance.BuildTestData(t, "azurerm_thing", "test") + r := ThingResource{} + + data.ResourceTest(t, r, []acceptance.TestStep{ + {Config: r.basic(data)}, + {Config: r.sku(data, sku)}, + }) +} + +// Should be flagged: a single config +func TestAccThing_updateTags(t *testing.T) { // want `TestAccThing_updateTags applies only one config, an update test needs a second, different config to apply` + data := acceptance.BuildTestData(t, "azurerm_thing", "test") + r := ThingResource{} + + data.ResourceTest(t, r, []acceptance.TestStep{ + {Config: r.complete(data)}, + data.ImportStep(), + }) +} + +// Should be flagged: the same config applied twice +func TestAccThing_list_update(t *testing.T) { // want `TestAccThing_list_update applies the same config every time, an update test needs a second, different config to apply` + data := acceptance.BuildTestData(t, "azurerm_thing", "test") + r := ThingResource{} + + data.ResourceTest(t, r, []acceptance.TestStep{ + {Config: r.basic(data)}, + data.ImportStep(), + {Config: r.basic(data)}, + data.ImportStep(), + }) +} + +// Should be flagged: a sequential helper is an update test too +func testAccThing_updateIdentity(t *testing.T) { // want `testAccThing_updateIdentity applies only one config, an update test needs a second, different config to apply` + data := acceptance.BuildTestData(t, "azurerm_thing", "test") + r := ThingResource{} + + data.ResourceTest(t, r, []acceptance.TestStep{ + {Config: r.complete(data)}, + }) +} From 6098587cfa13dfea90bfa3e8d1128c6921e51406 Mon Sep 17 00:00:00 2001 From: kt Date: Fri, 9 Oct 2026 14:05:45 -0700 Subject: [PATCH 2/3] AZT006 - lint fixes, changelog link --- CHANGELOG.md | 2 +- checks/AZT/AZT006_update_test_single_config/AZT006.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 127fb18..eddc00c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ ## Unreleased -- 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 +- 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 `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/checks/AZT/AZT006_update_test_single_config/AZT006.go b/checks/AZT/AZT006_update_test_single_config/AZT006.go index 14dbb5c..efe2990 100644 --- a/checks/AZT/AZT006_update_test_single_config/AZT006.go +++ b/checks/AZT/AZT006_update_test_single_config/AZT006.go @@ -76,10 +76,10 @@ func run(pass *analysis.Pass) (any, error) { // testAccThing_list_update, but not TestAccThing_autoUpdate. func isUpdateTest(name string) bool { _, rest, ok := strings.Cut(name, "_") - if !ok || !(strings.HasPrefix(name, "TestAcc") || strings.HasPrefix(name, "testAcc")) { + if !ok || (!strings.HasPrefix(name, "TestAcc") && !strings.HasPrefix(name, "testAcc")) { return false } - for _, seg := range strings.Split(rest, "_") { + for seg := range strings.SplitSeq(rest, "_") { if strings.HasPrefix(strings.ToLower(seg), "update") { return true } From 169519b747f7c1874da83075fe9a21ce823df850 Mon Sep 17 00:00:00 2001 From: kt Date: Fri, 9 Oct 2026 14:17:18 -0700 Subject: [PATCH 3/3] join split lines, blank lines between steps --- checks/AZT/AZT006_update_test_single_config/AZT006.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/checks/AZT/AZT006_update_test_single_config/AZT006.go b/checks/AZT/AZT006_update_test_single_config/AZT006.go index efe2990..d87473f 100644 --- a/checks/AZT/AZT006_update_test_single_config/AZT006.go +++ b/checks/AZT/AZT006_update_test_single_config/AZT006.go @@ -39,10 +39,12 @@ func run(pass *analysis.Pass) (any, error) { if !ok { return true } + named, ok := types.Unalias(pass.TypesInfo.TypeOf(cl)).(*types.Named) if !ok || named.Obj().Name() != "TestStep" { return true } + for _, elt := range cl.Elts { kv, ok := elt.(*ast.KeyValueExpr) if !ok { @@ -54,6 +56,7 @@ func run(pass *analysis.Pass) (any, error) { distinct[text] = true } } + return true })