From 57edb9c248746ece3e4e95d2cf68c38f2f6940e5 Mon Sep 17 00:00:00 2001 From: Liam Cervante Date: Mon, 13 Nov 2023 10:36:06 +0100 Subject: [PATCH] testing framework: add warnings for override blocks with invalid targets (#34181) * testing framework: add warnings for override blocks with invalid targets * pre-add the override sources for upcoming features * address comments --- internal/backend/local/test.go | 6 +- internal/command/test_test.go | 94 ++++++++++++++++ .../testdata/test/invalid-overrides/main.tf | 6 + .../test/invalid-overrides/main.tftest.hcl | 47 ++++++++ .../test/invalid-overrides/setup/main.tf | 2 + internal/command/validate.go | 10 +- internal/command/validate_test.go | 72 ++++++++++++ internal/configs/config.go | 41 +++++++ internal/configs/mock_provider.go | 35 ++++-- internal/configs/test_file.go | 103 ++++++++++++++++-- internal/configs/test_file_test.go | 2 +- 11 files changed, 395 insertions(+), 23 deletions(-) create mode 100644 internal/command/testdata/test/invalid-overrides/main.tf create mode 100644 internal/command/testdata/test/invalid-overrides/main.tftest.hcl create mode 100644 internal/command/testdata/test/invalid-overrides/setup/main.tf diff --git a/internal/backend/local/test.go b/internal/backend/local/test.go index afe65ebcc5..d6d0e91455 100644 --- a/internal/backend/local/test.go +++ b/internal/backend/local/test.go @@ -251,6 +251,10 @@ func (runner *TestFileRunner) Test(file *moduletest.File) { // First thing, initialise the global variables for the file runner.initVariables(file) + // The file validation only returns warnings so we'll just add them without + // checking anything about them. + file.Diagnostics = file.Diagnostics.Append(file.Config.Validate(runner.Suite.Config)) + // We'll execute the tests in the file. First, mark the overall status as // being skipped. This will ensure that if we've cancelled and the files not // going to do anything it'll be marked as skipped. @@ -354,7 +358,7 @@ func (runner *TestFileRunner) run(run *moduletest.Run, file *moduletest.File, st start := time.Now().UTC().UnixMilli() runner.Suite.View.Run(run, file, moduletest.Starting, 0) - run.Diagnostics = run.Diagnostics.Append(run.Config.Validate()) + run.Diagnostics = run.Diagnostics.Append(run.Config.Validate(config)) if run.Diagnostics.HasErrors() { run.Status = moduletest.Error return state, false diff --git a/internal/command/test_test.go b/internal/command/test_test.go index c335fcbd2c..ff2ea2006d 100644 --- a/internal/command/test_test.go +++ b/internal/command/test_test.go @@ -1782,6 +1782,100 @@ func TestTest_LongRunningTestJSON(t *testing.T) { } } +func TestTest_InvalidOverrides(t *testing.T) { + td := t.TempDir() + testCopyDir(t, testFixturePath(path.Join("test", "invalid-overrides")), td) + defer testChdir(t, td)() + + provider := testing_command.NewProvider(nil) + + providerSource, close := newMockProviderSource(t, map[string][]string{ + "test": {"1.0.0"}, + }) + defer close() + + streams, done := terminal.StreamsForTesting(t) + view := views.NewView(streams) + ui := new(cli.MockUi) + + meta := Meta{ + testingOverrides: metaOverridesForProvider(provider.Provider), + Ui: ui, + View: view, + Streams: streams, + ProviderSource: providerSource, + } + + init := &InitCommand{ + Meta: meta, + } + + if code := init.Run(nil); code != 0 { + t.Fatalf("expected status code 0 but got %d: %s", code, ui.ErrorWriter) + } + + c := &TestCommand{ + Meta: meta, + } + + code := c.Run([]string{"-no-color"}) + output := done(t) + + if code != 0 { + t.Errorf("expected status code 0 but got %d", code) + } + + expected := `main.tftest.hcl... in progress + run "setup"... pass + +Warning: Invalid override target + + on main.tftest.hcl line 39, in run "setup": + 39: target = test_resource.absent_five + +The override target test_resource.absent_five does not exist within the +configuration under test. This could indicate a typo in the target address or +an unnecessary override. + + run "test"... pass + +Warning: Invalid override target + + on main.tftest.hcl line 45, in run "test": + 45: target = module.setup.test_resource.absent_six + +The override target module.setup.test_resource.absent_six does not exist +within the configuration under test. This could indicate a typo in the target +address or an unnecessary override. + +main.tftest.hcl... tearing down +main.tftest.hcl... pass + +Warning: Invalid override target + + on main.tftest.hcl line 4, in mock_provider "test": + 4: target = test_resource.absent_one + +The override target test_resource.absent_one does not exist within the +configuration under test. This could indicate a typo in the target address or +an unnecessary override. + +(and 3 more similar warnings elsewhere) + +Success! 2 passed, 0 failed. +` + + actual := output.All() + + if diff := cmp.Diff(actual, expected); len(diff) > 0 { + t.Errorf("output didn't match expected:\nexpected:\n%s\nactual:\n%s\ndiff:\n%s", expected, actual, diff) + } + + if provider.ResourceCount() > 0 { + t.Errorf("should have deleted all resources on completion but left %v", provider.ResourceString()) + } +} + func TestTest_RunBlocksInProviders(t *testing.T) { td := t.TempDir() testCopyDir(t, testFixturePath(path.Join("test", "provider_runs")), td) diff --git a/internal/command/testdata/test/invalid-overrides/main.tf b/internal/command/testdata/test/invalid-overrides/main.tf new file mode 100644 index 0000000000..5c21823e72 --- /dev/null +++ b/internal/command/testdata/test/invalid-overrides/main.tf @@ -0,0 +1,6 @@ + +resource "test_resource" "resource" {} + +module "setup" { + source = "./setup" +} diff --git a/internal/command/testdata/test/invalid-overrides/main.tftest.hcl b/internal/command/testdata/test/invalid-overrides/main.tftest.hcl new file mode 100644 index 0000000000..c6317c7a9a --- /dev/null +++ b/internal/command/testdata/test/invalid-overrides/main.tftest.hcl @@ -0,0 +1,47 @@ + +mock_provider "test" { + override_resource { + target = test_resource.absent_one + } +} + +override_resource { + target = test_resource.absent_two +} + +override_resource { + target = module.setup.test_resource.absent_three +} + +override_module { + target = module.absent_four +} + +override_resource { + // This one only exists in the main configuration, but not the setup + // configuration. We shouldn't see a warning for this. + target = module.setup.test_resource.child_resource +} + +override_resource { + // This is the reverse, only exists if you load the setup module directly. + // We shouldn't see a warning for this even though it's not in the main + // configuration. + target = test_resource.child_resource +} + +run "setup" { + module { + source = "./setup" + } + + override_resource { + target = test_resource.absent_five + } +} + +run "test" { + override_resource { + target = module.setup.test_resource.absent_six + } +} diff --git a/internal/command/testdata/test/invalid-overrides/setup/main.tf b/internal/command/testdata/test/invalid-overrides/setup/main.tf new file mode 100644 index 0000000000..42d4148b29 --- /dev/null +++ b/internal/command/testdata/test/invalid-overrides/setup/main.tf @@ -0,0 +1,2 @@ + +resource "test_resource" "child_resource" {} diff --git a/internal/command/validate.go b/internal/command/validate.go index 2556c08a02..87172e2b96 100644 --- a/internal/command/validate.go +++ b/internal/command/validate.go @@ -108,6 +108,11 @@ func (c *ValidateCommand) validate(dir, testDir string, noTests bool) tfdiags.Di // We'll also do a quick validation of the Terraform test files. These live // outside the Terraform graph so we have to do this separately. for _, file := range cfg.Module.Tests { + + // The file validation only returns warnings so we'll just add them + // without checking anything about them. + diags = diags.Append(file.Validate(cfg)) + for _, run := range file.Runs { if run.Module != nil { @@ -130,9 +135,12 @@ func (c *ValidateCommand) validate(dir, testDir string, noTests bool) tfdiags.Di } } + + diags = diags.Append(run.Validate(run.ConfigUnderTest)) + } else { + diags = diags.Append(run.Validate(cfg)) } - diags = diags.Append(run.Validate()) } } diff --git a/internal/command/validate_test.go b/internal/command/validate_test.go index c52476734e..93f73c053d 100644 --- a/internal/command/validate_test.go +++ b/internal/command/validate_test.go @@ -309,6 +309,78 @@ func TestValidateWithInvalidTestModule(t *testing.T) { } } +func TestValidateWithInvalidOverrides(t *testing.T) { + + // We're reusing some testing configs that were written for testing the + // test command here, so we have to initalise things slightly differently + // to the other tests. + + td := t.TempDir() + testCopyDir(t, testFixturePath(path.Join("test", "invalid-overrides")), td) + defer testChdir(t, td)() + + streams, done := terminal.StreamsForTesting(t) + view := views.NewView(streams) + ui := new(cli.MockUi) + + provider := testing_command.NewProvider(nil) + + providerSource, close := newMockProviderSource(t, map[string][]string{ + "test": {"1.0.0"}, + }) + defer close() + + meta := Meta{ + testingOverrides: metaOverridesForProvider(provider.Provider), + Ui: ui, + View: view, + Streams: streams, + ProviderSource: providerSource, + } + + init := &InitCommand{ + Meta: meta, + } + + if code := init.Run(nil); code != 0 { + t.Fatalf("expected status code 0 but got %d: %s", code, ui.ErrorWriter) + } + + c := &ValidateCommand{ + Meta: meta, + } + + var args []string + args = append(args, "-no-color") + + code := c.Run(args) + output := done(t) + + if code != 0 { + t.Errorf("Should have passed: %d\n\n%s", code, output.Stderr()) + } + + actual := output.All() + expected := ` +Warning: Invalid override target + + on main.tftest.hcl line 4, in mock_provider "test": + 4: target = test_resource.absent_one + +The override target test_resource.absent_one does not exist within the +configuration under test. This could indicate a typo in the target address or +an unnecessary override. + +(and 5 more similar warnings elsewhere) +Success! The configuration is valid, but there were some validation warnings +as shown above. + +` + if diff := cmp.Diff(expected, actual); len(diff) > 0 { + t.Errorf("expected:\n%s\nactual:\n%s\ndiff:\n%s", expected, actual, diff) + } +} + func TestValidate_json(t *testing.T) { tests := []struct { path string diff --git a/internal/configs/config.go b/internal/configs/config.go index 7cbb911e77..c1404eb6fa 100644 --- a/internal/configs/config.go +++ b/internal/configs/config.go @@ -191,6 +191,47 @@ func (c *Config) DescendentForInstance(path addrs.ModuleInstance) *Config { return current } +// TargetExists returns true if it's possible for the provided target to exist +// within the configuration. +// +// This doesn't consider instance expansion, so we're only making sure the +// target could exist if the instance expansion expands correctly. +func (c *Config) TargetExists(target addrs.Targetable) bool { + switch target.AddrType() { + case addrs.ConfigResourceAddrType: + addr := target.(addrs.ConfigResource) + module := c.Descendent(addr.Module) + if module != nil { + return module.Module.ResourceByAddr(addr.Resource) != nil + } else { + return false + } + case addrs.AbsResourceInstanceAddrType: + addr := target.(addrs.AbsResourceInstance) + module := c.DescendentForInstance(addr.Module) + if module != nil { + return module.Module.ResourceByAddr(addr.Resource.Resource) != nil + } else { + return false + } + case addrs.AbsResourceAddrType: + addr := target.(addrs.AbsResource) + module := c.DescendentForInstance(addr.Module) + if module != nil { + return module.Module.ResourceByAddr(addr.Resource) != nil + } else { + return false + } + case addrs.ModuleAddrType: + return c.Descendent(target.(addrs.Module)) != nil + case addrs.ModuleInstanceAddrType: + return c.DescendentForInstance(target.(addrs.ModuleInstance)) != nil + default: + panic(fmt.Errorf("unrecognized targetable type: %d", target.AddrType())) + } + return true +} + // EntersNewPackage returns true if this call is to an external module, either // directly via a remote source address or indirectly via a registry source // address. diff --git a/internal/configs/mock_provider.go b/internal/configs/mock_provider.go index 766852d8e1..04c933e874 100644 --- a/internal/configs/mock_provider.go +++ b/internal/configs/mock_provider.go @@ -32,7 +32,8 @@ func decodeMockProviderBlock(block *hcl.Block) (*Provider, hcl.Diagnostics) { NameRange: block.LabelRanges[0], DeclRange: block.DefRange, - Config: config, + // Mock providers shouldn't need any additional data. + Config: hcl.EmptyBody(), // Mark this provider as being mocked. Mock: true, @@ -83,6 +84,16 @@ type MockResource struct { DefaultsRange hcl.Range } +type OverrideSource int + +const ( + UnknownOverrideSource OverrideSource = iota + RunBlockOverrideSource + TestFileOverrideSource + MockProviderOverrideSource + MockDataFileOverrideSource +) + // Override targets a specific module, resource or data source with a set of // replacement values that should be used in place of whatever the underlying // provider would normally do. @@ -90,6 +101,9 @@ type Override struct { Target *addrs.Target Values cty.Value + // Source tells us where this Override was defined. + Source OverrideSource + Range hcl.Range TypeRange hcl.Range TargetRange hcl.Range @@ -141,7 +155,7 @@ func decodeMockDataBody(body hcl.Body) (*MockData, hcl.Diagnostics) { } } case "override_resource": - override, overrideDiags := decodeOverrideResourceBlock(block) + override, overrideDiags := decodeOverrideResourceBlock(block, MockProviderOverrideSource) diags = append(diags, overrideDiags...) if override != nil && override.Target != nil { @@ -158,7 +172,7 @@ func decodeMockDataBody(body hcl.Body) (*MockData, hcl.Diagnostics) { data.Overrides.Put(subject, override) } case "override_data": - override, overrideDiags := decodeOverrideDataBlock(block) + override, overrideDiags := decodeOverrideDataBlock(block, MockProviderOverrideSource) diags = append(diags, overrideDiags...) if override != nil && override.Target != nil { @@ -213,8 +227,8 @@ func decodeMockResourceBlock(block *hcl.Block) (*MockResource, hcl.Diagnostics) return resource, diags } -func decodeOverrideModuleBlock(block *hcl.Block) (*Override, hcl.Diagnostics) { - override, diags := decodeOverrideBlock(block, "outputs", "override_module") +func decodeOverrideModuleBlock(block *hcl.Block, source OverrideSource) (*Override, hcl.Diagnostics) { + override, diags := decodeOverrideBlock(block, "outputs", "override_module", source) if override.Target != nil { switch override.Target.Subject.AddrType() { @@ -234,8 +248,8 @@ func decodeOverrideModuleBlock(block *hcl.Block) (*Override, hcl.Diagnostics) { return override, diags } -func decodeOverrideResourceBlock(block *hcl.Block) (*Override, hcl.Diagnostics) { - override, diags := decodeOverrideBlock(block, "values", "override_resource") +func decodeOverrideResourceBlock(block *hcl.Block, source OverrideSource) (*Override, hcl.Diagnostics) { + override, diags := decodeOverrideBlock(block, "values", "override_resource", source) if override.Target != nil { var mode addrs.ResourceMode @@ -271,8 +285,8 @@ func decodeOverrideResourceBlock(block *hcl.Block) (*Override, hcl.Diagnostics) return override, diags } -func decodeOverrideDataBlock(block *hcl.Block) (*Override, hcl.Diagnostics) { - override, diags := decodeOverrideBlock(block, "values", "override_data") +func decodeOverrideDataBlock(block *hcl.Block, source OverrideSource) (*Override, hcl.Diagnostics) { + override, diags := decodeOverrideBlock(block, "values", "override_data", source) if override.Target != nil { var mode addrs.ResourceMode @@ -308,7 +322,7 @@ func decodeOverrideDataBlock(block *hcl.Block) (*Override, hcl.Diagnostics) { return override, diags } -func decodeOverrideBlock(block *hcl.Block, attributeName string, blockName string) (*Override, hcl.Diagnostics) { +func decodeOverrideBlock(block *hcl.Block, attributeName string, blockName string, source OverrideSource) (*Override, hcl.Diagnostics) { var diags hcl.Diagnostics content, contentDiags := block.Body.Content(&hcl.BodySchema{ @@ -320,6 +334,7 @@ func decodeOverrideBlock(block *hcl.Block, attributeName string, blockName strin diags = append(diags, contentDiags...) override := &Override{ + Source: source, Range: block.DefRange, TypeRange: block.TypeRange, } diff --git a/internal/configs/test_file.go b/internal/configs/test_file.go index 89f704d827..cbf7c7620e 100644 --- a/internal/configs/test_file.go +++ b/internal/configs/test_file.go @@ -137,13 +137,83 @@ type TestRun struct { DeclRange hcl.Range } -// Validate does a very simple and cursory check across the run block to look +// Validate does a very simple and cursory check across the test file to look // for simple issues we can highlight early on. -func (run *TestRun) Validate() tfdiags.Diagnostics { +// +// This function only returns warnings in the diagnostics. Callers of this +// function usually do not validate the returned diagnostics as a result. If +// you change this function, make sure to update the callers as well. +func (file *TestFile) Validate(config *Config) tfdiags.Diagnostics { var diags tfdiags.Diagnostics - // For now, we only want to make sure all the ExpectFailure references are - // the correct kind of reference. + for _, provider := range file.Providers { + if !provider.Mock { + continue + } + + for _, elem := range provider.MockData.Overrides.Elems { + if elem.Value.Source != MockProviderOverrideSource { + // Only check overrides that are defined directly within the + // mock provider block of this file. This is a safety check + // against any override blocks loaded from a dedicated data + // file, for these we won't raise warnings if they target + // resources that don't exist. + continue + } + + if !file.canTarget(config, elem.Key) { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Invalid override target", + Detail: fmt.Sprintf("The override target %s does not exist within the configuration under test. This could indicate a typo in the target address or an unnecessary override.", elem.Key), + Subject: elem.Value.TargetRange.Ptr(), + }) + } + } + } + + for _, elem := range file.Overrides.Elems { + if !file.canTarget(config, elem.Key) { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Invalid override target", + Detail: fmt.Sprintf("The override target %s does not exist within the configuration under test. This could indicate a typo in the target address or an unnecessary override.", elem.Key), + Subject: elem.Value.TargetRange.Ptr(), + }) + } + } + + return diags +} + +// canTarget is a helper function, that just checks all the available +// configurations to make sure at least one contains the specified target. +func (file *TestFile) canTarget(config *Config, target addrs.Targetable) bool { + // If the target is in the main configuration, then easy. + if config.TargetExists(target) { + return true + } + + // But, we could be targeting something in configuration loaded by one of + // the run blocks. + for _, run := range file.Runs { + if run.Module != nil { + if run.ConfigUnderTest.TargetExists(target) { + return true + } + } + } + + return false +} + +// Validate does a very simple and cursory check across the run block to look +// for simple issues we can highlight early on. +func (run *TestRun) Validate(config *Config) tfdiags.Diagnostics { + var diags tfdiags.Diagnostics + + // First, we want to make sure all the ExpectFailure references are the + // correct kind of reference. for _, traversal := range run.ExpectFailures { reference, refDiags := addrs.ParseRefFromTestingScope(traversal) @@ -167,6 +237,19 @@ func (run *TestRun) Validate() tfdiags.Diagnostics { } + // All the overrides defined within a run block should target an existing + // configuration block. + for _, elem := range run.Overrides.Elems { + if !config.TargetExists(elem.Key) { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Invalid override target", + Detail: fmt.Sprintf("The override target %s does not exist within the configuration under test. This could indicate a typo in the target address or an unnecessary override.", elem.Key), + Subject: elem.Value.TargetRange.Ptr(), + }) + } + } + return diags } @@ -285,7 +368,7 @@ func loadTestFile(body hcl.Body) (*TestFile, hcl.Diagnostics) { tf.Providers[key] = provider } case "override_resource": - override, overrideDiags := decodeOverrideResourceBlock(block) + override, overrideDiags := decodeOverrideResourceBlock(block, TestFileOverrideSource) diags = append(diags, overrideDiags...) if override != nil && override.Target != nil { @@ -302,7 +385,7 @@ func loadTestFile(body hcl.Body) (*TestFile, hcl.Diagnostics) { tf.Overrides.Put(subject, override) } case "override_data": - override, overrideDiags := decodeOverrideDataBlock(block) + override, overrideDiags := decodeOverrideDataBlock(block, TestFileOverrideSource) diags = append(diags, overrideDiags...) if override != nil && override.Target != nil { @@ -319,7 +402,7 @@ func loadTestFile(body hcl.Body) (*TestFile, hcl.Diagnostics) { tf.Overrides.Put(subject, override) } case "override_module": - override, overrideDiags := decodeOverrideModuleBlock(block) + override, overrideDiags := decodeOverrideModuleBlock(block, TestFileOverrideSource) diags = append(diags, overrideDiags...) if override != nil && override.Target != nil { @@ -413,7 +496,7 @@ func decodeTestRunBlock(block *hcl.Block) (*TestRun, hcl.Diagnostics) { r.Module = module } case "override_resource": - override, overrideDiags := decodeOverrideResourceBlock(block) + override, overrideDiags := decodeOverrideResourceBlock(block, RunBlockOverrideSource) diags = append(diags, overrideDiags...) if override != nil && override.Target != nil { @@ -430,7 +513,7 @@ func decodeTestRunBlock(block *hcl.Block) (*TestRun, hcl.Diagnostics) { r.Overrides.Put(subject, override) } case "override_data": - override, overrideDiags := decodeOverrideDataBlock(block) + override, overrideDiags := decodeOverrideDataBlock(block, RunBlockOverrideSource) diags = append(diags, overrideDiags...) if override != nil && override.Target != nil { @@ -447,7 +530,7 @@ func decodeTestRunBlock(block *hcl.Block) (*TestRun, hcl.Diagnostics) { r.Overrides.Put(subject, override) } case "override_module": - override, overrideDiags := decodeOverrideModuleBlock(block) + override, overrideDiags := decodeOverrideModuleBlock(block, RunBlockOverrideSource) diags = append(diags, overrideDiags...) if override != nil && override.Target != nil { diff --git a/internal/configs/test_file_test.go b/internal/configs/test_file_test.go index fd40b2b401..749af9bd72 100644 --- a/internal/configs/test_file_test.go +++ b/internal/configs/test_file_test.go @@ -65,7 +65,7 @@ func TestTestRun_Validate(t *testing.T) { run.ExpectFailures = append(run.ExpectFailures, parseTraversal(t, addr)) } - diags := run.Validate() + diags := run.Validate(nil) if len(diags) > 1 { t.Fatalf("too many diags: %d", len(diags))