Update and add new tests for module-parsing logic (#37176)

* Add comments, rename tests, reorder tests

This commit doesn't change or add any logic

* Add test for overriding a cloud block with a backend block

* Add tests showing multiple instances of cloud or backend blocks cause an error

* Add test showing that backend and cloud blocks cannot be in the same terraform block

* Add test case asserting it's not valid to have both cloud and backend blocks across files in a module

* Clarify comments and replace some comments with test names

* Replace comments with test names

internal/configs/module_test.go

@@ -315,104 +315,203 @@ func TestImpliedProviderForUnqualifiedType(t *testing.T) {
 	}
 }
 
-func TestModule_backend_override(t *testing.T) {
-	mod, diags := testModuleFromDir("testdata/valid-modules/override-backend")
-	if diags.HasErrors() {
-		t.Fatal(diags.Error())
-	}
+func TestModule_backend_overrides_a_backend(t *testing.T) {
+	t.Run("it can override a backend block with a different backend block", func(t *testing.T) {
+		mod, diags := testModuleFromDir("testdata/valid-modules/override-backend")
+		if diags.HasErrors() {
+			t.Fatal(diags.Error())
+		}
 
-	gotType := mod.Backend.Type
-	wantType := "bar"
+		gotType := mod.Backend.Type
+		wantType := "bar"
 
-	if gotType != wantType {
-		t.Errorf("wrong result for backend type: got %#v, want %#v\n", gotType, wantType)
-	}
+		if gotType != wantType {
+			t.Errorf("wrong result for backend type: got %#v, want %#v\n", gotType, wantType)
+		}
 
-	attrs, _ := mod.Backend.Config.JustAttributes()
+		attrs, _ := mod.Backend.Config.JustAttributes()
 
-	gotAttr, diags := attrs["path"].Expr.Value(nil)
-	if diags.HasErrors() {
-		t.Fatal(diags.Error())
-	}
+		gotAttr, diags := attrs["path"].Expr.Value(nil)
+		if diags.HasErrors() {
+			t.Fatal(diags.Error())
+		}
 
-	wantAttr := cty.StringVal("CHANGED/relative/path/to/terraform.tfstate")
+		wantAttr := cty.StringVal("CHANGED/relative/path/to/terraform.tfstate")
 
-	if !gotAttr.RawEquals(wantAttr) {
-		t.Errorf("wrong result for backend 'path': got %#v, want %#v\n", gotAttr, wantAttr)
-	}
+		if !gotAttr.RawEquals(wantAttr) {
+			t.Errorf("wrong result for backend 'path': got %#v, want %#v\n", gotAttr, wantAttr)
+		}
+	})
 }
 
 // Unlike most other overrides, backend blocks do not require a base configuration in a primary
 // configuration file, as an omitted backend there implies the local backend.
-func TestModule_backend_override_no_base(t *testing.T) {
-	mod, diags := testModuleFromDir("testdata/valid-modules/override-backend-no-base")
-	if diags.HasErrors() {
-		t.Fatal(diags.Error())
-	}
+func TestModule_backend_overrides_no_base(t *testing.T) {
+	t.Run("it can introduce a backend block via overrides when the base config has has no cloud or backend blocks", func(t *testing.T) {
+		mod, diags := testModuleFromDir("testdata/valid-modules/override-backend-no-base")
+		if diags.HasErrors() {
+			t.Fatal(diags.Error())
+		}
 
-	if mod.Backend == nil {
-		t.Errorf("expected module Backend not to be nil")
-	}
+		if mod.Backend == nil {
+			t.Errorf("expected module Backend not to be nil")
+		}
+	})
 }
 
-func TestModule_cloud_override_backend(t *testing.T) {
-	mod, diags := testModuleFromDir("testdata/valid-modules/override-backend-with-cloud")
-	if diags.HasErrors() {
-		t.Fatal(diags.Error())
-	}
+func TestModule_cloud_overrides_a_backend(t *testing.T) {
+	t.Run("it can override a backend block with a cloud block", func(t *testing.T) {
+		mod, diags := testModuleFromDir("testdata/valid-modules/override-backend-with-cloud")
+		if diags.HasErrors() {
+			t.Fatal(diags.Error())
+		}
 
-	if mod.Backend != nil {
-		t.Errorf("expected module Backend to be nil")
-	}
+		if mod.Backend != nil {
+			t.Errorf("expected module Backend to be nil")
+		}
 
-	if mod.CloudConfig == nil {
-		t.Errorf("expected module CloudConfig not to be nil")
-	}
+		if mod.CloudConfig == nil {
+			t.Errorf("expected module CloudConfig not to be nil")
+		}
+	})
+}
+
+func TestModule_cloud_overrides_cloud(t *testing.T) {
+	t.Run("it can override a cloud block with a different cloud block", func(t *testing.T) {
+		mod, diags := testModuleFromDir("testdata/valid-modules/override-cloud")
+		if diags.HasErrors() {
+			t.Fatal(diags.Error())
+		}
+
+		attrs, _ := mod.CloudConfig.Config.JustAttributes()
+
+		gotAttr, diags := attrs["organization"].Expr.Value(nil)
+		if diags.HasErrors() {
+			t.Fatal(diags.Error())
+		}
+
+		wantAttr := cty.StringVal("CHANGED")
+
+		if !gotAttr.RawEquals(wantAttr) {
+			t.Errorf("wrong result for Cloud 'organization': got %#v, want %#v\n", gotAttr, wantAttr)
+		}
+
+		// The override should have completely replaced the cloud block in the primary file, no merging
+		if attrs["should_not_be_present_with_override"] != nil {
+			t.Errorf("expected 'should_not_be_present_with_override' attribute to be nil")
+		}
+	})
 }
 
 // Unlike most other overrides, cloud blocks do not require a base configuration in a primary
 // configuration file, as an omitted backend there implies the local backend and cloud blocks
 // override backends.
-func TestModule_cloud_override_no_base(t *testing.T) {
-	mod, diags := testModuleFromDir("testdata/valid-modules/override-cloud-no-base")
-	if diags.HasErrors() {
-		t.Fatal(diags.Error())
-	}
+func TestModule_cloud_overrides_no_base(t *testing.T) {
+	t.Run("it can introduce a cloud block via overrides when the base config has no cloud or backend blocks", func(t *testing.T) {
 
-	if mod.CloudConfig == nil {
-		t.Errorf("expected module CloudConfig not to be nil")
-	}
+		mod, diags := testModuleFromDir("testdata/valid-modules/override-cloud-no-base")
+		if diags.HasErrors() {
+			t.Fatal(diags.Error())
+		}
+
+		if mod.CloudConfig == nil {
+			t.Errorf("expected module CloudConfig not to be nil")
+		}
+	})
 }
 
-func TestModule_cloud_override(t *testing.T) {
-	mod, diags := testModuleFromDir("testdata/valid-modules/override-cloud")
-	if diags.HasErrors() {
-		t.Fatal(diags.Error())
-	}
+func TestModule_backend_overrides_cloud(t *testing.T) {
+	t.Run("it can override a cloud block with a backend block", func(t *testing.T) {
+		mod, diags := testModuleFromDir("testdata/valid-modules/override-cloud-with-backend")
+		if diags.HasErrors() {
+			t.Fatal(diags.Error())
+		}
 
-	attrs, _ := mod.CloudConfig.Config.JustAttributes()
+		gotType := mod.Backend.Type
+		wantType := "override"
 
-	gotAttr, diags := attrs["organization"].Expr.Value(nil)
-	if diags.HasErrors() {
-		t.Fatal(diags.Error())
-	}
+		if gotType != wantType {
+			t.Errorf("wrong result for backend type: got %#v, want %#v\n", gotType, wantType)
+		}
 
-	wantAttr := cty.StringVal("CHANGED")
+		attrs, _ := mod.Backend.Config.JustAttributes()
 
-	if !gotAttr.RawEquals(wantAttr) {
-		t.Errorf("wrong result for Cloud 'organization': got %#v, want %#v\n", gotAttr, wantAttr)
-	}
+		gotAttr, diags := attrs["path"].Expr.Value(nil)
+		if diags.HasErrors() {
+			t.Fatal(diags.Error())
+		}
 
-	// The override should have completely replaced the cloud block in the primary file, no merging
-	if attrs["should_not_be_present_with_override"] != nil {
-		t.Errorf("expected 'should_not_be_present_with_override' attribute to be nil")
-	}
+		wantAttr := cty.StringVal("value from override")
+
+		if !gotAttr.RawEquals(wantAttr) {
+			t.Errorf("wrong result for backend 'path': got %#v, want %#v\n", gotAttr, wantAttr)
+		}
+	})
 }
 
 func TestModule_cloud_duplicate_overrides(t *testing.T) {
-	_, diags := testModuleFromDir("testdata/invalid-modules/override-cloud-duplicates")
-	want := `Duplicate HCP Terraform configurations`
-	if got := diags.Error(); !strings.Contains(got, want) {
-		t.Fatalf("expected module error to contain %q\nerror was:\n%s", want, got)
-	}
+	t.Run("it raises an error when a override file contains multiple cloud blocks", func(t *testing.T) {
+		_, diags := testModuleFromDir("testdata/invalid-modules/override-cloud-duplicates")
+		want := `Duplicate HCP Terraform configurations`
+		if got := diags.Error(); !strings.Contains(got, want) {
+			t.Fatalf("expected module error to contain %q\nerror was:\n%s", want, got)
+		}
+	})
+}
+
+func TestModule_backend_multiple(t *testing.T) {
+	t.Run("it detects when two backend blocks are present within the same module in separate files", func(t *testing.T) {
+		_, diags := testModuleFromDir("testdata/invalid-modules/multiple-backends")
+		if !diags.HasErrors() {
+			t.Fatal("module should have error diags, but does not")
+		}
+
+		want := `Duplicate backend configuration`
+		if got := diags.Error(); !strings.Contains(got, want) {
+			t.Fatalf("expected error to contain %q\nerror was:\n%s", want, got)
+		}
+	})
+}
+
+func TestModule_cloud_multiple(t *testing.T) {
+	t.Run("it detects when two cloud blocks are present within the same module in separate files", func(t *testing.T) {
+
+		_, diags := testModuleFromDir("testdata/invalid-modules/multiple-cloud")
+		if !diags.HasErrors() {
+			t.Fatal("module should have error diags, but does not")
+		}
+
+		want := `Duplicate HCP Terraform configurations`
+		if got := diags.Error(); !strings.Contains(got, want) {
+			t.Fatalf("expected error to contain %q\nerror was:\n%s", want, got)
+		}
+	})
+}
+
+// Cannot combine use of backend, cloud blocks.
+func TestModule_conflicting_backend_cloud(t *testing.T) {
+
+	t.Run("it detects when both cloud and backend blocks are in the same terraform block", func(t *testing.T) {
+		_, diags := testModuleFromDir("testdata/invalid-modules/conflict-cloud-backend")
+		if !diags.HasErrors() {
+			t.Fatal("module should have error diags, but does not")
+		}
+
+		want := `Both a backend and cloud configuration are present`
+		if got := diags.Error(); !strings.Contains(got, want) {
+			t.Fatalf("expected error to contain %q\nerror was:\n%s", want, got)
+		}
+	})
+
+	t.Run("it detects when both cloud and backend blocks are present within the same module in separate files", func(t *testing.T) {
+		_, diags := testModuleFromDir("testdata/invalid-modules/conflict-cloud-backend-separate-files")
+		if !diags.HasErrors() {
+			t.Fatal("module should have error diags, but does not")
+		}
+
+		want := `Both a backend and cloud configuration are present`
+		if got := diags.Error(); !strings.Contains(got, want) {
+			t.Fatalf("expected error to contain %q\nerror was:\n%s", want, got)
+		}
+	})
 }

internal/configs/testdata/invalid-modules/conflict-cloud-backend-separate-files/backend.tf

@@ -0,0 +1,3 @@
+terraform {
+  backend "foo" {}
+}

internal/configs/testdata/invalid-modules/conflict-cloud-backend-separate-files/cloud.tf

@@ -0,0 +1,8 @@
+terraform {
+  cloud {
+    organization = "sarahfrench"
+    workspaces {
+      name = "test-cloud-backend"
+    }
+  }
+}

internal/configs/testdata/invalid-modules/conflict-cloud-backend/main.tf

@@ -0,0 +1,10 @@
+terraform {
+  backend "foo" {}
+
+  cloud {
+    organization = "sarahfrench"
+    workspaces {
+      name = "test-cloud-backend"
+    }
+  }
+}

internal/configs/testdata/invalid-modules/multiple-backends/a.tf

@@ -0,0 +1,3 @@
+terraform {
+  backend "foo" {}
+}

internal/configs/testdata/invalid-modules/multiple-backends/b.tf

@@ -0,0 +1,3 @@
+terraform {
+  backend "bar" {}
+}

internal/configs/testdata/invalid-modules/multiple-cloud/a.tf

@@ -0,0 +1,5 @@
+terraform {
+  cloud {
+    organization = "foo"
+  }
+}

internal/configs/testdata/invalid-modules/multiple-cloud/b.tf

@@ -0,0 +1,5 @@
+terraform {
+  cloud {
+    organization = "bar"
+  }
+}

internal/configs/testdata/valid-modules/override-cloud-with-backend/main.tf

@@ -0,0 +1,13 @@
+terraform {
+  cloud {
+    organization = "foo"
+  }
+}
+
+resource "aws_instance" "web" {
+  ami = "ami-1234"
+  security_groups = [
+    "foo",
+    "bar",
+  ]
+}

internal/configs/testdata/valid-modules/override-cloud-with-backend/override.tf

@@ -0,0 +1,5 @@
+terraform {
+  backend "override" {
+    path = "value from override"
+  }
+}