From f6f234fed607a60f5bdc2cfd13e8362dabf4094d Mon Sep 17 00:00:00 2001 From: Samsondeen Dare Date: Fri, 6 Mar 2026 16:57:43 +0100 Subject: [PATCH 1/4] Cleanup in appropriate order when running parallel execution --- internal/command/test_test.go | 126 ++++++++++++++++++ .../testdata/test/parallel_deps/main.tf | 20 +++ .../test/parallel_deps/main.tftest.hcl | 30 +++++ .../moduletest/graph/node_state_cleanup.go | 1 + .../graph/transform_state_cleanup.go | 22 ++- 5 files changed, 192 insertions(+), 7 deletions(-) create mode 100644 internal/command/testdata/test/parallel_deps/main.tf create mode 100644 internal/command/testdata/test/parallel_deps/main.tftest.hcl diff --git a/internal/command/test_test.go b/internal/command/test_test.go index 01d3f548de..b8db09802f 100644 --- a/internal/command/test_test.go +++ b/internal/command/test_test.go @@ -28,6 +28,7 @@ import ( "github.com/hashicorp/terraform/internal/addrs" testing_command "github.com/hashicorp/terraform/internal/command/testing" "github.com/hashicorp/terraform/internal/command/views" + viewsJson "github.com/hashicorp/terraform/internal/command/views/json" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/configs/configload" "github.com/hashicorp/terraform/internal/getproviders" @@ -40,6 +41,11 @@ import ( "github.com/hashicorp/terraform/internal/tfdiags" ) +type jsonLine struct { + Type string `json:"type"` + TestRun *viewsJson.TestRunStatus `json:"test_run,omitempty"` +} + func TestTest_Runs(t *testing.T) { tcs := map[string]struct { override string @@ -5674,6 +5680,126 @@ func TestTest_TeardownOrder(t *testing.T) { } } +func TestTest_ParallelDeps(t *testing.T) { + // This tests that parallel dependencies are handled correctly during teardown. + td := t.TempDir() + testCopyDir(t, testFixturePath(path.Join("test", "parallel_deps")), td) + t.Chdir(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, + AllowExperimentalFeatures: true, + } + + init := &InitCommand{ + Meta: meta, + } + + output := done(t) + + if code := init.Run(nil); code != 0 { + t.Fatalf("expected status code 0 but got %d: %s", code, output.All()) + } + + // Reset the streams for the next command. + streams, done = terminal.StreamsForTesting(t) + meta.Streams = streams + meta.View = views.NewView(streams) + + c := &TestCommand{ + Meta: meta, + } + + code := c.Run([]string{"-no-color", "-json"}) + output = done(t) + + if code != 0 { + t.Errorf("expected status code 0 but got %d", code) + } + + actual := output.All() + + var teardownOrder []string + lines, err := parseJSONLines(t, actual) + if err != nil { + t.Fatal(err) + } + for _, parsed := range lines { + if parsed.Type != "test_run" || parsed.TestRun == nil { + continue + } + if parsed.TestRun.Progress != "teardown" { + continue + } + + // We only care about teardowns with elapsed time of 0, indicating the start + // of the teardown phase. + if parsed.TestRun.Elapsed == nil || *parsed.TestRun.Elapsed != 0 { + continue + } + teardownOrder = append(teardownOrder, parsed.TestRun.Run) + } + + // test_two depends on test_three (via run.test_three.id), so during + // teardown the dependency order should be reversed, i.e test_two must + // be torn down before test_three. + testThreeIdx := -1 + testTwoIdx := -1 + for i, name := range teardownOrder { + switch name { + case "test_three": + testThreeIdx = i + case "test_two": + testTwoIdx = i + } + } + + if testThreeIdx == -1 { + t.Fatalf("expected test_three teardown (elapsed=0) in output but did not find it.\nteardown order: %v\nfull output:\n%s", teardownOrder, actual) + } + if testTwoIdx == -1 { + t.Fatalf("expected test_two teardown (elapsed=0) in output but did not find it.\nteardown order: %v\nfull output:\n%s", teardownOrder, actual) + } + if testThreeIdx <= testTwoIdx { + t.Errorf("expected test_two teardown to come before test_three teardown, but got test_three at index %d and test_two at index %d.\nteardown order: %v\nfull output:\n%s", + testThreeIdx, testTwoIdx, teardownOrder, actual) + } + + if provider.ResourceCount() != 0 { + t.Errorf("should have deleted all resources") + } +} + +func parseJSONLines(t *testing.T, actual string) ([]jsonLine, error) { + t.Helper() + var lines []jsonLine + for line := range strings.SplitSeq(strings.TrimSpace(actual), "\n") { + if line == "" { + continue + } + var parsed jsonLine + if err := json.Unmarshal([]byte(line), &parsed); err != nil { + return nil, err + } + lines = append(lines, parsed) + } + return lines, nil +} + // testModuleInline takes a map of path -> config strings and yields a config // structure with those files loaded from disk func testModuleInline(t *testing.T, sources map[string]string) (*configs.Config, string, func()) { diff --git a/internal/command/testdata/test/parallel_deps/main.tf b/internal/command/testdata/test/parallel_deps/main.tf new file mode 100644 index 0000000000..df92f1eb42 --- /dev/null +++ b/internal/command/testdata/test/parallel_deps/main.tf @@ -0,0 +1,20 @@ +variable "id" { + type = string +} + +variable "unused" { + type = string + default = "unused" +} + +resource "test_resource" "resource" { + value = var.id +} + +output "id" { + value = test_resource.resource.id +} + +output "unused" { + value = var.unused +} \ No newline at end of file diff --git a/internal/command/testdata/test/parallel_deps/main.tftest.hcl b/internal/command/testdata/test/parallel_deps/main.tftest.hcl new file mode 100644 index 0000000000..b9f66d1a51 --- /dev/null +++ b/internal/command/testdata/test/parallel_deps/main.tftest.hcl @@ -0,0 +1,30 @@ +test { + parallel = true +} + +run "test" { + variables { + id = "test" + unused = "unused" + } +} + +run "test_two" { + state_key = "state2" + variables { + // This dependency is a later run, but that should be fine because we are in parallel mode. + id = run.test_three.id + + // The output state data for this dependency will also be left behind, but the actual + // resource will have been destroyed by the cleanup step of test_three. + unused = run.test.unused + } +} + +run "test_three" { + state_key = "state3" + variables { + id = "test_three" + unused = run.test.unused + } +} diff --git a/internal/moduletest/graph/node_state_cleanup.go b/internal/moduletest/graph/node_state_cleanup.go index 92808d9912..ffed3c974f 100644 --- a/internal/moduletest/graph/node_state_cleanup.go +++ b/internal/moduletest/graph/node_state_cleanup.go @@ -36,6 +36,7 @@ func (n *NodeStateCleanup) Name() string { // Execute destroys the resources created in the state file. func (n *NodeStateCleanup) Execute(evalCtx *EvalContext) { + fmt.Printf("Cleaning up state for %s\n", n.stateKey) file := n.opts.File state := evalCtx.GetState(n.stateKey) log.Printf("[TRACE] TestStateManager: cleaning up state for %s", file.Name) diff --git a/internal/moduletest/graph/transform_state_cleanup.go b/internal/moduletest/graph/transform_state_cleanup.go index f41c26fba7..de263bb87d 100644 --- a/internal/moduletest/graph/transform_state_cleanup.go +++ b/internal/moduletest/graph/transform_state_cleanup.go @@ -111,31 +111,39 @@ func (t *TestStateCleanupTransformer) Transform(g *terraform.Graph) error { } // Depth-first traversal to connect the cleanup nodes based on their dependencies. + // if run "b" depends on run "a", the dependency should be reversed (cleanup of "a" should depend on cleanup of "b"). // If an edge would create a cycle, we skip it. + // + // The visited map tracks nodes that have been visited globally across all cleanup nodes. + // The localVisited map tracks nodes that have been visited locally starting from the current cleanup node. + // The allows us to deal with parallel run nodes, where the sequence of runs is not fixed, so an + // visited := make(map[string]bool) for _, node := range arr { - t.depthFirstTraverse(g, node, visited, cleanupMap, depStateKeys) + localVisited := make(map[string]bool) + t.depthFirstTraverse(g, node, localVisited, visited, cleanupMap, depStateKeys) } return nil } -func (t *TestStateCleanupTransformer) depthFirstTraverse(g *terraform.Graph, node *NodeStateCleanup, visited map[string]bool, cleanupNodes map[string]*NodeStateCleanup, depStateKeys map[string][]string) { - if visited[node.stateKey] { +func (t *TestStateCleanupTransformer) depthFirstTraverse(g *terraform.Graph, node *NodeStateCleanup, localVisited, globalVisited map[string]bool, cleanupNodes map[string]*NodeStateCleanup, depStateKeys map[string][]string) { + if globalVisited[node.stateKey] { return } // don't mark the node as visited if it's a leaf node, this ensures that other dependencies are still added to it if len(depStateKeys[node.stateKey]) == 0 { return } - visited[node.stateKey] = true + globalVisited[node.stateKey] = true + localVisited[node.stateKey] = true for _, refStateKey := range depStateKeys[node.stateKey] { - // If the reference node has already been visited, skip it. - if visited[refStateKey] { + // If the reference node has already been visited along this path, skip it. + if localVisited[refStateKey] { continue } refNode := cleanupNodes[refStateKey] g.Connect(dag.BasicEdge(refNode, node)) - t.depthFirstTraverse(g, refNode, visited, cleanupNodes, depStateKeys) + t.depthFirstTraverse(g, refNode, localVisited, globalVisited, cleanupNodes, depStateKeys) } } From 9d4c82452c2397a5da925e6584e049ebf28ff5d2 Mon Sep 17 00:00:00 2001 From: Samsondeen Dare Date: Sat, 7 Mar 2026 01:20:36 +0100 Subject: [PATCH 2/4] replace recursive traversal with iteration over run dependencies --- internal/command/test_test.go | 6 +- .../testdata/test/cleanup/main.tftest.hcl | 2 +- .../moduletest/graph/node_state_cleanup.go | 1 - .../moduletest/graph/test_graph_builder.go | 2 +- .../graph/transform_state_cleanup.go | 112 ++++++++---------- 5 files changed, 54 insertions(+), 69 deletions(-) diff --git a/internal/command/test_test.go b/internal/command/test_test.go index b8db09802f..ad48e6b9d4 100644 --- a/internal/command/test_test.go +++ b/internal/command/test_test.go @@ -1521,7 +1521,7 @@ func TestTest_ParallelTeardown(t *testing.T) { value = test_resource.foo.value } `, - // c2 => a1, b1 => a1, a2 => b1, b2 => c1 + // c2 => a2, b1 => a1, a2 => b1, b2 => c2 "parallel.tftest.hcl": ` test { parallel = true @@ -1564,7 +1564,7 @@ func TestTest_ParallelTeardown(t *testing.T) { run "a2" { state_key = "a" variables { - foo = run.b1.value + foo = run.b2.value } providers = { @@ -1594,7 +1594,7 @@ func TestTest_ParallelTeardown(t *testing.T) { run "c2" { state_key = "c" variables { - foo = run.a1.value + foo = run.a2.value } } `, diff --git a/internal/command/testdata/test/cleanup/main.tftest.hcl b/internal/command/testdata/test/cleanup/main.tftest.hcl index d1816814e0..b4dae88c1a 100644 --- a/internal/command/testdata/test/cleanup/main.tftest.hcl +++ b/internal/command/testdata/test/cleanup/main.tftest.hcl @@ -23,4 +23,4 @@ run "test_four" { variables { id = "test_four" } -} \ No newline at end of file +} diff --git a/internal/moduletest/graph/node_state_cleanup.go b/internal/moduletest/graph/node_state_cleanup.go index ffed3c974f..92808d9912 100644 --- a/internal/moduletest/graph/node_state_cleanup.go +++ b/internal/moduletest/graph/node_state_cleanup.go @@ -36,7 +36,6 @@ func (n *NodeStateCleanup) Name() string { // Execute destroys the resources created in the state file. func (n *NodeStateCleanup) Execute(evalCtx *EvalContext) { - fmt.Printf("Cleaning up state for %s\n", n.stateKey) file := n.opts.File state := evalCtx.GetState(n.stateKey) log.Printf("[TRACE] TestStateManager: cleaning up state for %s", file.Name) diff --git a/internal/moduletest/graph/test_graph_builder.go b/internal/moduletest/graph/test_graph_builder.go index d8a63bbba4..80858c9c8e 100644 --- a/internal/moduletest/graph/test_graph_builder.go +++ b/internal/moduletest/graph/test_graph_builder.go @@ -54,7 +54,7 @@ func (b *TestGraphBuilder) Steps() []terraform.GraphTransformer { &TestVariablesTransformer{File: b.File}, terraform.DynamicTransformer(validateRunConfigs), terraform.DynamicTransformer(func(g *terraform.Graph) error { - cleanup := &TeardownSubgraph{opts: opts, parent: g, mode: b.CommandMode} + cleanup := &TeardownSubgraph{opts: opts, runGraph: g, mode: b.CommandMode} g.Add(cleanup) // ensure that the teardown node runs after all the run nodes diff --git a/internal/moduletest/graph/transform_state_cleanup.go b/internal/moduletest/graph/transform_state_cleanup.go index de263bb87d..1a6a0734e7 100644 --- a/internal/moduletest/graph/transform_state_cleanup.go +++ b/internal/moduletest/graph/transform_state_cleanup.go @@ -24,31 +24,35 @@ type Subgrapher interface { // TeardownSubgraph is a subgraph for cleaning up the state of // resources defined in the state files created by the test runs. type TeardownSubgraph struct { - opts *graphOptions - parent *terraform.Graph - mode moduletest.CommandMode + opts *graphOptions + runGraph *terraform.Graph + mode moduletest.CommandMode } func (b *TeardownSubgraph) Execute(ctx *EvalContext) { ctx.Renderer().File(b.opts.File, moduletest.TearDown) - runRefMap := make(map[addrs.Run][]string) + runRefs := make(map[addrs.Run][]*moduletest.Run) + // Build a map of run nodes to other run nodes they depend on. + // In cleanup mode, the run node is the NodeTestRunCleanup struct. if b.mode == moduletest.CleanupMode { - for runNode := range dag.SelectSeq[*NodeTestRunCleanup](b.parent.VerticesSeq()) { - refs := b.parent.Ancestors(runNode) - for _, ref := range refs { - if ref, ok := ref.(*NodeTestRunCleanup); ok && ref.run.Config.StateKey != runNode.run.Config.StateKey { - runRefMap[runNode.run.Addr()] = append(runRefMap[runNode.run.Addr()], ref.run.Config.StateKey) + for runNode := range dag.SelectSeq[*NodeTestRunCleanup](b.runGraph.VerticesSeq()) { + addr := runNode.run.Addr() + parents := b.runGraph.Ancestors(runNode) + for _, ref := range parents { + if ref, ok := ref.(*NodeTestRunCleanup); ok { + runRefs[addr] = append(runRefs[addr], ref.run) } } } } else { - for runNode := range dag.SelectSeq[*NodeTestRun](b.parent.VerticesSeq()) { - refs := b.parent.Ancestors(runNode) - for _, ref := range refs { - if ref, ok := ref.(*NodeTestRun); ok && ref.run.Config.StateKey != runNode.run.Config.StateKey { - runRefMap[runNode.run.Addr()] = append(runRefMap[runNode.run.Addr()], ref.run.Config.StateKey) + for runNode := range dag.SelectSeq[*NodeTestRun](b.runGraph.VerticesSeq()) { + addr := runNode.run.Addr() + parents := b.runGraph.Ancestors(runNode) + for _, ref := range parents { + if ref, ok := ref.(*NodeTestRun); ok { + runRefs[addr] = append(runRefs[addr], ref.run) } } } @@ -57,7 +61,7 @@ func (b *TeardownSubgraph) Execute(ctx *EvalContext) { // Create a new graph for the cleanup nodes g, diags := (&terraform.BasicGraphBuilder{ Steps: []terraform.GraphTransformer{ - &TestStateCleanupTransformer{opts: b.opts, runStateRefs: runRefMap}, + &TestStateCleanupTransformer{opts: b.opts, runDependencyMap: runRefs}, &CloseTestGraphTransformer{}, &terraform.TransitiveReductionTransformer{}, }, @@ -78,20 +82,21 @@ func (b *TeardownSubgraph) isSubGrapher() {} // TestStateCleanupTransformer is a GraphTransformer that adds a cleanup node // for each state that is created by the test runs. type TestStateCleanupTransformer struct { - opts *graphOptions - runStateRefs map[addrs.Run][]string + opts *graphOptions + runDependencyMap map[addrs.Run][]*moduletest.Run } func (t *TestStateCleanupTransformer) Transform(g *terraform.Graph) error { - cleanupMap := make(map[string]*NodeStateCleanup) - arr := make([]*NodeStateCleanup, 0, len(t.opts.File.Runs)) + type cleanupObj struct { + node *NodeStateCleanup + dependencies []*moduletest.Run + } - // dependency map for state keys, which will be used to traverse - // the cleanup nodes in a depth-first manner. - depStateKeys := make(map[string][]string) + cleanupMap := make(map[string]cleanupObj) + runNodesUsedForCleanup := make(map[addrs.Run]bool) - // iterate in reverse order of the run index, so that the last run for each state key - // is attached to the cleanup node. + // iterate in reverse order of the run index, so that the dependency map of the last + // run for each state key is used for the cleanup node. for _, run := range slices.Backward(t.opts.File.Runs) { key := run.Config.StateKey @@ -100,50 +105,31 @@ func (t *TestStateCleanupTransformer) Transform(g *terraform.Graph) error { stateKey: key, opts: t.opts, } - cleanupMap[key] = node - arr = append(arr, node) + cleanupMap[key] = cleanupObj{ + node: node, + dependencies: t.runDependencyMap[run.Addr()], + } g.Add(node) - - // The dependency map for the state's last run will be used for the cleanup node. - depStateKeys[key] = t.runStateRefs[run.Addr()] + runNodesUsedForCleanup[run.Addr()] = true continue } } - // Depth-first traversal to connect the cleanup nodes based on their dependencies. - // if run "b" depends on run "a", the dependency should be reversed (cleanup of "a" should depend on cleanup of "b"). - // If an edge would create a cycle, we skip it. - // - // The visited map tracks nodes that have been visited globally across all cleanup nodes. - // The localVisited map tracks nodes that have been visited locally starting from the current cleanup node. - // The allows us to deal with parallel run nodes, where the sequence of runs is not fixed, so an - // - visited := make(map[string]bool) - for _, node := range arr { - localVisited := make(map[string]bool) - t.depthFirstTraverse(g, node, localVisited, visited, cleanupMap, depStateKeys) + // We connect the cleanup nodes to their dependencies in reverse order, + // i.e a cleanup node for a run will evaluate before its references. + // We only connect references that are also cleanup nodes. If a referenced run + // is not used by a cleanup node, it will not be connected. + for _, obj := range cleanupMap { + for _, dep := range obj.dependencies { + if _, exists := runNodesUsedForCleanup[dep.Addr()]; exists { + depCleanupNode := cleanupMap[dep.Config.StateKey].node + objCleanupNode := obj.node + if depCleanupNode == objCleanupNode { + continue + } + g.Connect(dag.BasicEdge(depCleanupNode, objCleanupNode)) + } + } } return nil } - -func (t *TestStateCleanupTransformer) depthFirstTraverse(g *terraform.Graph, node *NodeStateCleanup, localVisited, globalVisited map[string]bool, cleanupNodes map[string]*NodeStateCleanup, depStateKeys map[string][]string) { - if globalVisited[node.stateKey] { - return - } - // don't mark the node as visited if it's a leaf node, this ensures that other dependencies are still added to it - if len(depStateKeys[node.stateKey]) == 0 { - return - } - globalVisited[node.stateKey] = true - localVisited[node.stateKey] = true - - for _, refStateKey := range depStateKeys[node.stateKey] { - // If the reference node has already been visited along this path, skip it. - if localVisited[refStateKey] { - continue - } - refNode := cleanupNodes[refStateKey] - g.Connect(dag.BasicEdge(refNode, node)) - t.depthFirstTraverse(g, refNode, localVisited, globalVisited, cleanupNodes, depStateKeys) - } -} From 8383ded554737fd16bdcae313e3718e77dd4f6cb Mon Sep 17 00:00:00 2001 From: Samsondeen Dare Date: Mon, 9 Mar 2026 08:43:54 +0100 Subject: [PATCH 3/4] add changelog --- .changes/v1.15/BUG FIXES-20260309-084347.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changes/v1.15/BUG FIXES-20260309-084347.yaml diff --git a/.changes/v1.15/BUG FIXES-20260309-084347.yaml b/.changes/v1.15/BUG FIXES-20260309-084347.yaml new file mode 100644 index 0000000000..437fc538a1 --- /dev/null +++ b/.changes/v1.15/BUG FIXES-20260309-084347.yaml @@ -0,0 +1,5 @@ +kind: BUG FIXES +body: 'test: fix dependency ordering in parallel cleanup' +time: 2026-03-09T08:43:47.688216+01:00 +custom: + Issue: "38247" From f0416b95cca875c7ec8129de1c3ff9aa575a711f Mon Sep 17 00:00:00 2001 From: Samsondeen Dare Date: Thu, 21 May 2026 09:02:53 +0200 Subject: [PATCH 4/4] update after rebase --- .changes/v1.15/BUG FIXES-20260309-084347.yaml | 5 ----- .changes/v1.16/BUG FIXES-20260521-090519.yaml | 5 +++++ internal/command/test_test.go | 3 +-- 3 files changed, 6 insertions(+), 7 deletions(-) delete mode 100644 .changes/v1.15/BUG FIXES-20260309-084347.yaml create mode 100644 .changes/v1.16/BUG FIXES-20260521-090519.yaml diff --git a/.changes/v1.15/BUG FIXES-20260309-084347.yaml b/.changes/v1.15/BUG FIXES-20260309-084347.yaml deleted file mode 100644 index 437fc538a1..0000000000 --- a/.changes/v1.15/BUG FIXES-20260309-084347.yaml +++ /dev/null @@ -1,5 +0,0 @@ -kind: BUG FIXES -body: 'test: fix dependency ordering in parallel cleanup' -time: 2026-03-09T08:43:47.688216+01:00 -custom: - Issue: "38247" diff --git a/.changes/v1.16/BUG FIXES-20260521-090519.yaml b/.changes/v1.16/BUG FIXES-20260521-090519.yaml new file mode 100644 index 0000000000..df7d5cf167 --- /dev/null +++ b/.changes/v1.16/BUG FIXES-20260521-090519.yaml @@ -0,0 +1,5 @@ +kind: BUG FIXES +body: "test: fix dependency ordering in parallel cleanup" +time: 2026-05-21T09:05:19.581965+02:00 +custom: + Issue: "38247" diff --git a/internal/command/test_test.go b/internal/command/test_test.go index d456659e1c..356ee0c4c2 100644 --- a/internal/command/test_test.go +++ b/internal/command/test_test.go @@ -5957,10 +5957,9 @@ func TestTest_ParallelDeps(t *testing.T) { t.Chdir(td) provider := testing_command.NewProvider(nil) - providerSource, close := newMockProviderSource(t, map[string][]string{ + providerSource := newMockProviderSource(t, map[string][]string{ "test": {"1.0.0"}, }) - defer close() streams, done := terminal.StreamsForTesting(t) view := views.NewView(streams)