From 466042ae2ded049f4597a1e66c5e1b103aa05c2a Mon Sep 17 00:00:00 2001 From: Sarah French Date: Wed, 29 Mar 2023 11:44:51 +0100 Subject: [PATCH 01/11] Add unit tests for `getProject` util --- .../third_party/terraform/utils/utils_test.go | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/mmv1/third_party/terraform/utils/utils_test.go b/mmv1/third_party/terraform/utils/utils_test.go index 4192a3c8ecd1..2f9155a9c315 100644 --- a/mmv1/third_party/terraform/utils/utils_test.go +++ b/mmv1/third_party/terraform/utils/utils_test.go @@ -144,6 +144,65 @@ func TestRfc3339TimeDiffSuppress(t *testing.T) { } } +func TestGetProject(t *testing.T) { + cases := map[string]struct { + ResourceProject string + ProviderProject string + ExpectedProject string + ExpectedError bool + }{ + "project is pulled from resource config instead of provider config": { + ResourceProject: "foo", + ProviderProject: "bar", + ExpectedProject: "foo", + }, + "project is pulled from provider config when not set on resource": { + ProviderProject: "bar", + ExpectedProject: "bar", + }, + "error returned when project not set on either provider or resource": { + ExpectedError: true, + }, + } + for tn, tc := range cases { + t.Run(tn, func(t *testing.T) { + // Arrange + + // Create provider config + var config Config + if tc.ProviderProject != "" { + config.Project = tc.ProviderProject + } + + // Create resource config + // Here use ResourceComputeDisk schema as example + d := schema.TestResourceDataRaw(t, ResourceComputeDisk().Schema, map[string]interface{}{ + "project": tc.ResourceProject, + }) + if tc.ResourceProject != "" { + if err := d.Set("project", tc.ResourceProject); err != nil { + t.Fatalf("Cannot set project: %s", err) + } + } + + // Act + project, err := getProject(d, &config) + + // Assert + if err != nil { + if tc.ExpectedError { + return + } + t.Fatalf("Unexpected error using test: %s", err) + } + + if project != tc.ExpectedProject { + t.Fatalf("Incorrect project: got %s, want %s", project, tc.ExpectedProject) + } + }) + } +} + func TestGetZone(t *testing.T) { d := schema.TestResourceDataRaw(t, ResourceComputeDisk().Schema, map[string]interface{}{ "zone": "foo", From 5bf561d3606c67c01a531ef27585fb7be56cc42a Mon Sep 17 00:00:00 2001 From: Sarah French Date: Wed, 29 Mar 2023 11:45:09 +0100 Subject: [PATCH 02/11] Refactor existing `TestGetZone` test --- .../third_party/terraform/utils/utils_test.go | 81 ++++++++++++++----- 1 file changed, 59 insertions(+), 22 deletions(-) diff --git a/mmv1/third_party/terraform/utils/utils_test.go b/mmv1/third_party/terraform/utils/utils_test.go index 2f9155a9c315..b3461e74dd0d 100644 --- a/mmv1/third_party/terraform/utils/utils_test.go +++ b/mmv1/third_party/terraform/utils/utils_test.go @@ -204,29 +204,66 @@ func TestGetProject(t *testing.T) { } func TestGetZone(t *testing.T) { - d := schema.TestResourceDataRaw(t, ResourceComputeDisk().Schema, map[string]interface{}{ - "zone": "foo", - }) - var config Config - if err := d.Set("zone", "foo"); err != nil { - t.Fatalf("Cannot set zone: %s", err) - } - if zone, err := getZone(d, &config); err != nil || zone != "foo" { - t.Fatalf("Zone '%s' != 'foo', %s", zone, err) - } - config.Zone = "bar" - if zone, err := getZone(d, &config); err != nil || zone != "foo" { - t.Fatalf("Zone '%s' != 'foo', %s", zone, err) - } - if err := d.Set("zone", ""); err != nil { - t.Fatalf("Error setting zone: %s", err) - } - if zone, err := getZone(d, &config); err != nil || zone != "bar" { - t.Fatalf("Zone '%s' != 'bar', %s", zone, err) + cases := map[string]struct { + ResourceZone string + ProviderZone string + ExpectedZone string + ExpectedError bool + }{ + "zone is pulled from resource config instead of provider config": { + ResourceZone: "foo", + ProviderZone: "bar", + ExpectedZone: "foo", + }, + "zone is pulled from provider config when not set on resource": { + ProviderZone: "bar", + ExpectedZone: "bar", + }, + "zone value from resource is retrieved by splitting on slashes and selecting last element": { + // Unclear why this is the case - documenting this behaviour in a test case + ResourceZone: "this/is/foo/bar", + ExpectedZone: "bar", + }, + "error returned when zone not set on either provider or resource": { + ExpectedError: true, + }, } - config.Zone = "" - if zone, err := getZone(d, &config); err == nil || zone != "" { - t.Fatalf("Zone '%s' != '', err=%s", zone, err) + for tn, tc := range cases { + t.Run(tn, func(t *testing.T) { + // Arrange + + // Create provider config + var config Config + if tc.ProviderZone != "" { + config.Zone = tc.ProviderZone + } + + // Create resource config + // Here use ResourceComputeDisk schema as example + d := schema.TestResourceDataRaw(t, ResourceComputeDisk().Schema, map[string]interface{}{ + "zone": tc.ResourceZone, + }) + if tc.ResourceZone != "" { + if err := d.Set("zone", tc.ResourceZone); err != nil { + t.Fatalf("Cannot set zone: %s", err) + } + } + + // Act + zone, err := getZone(d, &config) + + // Assert + if err != nil { + if tc.ExpectedError { + return + } + t.Fatalf("Unexpected error using test: %s", err) + } + + if zone != tc.ExpectedZone { + t.Fatalf("Incorrect zone: got %s, want %s", zone, tc.ExpectedZone) + } + }) } } From 98805190ddd6ea787991bde0a0dfe66fe4f331c9 Mon Sep 17 00:00:00 2001 From: Sarah French Date: Wed, 29 Mar 2023 15:27:13 +0100 Subject: [PATCH 03/11] Move existing `getRegionFromZone` test alongside other util unit tests --- mmv1/third_party/terraform/utils/provider_test.go.erb | 8 -------- mmv1/third_party/terraform/utils/utils.go | 3 +++ mmv1/third_party/terraform/utils/utils_test.go | 8 ++++++++ 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/mmv1/third_party/terraform/utils/provider_test.go.erb b/mmv1/third_party/terraform/utils/provider_test.go.erb index 7c8a9eb64a5e..66e24448c8ac 100644 --- a/mmv1/third_party/terraform/utils/provider_test.go.erb +++ b/mmv1/third_party/terraform/utils/provider_test.go.erb @@ -166,14 +166,6 @@ func testAccPreCheck(t *testing.T) { } } -func TestProvider_getRegionFromZone(t *testing.T) { - expected := "us-central1" - actual := getRegionFromZone("us-central1-f") - if expected != actual { - t.Fatalf("Region (%s) did not match expected value: %s", actual, expected) - } -} - func TestProvider_loadCredentialsFromFile(t *testing.T) { ws, es := validateCredentials(testFakeCredentialsPath, "") if len(ws) != 0 { diff --git a/mmv1/third_party/terraform/utils/utils.go b/mmv1/third_party/terraform/utils/utils.go index eb202671cc30..d0d4e73810a9 100644 --- a/mmv1/third_party/terraform/utils/utils.go +++ b/mmv1/third_party/terraform/utils/utils.go @@ -49,6 +49,9 @@ type TerraformResourceDiff interface { } // getRegionFromZone returns the region from a zone for Google cloud. +// This is by removing the last two chars from the zone name to leave the region +// If there aren't enough characters in the input string, an empty string is returned +// e.g. southamerica-west1-a => southamerica-west1 func getRegionFromZone(zone string) string { if zone != "" && len(zone) > 2 { region := zone[:len(zone)-2] diff --git a/mmv1/third_party/terraform/utils/utils_test.go b/mmv1/third_party/terraform/utils/utils_test.go index b3461e74dd0d..c8c667aaea99 100644 --- a/mmv1/third_party/terraform/utils/utils_test.go +++ b/mmv1/third_party/terraform/utils/utils_test.go @@ -292,6 +292,14 @@ func TestGetRegion(t *testing.T) { } } +func TestProvider_getRegionFromZone(t *testing.T) { + expected := "us-central1" + actual := getRegionFromZone("us-central1-f") + if expected != actual { + t.Fatalf("Region (%s) did not match expected value: %s", actual, expected) + } +} + func TestDatasourceSchemaFromResourceSchema(t *testing.T) { type args struct { rs map[string]*schema.Schema From 225c1515ca74ddb401287f6ad127c8c14e6bd5f7 Mon Sep 17 00:00:00 2001 From: Sarah French Date: Wed, 29 Mar 2023 15:27:37 +0100 Subject: [PATCH 04/11] Rename `getRegionFromZone` unit test --- mmv1/third_party/terraform/utils/utils_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mmv1/third_party/terraform/utils/utils_test.go b/mmv1/third_party/terraform/utils/utils_test.go index c8c667aaea99..c164d2e30811 100644 --- a/mmv1/third_party/terraform/utils/utils_test.go +++ b/mmv1/third_party/terraform/utils/utils_test.go @@ -292,7 +292,7 @@ func TestGetRegion(t *testing.T) { } } -func TestProvider_getRegionFromZone(t *testing.T) { +func TestGetRegionFromZone(t *testing.T) { expected := "us-central1" actual := getRegionFromZone("us-central1-f") if expected != actual { From 648333f346f809ac238a0415862fb7774e82dffd Mon Sep 17 00:00:00 2001 From: Sarah French Date: Wed, 29 Mar 2023 19:37:11 +0100 Subject: [PATCH 05/11] Refactor `TestGetRegion` unit test --- .../third_party/terraform/utils/utils_test.go | 82 ++++++++++++++----- 1 file changed, 63 insertions(+), 19 deletions(-) diff --git a/mmv1/third_party/terraform/utils/utils_test.go b/mmv1/third_party/terraform/utils/utils_test.go index c164d2e30811..b442b4aa05a8 100644 --- a/mmv1/third_party/terraform/utils/utils_test.go +++ b/mmv1/third_party/terraform/utils/utils_test.go @@ -268,27 +268,71 @@ func TestGetZone(t *testing.T) { } func TestGetRegion(t *testing.T) { - d := schema.TestResourceDataRaw(t, ResourceComputeDisk().Schema, map[string]interface{}{ - "zone": "foo", - }) - var config Config - barRegionName := getRegionFromZone("bar") - fooRegionName := getRegionFromZone("foo") - - if region, err := getRegion(d, &config); err != nil || region != fooRegionName { - t.Fatalf("Zone '%s' != '%s', %s", region, fooRegionName, err) + cases := map[string]struct { + ResourceRegion string + ProviderRegion string + ProviderZone string + ExpectedRegion string + ExpectedZone string + ExpectedError bool + }{ + "region is pulled from resource config instead of provider config": { + ResourceRegion: "foo", + ProviderRegion: "bar", + ProviderZone: "lol-a", + ExpectedRegion: "foo", + }, + "region is pulled from region on provider config when region unset in resource config": { + ProviderRegion: "bar", + ProviderZone: "lol-a", + ExpectedRegion: "bar", + }, + "region is pulled from zone on provider config when region unset in both resource and provider config": { + ProviderZone: "lol-a", + ExpectedRegion: "lol", + }, + "error returned when region not set on resource and neither region or zone set on provider": { + ExpectedError: true, + }, } + for tn, tc := range cases { + t.Run(tn, func(t *testing.T) { + // Arrange - config.Zone = "bar" - if err := d.Set("zone", ""); err != nil { - t.Fatalf("Error setting zone: %s", err) - } - if region, err := getRegion(d, &config); err != nil || region != barRegionName { - t.Fatalf("Zone '%s' != '%s', %s", region, barRegionName, err) - } - config.Region = "something-else" - if region, err := getRegion(d, &config); err != nil || region != config.Region { - t.Fatalf("Zone '%s' != '%s', %s", region, config.Region, err) + // Create provider config + var config Config + if tc.ProviderRegion != "" { + config.Region = tc.ProviderRegion + } + if tc.ProviderZone != "" { + config.Zone = tc.ProviderZone + } + + // Create resource config + // Here use ComputeRegionInstanceTemplate schema as example - because it has a region field in schema + emptyConfigMap := map[string]interface{}{} + d := schema.TestResourceDataRaw(t, ResourceComputeRegionInstanceTemplate().Schema, emptyConfigMap) + if tc.ResourceRegion != "" { + if err := d.Set("region", tc.ResourceRegion); err != nil { + t.Fatalf("Cannot set region: %s", err) + } + } + + // Act + region, err := getRegion(d, &config) + + // Assert + if err != nil { + if tc.ExpectedError { + return + } + t.Fatalf("Unexpected error using test: %s", err) + } + + if region != tc.ExpectedRegion { + t.Fatalf("Incorrect region: got %s, want %s", region, tc.ExpectedRegion) + } + }) } } From 097f2a12bf562a454101d893e847a618e42050cd Mon Sep 17 00:00:00 2001 From: Sarah French Date: Wed, 29 Mar 2023 19:37:45 +0100 Subject: [PATCH 06/11] Simplify acc test setup code --- mmv1/third_party/terraform/utils/utils_test.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/mmv1/third_party/terraform/utils/utils_test.go b/mmv1/third_party/terraform/utils/utils_test.go index b442b4aa05a8..dec2894b1a71 100644 --- a/mmv1/third_party/terraform/utils/utils_test.go +++ b/mmv1/third_party/terraform/utils/utils_test.go @@ -175,10 +175,9 @@ func TestGetProject(t *testing.T) { } // Create resource config - // Here use ResourceComputeDisk schema as example - d := schema.TestResourceDataRaw(t, ResourceComputeDisk().Schema, map[string]interface{}{ - "project": tc.ResourceProject, - }) + // Here use ResourceComputeDisk schema as example - because it has a zone field in schema + emptyConfigMap := map[string]interface{}{} + d := schema.TestResourceDataRaw(t, ResourceComputeDisk().Schema, emptyConfigMap) if tc.ResourceProject != "" { if err := d.Set("project", tc.ResourceProject); err != nil { t.Fatalf("Cannot set project: %s", err) @@ -239,10 +238,9 @@ func TestGetZone(t *testing.T) { } // Create resource config - // Here use ResourceComputeDisk schema as example - d := schema.TestResourceDataRaw(t, ResourceComputeDisk().Schema, map[string]interface{}{ - "zone": tc.ResourceZone, - }) + // Here use ResourceComputeDisk schema as example - because it has a zone field in schema + emptyConfigMap := map[string]interface{}{} + d := schema.TestResourceDataRaw(t, ResourceComputeDisk().Schema, emptyConfigMap) if tc.ResourceZone != "" { if err := d.Set("zone", tc.ResourceZone); err != nil { t.Fatalf("Cannot set zone: %s", err) From 2abb6245c691bac8802c39cf518fa5cf23f3989c Mon Sep 17 00:00:00 2001 From: Sarah French Date: Wed, 29 Mar 2023 19:38:31 +0100 Subject: [PATCH 07/11] Change unit test to use subtests via `t.Run` --- .../terraform/utils/transport_test.go | 40 ++++++++++--------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/mmv1/third_party/terraform/utils/transport_test.go b/mmv1/third_party/terraform/utils/transport_test.go index 0f9aedf683e3..f9f60c5b9629 100644 --- a/mmv1/third_party/terraform/utils/transport_test.go +++ b/mmv1/third_party/terraform/utils/transport_test.go @@ -199,30 +199,32 @@ func TestReplaceVars(t *testing.T) { } for tn, tc := range cases { - d := &ResourceDataMock{ - FieldsInSchema: tc.SchemaValues, - } + t.Run(tn, func(t *testing.T) { + d := &ResourceDataMock{ + FieldsInSchema: tc.SchemaValues, + } - config := tc.Config - if config == nil { - config = &Config{} - } + config := tc.Config + if config == nil { + config = &Config{} + } - v, err := replaceVars(d, config, tc.Template) + v, err := replaceVars(d, config, tc.Template) - if err != nil { - if !tc.ExpectedError { - t.Errorf("bad: %s; unexpected error %s", tn, err) + if err != nil { + if !tc.ExpectedError { + t.Errorf("bad: %s; unexpected error %s", tn, err) + } + return } - continue - } - if tc.ExpectedError { - t.Errorf("bad: %s; expected error", tn) - } + if tc.ExpectedError { + t.Errorf("bad: %s; expected error", tn) + } - if v != tc.Expected { - t.Errorf("bad: %s; expected %q, got %q", tn, tc.Expected, v) - } + if v != tc.Expected { + t.Errorf("bad: %s; expected %q, got %q", tn, tc.Expected, v) + } + }) } } From ca33bba8928faa95e5a3904f4f8f4d057433997b Mon Sep 17 00:00:00 2001 From: Sarah French Date: Wed, 29 Mar 2023 20:29:18 +0100 Subject: [PATCH 08/11] Add unit test for `getProjectFramework` util function --- .../framework_utils/framework_utils_test.go | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 mmv1/third_party/terraform/framework_utils/framework_utils_test.go diff --git a/mmv1/third_party/terraform/framework_utils/framework_utils_test.go b/mmv1/third_party/terraform/framework_utils/framework_utils_test.go new file mode 100644 index 000000000000..ffa84ea21b73 --- /dev/null +++ b/mmv1/third_party/terraform/framework_utils/framework_utils_test.go @@ -0,0 +1,52 @@ +package google + +import ( + "testing" + + "github.com/hashicorp/terraform-plugin-framework/diag" + "github.com/hashicorp/terraform-plugin-framework/types" +) + +func TestGetProjectFramework(t *testing.T) { + cases := map[string]struct { + ResourceProject types.String + ProviderProject types.String + ExpectedProject types.String + ExpectedError bool + }{ + "project is pulled from the resource config value instead of the provider config value, even if both set": { + ResourceProject: types.StringValue("foo"), + ProviderProject: types.StringValue("bar"), + ExpectedProject: types.StringValue("foo"), + }, + "project is pulled from the provider config value when unset on the resource": { + // Note - ResourceProject is not set to non-zero value here + ProviderProject: types.StringValue("bar"), + ExpectedProject: types.StringValue("bar"), + }, + "error when project is not set on the provider or the resource": { + ExpectedError: true, + }, + } + for tn, tc := range cases { + t.Run(tn, func(t *testing.T) { + // Arrange + var diags diag.Diagnostics + + // Act + project := getProjectFramework(tc.ResourceProject, tc.ProviderProject, &diags) + + // Assert + if diags.HasError() { + if tc.ExpectedError { + return + } + t.Fatalf("Got %d unexpected error(s) during test: %s", diags.ErrorsCount(), diags.Errors()) + } + + if project != tc.ExpectedProject { + t.Fatalf("Incorrect project: got %s, want %s", project, tc.ExpectedProject) + } + }) + } +} From b27886c5c4bb71184e5ee321780c6cf77f2a06ac Mon Sep 17 00:00:00 2001 From: Sarah French Date: Thu, 30 Mar 2023 16:08:07 +0100 Subject: [PATCH 09/11] Fix comment --- mmv1/third_party/terraform/utils/utils_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mmv1/third_party/terraform/utils/utils_test.go b/mmv1/third_party/terraform/utils/utils_test.go index dec2894b1a71..6cb1014315f0 100644 --- a/mmv1/third_party/terraform/utils/utils_test.go +++ b/mmv1/third_party/terraform/utils/utils_test.go @@ -175,7 +175,7 @@ func TestGetProject(t *testing.T) { } // Create resource config - // Here use ResourceComputeDisk schema as example - because it has a zone field in schema + // Here use ResourceComputeDisk schema as example emptyConfigMap := map[string]interface{}{} d := schema.TestResourceDataRaw(t, ResourceComputeDisk().Schema, emptyConfigMap) if tc.ResourceProject != "" { From e165f146c234102ac53eeb934fbe9975063f63e6 Mon Sep 17 00:00:00 2001 From: Sarah French Date: Thu, 30 Mar 2023 16:09:40 +0100 Subject: [PATCH 10/11] Update test to set null value more explicitly using `types.StringNull()` --- .../terraform/framework_utils/framework_utils_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mmv1/third_party/terraform/framework_utils/framework_utils_test.go b/mmv1/third_party/terraform/framework_utils/framework_utils_test.go index ffa84ea21b73..5926315f1918 100644 --- a/mmv1/third_party/terraform/framework_utils/framework_utils_test.go +++ b/mmv1/third_party/terraform/framework_utils/framework_utils_test.go @@ -20,7 +20,7 @@ func TestGetProjectFramework(t *testing.T) { ExpectedProject: types.StringValue("foo"), }, "project is pulled from the provider config value when unset on the resource": { - // Note - ResourceProject is not set to non-zero value here + ResourceProject: types.StringNull(), ProviderProject: types.StringValue("bar"), ExpectedProject: types.StringValue("bar"), }, From 214a80c0cc19bd6cc563b7b0aef32c7c852a1453 Mon Sep 17 00:00:00 2001 From: Sarah French Date: Tue, 4 Apr 2023 19:59:33 +0100 Subject: [PATCH 11/11] Replace example schema for test `ResourceComputeRegionInstanceTemplate` was removed? --- mmv1/third_party/terraform/utils/utils_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mmv1/third_party/terraform/utils/utils_test.go b/mmv1/third_party/terraform/utils/utils_test.go index 6cb1014315f0..e6a133996d0b 100644 --- a/mmv1/third_party/terraform/utils/utils_test.go +++ b/mmv1/third_party/terraform/utils/utils_test.go @@ -307,9 +307,9 @@ func TestGetRegion(t *testing.T) { } // Create resource config - // Here use ComputeRegionInstanceTemplate schema as example - because it has a region field in schema + // Here use ResourceComputeSubnetwork schema as example - because it has a region field in schema emptyConfigMap := map[string]interface{}{} - d := schema.TestResourceDataRaw(t, ResourceComputeRegionInstanceTemplate().Schema, emptyConfigMap) + d := schema.TestResourceDataRaw(t, ResourceComputeSubnetwork().Schema, emptyConfigMap) if tc.ResourceRegion != "" { if err := d.Set("region", tc.ResourceRegion); err != nil { t.Fatalf("Cannot set region: %s", err)