add AZT006: update tests must apply two different configs

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 ([#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))
 - 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))

README.md

@@ -105,8 +105,9 @@ 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 |
+| [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 |
 
 ### AZN - Naming Conventions
 

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"
+	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"
 )
 
@@ -16,6 +17,7 @@ var Checks = []*analysis.Analyzer{
 	AZT001.Analyzer,
 	AZT002.Analyzer,
 	AZT003.Analyzer,
-	AZT007.Analyzer,
 	AZT004.Analyzer,
+	AZT006.Analyzer,
+	AZT007.Analyzer,
 }

checks/AZT/AZT006_update_test_single_config/AZT006.go

@@ -0,0 +1,91 @@
+// 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.SplitSeq(rest, "_") {
+		if strings.HasPrefix(strings.ToLower(seg), "update") {
+			return true
+		}
+	}
+	return false
+}

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")
+}

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 - <reason>` 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 - <reason>
+```
+
+Under golangci-lint, `//nolint:azproviderlint` in the same place also works, but it silences every azproviderlint check on that line.

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}
+}

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{}

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)},
+	})
+}