From 817a211c49f37b22cd5f9bf457904f2246af4f9c Mon Sep 17 00:00:00 2001 From: Ivan De Marino Date: Thu, 28 Apr 2022 09:11:08 +0100 Subject: [PATCH 1/6] Adding `resource.TestCheckResourceAttrWith` test utility --- helper/resource/testing.go | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/helper/resource/testing.go b/helper/resource/testing.go index 79eb30eeac8..3bd5a3d0061 100644 --- a/helper/resource/testing.go +++ b/helper/resource/testing.go @@ -13,10 +13,11 @@ import ( "time" "github.com/hashicorp/go-multierror" - testing "github.com/mitchellh/go-testing-interface" + "github.com/mitchellh/go-testing-interface" "github.com/hashicorp/terraform-plugin-go/tfprotov5" "github.com/hashicorp/terraform-plugin-go/tfprotov6" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/internal/addrs" "github.com/hashicorp/terraform-plugin-sdk/v2/internal/logging" @@ -49,7 +50,7 @@ var flagSweepAllowFailures = flag.Bool("sweep-allow-failures", false, "Enable to var flagSweepRun = flag.String("sweep-run", "", "Comma seperated list of Sweeper Tests to run") var sweeperFuncs map[string]*Sweeper -// type SweeperFunc is a signature for a function that acts as a sweeper. It +// SweeperFunc is a signature for a function that acts as a sweeper. It // accepts a string for the region that the sweeper is to be ran in. This // function must be able to construct a valid client for that region. type SweeperFunc func(r string) error @@ -959,6 +960,34 @@ func testCheckResourceAttr(is *terraform.InstanceState, name string, key string, return nil } +// CheckResourceAttrWithFunc is the callback type used to apply a custom checking logic +// when using TestCheckResourceAttrWith and a value is found for the given name and key. +// +// When this function returns an error, TestCheckResourceAttrWith will fail the check. +type CheckResourceAttrWithFunc func(value string) error + +// TestCheckResourceAttrWith - as per TestCheckResourceAttr but the attribute value +// checking logic can be customised. +// +// If a value is found for the given name and key, it is passed to the CheckResourceAttrWithFunc function. +// The CheckResourceAttrWithFunc function can then apply any checking logic: +// should return an error for the check to fail, or `nil` to succeed. +func TestCheckResourceAttrWith(name, key string, f CheckResourceAttrWithFunc) TestCheckFunc { + return checkIfIndexesIntoTypeSet(key, func(s *terraform.State) error { + is, err := primaryInstanceState(s, name) + if err != nil { + return err + } + + err = testCheckResourceAttrSet(is, name, key) + if err != nil { + return err + } + + return f(is.Attributes[key]) + }) +} + // TestCheckNoResourceAttr ensures no value exists in the state for the // given name and key combination. The opposite of this TestCheckFunc is // TestCheckResourceAttrSet. State value checking is only recommended for From 53dc044ca97ccf42d07bda433f3330f576b661b9 Mon Sep 17 00:00:00 2001 From: Ivan De Marino Date: Thu, 28 Apr 2022 09:11:21 +0100 Subject: [PATCH 2/6] Adding a test suite for `resource.TestCheckResourceAttrWith` --- helper/resource/testing_test.go | 438 ++++++++++++++++++++++++++++++++ 1 file changed, 438 insertions(+) diff --git a/helper/resource/testing_test.go b/helper/resource/testing_test.go index 660471a2c37..90caae7681d 100644 --- a/helper/resource/testing_test.go +++ b/helper/resource/testing_test.go @@ -1402,6 +1402,444 @@ func TestTestCheckResourceAttr(t *testing.T) { } } +func TestTestCheckResourceAttrWith(t *testing.T) { + t.Parallel() + + testCases := map[string]struct { + state *terraform.State + key string + value string + expectedError error + }{ + "attribute not found": { + state: &terraform.State{ + IsBinaryDrivenTest: true, // Always true now + Modules: []*terraform.ModuleState{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.ResourceState{ + "test_resource": { + Primary: &terraform.InstanceState{ + Attributes: map[string]string{}, + }, + }, + }, + }, + }, + }, + key: "nonexistent", + value: "test-value", + expectedError: fmt.Errorf("Attribute 'nonexistent' expected to be set"), + }, + "bool attribute match": { + state: &terraform.State{ + IsBinaryDrivenTest: true, // Always true now + Modules: []*terraform.ModuleState{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.ResourceState{ + "test_resource": { + Primary: &terraform.InstanceState{ + Attributes: map[string]string{ + "test_bool_attribute": "true", + }, + }, + }, + }, + }, + }, + }, + key: "test_bool_attribute", + value: "true", + }, + "bool attribute mismatch": { + state: &terraform.State{ + IsBinaryDrivenTest: true, // Always true now + Modules: []*terraform.ModuleState{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.ResourceState{ + "test_resource": { + Primary: &terraform.InstanceState{ + Attributes: map[string]string{ + "test_bool_attribute": "true", + }, + }, + }, + }, + }, + }, + }, + key: "test_bool_attribute", + value: "false", + expectedError: fmt.Errorf("attribute 'test_bool_attribute' expected 'false', got 'true'"), + }, + "list attribute directly": { + state: &terraform.State{ + IsBinaryDrivenTest: true, // Always true now + Modules: []*terraform.ModuleState{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.ResourceState{ + "test_resource": { + Primary: &terraform.InstanceState{ + Attributes: map[string]string{ + "test_list_attribute.#": "1", + "test_list_attribute.0": "test-value", + }, + }, + }, + }, + }, + }, + }, + key: "test_list_attribute", + value: "test-value", + expectedError: fmt.Errorf("list or set attribute 'test_list_attribute' must be checked by element count key (test_list_attribute.#) or element value keys (e.g. test_list_attribute.0)"), + }, + "list attribute element count match": { + state: &terraform.State{ + IsBinaryDrivenTest: true, // Always true now + Modules: []*terraform.ModuleState{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.ResourceState{ + "test_resource": { + Primary: &terraform.InstanceState{ + Attributes: map[string]string{ + "test_list_attribute.#": "1", + "test_list_attribute.0": "test-value", + }, + }, + }, + }, + }, + }, + }, + key: "test_list_attribute.#", + value: "1", + }, + "list attribute element count mismatch": { + state: &terraform.State{ + IsBinaryDrivenTest: true, // Always true now + Modules: []*terraform.ModuleState{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.ResourceState{ + "test_resource": { + Primary: &terraform.InstanceState{ + Attributes: map[string]string{ + "test_list_attribute.#": "1", + "test_list_attribute.0": "test-value", + }, + }, + }, + }, + }, + }, + }, + key: "test_list_attribute.#", + value: "2", + expectedError: fmt.Errorf("attribute 'test_list_attribute.#' expected '2', got '1'"), + }, + "list attribute element count match 0 when empty": { + state: &terraform.State{ + IsBinaryDrivenTest: true, // Always true now + Modules: []*terraform.ModuleState{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.ResourceState{ + "test_resource": { + Primary: &terraform.InstanceState{ + Attributes: map[string]string{ + "test_list_attribute.#": "0", + }, + }, + }, + }, + }, + }, + }, + key: "test_list_attribute.#", + value: "0", + }, + "list attribute element value match": { + state: &terraform.State{ + IsBinaryDrivenTest: true, // Always true now + Modules: []*terraform.ModuleState{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.ResourceState{ + "test_resource": { + Primary: &terraform.InstanceState{ + Attributes: map[string]string{ + "test_list_attribute.#": "1", + "test_list_attribute.0": "test-value", + }, + }, + }, + }, + }, + }, + }, + key: "test_list_attribute.0", + value: "test-value", + }, + "list attribute element value mismatch": { + state: &terraform.State{ + IsBinaryDrivenTest: true, // Always true now + Modules: []*terraform.ModuleState{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.ResourceState{ + "test_resource": { + Primary: &terraform.InstanceState{ + Attributes: map[string]string{ + "test_list_attribute.#": "1", + "test_list_attribute.0": "test-value", + }, + }, + }, + }, + }, + }, + }, + key: "test_list_attribute.0", + value: "not-test-value", + expectedError: fmt.Errorf("attribute 'test_list_attribute.0' expected 'not-test-value', got 'test-value'"), + }, + "map attribute directly": { + state: &terraform.State{ + IsBinaryDrivenTest: true, // Always true now + Modules: []*terraform.ModuleState{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.ResourceState{ + "test_resource": { + Primary: &terraform.InstanceState{ + Attributes: map[string]string{ + "test_map_attribute.%": "1", + "test_map_attribute.testkey1": "test-value-1", + }, + }, + }, + }, + }, + }, + }, + key: "test_map_attribute", + value: "test-value", + expectedError: fmt.Errorf("map attribute 'test_map_attribute' must be checked by element count key (test_map_attribute.%%) or element value keys (e.g. test_map_attribute.examplekey)"), + }, + "map attribute element count match": { + state: &terraform.State{ + IsBinaryDrivenTest: true, // Always true now + Modules: []*terraform.ModuleState{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.ResourceState{ + "test_resource": { + Primary: &terraform.InstanceState{ + Attributes: map[string]string{ + "test_map_attribute.%": "1", + "test_map_attribute.testkey1": "test-value-1", + }, + }, + }, + }, + }, + }, + }, + key: "test_map_attribute.%", + value: "1", + }, + "map attribute element count mismatch": { + state: &terraform.State{ + IsBinaryDrivenTest: true, // Always true now + Modules: []*terraform.ModuleState{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.ResourceState{ + "test_resource": { + Primary: &terraform.InstanceState{ + Attributes: map[string]string{ + "test_map_attribute.%": "1", + "test_map_attribute.testkey1": "test-value-1", + }, + }, + }, + }, + }, + }, + }, + key: "test_map_attribute.%", + value: "2", + expectedError: fmt.Errorf("attribute 'test_map_attribute.%%' expected '2', got '1'"), + }, + "map attribute element count match 0 when empty": { + state: &terraform.State{ + IsBinaryDrivenTest: true, // Always true now + Modules: []*terraform.ModuleState{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.ResourceState{ + "test_resource": { + Primary: &terraform.InstanceState{ + Attributes: map[string]string{ + "test_map_attribute.%": "0", + }, + }, + }, + }, + }, + }, + }, + key: "test_map_attribute.%", + value: "0", + }, + "map attribute element value match": { + state: &terraform.State{ + IsBinaryDrivenTest: true, // Always true now + Modules: []*terraform.ModuleState{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.ResourceState{ + "test_resource": { + Primary: &terraform.InstanceState{ + Attributes: map[string]string{ + "test_map_attribute.%": "1", + "test_map_attribute.testkey1": "test-value-1", + }, + }, + }, + }, + }, + }, + }, + key: "test_map_attribute.testkey1", + value: "test-value-1", + }, + "map attribute element value mismatch": { + state: &terraform.State{ + IsBinaryDrivenTest: true, // Always true now + Modules: []*terraform.ModuleState{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.ResourceState{ + "test_resource": { + Primary: &terraform.InstanceState{ + Attributes: map[string]string{ + "test_map_attribute.%": "1", + "test_map_attribute.testkey1": "test-value-1", + }, + }, + }, + }, + }, + }, + }, + key: "test_map_attribute.testkey1", + value: "test-value-2", + expectedError: fmt.Errorf("attribute 'test_map_attribute.testkey1' expected 'test-value-2', got 'test-value-1'"), + }, + "set attribute indexing error": { + state: &terraform.State{ + IsBinaryDrivenTest: true, // Always true now + Modules: []*terraform.ModuleState{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.ResourceState{ + "test_resource": { + Primary: &terraform.InstanceState{ + Attributes: map[string]string{ + "test_set_attribute.#": "1", + "test_set_attribute.101.test_string_attribute": "test-value", + }, + }, + }, + }, + }, + }, + }, + key: "test_set_attribute.101.nonexistent", + value: "test-value", + expectedError: fmt.Errorf("likely indexes into TypeSet"), + }, + "string attribute match": { + state: &terraform.State{ + IsBinaryDrivenTest: true, // Always true now + Modules: []*terraform.ModuleState{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.ResourceState{ + "test_resource": { + Primary: &terraform.InstanceState{ + Attributes: map[string]string{ + "test_string_attribute": "test-value", + }, + }, + }, + }, + }, + }, + }, + key: "test_string_attribute", + value: "test-value", + }, + "string attribute mismatch": { + state: &terraform.State{ + IsBinaryDrivenTest: true, // Always true now + Modules: []*terraform.ModuleState{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.ResourceState{ + "test_resource": { + Primary: &terraform.InstanceState{ + Attributes: map[string]string{ + "test_string_attribute": "test-value", + }, + }, + }, + }, + }, + }, + }, + key: "test_string_attribute", + value: "not-test-value", + expectedError: fmt.Errorf("attribute 'test_string_attribute' expected 'not-test-value', got 'test-value'"), + }, + } + + for name, testCase := range testCases { + name, testCase := name, testCase + + t.Run(name, func(t *testing.T) { + t.Parallel() + + err := TestCheckResourceAttrWith("test_resource", testCase.key, func(v string) error { + if testCase.value != v { + return fmt.Errorf("attribute '%s' expected '%s', got '%s'", testCase.key, testCase.value, v) + } + return nil + })(testCase.state) + + if err != nil { + if testCase.expectedError == nil { + t.Fatalf("expected no error, got: %s", err) + } + + if !strings.Contains(err.Error(), testCase.expectedError.Error()) { + t.Fatalf("expected error %q, got: %s", testCase.expectedError, err) + } + } + + if err == nil && testCase.expectedError != nil { + t.Fatalf("expected error: %s", testCase.expectedError) + } + }) + } +} + func TestTestCheckNoResourceAttr(t *testing.T) { t.Parallel() From f9134b7563d186867115244df257f5c033ff1f41 Mon Sep 17 00:00:00 2001 From: Ivan De Marino Date: Thu, 28 Apr 2022 09:18:07 +0100 Subject: [PATCH 3/6] Adding changelog entry --- .changelog/950.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/950.txt diff --git a/.changelog/950.txt b/.changelog/950.txt new file mode 100644 index 00000000000..6f46a5a5615 --- /dev/null +++ b/.changelog/950.txt @@ -0,0 +1,3 @@ +```release-note:feature +helper/resource: New `TestCheckResourceAttrWith` test helper, that allows for custom checking of attribute values. +``` From b172a8812270e0dfd069bc7a738acc9dedaf26ff Mon Sep 17 00:00:00 2001 From: Ivan De Marino Date: Thu, 28 Apr 2022 13:41:20 +0100 Subject: [PATCH 4/6] Adding examples --- helper/resource/testing_example_test.go | 63 +++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/helper/resource/testing_example_test.go b/helper/resource/testing_example_test.go index 9d7b4630608..9034dae4600 100644 --- a/helper/resource/testing_example_test.go +++ b/helper/resource/testing_example_test.go @@ -1,7 +1,9 @@ package resource_test import ( + "fmt" "regexp" + "strconv" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" ) @@ -209,6 +211,67 @@ func ExampleTestCheckResourceAttr_typeString() { resource.TestCheckResourceAttr("example_thing.test", "example_string_attribute", "test-value") } +func ExampleTestCheckResourceAttrWith_typeString() { + // This function is typically implemented in a TestStep type Check field, + // wrapped with ComposeAggregateTestCheckFunc to combine results from + // multiple checks. + // + // Given the following example configuration: + // + // resource "example_thing" "test" { + // example_string_attribute = "Very long string..." + // } + // + // The following TestCheckResourceAttrWith can be written to assert against + // the expected state values. + // + // NOTE: State value checking is only necessary for Computed attributes, + // as the testing framework will automatically return test failures + // for configured attributes that mismatch the saved state, however + // this configuration and test is shown for illustrative purposes. + + // Verify the attribute value string length is above 1000 + resource.TestCheckResourceAttrWith("example_thing.test", "example_string_attribute", func(value string) error { + if len(value) <= 1000 { + return fmt.Errorf("example_string_attribute should be longer than 1000 characters") + } + return nil + }) +} + +func ExampleTestCheckResourceAttrWith_typeInt() { + // This function is typically implemented in a TestStep type Check field, + // wrapped with ComposeAggregateTestCheckFunc to combine results from + // multiple checks. + // + // Given the following example configuration: + // + // resource "example_thing" "test" { + // example_int_attribute = 10 + // } + // + // The following TestCheckResourceAttrWith can be written to assert against + // the expected state values. + // + // NOTE: State value checking is only necessary for Computed attributes, + // as the testing framework will automatically return test failures + // for configured attributes that mismatch the saved state, however + // this configuration and test is shown for illustrative purposes. + + // Verify the attribute value is an integer, and it's between 5 (included) and 20 (excluded) + resource.TestCheckResourceAttrWith("example_thing.test", "example_string_attribute", func(value string) error { + valueInt, err := strconv.Atoi(value) + if err != nil { + return err + } + + if valueInt < 5 && valueInt >= 20 { + return fmt.Errorf("example error message") + } + return nil + }) +} + func ExampleTestCheckResourceAttrPair() { // This function is typically implemented in a TestStep type Check field, // wrapped with ComposeAggregateTestCheckFunc to combine results from From 9f8bec45e237b78e185922c976c69f47453526db Mon Sep 17 00:00:00 2001 From: Ivan De Marino Date: Thu, 28 Apr 2022 17:18:16 +0100 Subject: [PATCH 5/6] Apply suggestions from code review Co-authored-by: Brian Flad --- .changelog/950.txt | 2 +- helper/resource/testing.go | 8 +++++++- helper/resource/testing_example_test.go | 4 ++-- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/.changelog/950.txt b/.changelog/950.txt index 6f46a5a5615..0c6e05a6b2d 100644 --- a/.changelog/950.txt +++ b/.changelog/950.txt @@ -1,3 +1,3 @@ ```release-note:feature -helper/resource: New `TestCheckResourceAttrWith` test helper, that allows for custom checking of attribute values. +helper/resource: New `TestCheckResourceAttrWith` test helper, that simplifies checking of attribute values via custom functions ``` diff --git a/helper/resource/testing.go b/helper/resource/testing.go index 3bd5a3d0061..24dec911d44 100644 --- a/helper/resource/testing.go +++ b/helper/resource/testing.go @@ -984,7 +984,13 @@ func TestCheckResourceAttrWith(name, key string, f CheckResourceAttrWithFunc) Te return err } - return f(is.Attributes[key]) + err = f(is.Attributes[key]) + + if err != nil { + return fmt.Errorf("%s: Attribute %q value: %w", name, key, err) + } + + return nil }) } diff --git a/helper/resource/testing_example_test.go b/helper/resource/testing_example_test.go index 9034dae4600..e5e88725ef8 100644 --- a/helper/resource/testing_example_test.go +++ b/helper/resource/testing_example_test.go @@ -233,7 +233,7 @@ func ExampleTestCheckResourceAttrWith_typeString() { // Verify the attribute value string length is above 1000 resource.TestCheckResourceAttrWith("example_thing.test", "example_string_attribute", func(value string) error { if len(value) <= 1000 { - return fmt.Errorf("example_string_attribute should be longer than 1000 characters") + return fmt.Errorf("should be longer than 1000 characters") } return nil }) @@ -266,7 +266,7 @@ func ExampleTestCheckResourceAttrWith_typeInt() { } if valueInt < 5 && valueInt >= 20 { - return fmt.Errorf("example error message") + return fmt.Errorf("should be between 5 and 20") } return nil }) From c8afd7bba7204873c8086fae13fa121515e32c5b Mon Sep 17 00:00:00 2001 From: Ivan De Marino Date: Thu, 28 Apr 2022 17:38:03 +0100 Subject: [PATCH 6/6] Providing `TestCheckResourceAttrWith` with a more comprehensive godoc --- helper/resource/testing.go | 44 +++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 8 deletions(-) diff --git a/helper/resource/testing.go b/helper/resource/testing.go index 24dec911d44..45180d2c779 100644 --- a/helper/resource/testing.go +++ b/helper/resource/testing.go @@ -966,13 +966,42 @@ func testCheckResourceAttr(is *terraform.InstanceState, name string, key string, // When this function returns an error, TestCheckResourceAttrWith will fail the check. type CheckResourceAttrWithFunc func(value string) error -// TestCheckResourceAttrWith - as per TestCheckResourceAttr but the attribute value -// checking logic can be customised. +// TestCheckResourceAttrWith ensures a value stored in state for the +// given name and key combination, is checked against a custom logic. +// State value checking is only recommended for testing Computed attributes +// and attribute defaults. +// +// For managed resources, the name parameter is combination of the resource +// type, a period (.), and the name label. The name for the below example +// configuration would be "myprovider_thing.example". +// +// resource "myprovider_thing" "example" { ... } +// +// For data sources, the name parameter is a combination of the keyword "data", +// a period (.), the data source type, a period (.), and the name label. The +// name for the below example configuration would be +// "data.myprovider_thing.example". +// +// data "myprovider_thing" "example" { ... } // -// If a value is found for the given name and key, it is passed to the CheckResourceAttrWithFunc function. -// The CheckResourceAttrWithFunc function can then apply any checking logic: -// should return an error for the check to fail, or `nil` to succeed. -func TestCheckResourceAttrWith(name, key string, f CheckResourceAttrWithFunc) TestCheckFunc { +// The key parameter is an attribute path in Terraform CLI 0.11 and earlier +// "flatmap" syntax. Keys start with the attribute name of a top-level +// attribute. Use the following special key syntax to inspect list, map, and +// set attributes: +// +// - .{NUMBER}: List value at index, e.g. .0 to inspect the first element. +// Use the TestCheckTypeSet* and TestMatchTypeSet* functions instead +// for sets. +// - .{KEY}: Map value at key, e.g. .example to inspect the example key +// value. +// - .#: Number of elements in list or set. +// - .%: Number of elements in map. +// +// The checkValueFunc parameter is a CheckResourceAttrWithFunc, +// and it's provided with the attribute value to apply a custom checking logic, +// if it was found in the state. The function must return an error for the +// check to fail, or `nil` to succeed. +func TestCheckResourceAttrWith(name, key string, checkValueFunc CheckResourceAttrWithFunc) TestCheckFunc { return checkIfIndexesIntoTypeSet(key, func(s *terraform.State) error { is, err := primaryInstanceState(s, name) if err != nil { @@ -984,8 +1013,7 @@ func TestCheckResourceAttrWith(name, key string, f CheckResourceAttrWithFunc) Te return err } - err = f(is.Attributes[key]) - + err = checkValueFunc(is.Attributes[key]) if err != nil { return fmt.Errorf("%s: Attribute %q value: %w", name, key, err) }