diff --git a/CHANGELOG.md b/CHANGELOG.md index eb84acc..88ac409 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 ([#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)) diff --git a/README.md b/README.md index 930959c..1a009cf 100644 --- a/README.md +++ b/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 diff --git a/checks/AZT/AZT.go b/checks/AZT/AZT.go index 10a670e..f5a6ca1 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" + 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, } 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..d87473f --- /dev/null +++ b/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 +} 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)}, + }) +}