From f256ef9a9a971628b2ab8a0c80c22f84747539fe Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Wed, 29 Nov 2023 20:27:20 +0100 Subject: [PATCH] helper/resource: Refresh during plan rather than separately and remove state shimming when possible (#223) Reference: https://github.com/hashicorp/terraform-plugin-sdk/pull/515 Reference: https://github.com/hashicorp/terraform-plugin-testing/issues/194 Reference: https://github.com/hashicorp/terraform-plugin-testing/issues/222 Reference: https://developer.hashicorp.com/terraform/cli/commands/refresh This change replaces the usage of explicit refresh commands by instead calling the next command with the relevant refresh flags. Separating the refresh from plan was previously done with the idea that the testing logic may introduce further assertion hooks based on that explicit refresh. Over time though, the Terraform recommendations for refreshing and planning have solidified around always recommending to use a plan with behavior changing flags as necessary. Even the Terraform CLI implementation of the refresh command itself has been shifted to call a plan with special flags. This should reduce testing times decently as it means that the testing logic will skip spinning up and tearing down any relevant providers two times per `TestStep` and whatever time would be associated with Terraform CLI loading for those commands. Comparing the testing performed across this Go module: Prior: `TF_ACC=1 go test -count=1 ./... 111.07s user 59.37s system 233% cpu 1:12.99 total` Now: `TF_ACC=1 go test -count=1 ./... 90.81s user 48.25s system 229% cpu 1:00.54 total` The separated refresh also prevented `PreApply` plan checks from catching certain situations because the refreshed state could cause the plan to be missing changes. --- .../ENHANCEMENTS-20231122-115402.yaml | 6 ++ helper/resource/testing_new_config.go | 102 ++++++++++-------- internal/plugintest/working_dir.go | 37 +------ 3 files changed, 68 insertions(+), 77 deletions(-) create mode 100644 .changes/unreleased/ENHANCEMENTS-20231122-115402.yaml diff --git a/.changes/unreleased/ENHANCEMENTS-20231122-115402.yaml b/.changes/unreleased/ENHANCEMENTS-20231122-115402.yaml new file mode 100644 index 000000000..078331d1b --- /dev/null +++ b/.changes/unreleased/ENHANCEMENTS-20231122-115402.yaml @@ -0,0 +1,6 @@ +kind: ENHANCEMENTS +body: 'helper/resource: Removed separate refresh commands, which increases testing + performance' +time: 2023-11-22T11:54:02.542726-05:00 +custom: + Issue: "223" diff --git a/helper/resource/testing_new_config.go b/helper/resource/testing_new_config.go index 5df394d94..697f71049 100644 --- a/helper/resource/testing_new_config.go +++ b/helper/resource/testing_new_config.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" + "github.com/hashicorp/terraform-exec/tfexec" tfjson "github.com/hashicorp/terraform-json" "github.com/mitchellh/go-testing-interface" @@ -80,15 +81,6 @@ func testStepNewConfig(ctx context.Context, t testing.T, c TestCase, wd *plugint return fmt.Errorf("Error setting config: %w", err) } - // require a refresh before applying - // failing to do this will result in data sources not being updated - err = runProviderCommand(ctx, t, func() error { - return wd.Refresh(ctx) - }, wd, providers) - if err != nil { - return fmt.Errorf("Error running pre-apply refresh: %w", err) - } - // If this step is a PlanOnly step, skip over this first Plan and // subsequent Apply, and use the follow-up Plan that checks for // permadiffs @@ -97,10 +89,11 @@ func testStepNewConfig(ctx context.Context, t testing.T, c TestCase, wd *plugint // Plan! err := runProviderCommand(ctx, t, func() error { + var opts []tfexec.PlanOption if step.Destroy { - return wd.CreateDestroyPlan(ctx) + opts = append(opts, tfexec.Destroy(true)) } - return wd.CreatePlan(ctx) + return wd.CreatePlan(ctx, opts...) }, wd, providers) if err != nil { return fmt.Errorf("Error running pre-apply plan: %w", err) @@ -128,15 +121,35 @@ func testStepNewConfig(ctx context.Context, t testing.T, c TestCase, wd *plugint // that the destroy steps can verify their behavior in the // check function var stateBeforeApplication *terraform.State - err = runProviderCommand(ctx, t, func() error { - stateBeforeApplication, err = getState(ctx, t, wd) + + if step.Check != nil && step.Destroy { + // Refresh the state before shimming it for destroy checks later. + // This re-implements previously existing test step logic for the + // specific situation that a provider developer has applied a + // resource with a previous schema version and is destroying it with + // a provider that has a newer schema version. Without this refresh + // the shim logic will return an error such as: + // + // Failed to marshal state to json: schema version 0 for null_resource.test in state does not match version 1 from the provider + err := runProviderCommand(ctx, t, func() error { + return wd.Refresh(ctx) + }, wd, providers) + if err != nil { - return err + return fmt.Errorf("Error running pre-apply refresh: %w", err) + } + + err = runProviderCommand(ctx, t, func() error { + stateBeforeApplication, err = getState(ctx, t, wd) + if err != nil { + return err + } + return nil + }, wd, providers) + + if err != nil { + return fmt.Errorf("Error retrieving pre-apply state: %w", err) } - return nil - }, wd, providers) - if err != nil { - return fmt.Errorf("Error retrieving pre-apply state: %w", err) } // Apply the diff, creating real resources @@ -150,19 +163,6 @@ func testStepNewConfig(ctx context.Context, t testing.T, c TestCase, wd *plugint return fmt.Errorf("Error running apply: %w", err) } - // Get the new state - var state *terraform.State - err = runProviderCommand(ctx, t, func() error { - state, err = getState(ctx, t, wd) - if err != nil { - return err - } - return nil - }, wd, providers) - if err != nil { - return fmt.Errorf("Error retrieving state after apply: %w", err) - } - // Run any configured checks if step.Check != nil { logging.HelperResourceTrace(ctx, "Using TestStep Check") @@ -172,6 +172,20 @@ func testStepNewConfig(ctx context.Context, t testing.T, c TestCase, wd *plugint return fmt.Errorf("Check failed: %w", err) } } else { + var state *terraform.State + + err := runProviderCommand(ctx, t, func() error { + state, err = getState(ctx, t, wd) + if err != nil { + return err + } + return nil + }, wd, providers) + + if err != nil { + return fmt.Errorf("Error retrieving state after apply: %w", err) + } + if err := step.Check(state); err != nil { return fmt.Errorf("Check failed: %w", err) } @@ -184,10 +198,13 @@ func testStepNewConfig(ctx context.Context, t testing.T, c TestCase, wd *plugint // do a plan err = runProviderCommand(ctx, t, func() error { + opts := []tfexec.PlanOption{ + tfexec.Refresh(false), + } if step.Destroy { - return wd.CreateDestroyPlan(ctx) + opts = append(opts, tfexec.Destroy(true)) } - return wd.CreatePlan(ctx) + return wd.CreatePlan(ctx, opts...) }, wd, providers) if err != nil { return fmt.Errorf("Error running post-apply plan: %w", err) @@ -224,22 +241,17 @@ func testStepNewConfig(ctx context.Context, t testing.T, c TestCase, wd *plugint return fmt.Errorf("After applying this test step, the plan was not empty.\nstdout:\n\n%s", stdout) } - // do a refresh - if !step.Destroy || (step.Destroy && !step.PreventPostDestroyRefresh) { - err := runProviderCommand(ctx, t, func() error { - return wd.Refresh(ctx) - }, wd, providers) - if err != nil { - return fmt.Errorf("Error running post-apply refresh: %w", err) - } - } - // do another plan err = runProviderCommand(ctx, t, func() error { + var opts []tfexec.PlanOption if step.Destroy { - return wd.CreateDestroyPlan(ctx) + opts = append(opts, tfexec.Destroy(true)) + + if step.PreventPostDestroyRefresh { + opts = append(opts, tfexec.Refresh(false)) + } } - return wd.CreatePlan(ctx) + return wd.CreatePlan(ctx, opts...) }, wd, providers) if err != nil { return fmt.Errorf("Error running second post-apply plan: %w", err) diff --git a/internal/plugintest/working_dir.go b/internal/plugintest/working_dir.go index a6e580812..95d6b1af8 100644 --- a/internal/plugintest/working_dir.go +++ b/internal/plugintest/working_dir.go @@ -222,10 +222,13 @@ func (wd *WorkingDir) planFilename() string { // CreatePlan runs "terraform plan" to create a saved plan file, which if successful // will then be used for the next call to Apply. -func (wd *WorkingDir) CreatePlan(ctx context.Context) error { +func (wd *WorkingDir) CreatePlan(ctx context.Context, opts ...tfexec.PlanOption) error { logging.HelperResourceTrace(ctx, "Calling Terraform CLI plan command") - hasChanges, err := wd.tf.Plan(context.Background(), tfexec.Reattach(wd.reattachInfo), tfexec.Refresh(false), tfexec.Out(PlanFileName)) + opts = append(opts, tfexec.Reattach(wd.reattachInfo)) + opts = append(opts, tfexec.Out(PlanFileName)) + + hasChanges, err := wd.tf.Plan(context.Background(), opts...) logging.HelperResourceTrace(ctx, "Called Terraform CLI plan command") @@ -250,36 +253,6 @@ func (wd *WorkingDir) CreatePlan(ctx context.Context) error { return nil } -// CreateDestroyPlan runs "terraform plan -destroy" to create a saved plan -// file, which if successful will then be used for the next call to Apply. -func (wd *WorkingDir) CreateDestroyPlan(ctx context.Context) error { - logging.HelperResourceTrace(ctx, "Calling Terraform CLI plan -destroy command") - - hasChanges, err := wd.tf.Plan(context.Background(), tfexec.Reattach(wd.reattachInfo), tfexec.Refresh(false), tfexec.Out(PlanFileName), tfexec.Destroy(true)) - - logging.HelperResourceTrace(ctx, "Called Terraform CLI plan -destroy command") - - if err != nil { - return err - } - - if !hasChanges { - logging.HelperResourceTrace(ctx, "Created destroy plan with no changes") - - return nil - } - - stdout, err := wd.SavedPlanRawStdout(ctx) - - if err != nil { - return fmt.Errorf("error retrieving formatted plan output: %w", err) - } - - logging.HelperResourceTrace(ctx, "Created destroy plan with changes", map[string]any{logging.KeyTestTerraformPlan: stdout}) - - return nil -} - // Apply runs "terraform apply". If CreatePlan has previously completed // successfully and the saved plan has not been cleared in the meantime then // this will apply the saved plan. Otherwise, it will implicitly create a new