idiomatic tests and review notes

Author: SasSwart

provider/workspace_preset.go

@@ -5,6 +5,7 @@ import (
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
 	"github.com/mitchellh/mapstructure"
 )
 
@@ -31,12 +32,11 @@ func workspacePresetDataSource() *schema.Resource {
 				return diag.Errorf("decode workspace preset: %s", err)
 			}
 
-			if preset.Name == "" {
-				return diag.Errorf("workspace preset name must be set")
-			}
-
+			// MinItems doesn't work with maps, so we need to check the length
+			// of the map manually. All other validation is handled by the
+			// schema.
 			if len(preset.Parameters) == 0 {
-				return diag.Errorf("workspace preset must define a value for at least one parameter")
+				return diag.Errorf("expected \"parameters\" to not be an empty map")
 			}
 
 			rd.SetId(preset.Name)
@@ -50,14 +50,20 @@ func workspacePresetDataSource() *schema.Resource {
 				Computed:    true,
 			},
 			"name": {
-				Type:        schema.TypeString,
-				Description: "Name of the workspace preset.",
-				Required:    true,
+				Type:         schema.TypeString,
+				Description:  "Name of the workspace preset.",
+				Required:     true,
+				ValidateFunc: validation.StringIsNotEmpty,
 			},
 			"parameters": {
 				Type:        schema.TypeMap,
 				Description: "Parameters of the workspace preset.",
 				Required:    true,
+				Elem: &schema.Schema{
+					Type:         schema.TypeString,
+					Required:     true,
+					ValidateFunc: validation.StringIsNotEmpty,
+				},
 			},
 		},
 	}

provider/workspace_preset_test.go

@@ -10,11 +10,16 @@ import (
 )
 
 func TestWorkspacePreset(t *testing.T) {
-	// Happy Path:
-	resource.Test(t, resource.TestCase{
-		ProviderFactories: coderFactory(),
-		IsUnitTest:        true,
-		Steps: []resource.TestStep{{
+	t.Parallel()
+	type testcase struct {
+		Name        string
+		Config      string
+		ExpectError *regexp.Regexp
+		Check       func(state *terraform.State) error
+	}
+	testcases := []testcase{
+		{
+			Name: "Happy Path",
 			Config: `
 			data "coder_workspace_preset" "preset_1" {
 				name = "preset_1"
@@ -32,96 +37,92 @@ func TestWorkspacePreset(t *testing.T) {
 				require.Equal(t, attrs["parameters.region"], "us-east1-a")
 				return nil
 			},
-		}},
-	})
-
-	// Given the Name field is not provided
-	resource.Test(t, resource.TestCase{
-		ProviderFactories: coderFactory(),
-		IsUnitTest:        true,
-		Steps: []resource.TestStep{{
+		},
+		{
+			Name: "Name field is not provided",
 			Config: `
 			data "coder_workspace_preset" "preset_1" {
 				parameters = {
 					"region" = "us-east1-a"
 				}
 			}`,
-			// This is from terraform's validation based on our schema, not based on our validation in ReadContext:
+			// This validation is done by Terraform, but it could still break if we misconfigure the schema.
+			// So we test it here to make sure we don't regress.
 			ExpectError: regexp.MustCompile("The argument \"name\" is required, but no definition was found"),
-		}},
-	})
-
-	// Given the Name field is empty
-	resource.Test(t, resource.TestCase{
-		ProviderFactories: coderFactory(),
-		IsUnitTest:        true,
-		Steps: []resource.TestStep{{
+		},
+		{
+			Name: "Name field is empty",
 			Config: `
 			data "coder_workspace_preset" "preset_1" {
 				name = ""
 				parameters = {
 					"region" = "us-east1-a"
 				}
 			}`,
-			ExpectError: regexp.MustCompile("workspace preset name must be set"),
-		}},
-	})
-
-	// Given the Name field is not a string
-	resource.Test(t, resource.TestCase{
-		ProviderFactories: coderFactory(),
-		IsUnitTest:        true,
-		Steps: []resource.TestStep{{
+			// This validation is done by Terraform, but it could still break if we misconfigure the schema.
+			// So we test it here to make sure we don't regress.
+			ExpectError: regexp.MustCompile("expected \"name\" to not be an empty string"),
+		},
+		{
+			Name: "Name field is not a string",
 			Config: `
 			data "coder_workspace_preset" "preset_1" {
 				name = [1, 2, 3]
 				parameters = {
 					"region" = "us-east1-a"
 				}
 			}`,
+			// This validation is done by Terraform, but it could still break if we misconfigure the schema.
+			// So we test it here to make sure we don't regress.
 			ExpectError: regexp.MustCompile("Incorrect attribute value type"),
-		}},
-	})
-
-	// Given the Parameters field is not provided
-	resource.Test(t, resource.TestCase{
-		ProviderFactories: coderFactory(),
-		IsUnitTest:        true,
-		Steps: []resource.TestStep{{
+		},
+		{
+			Name: "Parameters field is not provided",
 			Config: `
 			data "coder_workspace_preset" "preset_1" {
 				name = "preset_1"
 			}`,
+			// This validation is done by Terraform, but it could still break if we misconfigure the schema.
+			// So we test it here to make sure we don't regress.
 			ExpectError: regexp.MustCompile("The argument \"parameters\" is required, but no definition was found"),
-		}},
-	})
-
-	// Given the Parameters field is empty
-	resource.Test(t, resource.TestCase{
-		ProviderFactories: coderFactory(),
-		IsUnitTest:        true,
-		Steps: []resource.TestStep{{
+		},
+		{
+			Name: "Parameters field is empty",
 			Config: `
 			data "coder_workspace_preset" "preset_1" {
 				name = "preset_1"
 				parameters = {}
 			}`,
-			ExpectError: regexp.MustCompile("workspace preset must define a value for at least one parameter"),
-		}},
-	})
-
-	// Given the Parameters field is not a map
-	resource.Test(t, resource.TestCase{
-		ProviderFactories: coderFactory(),
-		IsUnitTest:        true,
-		Steps: []resource.TestStep{{
+			// This validation is *not* done by Terraform, because MinItems doesn't work with maps.
+			// We've implemented the validation in ReadContext, so we test it here to make sure we don't regress.
+			ExpectError: regexp.MustCompile("expected \"parameters\" to not be an empty map"),
+		},
+		{
+			Name: "Parameters field is not a map",
 			Config: `
 			data "coder_workspace_preset" "preset_1" {
 				name = "preset_1"
 				parameters = "not a map"
 			}`,
-			// This is from terraform's validation based on our schema, not based on our validation in ReadContext:
+			// This validation is done by Terraform, but it could still break if we misconfigure the schema.
+			// So we test it here to make sure we don't regress.
 			ExpectError: regexp.MustCompile("Inappropriate value for attribute \"parameters\": map of string required"),
-		}},
-	})
+		},
+	}
+
+	for _, testcase := range testcases {
+		t.Run(testcase.Name, func(t *testing.T) {
+			t.Parallel()
+
+			resource.Test(t, resource.TestCase{
+				ProviderFactories: coderFactory(),
+				IsUnitTest:        true,
+				Steps: []resource.TestStep{{
+					Config:      testcase.Config,
+					ExpectError: testcase.ExpectError,
+					Check:       testcase.Check,
+				}},
+			})
+		})
+	}
 }