helper/resource: Refresh during plan rather than separately and remove state shimming when possible

.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