From 7006539370f140ecc9facd8cc124b579249eb367 Mon Sep 17 00:00:00 2001 From: Volodymyr Manilo Date: Thu, 20 Apr 2023 22:47:05 +0200 Subject: [PATCH 1/5] add RemoveState option to TestStep --- helper/resource/testing.go | 6 ++++++ helper/resource/testing_config.go | 16 ++++++++++++++++ helper/resource/testing_new.go | 12 ++++++++++++ internal/plugintest/working_dir.go | 11 +++++++++++ 4 files changed, 45 insertions(+) diff --git a/helper/resource/testing.go b/helper/resource/testing.go index 71dbeb921..0f7fd5375 100644 --- a/helper/resource/testing.go +++ b/helper/resource/testing.go @@ -469,6 +469,12 @@ type TestStep struct { // resources in the root module path. Taint []string + // RemoveState list of resource addresses to be removed from state after + // applying config. Be sure to only include this at a step where the referenced + // address will be present in state, as it will fail the test if the resource + // is missing. + RemoveState []string + //--------------------------------------------------------------- // Test modes. One of the following groups of settings must be // set to determine what the test step will do. Ideally we would've diff --git a/helper/resource/testing_config.go b/helper/resource/testing_config.go index 57bc0d8ce..72b0218f6 100644 --- a/helper/resource/testing_config.go +++ b/helper/resource/testing_config.go @@ -27,3 +27,19 @@ func testStepTaint(ctx context.Context, step TestStep, wd *plugintest.WorkingDir } return nil } + +func testStepRemoveState(ctx context.Context, step TestStep, wd *plugintest.WorkingDir) error { + if len(step.RemoveState) == 0 { + return nil + } + + logging.HelperResourceTrace(ctx, fmt.Sprintf("Using TestStep RemoveState: %v", step.RemoveState)) + + for _, p := range step.RemoveState { + err := wd.RemoveState(ctx, p) + if err != nil { + return fmt.Errorf("error remove state resource: %s", err) + } + } + return nil +} diff --git a/helper/resource/testing_new.go b/helper/resource/testing_new.go index da4785f0a..b8e402a22 100644 --- a/helper/resource/testing_new.go +++ b/helper/resource/testing_new.go @@ -328,6 +328,18 @@ func runNewTest(ctx context.Context, t testing.T, c TestCase, helper *plugintest appliedCfg = step.mergedConfig(ctx, c) + if stepNumber > 1 && len(step.RemoveState) > 0 && !step.ImportState && !step.RefreshState { + err := testStepRemoveState(ctx, step, wd) + + if err != nil { + logging.HelperResourceError(ctx, + "TestStep error remove state resources", + map[string]interface{}{logging.KeyError: err}, + ) + t.Fatalf("TestStep %d/%d error remove state resources: %s", stepNumber, len(c.Steps), err) + } + } + logging.HelperResourceDebug(ctx, "Finished TestStep") continue diff --git a/internal/plugintest/working_dir.go b/internal/plugintest/working_dir.go index 9ee2046f2..3b0fea1a8 100644 --- a/internal/plugintest/working_dir.go +++ b/internal/plugintest/working_dir.go @@ -355,6 +355,17 @@ func (wd *WorkingDir) Taint(ctx context.Context, address string) error { return err } +// RemoveState runs terraform state rm +func (wd *WorkingDir) RemoveState(ctx context.Context, address string) error { + logging.HelperResourceTrace(ctx, "Calling Terraform CLI state rm command") + + err := wd.tf.StateRm(context.Background(), address) + + logging.HelperResourceTrace(ctx, "Called Terraform CLI state rm command") + + return err +} + // Refresh runs terraform refresh func (wd *WorkingDir) Refresh(ctx context.Context) error { logging.HelperResourceTrace(ctx, "Calling Terraform CLI refresh command") From 4e0a77f41d10d2d8785b1915b395fa09885e65ae Mon Sep 17 00:00:00 2001 From: Volodymyr Manilo Date: Sun, 18 Jun 2023 23:20:09 +0200 Subject: [PATCH 2/5] added validation and tests --- helper/resource/testing_config.go | 16 ------ helper/resource/testing_new.go | 39 +++++++++++-- helper/resource/testing_new_remove_state.go | 29 ++++++++++ .../resource/testing_new_remove_state_test.go | 56 +++++++++++++++++++ helper/resource/teststep_validate.go | 30 +++++++++- helper/resource/teststep_validate_test.go | 32 ++++++++++- 6 files changed, 179 insertions(+), 23 deletions(-) create mode 100644 helper/resource/testing_new_remove_state.go create mode 100644 helper/resource/testing_new_remove_state_test.go diff --git a/helper/resource/testing_config.go b/helper/resource/testing_config.go index 72b0218f6..57bc0d8ce 100644 --- a/helper/resource/testing_config.go +++ b/helper/resource/testing_config.go @@ -27,19 +27,3 @@ func testStepTaint(ctx context.Context, step TestStep, wd *plugintest.WorkingDir } return nil } - -func testStepRemoveState(ctx context.Context, step TestStep, wd *plugintest.WorkingDir) error { - if len(step.RemoveState) == 0 { - return nil - } - - logging.HelperResourceTrace(ctx, fmt.Sprintf("Using TestStep RemoveState: %v", step.RemoveState)) - - for _, p := range step.RemoveState { - err := wd.RemoveState(ctx, p) - if err != nil { - return fmt.Errorf("error remove state resource: %s", err) - } - } - return nil -} diff --git a/helper/resource/testing_new.go b/helper/resource/testing_new.go index b8e402a22..1456a881e 100644 --- a/helper/resource/testing_new.go +++ b/helper/resource/testing_new.go @@ -328,15 +328,46 @@ func runNewTest(ctx context.Context, t testing.T, c TestCase, helper *plugintest appliedCfg = step.mergedConfig(ctx, c) - if stepNumber > 1 && len(step.RemoveState) > 0 && !step.ImportState && !step.RefreshState { - err := testStepRemoveState(ctx, step, wd) + logging.HelperResourceDebug(ctx, "Finished TestStep") + + continue + } + + if len(step.RemoveState) > 0 { + logging.HelperResourceTrace(ctx, "TestStep is RemoveState mode") + + err := testStepRemoveState(ctx, step, wd) + + if step.ExpectError != nil { + logging.HelperResourceDebug(ctx, "Checking TestStep ExpectError") + + if err == nil { + logging.HelperResourceError(ctx, + "Expected an error but got none", + ) + t.Fatalf("Step %d/%d, expected an error but got none", stepNumber, len(c.Steps)) + } + if !step.ExpectError.MatchString(err.Error()) { + logging.HelperResourceError(ctx, + fmt.Sprintf("Expected an error with pattern (%s)", step.ExpectError.String()), + map[string]interface{}{logging.KeyError: err}, + ) + t.Fatalf("Step %d/%d, expected an error with pattern, no match on: %s", stepNumber, len(c.Steps), err) + } + } else { + if err != nil && c.ErrorCheck != nil { + logging.HelperResourceDebug(ctx, "Calling TestCase ErrorCheck") + + err = c.ErrorCheck(err) + logging.HelperResourceDebug(ctx, "Called TestCase ErrorCheck") + } if err != nil { logging.HelperResourceError(ctx, - "TestStep error remove state resources", + "Unexpected error", map[string]interface{}{logging.KeyError: err}, ) - t.Fatalf("TestStep %d/%d error remove state resources: %s", stepNumber, len(c.Steps), err) + t.Fatalf("Step %d/%d error: %s", stepNumber, len(c.Steps), err) } } diff --git a/helper/resource/testing_new_remove_state.go b/helper/resource/testing_new_remove_state.go new file mode 100644 index 000000000..026936cfb --- /dev/null +++ b/helper/resource/testing_new_remove_state.go @@ -0,0 +1,29 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package resource + +import ( + "context" + "fmt" + + "github.com/hashicorp/terraform-plugin-testing/internal/logging" + + "github.com/hashicorp/terraform-plugin-testing/internal/plugintest" +) + +func testStepRemoveState(ctx context.Context, step TestStep, wd *plugintest.WorkingDir) error { + if len(step.RemoveState) == 0 { + return nil + } + + logging.HelperResourceTrace(ctx, fmt.Sprintf("Using TestStep RemoveState: %v", step.RemoveState)) + + for _, p := range step.RemoveState { + err := wd.RemoveState(ctx, p) + if err != nil { + return fmt.Errorf("error remove state resource: %s", err) + } + } + return nil +} diff --git a/helper/resource/testing_new_remove_state_test.go b/helper/resource/testing_new_remove_state_test.go new file mode 100644 index 000000000..87ae19aa6 --- /dev/null +++ b/helper/resource/testing_new_remove_state_test.go @@ -0,0 +1,56 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package resource + +import ( + "regexp" + "testing" +) + +func Test_RemoveState_Ok(t *testing.T) { + t.Parallel() + + Test(t, TestCase{ + ExternalProviders: map[string]ExternalProvider{ + "random": { + Source: "registry.terraform.io/hashicorp/random", + }, + }, + Steps: []TestStep{ + { + Config: `resource "random_string" "one" { + length = 2 + }`, + Check: TestCheckResourceAttr("random_string.one", "length", "2"), + }, + { + RemoveState: []string{"random_string.one"}, + Check: TestCheckNoResourceAttr("random_string.one", "length"), + }, + }, + }) +} + +func Test_RemoveState_Error(t *testing.T) { + t.Parallel() + + Test(t, TestCase{ + ExternalProviders: map[string]ExternalProvider{ + "random": { + Source: "registry.terraform.io/hashicorp/random", + }, + }, + Steps: []TestStep{ + { + Config: `resource "random_string" "one" { + length = 2 + }`, + }, + { + RemoveState: []string{"resource.other"}, + ExpectError: regexp.MustCompile("Error: Invalid target address"), + }, + }, + }) +} diff --git a/helper/resource/teststep_validate.go b/helper/resource/teststep_validate.go index b1da44ad4..d71a71826 100644 --- a/helper/resource/teststep_validate.go +++ b/helper/resource/teststep_validate.go @@ -67,8 +67,8 @@ func (s TestStep) validate(ctx context.Context, req testStepValidateRequest) err logging.HelperResourceTrace(ctx, "Validating TestStep") - if s.Config == "" && !s.ImportState && !s.RefreshState { - err := fmt.Errorf("TestStep missing Config or ImportState or RefreshState") + if s.Config == "" && !s.ImportState && !s.RefreshState && len(s.RemoveState) == 0 { + err := fmt.Errorf("TestStep missing Config or ImportState or RefreshState or RemoveState") logging.HelperResourceError(ctx, "TestStep validation error", map[string]interface{}{logging.KeyError: err}) return err } @@ -97,6 +97,32 @@ func (s TestStep) validate(ctx context.Context, req testStepValidateRequest) err return err } + if len(s.RemoveState) > 0 { + if req.StepNumber == 1 { + err := fmt.Errorf("TestStep cannot have RemoveState as first step") + logging.HelperResourceError(ctx, "TestStep validation error", map[string]interface{}{logging.KeyError: err}) + return err + } + + if s.ImportState { + err := fmt.Errorf("TestStep cannot have RemoveState and ImportState in same step") + logging.HelperResourceError(ctx, "TestStep validation error", map[string]interface{}{logging.KeyError: err}) + return err + } + + if s.RefreshState { + err := fmt.Errorf("TestStep cannot have RemoveState and RefreshState in same step") + logging.HelperResourceError(ctx, "TestStep validation error", map[string]interface{}{logging.KeyError: err}) + return err + } + + if s.Config != "" { + err := fmt.Errorf("TestStep cannot have RemoveState and Config") + logging.HelperResourceError(ctx, "TestStep validation error", map[string]interface{}{logging.KeyError: err}) + return err + } + } + for name := range s.ExternalProviders { if _, ok := s.ProviderFactories[name]; ok { err := fmt.Errorf("TestStep provider %q set in both ExternalProviders and ProviderFactories", name) diff --git a/helper/resource/teststep_validate_test.go b/helper/resource/teststep_validate_test.go index cf910c576..11fbc3fb6 100644 --- a/helper/resource/teststep_validate_test.go +++ b/helper/resource/teststep_validate_test.go @@ -88,7 +88,7 @@ func TestTestStepValidate(t *testing.T) { "config-and-importstate-and-refreshstate-missing": { testStep: TestStep{}, testStepValidateRequest: testStepValidateRequest{}, - expectedError: fmt.Errorf("TestStep missing Config or ImportState or RefreshState"), + expectedError: fmt.Errorf("TestStep missing Config or ImportState or RefreshState or RemoveState"), }, "config-and-refreshstate-both-set": { testStep: TestStep{ @@ -243,6 +243,36 @@ func TestTestStepValidate(t *testing.T) { testStepValidateRequest: testStepValidateRequest{TestCaseHasProviders: true}, expectedError: errors.New("TestStep RefreshPlanChecks.PostRefresh must only be specified with RefreshState"), }, + "removestate-first-step": { + testStep: TestStep{ + RemoveState: []string{"resource.name"}, + }, + testStepValidateRequest: testStepValidateRequest{ + StepNumber: 1, + }, + expectedError: fmt.Errorf("TestStep cannot have RemoveState as first step"), + }, + "removestate-and-importstate": { + testStep: TestStep{ + ImportState: true, + RemoveState: []string{"resource.name"}, + }, + expectedError: fmt.Errorf("TestStep cannot have RemoveState and ImportState in same step"), + }, + "removestate-and-refreshstate": { + testStep: TestStep{ + RefreshState: true, + RemoveState: []string{"resource.name"}, + }, + expectedError: fmt.Errorf("TestStep cannot have RemoveState and RefreshState in same step"), + }, + "removestate-and-config": { + testStep: TestStep{ + Config: "# not empty", + RemoveState: []string{"resource.name"}, + }, + expectedError: fmt.Errorf("TestStep cannot have RemoveState and Config"), + }, } for name, test := range tests { From 8ab6d9b0ff9448bf6c2814dede16c4c9ebf2aa90 Mon Sep 17 00:00:00 2001 From: Volodymyr Manilo Date: Fri, 14 Jul 2023 06:57:37 +0200 Subject: [PATCH 3/5] - updated docs - added changelog --- .../ENHANCEMENTS-20230714-065551.yaml | 6 ++++++ helper/resource/testing.go | 20 +++++++++++++------ .../testing/acceptance-tests/teststep.mdx | 13 +++++++++++- 3 files changed, 32 insertions(+), 7 deletions(-) create mode 100644 .changes/unreleased/ENHANCEMENTS-20230714-065551.yaml diff --git a/.changes/unreleased/ENHANCEMENTS-20230714-065551.yaml b/.changes/unreleased/ENHANCEMENTS-20230714-065551.yaml new file mode 100644 index 000000000..3b99bfd55 --- /dev/null +++ b/.changes/unreleased/ENHANCEMENTS-20230714-065551.yaml @@ -0,0 +1,6 @@ +kind: ENHANCEMENTS +body: Added `RemoveState` - new testing mode, which allows to delete listed resources + from state, to prevent their automatic destroying. +time: 2023-07-14T06:55:51.777536+02:00 +custom: + Issue: "85" diff --git a/helper/resource/testing.go b/helper/resource/testing.go index af28b6227..c78368db5 100644 --- a/helper/resource/testing.go +++ b/helper/resource/testing.go @@ -476,12 +476,6 @@ type TestStep struct { // resources in the root module path. Taint []string - // RemoveState list of resource addresses to be removed from state after - // applying config. Be sure to only include this at a step where the referenced - // address will be present in state, as it will fail the test if the resource - // is missing. - RemoveState []string - //--------------------------------------------------------------- // Test modes. One of the following groups of settings must be // set to determine what the test step will do. Ideally we would've @@ -490,6 +484,20 @@ type TestStep struct { // to run based on what settings below are set. //--------------------------------------------------------------- + //--------------------------------------------------------------- + // RemoveState testing + //--------------------------------------------------------------- + + // RemoveState is a list of resource addresses to be removed from state after + // applying config. Be sure to only include this at a step where the referenced + // address will be present in state, as it will fail the test if the resource + // is missing. + // + // Usage: + // - RemoveState should not be present in the same TestStep as Config: "not empty", ImportState or RefreshState. + // - RemoveState should not be present in the first TestStep similar to RefreshState. + RemoveState []string + //--------------------------------------------------------------- // Plan, Apply testing //--------------------------------------------------------------- diff --git a/website/docs/plugin/testing/acceptance-tests/teststep.mdx b/website/docs/plugin/testing/acceptance-tests/teststep.mdx index 0c66a9ff7..3dd3fb363 100644 --- a/website/docs/plugin/testing/acceptance-tests/teststep.mdx +++ b/website/docs/plugin/testing/acceptance-tests/teststep.mdx @@ -15,7 +15,7 @@ under test. ## Test Modes Terraform's test framework facilitates three distinct modes of acceptance tests, -_Lifecycle (config)_, _Import_ and _Refresh_. +_Lifecycle (config)_, _Import_, _Refresh_ and _RemoveState_. _Lifecycle (config)_ mode is the most common mode, and is used for testing plugins by providing one or more configuration files with the same logic as would be used @@ -29,6 +29,17 @@ _Refresh_ mode is used for testing resource functionality to refresh existing infrastructure, using the same logic as would be used when running `terraform refresh`. +_RemoveState_ mode is used for testing resources that should not be deleted +at the end of the `TestCase`. It allows to clean up state from particular resources +that should not be automatically destroyed. For example, you want to write test +for existing resource: you import it, do some test steps and at the end of all steps +you need to keep, preventing automatic destroying. + +Usage: +- should not be present in the same `TestStep` as `Config: "not empty"`, `ImportState` or `RefreshState`. +- should not be present in the first `TestStep` similar to `RefreshState`. + + An acceptance test's mode is implicitly determined by the fields provided in the `TestStep` definition. The applicable fields are defined in the [TestStep Reference API](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-testing/helper/resource#TestStep). From eeb042ec5d3019baccc93f58749ea806bd29ed97 Mon Sep 17 00:00:00 2001 From: Volodymyr Manilo <35466116+vmanilo@users.noreply.github.com> Date: Tue, 25 Jul 2023 22:35:28 +0200 Subject: [PATCH 4/5] Update website/docs/plugin/testing/acceptance-tests/teststep.mdx Co-authored-by: Austin Valle --- website/docs/plugin/testing/acceptance-tests/teststep.mdx | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/website/docs/plugin/testing/acceptance-tests/teststep.mdx b/website/docs/plugin/testing/acceptance-tests/teststep.mdx index 3dd3fb363..2e2335cb5 100644 --- a/website/docs/plugin/testing/acceptance-tests/teststep.mdx +++ b/website/docs/plugin/testing/acceptance-tests/teststep.mdx @@ -30,10 +30,9 @@ infrastructure, using the same logic as would be used when running `terraform refresh`. _RemoveState_ mode is used for testing resources that should not be deleted -at the end of the `TestCase`. It allows to clean up state from particular resources -that should not be automatically destroyed. For example, you want to write test -for existing resource: you import it, do some test steps and at the end of all steps -you need to keep, preventing automatic destroying. +at the end of the `TestCase`; providing a mechanism to remove said resources from state +before the final `destroy` command is executed. For example, this can be used with `Import`, +to test resources that are time-consuming or expensive to destroy and re-create for each test. Usage: - should not be present in the same `TestStep` as `Config: "not empty"`, `ImportState` or `RefreshState`. From 8f95c4c2e43b7f27b4d69d2c1ba3b370b8910f20 Mon Sep 17 00:00:00 2001 From: Volodymyr Manilo <35466116+vmanilo@users.noreply.github.com> Date: Tue, 25 Jul 2023 22:35:55 +0200 Subject: [PATCH 5/5] Update website/docs/plugin/testing/acceptance-tests/teststep.mdx Co-authored-by: Austin Valle --- website/docs/plugin/testing/acceptance-tests/teststep.mdx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/website/docs/plugin/testing/acceptance-tests/teststep.mdx b/website/docs/plugin/testing/acceptance-tests/teststep.mdx index 2e2335cb5..196c1b5fd 100644 --- a/website/docs/plugin/testing/acceptance-tests/teststep.mdx +++ b/website/docs/plugin/testing/acceptance-tests/teststep.mdx @@ -35,8 +35,8 @@ before the final `destroy` command is executed. For example, this can be used wi to test resources that are time-consuming or expensive to destroy and re-create for each test. Usage: -- should not be present in the same `TestStep` as `Config: "not empty"`, `ImportState` or `RefreshState`. -- should not be present in the first `TestStep` similar to `RefreshState`. +- Should not be present in the same `TestStep` as `Config: "not empty"`, `ImportState` or `RefreshState`. +- Should not be present in the first `TestStep` similar to `RefreshState`. An acceptance test's mode is implicitly determined by the fields provided in the