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

@@ -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

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)

internal/command/testdata/test/invalid-overrides/main.tf

@@ -0,0 +1,6 @@
+
+resource "test_resource" "resource" {}
+
+module "setup" {
+  source = "./setup"
+}

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

internal/command/testdata/test/invalid-overrides/setup/main.tf

@@ -0,0 +1,2 @@
+
+resource "test_resource" "child_resource" {}

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())
 		}
 	}
 

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

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.

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

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 {

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))