helper/resource: Refresh during plan rather than separately and remov…

…e state shimming when possible (#223)

Reference: hashicorp/terraform-plugin-sdk#515
Reference: #194
Reference: #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.

.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"

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)

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