Fix test command teardown order (#1043)

Signed-off-by: Ronny Orot <ronny.orot@gmail.com>

CHANGELOG.md

@@ -7,6 +7,7 @@ NEW FEATURES:
 ENHANCEMENTS:
 
 BUG FIXES:
+* `tofu test` resources cleanup at the end of tests changed to use simple reverse run block order. ([#1043](https://github.com/opentofu/opentofu/pull/1043))
 
 ## Previous Releases
 

internal/command/test.go

@@ -903,42 +903,8 @@ func (runner *TestFileRunner) Cleanup(file *moduletest.File) {
 		return
 	}
 
-	// First, we'll clean up the main state.
-	main := runner.States[MainStateIdentifier]
-
-	var diags tfdiags.Diagnostics
-	updated := main.State
-	if main.Run == nil {
-		if !main.State.Empty() {
-			log.Printf("[ERROR] TestFileRunner: found inconsistent run block and state file in %s", file.Name)
-			diags = diags.Append(tfdiags.Sourceless(tfdiags.Error, "Inconsistent state", fmt.Sprintf("Found inconsistent state while cleaning up %s. This is a bug in OpenTofu - please report it", file.Name)))
-		}
-	} else {
-		reset, configDiags := runner.Suite.Config.TransformForTest(main.Run.Config, file.Config)
-		diags = diags.Append(configDiags)
-
-		if !configDiags.HasErrors() {
-			var destroyDiags tfdiags.Diagnostics
-			updated, destroyDiags = runner.destroy(runner.Suite.Config, main.State, main.Run, file)
-			diags = diags.Append(destroyDiags)
-		}
-
-		reset()
-	}
-	runner.Suite.View.DestroySummary(diags, main.Run, file, updated)
-
-	if runner.Suite.Cancelled {
-		// In case things were cancelled during the last execution.
-		return
-	}
-
 	var states []*TestFileState
 	for key, state := range runner.States {
-		if key == MainStateIdentifier {
-			// We processed the main state above.
-			continue
-		}
-
 		if state.Run == nil {
 			if state.State.Empty() {
 				// We can see a run block being empty when the state is empty if
@@ -947,7 +913,12 @@ func (runner *TestFileRunner) Cleanup(file *moduletest.File) {
 				// skip it.
 				continue
 			}
-			log.Printf("[ERROR] TestFileRunner: found inconsistent run block and state file in %s for module %s", file.Name, key)
+
+			if key == MainStateIdentifier {
+				log.Printf("[ERROR] TestFileRunner: found inconsistent run block and state file in %s", file.Name)
+			} else {
+				log.Printf("[ERROR] TestFileRunner: found inconsistent run block and state file in %s for module %s", file.Name, key)
+			}
 
 			// Otherwise something bad has happened, and we have no way to
 			// recover from it. This shouldn't happen in reality, but we'll
@@ -965,19 +936,10 @@ func (runner *TestFileRunner) Cleanup(file *moduletest.File) {
 	slices.SortFunc(states, func(a, b *TestFileState) int {
 		// We want to clean up later run blocks first. So, we'll sort this in
 		// reverse according to index. This means larger indices first.
-		if a.Run.Index == b.Run.Index {
+		return b.Run.Index - a.Run.Index
-			return 0
-		}
-
-		const flag = 1
-		if a.Run.Index < b.Run.Index {
-			return -flag
-		}
-		return flag
 	})
 
-	// Then we'll clean up the additional states for custom modules in reverse
+	// Clean up all the states (for main and custom modules) in reverse order.
-	// order.
 	for _, state := range states {
 		log.Printf("[DEBUG] TestStateManager: cleaning up state for %s/%s", file.Name, state.Run.Name)
 
@@ -989,14 +951,22 @@ func (runner *TestFileRunner) Cleanup(file *moduletest.File) {
 		}
 
 		var diags tfdiags.Diagnostics
+		var runConfig *configs.Config
 
-		reset, configDiags := state.Run.Config.ConfigUnderTest.TransformForTest(state.Run.Config, file.Config)
+		isMainState := state.Run.Config.Module == nil
+		if isMainState {
+			runConfig = runner.Suite.Config
+		} else {
+			runConfig = state.Run.Config.ConfigUnderTest
+		}
+
+		reset, configDiags := runConfig.TransformForTest(state.Run.Config, file.Config)
 		diags = diags.Append(configDiags)
 
 		updated := state.State
 		if !diags.HasErrors() {
 			var destroyDiags tfdiags.Diagnostics
-			updated, destroyDiags = runner.destroy(state.Run.Config.ConfigUnderTest, state.State, state.Run, file)
+			updated, destroyDiags = runner.destroy(runConfig, state.State, state.Run, file)
 			diags = diags.Append(destroyDiags)
 		}
 		runner.Suite.View.DestroySummary(diags, state.Run, file, updated)

internal/command/test_test.go

@@ -780,58 +780,98 @@ can remove the provider configuration again.
 	}
 }
 
-func TestTest_NestedSetupModules(t *testing.T) {
+func TestTest_Modules(t *testing.T) {
-	td := t.TempDir()
+	tcs := map[string]struct {
-	testCopyDir(t, testFixturePath(path.Join("test", "with_nested_setup_modules")), td)
+		expected string
-	defer testChdir(t, td)()
+		code     int
-
+		skip     bool
-	provider := testing_command.NewProvider(nil)
+	}{
-
+		"with_nested_setup_modules": {
-	providerSource, close := newMockProviderSource(t, map[string][]string{
+			expected: "main.tftest.hcl... pass\n  run \"load_module\"... pass\n\nSuccess! 1 passed, 0 failed.\n",
-		"test": {"1.0.0"},
+			code:     0,
-	})
+		},
-	defer close()
+		"with_verify_module": {
-
+			expected: "main.tftest.hcl... pass\n  run \"test\"... pass\n  run \"verify\"... pass\n\nSuccess! 2 passed, 0 failed.\n",
-	streams, done := terminal.StreamsForTesting(t)
+			code:     0,
-	view := views.NewView(streams)
+		},
-	ui := new(cli.MockUi)
+		"only_modules": {
-
+			expected: "main.tftest.hcl... pass\n  run \"first\"... pass\n  run \"second\"... pass\n\nSuccess! 2 passed, 0 failed.\n",
-	meta := Meta{
+			code:     0,
-		testingOverrides: metaOverridesForProvider(provider.Provider),
+		},
-		Ui:               ui,
-		View:             view,
-		Streams:          streams,
-		ProviderSource:   providerSource,
 	}
 
-	init := &InitCommand{
+	for name, tc := range tcs {
-		Meta: meta,
+		t.Run(name, func(t *testing.T) {
-	}
+			if tc.skip {
+				t.Skip()
+			}
 
-	if code := init.Run(nil); code != 0 {
+			file := name
-		t.Fatalf("expected status code 0 but got %d: %s", code, ui.ErrorWriter)
-	}
 
-	command := &TestCommand{
+			td := t.TempDir()
-		Meta: meta,
+			testCopyDir(t, testFixturePath(path.Join("test", file)), td)
-	}
+			defer testChdir(t, td)()
 
-	code := command.Run(nil)
+			provider := testing_command.NewProvider(nil)
-	output := done(t)
+			providerSource, close := newMockProviderSource(t, map[string][]string{
+				"test": {"1.0.0"},
+			})
+			defer close()
 
-	printedOutput := false
+			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,
+			}
 
-	if code != 0 {
+			init := &InitCommand{
-		printedOutput = true
+				Meta: meta,
-		t.Errorf("expected status code 0 but got %d: %s", code, output.All())
+			}
-	}
 
-	if provider.ResourceCount() > 0 {
+			if code := init.Run(nil); code != 0 {
-		if !printedOutput {
+				t.Fatalf("expected status code 0 but got %d: %s", code, ui.ErrorWriter)
-			t.Errorf("should have deleted all resources on completion but left %s\n\n%s", provider.ResourceString(), output.All())
+			}
-		} else {
+
-			t.Errorf("should have deleted all resources on completion but left %s", provider.ResourceString())
+			command := &TestCommand{
-		}
+				Meta: meta,
+			}
+
+			code := command.Run([]string{"-no-color"})
+			output := done(t)
+			printedOutput := false
+
+			if code != tc.code {
+				printedOutput = true
+				t.Errorf("expected status code %d but got %d: %s", tc.code, code, output.All())
+			}
+
+			actual := output.All()
+
+			if diff := cmp.Diff(actual, tc.expected); len(diff) > 0 {
+				t.Errorf("output didn't match expected:\nexpected:\n%s\nactual:\n%s\ndiff:\n%s", tc.expected, actual, diff)
+			}
+
+			if provider.ResourceCount() > 0 {
+				if !printedOutput {
+					t.Errorf("should have deleted all resources on completion but left %s\n\n%s", provider.ResourceString(), output.All())
+				} else {
+					t.Errorf("should have deleted all resources on completion but left %s", provider.ResourceString())
+				}
+			}
+
+			if provider.DataSourceCount() > 0 {
+				if !printedOutput {
+					t.Errorf("should have deleted all data sources on completion but left %s\n\n%s", provider.DataSourceString(), output.All())
+				} else {
+					t.Errorf("should have deleted all data sources on completion but left %s", provider.DataSourceString())
+				}
+			}
+		})
 	}
 }
 
@@ -951,67 +991,6 @@ Success! 5 passed, 0 failed.
 	}
 }
 
-func TestTest_OnlyExternalModules(t *testing.T) {
-	td := t.TempDir()
-	testCopyDir(t, testFixturePath(path.Join("test", "only_modules")), 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... pass
-  run "first"... pass
-  run "second"... pass
-
-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_PartialUpdates(t *testing.T) {
 	tcs := map[string]struct {
 		expectedOut  string

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

@@ -0,0 +1,12 @@
+variable "id" {
+  type = string
+}
+
+variable "value" {
+  type = string
+}
+
+resource "test_resource" "resource" {
+  id = var.id
+  value = var.value
+}

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

@@ -0,0 +1,23 @@
+variables {
+  id = "resource"
+  value = "Hello, world!"
+}
+
+run "test" {
+}
+
+run "verify" {
+  module {
+    source = "./verify"
+  }
+
+  assert {
+    condition = data.test_data_source.resource_data.value == "Hello, world!"
+    error_message = "bad value"
+  }
+
+  assert {
+    condition = test_resource.another_resource.id == "hi"
+    error_message = "bad value"
+  }
+}

internal/command/testdata/test/with_verify_module/verify/main.tf

@@ -0,0 +1,11 @@
+variable "id" {
+  type = string
+}
+
+data "test_data_source" "resource_data" {
+  id = var.id
+}
+
+resource "test_resource" "another_resource" {
+  id = "hi"
+}