Modifying underlying logic behind TestStep.Taint to call the terrafom taint command (#1031)

* Modifying underlying logic behind TestStep.Taint to call the terraform taint command (#1030)

* Apply suggestions from code review

Co-authored-by: Brian Flad <[email protected]>

* Adding time import for test (#1030)

Co-authored-by: Brian Flad <[email protected]>

Author: bendbennett

.changelog/1031.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+helper/resource: Fixed `TestStep` type `Taint` field usage to properly recreate resources
+```

helper/resource/testing_config.go

@@ -2,31 +2,24 @@ package resource
 
 import (
 	"context"
-	"errors"
 	"fmt"
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/internal/logging"
-	"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/internal/plugintest"
 )
 
-func testStepTaint(ctx context.Context, state *terraform.State, step TestStep) error {
+func testStepTaint(ctx context.Context, step TestStep, wd *plugintest.WorkingDir) error {
 	if len(step.Taint) == 0 {
 		return nil
 	}
 
 	logging.HelperResourceTrace(ctx, fmt.Sprintf("Using TestStep Taint: %v", step.Taint))
 
 	for _, p := range step.Taint {
-		m := state.RootModule()
-		if m == nil {
-			return errors.New("no state")
+		err := wd.Taint(ctx, p)
+		if err != nil {
+			return fmt.Errorf("error tainting resource: %s", err)
 		}
-		rs, ok := m.Resources[p]
-		if !ok {
-			return fmt.Errorf("resource %q not found in state", p)
-		}
-		logging.HelperResourceWarn(ctx, fmt.Sprintf("Explicitly tainting resource %q", p))
-		rs.Taint()
 	}
 	return nil
 }

helper/resource/testing_new.go

@@ -8,7 +8,7 @@ import (
 
 	"github.com/davecgh/go-spew/spew"
 	tfjson "github.com/hashicorp/terraform-json"
-	testing "github.com/mitchellh/go-testing-interface"
+	"github.com/mitchellh/go-testing-interface"
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/internal/logging"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/internal/plugintest"
@@ -152,29 +152,7 @@ func runNewTest(ctx context.Context, t testing.T, c TestCase, helper *plugintest
 		}
 
 		if step.Config != "" && !step.Destroy && len(step.Taint) > 0 {
-			var state *terraform.State
-
-			err := runProviderCommand(ctx, t, func() error {
-				var err error
-
-				state, err = getState(ctx, t, wd)
-
-				if err != nil {
-					return err
-				}
-
-				return nil
-			}, wd, providers)
-
-			if err != nil {
-				logging.HelperResourceError(ctx,
-					"TestStep error reading prior state before tainting resources",
-					map[string]interface{}{logging.KeyError: err},
-				)
-				t.Fatalf("TestStep %d/%d error reading prior state before tainting resources: %s", stepNumber, len(c.Steps), err)
-			}
-
-			err = testStepTaint(ctx, state, step)
+			err := testStepTaint(ctx, step, wd)
 
 			if err != nil {
 				logging.HelperResourceError(ctx,

helper/resource/teststep_providers_test.go

@@ -5,13 +5,16 @@ import (
 	"fmt"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/hashicorp/go-cty/cty"
 	"github.com/hashicorp/terraform-plugin-go/tfprotov5"
 	"github.com/hashicorp/terraform-plugin-go/tfprotov6"
+
 	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
 )
 
 func TestStepProviderConfig(t *testing.T) {
@@ -342,6 +345,75 @@ func TestTest_TestStep_ExternalProviders_To_ProviderFactories_StateUpgraders(t *
 	})
 }
 
+func TestTest_TestStep_Taint(t *testing.T) {
+	t.Parallel()
+
+	var idOne, idTwo string
+
+	Test(t, TestCase{
+		ProviderFactories: map[string]func() (*schema.Provider, error){
+			"random": func() (*schema.Provider, error) { //nolint:unparam // required signature
+				return &schema.Provider{
+					ResourcesMap: map[string]*schema.Resource{
+						"random_id": {
+							CreateContext: func(_ context.Context, d *schema.ResourceData, _ interface{}) diag.Diagnostics {
+								d.SetId(time.Now().String())
+								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{},
+						},
+					},
+				}, nil
+			},
+		},
+		Steps: []TestStep{
+			{
+				Config: `resource "random_id" "test" {}`,
+				Check: ComposeAggregateTestCheckFunc(
+					extractResourceAttr("random_id.test", "id", &idOne),
+				),
+			},
+			{
+				Taint:  []string{"random_id.test"},
+				Config: `resource "random_id" "test" {}`,
+				Check: ComposeAggregateTestCheckFunc(
+					extractResourceAttr("random_id.test", "id", &idTwo),
+				),
+			},
+		},
+	})
+
+	if idOne == idTwo {
+		t.Errorf("taint is not causing destroy-create cycle, idOne == idTwo: %s == %s", idOne, idTwo)
+	}
+}
+
+func extractResourceAttr(resourceName string, attributeName string, attributeValue *string) TestCheckFunc {
+	return func(s *terraform.State) error {
+		rs, ok := s.RootModule().Resources[resourceName]
+
+		if !ok {
+			return fmt.Errorf("resource name %s not found in state", resourceName)
+		}
+
+		attrValue, ok := rs.Primary.Attributes[attributeName]
+
+		if !ok {
+			return fmt.Errorf("attribute %s not found in resource %s state", attributeName, resourceName)
+		}
+
+		*attributeValue = attrValue
+
+		return nil
+	}
+}
+
 func TestTest_TestStep_ProtoV5ProviderFactories(t *testing.T) {
 	t.Parallel()
 

internal/plugintest/working_dir.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/hashicorp/terraform-exec/tfexec"
 	tfjson "github.com/hashicorp/terraform-json"
+
 	"github.com/hashicorp/terraform-plugin-sdk/v2/internal/logging"
 )
 
@@ -303,6 +304,17 @@ func (wd *WorkingDir) Import(ctx context.Context, resource, id string) error {
 	return err
 }
 
+// Taint runs terraform taint
+func (wd *WorkingDir) Taint(ctx context.Context, address string) error {
+	logging.HelperResourceTrace(ctx, "Calling Terraform CLI taint command")
+
+	err := wd.tf.Taint(context.Background(), address)
+
+	logging.HelperResourceTrace(ctx, "Called Terraform CLI taint command")
+
+	return err
+}
+
 // Refresh runs terraform refresh
 func (wd *WorkingDir) Refresh(ctx context.Context) error {
 	logging.HelperResourceTrace(ctx, "Calling Terraform CLI refresh command")

terraform/state.go

@@ -14,8 +14,8 @@ import (
 	"sync"
 
 	"github.com/hashicorp/go-cty/cty"
-	multierror "github.com/hashicorp/go-multierror"
-	uuid "github.com/hashicorp/go-uuid"
+	"github.com/hashicorp/go-multierror"
+	"github.com/hashicorp/go-uuid"
 	"github.com/mitchellh/copystructure"
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/internal/addrs"
@@ -1225,26 +1225,6 @@ func (s *ResourceState) Equal(other *ResourceState) bool {
 	return s.Primary.Equal(other.Primary)
 }
 
-// Taint marks a resource as tainted.
-func (s *ResourceState) Taint() {
-	s.Lock()
-	defer s.Unlock()
-
-	if s.Primary != nil {
-		s.Primary.Tainted = true
-	}
-}
-
-// Untaint unmarks a resource as tainted.
-func (s *ResourceState) Untaint() {
-	s.Lock()
-	defer s.Unlock()
-
-	if s.Primary != nil {
-		s.Primary.Tainted = false
-	}
-}
-
 func (s *ResourceState) init() {
 	s.Lock()
 	defer s.Unlock()

terraform/state_test.go

@@ -907,95 +907,6 @@ func TestResourceStateEqual(t *testing.T) {
 	}
 }
 
-func TestResourceStateTaint(t *testing.T) {
-	cases := map[string]struct {
-		Input  *ResourceState
-		Output *ResourceState
-	}{
-		"no primary": {
-			&ResourceState{},
-			&ResourceState{},
-		},
-
-		"primary, not tainted": {
-			&ResourceState{
-				Primary: &InstanceState{ID: "foo"},
-			},
-			&ResourceState{
-				Primary: &InstanceState{
-					ID:      "foo",
-					Tainted: true,
-				},
-			},
-		},
-
-		"primary, tainted": {
-			&ResourceState{
-				Primary: &InstanceState{
-					ID:      "foo",
-					Tainted: true,
-				},
-			},
-			&ResourceState{
-				Primary: &InstanceState{
-					ID:      "foo",
-					Tainted: true,
-				},
-			},
-		},
-	}
-
-	for k, tc := range cases {
-		tc.Input.Taint()
-		if !reflect.DeepEqual(tc.Input, tc.Output) {
-			t.Fatalf(
-				"Failure: %s\n\nExpected: %#v\n\nGot: %#v",
-				k, tc.Output, tc.Input)
-		}
-	}
-}
-
-func TestResourceStateUntaint(t *testing.T) {
-	cases := map[string]struct {
-		Input          *ResourceState
-		ExpectedOutput *ResourceState
-	}{
-		"no primary, err": {
-			Input:          &ResourceState{},
-			ExpectedOutput: &ResourceState{},
-		},
-
-		"primary, not tainted": {
-			Input: &ResourceState{
-				Primary: &InstanceState{ID: "foo"},
-			},
-			ExpectedOutput: &ResourceState{
-				Primary: &InstanceState{ID: "foo"},
-			},
-		},
-		"primary, tainted": {
-			Input: &ResourceState{
-				Primary: &InstanceState{
-					ID:      "foo",
-					Tainted: true,
-				},
-			},
-			ExpectedOutput: &ResourceState{
-				Primary: &InstanceState{ID: "foo"},
-			},
-		},
-	}
-
-	for k, tc := range cases {
-		tc.Input.Untaint()
-		if !reflect.DeepEqual(tc.Input, tc.ExpectedOutput) {
-			t.Fatalf(
-				"Failure: %s\n\nExpected: %#v\n\nGot: %#v",
-				k, tc.ExpectedOutput, tc.Input)
-		}
-	}
-}
-
 func TestInstanceStateEmpty(t *testing.T) {
 	cases := map[string]struct {
 		In     *InstanceState