terraform test: Keep modules in state if they are required by the ext…

…ernal references (#34482)

* Add test verifying issue #34476 has been fixed

* Keep modules in state if they are required by the external references

* tweak logic so it doesn't delete twice

internal/command/test_test.go

@@ -223,6 +223,10 @@ func TestTest_Runs(t *testing.T) {
 			expectedOut: "3 passed, 0 failed.",
 			code:        0,
 		},
+		"empty_module_with_output": {
+			expectedOut: "1 passed, 0 failed.",
+			code:        0,
+		},
 	}
 	for name, tc := range tcs {
 		t.Run(name, func(t *testing.T) {

internal/command/testdata/test/empty_module_with_output/empty/main.tf

@@ -0,0 +1,3 @@
+output "value" {
+  value = "Hello, World!"
+}

internal/command/testdata/test/empty_module_with_output/main.tf

@@ -0,0 +1,3 @@
+module "empty" {
+  source = "./empty"
+}

internal/command/testdata/test/empty_module_with_output/main.tftest.hcl

@@ -0,0 +1,6 @@
+run "empty" {
+  assert {
+    condition = module.empty.value == "Hello, World!"
+    error_message = "wrong output value"
+  }
+}

internal/terraform/graph_builder_apply.go

@@ -202,7 +202,9 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer {
 		&CloseProviderTransformer{},
 
 		// close the root module
-		&CloseRootModuleTransformer{},
+		&CloseRootModuleTransformer{
+			ExternalReferences: b.ExternalReferences,
+		},
 
 		// Perform the transitive reduction to make our graph a bit
 		// more understandable if possible (it usually is possible).

internal/terraform/graph_builder_plan.go

@@ -256,7 +256,9 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer {
 		&CloseProviderTransformer{},
 
 		// Close the root module
-		&CloseRootModuleTransformer{},
+		&CloseRootModuleTransformer{
+			ExternalReferences: b.ExternalReferences,
+		},
 
 		// Perform the transitive reduction to make our graph a bit
 		// more understandable if possible (it usually is possible).

internal/terraform/node_module_expand.go

@@ -148,6 +148,11 @@ func (n *nodeExpandModule) Execute(ctx EvalContext, op walkOperation) (diags tfd
 // do not have a lifecycle controlled by individual graph nodes.
 type nodeCloseModule struct {
 	Addr addrs.Module
+
+	// The ExternalReferences mean the closer can keep modules in state that
+	// have no resources if they contain outputs referenced externally. They are
+	// only included by the root closer.
+	ExternalReferences []*addrs.Reference
 }
 
 var (
@@ -217,7 +222,29 @@ func (n *nodeCloseModule) Execute(ctx EvalContext, op walkOperation) (diags tfdi
 
 			// empty child modules are always removed
 			if len(mod.Resources) == 0 && !mod.Addr.IsRoot() && !overridden {
-				delete(state.Modules, modKey)
+				removeModuleFromState := false
+
+				if len(mod.Addr) > 1 {
+					// Then it's a deeply nested module that external callers
+					// shouldn't have access to anyway.
+					removeModuleFromState = true
+				} else {
+					// Otherwise, it might be that we want to keep this module
+					// in state if it's being referenced by the testing
+					// framework.
+					for _, reference := range n.ExternalReferences {
+						if call, ok := reference.Subject.(addrs.ModuleCallInstanceOutput); ok {
+							if call.Call.Call.Name == mod.Addr[0].Name && call.Call.Key == mod.Addr[0].InstanceKey {
+								removeModuleFromState = true
+								break
+							}
+						}
+					}
+				}
+
+				if !removeModuleFromState {
+					delete(state.Modules, modKey)
+				}
 			}
 		}
 		return nil

internal/terraform/node_module_expand_test.go

@@ -7,11 +7,12 @@ import (
 	"testing"
 
 	"github.com/hashicorp/hcl/v2/hcltest"
+	"github.com/zclconf/go-cty/cty"
+
 	"github.com/hashicorp/terraform/internal/addrs"
 	"github.com/hashicorp/terraform/internal/configs"
 	"github.com/hashicorp/terraform/internal/instances"
 	"github.com/hashicorp/terraform/internal/states"
-	"github.com/zclconf/go-cty/cty"
 )
 
 func TestNodeExpandModuleExecute(t *testing.T) {
@@ -44,7 +45,7 @@ func TestNodeCloseModuleExecute(t *testing.T) {
 		ctx := &MockEvalContext{
 			StateState: state.SyncWrapper(),
 		}
-		node := nodeCloseModule{addrs.Module{"child"}}
+		node := nodeCloseModule{Addr: addrs.Module{"child"}}
 		diags := node.Execute(ctx, walkApply)
 		if diags.HasErrors() {
 			t.Fatalf("unexpected error: %s", diags.Err())
@@ -56,7 +57,38 @@ func TestNodeCloseModuleExecute(t *testing.T) {
 		}
 
 		// the root module should do all the module cleanup
-		node = nodeCloseModule{addrs.RootModule}
+		node = nodeCloseModule{Addr: addrs.RootModule}
+		diags = node.Execute(ctx, walkApply)
+		if diags.HasErrors() {
+			t.Fatalf("unexpected error: %s", diags.Err())
+		}
+
+		// Since module.child has no resources, it should be removed
+		if _, ok := state.Modules["module.child"]; ok {
+			t.Fatal("module.child was not removed from state")
+		}
+	})
+	t.Run("walkApplyWithOutputs", func(t *testing.T) {
+		state := states.NewState()
+		state.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey))
+		state.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey)).SetOutputValue("foo", cty.StringVal("bar"), false)
+		ctx := &MockEvalContext{
+			StateState: state.SyncWrapper(),
+		}
+		node := nodeCloseModule{Addr: addrs.Module{"child"}}
+		diags := node.Execute(ctx, walkApply)
+		if diags.HasErrors() {
+			t.Fatalf("unexpected error: %s", diags.Err())
+		}
+
+		// Since module.child has no resources, it should be removed even though
+		// it has outputs.
+		if _, ok := state.Modules["module.child"]; !ok {
+			t.Fatal("module.child should not be removed from state yet")
+		}
+
+		// the root module should do all the module cleanup
+		node = nodeCloseModule{Addr: addrs.RootModule}
 		diags = node.Execute(ctx, walkApply)
 		if diags.HasErrors() {
 			t.Fatalf("unexpected error: %s", diags.Err())
@@ -67,6 +99,49 @@ func TestNodeCloseModuleExecute(t *testing.T) {
 			t.Fatal("module.child was not removed from state")
 		}
 	})
+	t.Run("walkApplyWithReferencedOutputs", func(t *testing.T) {
+		state := states.NewState()
+		state.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey))
+		state.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey)).SetOutputValue("foo", cty.StringVal("bar"), false)
+		ctx := &MockEvalContext{
+			StateState: state.SyncWrapper(),
+		}
+		node := nodeCloseModule{Addr: addrs.Module{"child"}}
+		diags := node.Execute(ctx, walkApply)
+		if diags.HasErrors() {
+			t.Fatalf("unexpected error: %s", diags.Err())
+		}
+
+		// module.child should not be removed, since we have referenced it from
+		// the external references.
+		if _, ok := state.Modules["module.child"]; !ok {
+			t.Fatal("module.child should not be removed from state yet")
+		}
+
+		// the root module should do all the module cleanup
+		node = nodeCloseModule{Addr: addrs.RootModule, ExternalReferences: []*addrs.Reference{
+			{
+				Subject: addrs.ModuleCallInstanceOutput{
+					Call: addrs.ModuleCallInstance{
+						Call: addrs.ModuleCall{
+							Name: "child",
+						},
+						Key: addrs.NoKey,
+					},
+					Name: "foo",
+				},
+			},
+		}}
+		diags = node.Execute(ctx, walkApply)
+		if diags.HasErrors() {
+			t.Fatalf("unexpected error: %s", diags.Err())
+		}
+
+		// Since module.child has no resources, it should be removed
+		if _, ok := state.Modules["module.child"]; !ok {
+			t.Fatal("module.child should not have been removed from state at all")
+		}
+	})
 
 	// walkImport is a no-op
 	t.Run("walkImport", func(t *testing.T) {
@@ -75,7 +150,7 @@ func TestNodeCloseModuleExecute(t *testing.T) {
 		ctx := &MockEvalContext{
 			StateState: state.SyncWrapper(),
 		}
-		node := nodeCloseModule{addrs.Module{"child"}}
+		node := nodeCloseModule{Addr: addrs.Module{"child"}}
 
 		diags := node.Execute(ctx, walkImport)
 		if diags.HasErrors() {

internal/terraform/transform_root.go

@@ -4,6 +4,7 @@
 package terraform
 
 import (
+	"github.com/hashicorp/terraform/internal/addrs"
 	"github.com/hashicorp/terraform/internal/dag"
 )
 
@@ -60,11 +61,19 @@ func (n graphNodeRoot) Name() string {
 }
 
 // CloseRootModuleTransformer is a GraphTransformer that adds a root to the graph.
-type CloseRootModuleTransformer struct{}
+type CloseRootModuleTransformer struct {
+	// ExternalReferences contains the list of references made by an external
+	// observer (such as the testing framework). We use them in here to ensure
+	// that any modules that contain outputs referenced externally are not
+	// removed because they are considered empty.
+	ExternalReferences []*addrs.Reference
+}
 
 func (t *CloseRootModuleTransformer) Transform(g *Graph) error {
 	// close the root module
-	closeRoot := &nodeCloseModule{}
+	closeRoot := &nodeCloseModule{
+		ExternalReferences: t.ExternalReferences,
+	}
 	g.Add(closeRoot)
 
 	// since this is closing the root module, make it depend on everything in