From b6ed17365dd7353fc166892b86b59b92020c7972 Mon Sep 17 00:00:00 2001 From: Benjamin Bennett Date: Mon, 10 Oct 2022 11:22:17 +0100 Subject: [PATCH 1/3] Adding test coverage for changes in https://github.com/hashicorp/terraform-plugin-sdk/pull/1074 (#1066) --- helper/resource/teststep_providers_test.go | 249 ++++++++++++++++++--- 1 file changed, 216 insertions(+), 33 deletions(-) diff --git a/helper/resource/teststep_providers_test.go b/helper/resource/teststep_providers_test.go index 92ddbe0875..09ed0bfb31 100644 --- a/helper/resource/teststep_providers_test.go +++ b/helper/resource/teststep_providers_test.go @@ -1076,6 +1076,7 @@ func TestTest_TestStep_Taint(t *testing.T) { } } +//nolint:unparam func extractResourceAttr(resourceName string, attributeName string, attributeValue *string) TestCheckFunc { return func(s *terraform.State) error { rs, ok := s.RootModule().Resources[resourceName] @@ -1253,6 +1254,8 @@ func TestTest_TestStep_ProviderFactories_To_ExternalProviders(t *testing.T) { } func TestTest_TestStep_ProviderFactories_Import_Inline(t *testing.T) { + id := "none" + t.Parallel() Test(t, TestCase{ @@ -1316,9 +1319,8 @@ func TestTest_TestStep_ProviderFactories_Import_Inline(t *testing.T) { ImportStateId: "Z=:cbrJE?Ltg", ImportStatePersist: true, ImportStateCheck: composeImportStateCheck( - testCheckResourceAttrInstanceState("id", "none"), - testCheckResourceAttrInstanceState("result", "Z=:cbrJE?Ltg"), - testCheckResourceAttrInstanceState("length", "12"), + testCheckResourceAttrInstanceState(&id, "result", "Z=:cbrJE?Ltg"), + testCheckResourceAttrInstanceState(&id, "length", "12"), ), }, }, @@ -1391,7 +1393,7 @@ func TestTest_TestStep_ProviderFactories_Import_Inline_WithPersistMatch(t *testi ImportStateId: "Z=:cbrJE?Ltg", ImportStatePersist: true, ImportStateCheck: composeImportStateCheck( - testExtractResourceAttrInstanceState("result", &result1), + testExtractResourceAttrInstanceState("none", "result", &result1), ), }, { @@ -1484,6 +1486,8 @@ func TestTest_TestStep_ProviderFactories_Import_Inline_WithoutPersist(t *testing } func TestTest_TestStep_ProviderFactories_Import_External(t *testing.T) { + id := "none" + t.Parallel() Test(t, TestCase{ @@ -1500,9 +1504,8 @@ func TestTest_TestStep_ProviderFactories_Import_External(t *testing.T) { ImportStateId: "Z=:cbrJE?Ltg", ImportStatePersist: true, ImportStateCheck: composeImportStateCheck( - testCheckResourceAttrInstanceState("id", "none"), - testCheckResourceAttrInstanceState("result", "Z=:cbrJE?Ltg"), - testCheckResourceAttrInstanceState("length", "12"), + testCheckResourceAttrInstanceState(&id, "result", "Z=:cbrJE?Ltg"), + testCheckResourceAttrInstanceState(&id, "length", "12"), ), }, }, @@ -1528,7 +1531,7 @@ func TestTest_TestStep_ProviderFactories_Import_External_WithPersistMatch(t *tes ImportStateId: "Z=:cbrJE?Ltg", ImportStatePersist: true, ImportStateCheck: composeImportStateCheck( - testExtractResourceAttrInstanceState("result", &result1), + testExtractResourceAttrInstanceState("none", "result", &result1), ), }, { @@ -1561,7 +1564,7 @@ func TestTest_TestStep_ProviderFactories_Import_External_WithoutPersistNonMatch( ImportStateId: "Z=:cbrJE?Ltg", ImportStatePersist: false, ImportStateCheck: composeImportStateCheck( - testExtractResourceAttrInstanceState("result", &result1), + testExtractResourceAttrInstanceState("none", "result", &result1), ), }, { @@ -1714,6 +1717,188 @@ func TestTest_TestStep_ProviderFactories_RefreshWithPlanModifier_Inline(t *testi }) } +func TestTest_TestStep_ProviderFactories_Import_Inline_With_Data_Source(t *testing.T) { + var id string + + t.Parallel() + + Test(t, TestCase{ + ProviderFactories: map[string]func() (*schema.Provider, error){ + "http": func() (*schema.Provider, error) { //nolint:unparam // required signature + return &schema.Provider{ + DataSourcesMap: map[string]*schema.Resource{ + "http": { + ReadContext: func(ctx context.Context, d *schema.ResourceData, i interface{}) (diags diag.Diagnostics) { + url := d.Get("url").(string) + + responseHeaders := map[string]string{ + "headerOne": "one", + "headerTwo": "two", + "headerThree": "three", + "headerFour": "four", + } + if err := d.Set("response_headers", responseHeaders); err != nil { + return append(diags, diag.Errorf("Error setting HTTP response headers: %s", err)...) + } + + d.SetId(url) + + return diags + }, + Schema: map[string]*schema.Schema{ + "url": { + Type: schema.TypeString, + Required: true, + }, + "response_headers": { + Type: schema.TypeMap, + Computed: true, + Elem: &schema.Schema{ + Type: schema.TypeString, + }, + }, + }, + }, + }, + }, nil + }, + "random": func() (*schema.Provider, error) { //nolint:unparam // required signature + return &schema.Provider{ + ResourcesMap: map[string]*schema.Resource{ + "random_string": { + CreateContext: func(_ context.Context, d *schema.ResourceData, _ interface{}) diag.Diagnostics { + d.SetId("none") + err := d.Set("length", 4) + if err != nil { + panic(err) + } + err = d.Set("result", "none") + if err != nil { + panic(err) + } + return nil + }, + DeleteContext: func(_ context.Context, _ *schema.ResourceData, _ interface{}) diag.Diagnostics { + return nil + }, + ReadContext: func(_ context.Context, _ *schema.ResourceData, _ interface{}) diag.Diagnostics { + return nil + }, + Schema: map[string]*schema.Schema{ + "length": { + Required: true, + ForceNew: true, + Type: schema.TypeInt, + }, + "result": { + Type: schema.TypeString, + Computed: true, + Sensitive: true, + }, + + "id": { + Computed: true, + Type: schema.TypeString, + }, + }, + Importer: &schema.ResourceImporter{ + StateContext: func(ctx context.Context, d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) { + val := d.Id() + + d.SetId(val) + + err := d.Set("result", val) + if err != nil { + panic(err) + } + + err = d.Set("length", len(val)) + if err != nil { + panic(err) + } + + return []*schema.ResourceData{d}, nil + }, + }, + }, + }, + }, nil + }, + }, + Steps: []TestStep{ + { + Config: `data "http" "example" { + url = "https://checkpoint-api.hashicorp.com/v1/check/terraform" + } + + resource "random_string" "example" { + length = length(data.http.example.response_headers) + }`, + Check: extractResourceAttr("random_string.example", "id", &id), + }, + { + Config: `data "http" "example" { + url = "https://checkpoint-api.hashicorp.com/v1/check/terraform" + } + + resource "random_string" "example" { + length = length(data.http.example.response_headers) + }`, + ResourceName: "random_string.example", + ImportState: true, + ImportStateCheck: composeImportStateCheck( + testCheckResourceAttrInstanceState(&id, "length", "4"), + ), + ImportStateVerify: true, + }, + }, + }) +} + +func TestTest_TestStep_ProviderFactories_Import_External_With_Data_Source(t *testing.T) { + var id string + + t.Parallel() + + Test(t, TestCase{ + ExternalProviders: map[string]ExternalProvider{ + "http": { + Source: "registry.terraform.io/hashicorp/http", + }, + "random": { + Source: "registry.terraform.io/hashicorp/random", + }, + }, + Steps: []TestStep{ + { + Config: `data "http" "example" { + url = "https://checkpoint-api.hashicorp.com/v1/check/terraform" + } + + resource "random_string" "example" { + length = length(data.http.example.response_headers) + }`, + Check: extractResourceAttr("random_string.example", "id", &id), + }, + { + Config: `data "http" "example" { + url = "https://checkpoint-api.hashicorp.com/v1/check/terraform" + } + + resource "random_string" "example" { + length = length(data.http.example.response_headers) + }`, + ResourceName: "random_string.example", + ImportState: true, + ImportStateCheck: composeImportStateCheck( + testCheckResourceAttrInstanceState(&id, "length", "12"), + ), + ImportStateVerify: true, + }, + }, + }) +} + func setTimeForTest(t time.Time) func() { return func() { getTimeForTest = func() time.Time { @@ -1738,43 +1923,41 @@ func composeImportStateCheck(fs ...ImportStateCheckFunc) ImportStateCheckFunc { } } -func testExtractResourceAttrInstanceState(attributeName string, attributeValue *string) ImportStateCheckFunc { +func testExtractResourceAttrInstanceState(id, attributeName string, attributeValue *string) ImportStateCheckFunc { return func(is []*terraform.InstanceState) error { - if len(is) != 1 { - return fmt.Errorf("unexpected number of instance states: %d", len(is)) - } + for _, v := range is { + if v.ID != id { + continue + } - s := is[0] + if attrVal, ok := v.Attributes[attributeName]; ok { + *attributeValue = attrVal - attrValue, ok := s.Attributes[attributeName] - if !ok { - return fmt.Errorf("attribute %s not found in instance state", attributeName) + return nil + } } - *attributeValue = attrValue - - return nil + return fmt.Errorf("attribute %s not found in instance state", attributeName) } } -func testCheckResourceAttrInstanceState(attributeName, attributeValue string) ImportStateCheckFunc { +func testCheckResourceAttrInstanceState(id *string, attributeName, attributeValue string) ImportStateCheckFunc { return func(is []*terraform.InstanceState) error { - if len(is) != 1 { - return fmt.Errorf("unexpected number of instance states: %d", len(is)) - } - - s := is[0] + for _, v := range is { + if v.ID != *id { + continue + } - attrVal, ok := s.Attributes[attributeName] - if !ok { - return fmt.Errorf("attribute %s found in instance state", attributeName) - } + if attrVal, ok := v.Attributes[attributeName]; ok { + if attrVal != attributeValue { + return fmt.Errorf("expected: %s got: %s", attributeValue, attrVal) + } - if attrVal != attributeValue { - return fmt.Errorf("expected: %s got: %s", attributeValue, attrVal) + return nil + } } - return nil + return fmt.Errorf("attribute %s not found in instance state", attributeName) } } From 86ba558c0c0f5bea750d9609b09fce64a657529f Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Mon, 3 Oct 2022 18:12:39 -0700 Subject: [PATCH 2/3] Skips data sources from both original state and imported state --- helper/resource/testing_new_import_state.go | 27 +++++++++++++-------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/helper/resource/testing_new_import_state.go b/helper/resource/testing_new_import_state.go index fc4ebc9cb0..17b4c2add4 100644 --- a/helper/resource/testing_new_import_state.go +++ b/helper/resource/testing_new_import_state.go @@ -158,20 +158,27 @@ func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest if step.ImportStateVerify { logging.HelperResourceTrace(ctx, "Using TestStep ImportStateVerify") - newResources := importState.RootModule().Resources - oldResources := state.RootModule().Resources + // Ensure that we do not match against data sources as they + // cannot be imported and are not what we want to verify. + // Mode is not present in ResourceState so we use the + // stringified ResourceStateKey for comparison. + newResources := make(map[string]*terraform.ResourceState) + for k, v := range importState.RootModule().Resources { + if !strings.HasPrefix(k, "data.") { + newResources[k] = v + } + } + oldResources := make(map[string]*terraform.ResourceState) + for k, v := range state.RootModule().Resources { + if !strings.HasPrefix(k, "data.") { + oldResources[k] = v + } + } for _, r := range newResources { // Find the existing resource var oldR *terraform.ResourceState - for r2Key, r2 := range oldResources { - // Ensure that we do not match against data sources as they - // cannot be imported and are not what we want to verify. - // Mode is not present in ResourceState so we use the - // stringified ResourceStateKey for comparison. - if strings.HasPrefix(r2Key, "data.") { - continue - } + for _, r2 := range oldResources { if r2.Primary != nil && r2.Primary.ID == r.Primary.ID && r2.Type == r.Type && r2.Provider == r.Provider { oldR = r2 From 95a42a4455a699f34b3da77e5314b38ee448ccd7 Mon Sep 17 00:00:00 2001 From: Benjamin Bennett Date: Thu, 13 Oct 2022 05:25:15 +0100 Subject: [PATCH 3/3] Adding changelog entry (#1066) --- .changelog/1077.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/1077.txt diff --git a/.changelog/1077.txt b/.changelog/1077.txt new file mode 100644 index 0000000000..d79052b988 --- /dev/null +++ b/.changelog/1077.txt @@ -0,0 +1,3 @@ +```release-note:bug +helper/resource: Fixed `TestStep` type `ImportStateVerify` field so that it only matches against resources following a change in behaviour in Terraform 1.3 that imports both resources and their dependent data sources +```