diff --git a/.changes/unreleased/BUG FIXES-20260210-153921.yaml b/.changes/unreleased/BUG FIXES-20260210-153921.yaml new file mode 100644 index 000000000..122903f6a --- /dev/null +++ b/.changes/unreleased/BUG FIXES-20260210-153921.yaml @@ -0,0 +1,5 @@ +kind: BUG FIXES +body: 'helper/resource: Test steps in `Config` mode using the `TF_ACC_REFRESH_AFTER_APPLY` compatibility flag will now force post-apply plans to refresh.' +time: 2026-02-10T15:39:21.121944-05:00 +custom: + Issue: "602" diff --git a/.changes/unreleased/BUG FIXES-20260210-154312.yaml b/.changes/unreleased/BUG FIXES-20260210-154312.yaml new file mode 100644 index 000000000..31950c870 --- /dev/null +++ b/.changes/unreleased/BUG FIXES-20260210-154312.yaml @@ -0,0 +1,6 @@ +kind: BUG FIXES +body: 'helper/resource: Test steps in `Config` mode using `Destroy: true` and `Check` functions will now create an additional destroy plan prior to + running `terraform apply` to avoid a potential "Saved Plan is Stale" error from Terraform.' +time: 2026-02-10T15:43:12.029656-05:00 +custom: + Issue: "602" diff --git a/.changes/unreleased/BUG FIXES-20260210-154602.yaml b/.changes/unreleased/BUG FIXES-20260210-154602.yaml new file mode 100644 index 000000000..8b66da13a --- /dev/null +++ b/.changes/unreleased/BUG FIXES-20260210-154602.yaml @@ -0,0 +1,5 @@ +kind: BUG FIXES +body: 'helper/resource: Test steps in `Config` mode using the `TF_ACC_REFRESH_AFTER_APPLY` compatibility flag will not refresh if `ExpectNonEmptyPlan` is true.' +time: 2026-02-10T15:46:02.221648-05:00 +custom: + Issue: "602" diff --git a/.github/workflows/ci-go.yml b/.github/workflows/ci-go.yml index 27e57fd57..20e02f58d 100644 --- a/.github/workflows/ci-go.yml +++ b/.github/workflows/ci-go.yml @@ -21,11 +21,12 @@ jobs: with: go-version-file: 'go.mod' - run: go mod download - - uses: golangci/golangci-lint-action@e7fa5ac41e1cf5b7d48e45e42232ce7ada589601 # v9.1.0 + - uses: golangci/golangci-lint-action@1e7e51e771db61008b38414a730f564565cf7c20 # v9.2.0 test: name: test (Go ${{ matrix.go-version }} / TF ${{ matrix.terraform }}) runs-on: ubuntu-latest strategy: + fail-fast: false matrix: go-version: [ '1.25', '1.24' ] terraform: ${{ fromJSON(vars.TF_VERSIONS_PROTOCOL_V5) }} diff --git a/helper/resource/testcase_providers.go b/helper/resource/testcase_providers.go index 9639cb04d..aedb0a0ed 100644 --- a/helper/resource/testcase_providers.go +++ b/helper/resource/testcase_providers.go @@ -20,26 +20,26 @@ func (c TestCase) providerConfig(_ context.Context, skipProviderBlock bool) stri // is being used and not the others, but leaving it here just in case // it does have a special purpose that wasn't being unit tested prior. for name := range c.Providers { - providerBlocks.WriteString(fmt.Sprintf("provider %q {}\n", name)) + fmt.Fprintf(&providerBlocks, "provider %q {}\n", name) } for name, externalProvider := range c.ExternalProviders { if !skipProviderBlock { - providerBlocks.WriteString(fmt.Sprintf("provider %q {}\n", name)) + fmt.Fprintf(&providerBlocks, "provider %q {}\n", name) } if externalProvider.Source == "" && externalProvider.VersionConstraint == "" { continue } - requiredProviderBlocks.WriteString(fmt.Sprintf(" %s = {\n", name)) + fmt.Fprintf(&requiredProviderBlocks, " %s = {\n", name) if externalProvider.Source != "" { - requiredProviderBlocks.WriteString(fmt.Sprintf(" source = %q\n", externalProvider.Source)) + fmt.Fprintf(&requiredProviderBlocks, " source = %q\n", externalProvider.Source) } if externalProvider.VersionConstraint != "" { - requiredProviderBlocks.WriteString(fmt.Sprintf(" version = %q\n", externalProvider.VersionConstraint)) + fmt.Fprintf(&requiredProviderBlocks, " version = %q\n", externalProvider.VersionConstraint) } requiredProviderBlocks.WriteString(" }\n") diff --git a/helper/resource/testing_new_config.go b/helper/resource/testing_new_config.go index 1145c8e4e..4b8f7d382 100644 --- a/helper/resource/testing_new_config.go +++ b/helper/resource/testing_new_config.go @@ -182,6 +182,17 @@ func testStepNewConfig(ctx context.Context, t testing.T, c TestCase, wd *plugint if err != nil { return fmt.Errorf("Error retrieving pre-apply state: %w", err) } + + // Create Destroy Plan + err = runProviderCommand(ctx, t, wd, providers, func() error { + var opts []tfexec.PlanOption + opts = append(opts, tfexec.Destroy(true)) + + return wd.CreatePlan(ctx, opts...) + }) + if err != nil { + return fmt.Errorf("Error running destroy plan after step.Check shimmed state was retrieved: %w", err) + } } // Apply the diff, creating real resources @@ -256,8 +267,11 @@ func testStepNewConfig(ctx context.Context, t testing.T, c TestCase, wd *plugint // do a plan err = runProviderCommand(ctx, t, wd, providers, func() error { - opts := []tfexec.PlanOption{ - tfexec.Refresh(false), + var opts []tfexec.PlanOption + if refreshAfterApply { + opts = append(opts, tfexec.Refresh(true)) + } else { + opts = append(opts, tfexec.Refresh(false)) } if step.Destroy { opts = append(opts, tfexec.Destroy(true)) @@ -457,7 +471,7 @@ func testStepNewConfig(ctx context.Context, t testing.T, c TestCase, wd *plugint logging.HelperResourceDebug(ctx, "Called TestCase PostApplyFunc") } - if refreshAfterApply && !step.Destroy && !step.PlanOnly { + if refreshAfterApply && !step.Destroy && !step.PlanOnly && !step.ExpectNonEmptyPlan { if len(c.Steps) > stepIndex+1 { // If the next step is a refresh, then we have no need to refresh here if !c.Steps[stepIndex+1].RefreshState { diff --git a/helper/resource/testing_new_config_test.go b/helper/resource/testing_new_config_test.go index 15cf5db0c..f7127616d 100644 --- a/helper/resource/testing_new_config_test.go +++ b/helper/resource/testing_new_config_test.go @@ -16,6 +16,7 @@ import ( "github.com/hashicorp/terraform-plugin-testing/internal/testing/testsdk/resource" "github.com/hashicorp/terraform-plugin-testing/plancheck" "github.com/hashicorp/terraform-plugin-testing/statecheck" + "github.com/hashicorp/terraform-plugin-testing/terraform" "github.com/hashicorp/terraform-plugin-testing/tfversion" ) @@ -906,3 +907,58 @@ func Test_PostApplyFunc_Called(t *testing.T) { t.Error("expected ConfigStateChecks spy1 to be called at least once") } } + +// This regression test ensures that the combination of Config, Destroy, and Check never result in +// a "Saved Plan is Stale" error message, which occurs when the state serial does not match the plan. +// +// This can occur when the refresh that is only done to produce the shimmed "Check" state produces a new state serial. +// Running a fresh plan after refreshing solves that issue, which was introduced in: https://github.com/hashicorp/terraform-plugin-testing/pull/602 +func Test_Destroy_Checks_Avoid_Stale_Plan(t *testing.T) { + t.Parallel() + + UnitTest(t, TestCase{ + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.SkipBelow(tfversion.Version1_0_0), // ProtoV6ProviderFactories + }, + ProtoV6ProviderFactories: map[string]func() (tfprotov6.ProviderServer, error){ + "test": providerserver.NewProviderServer(testprovider.Provider{ + Resources: map[string]testprovider.Resource{ + "test_resource": { + CreateResponse: &resource.CreateResponse{ + NewState: tftypes.NewValue( + tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "id": tftypes.String, + }, + }, + map[string]tftypes.Value{ + "id": tftypes.NewValue(tftypes.String, "test"), + }, + ), + }, + SchemaResponse: &resource.SchemaResponse{ + Schema: &tfprotov6.Schema{ + Block: &tfprotov6.SchemaBlock{ + Attributes: []*tfprotov6.SchemaAttribute{ + { + Name: "id", + Type: tftypes.String, + Computed: true, + }, + }, + }, + }, + }, + }, + }, + }), + }, + Steps: []TestStep{ + { + Config: `resource "test_resource" "test" {}`, + Destroy: true, + Check: func(s *terraform.State) error { return nil }, + }, + }, + }) +} diff --git a/helper/resource/testing_new_import_state.go b/helper/resource/testing_new_import_state.go index d2d71311c..f1b463057 100644 --- a/helper/resource/testing_new_import_state.go +++ b/helper/resource/testing_new_import_state.go @@ -423,11 +423,11 @@ func appendImportBlock(config teststep.Config, resourceName string, importID str func appendImportBlockWithIdentity(config teststep.Config, resourceName string, identityValues map[string]any) teststep.Config { configBuilder := strings.Builder{} - configBuilder.WriteString(fmt.Sprintf(``+"\n"+ + fmt.Fprintf(&configBuilder, ``+"\n"+ `import {`+"\n"+ ` to = %s`+"\n"+ ` identity = {`+"\n", - resourceName)) + resourceName) for k, v := range identityValues { // It's valid for identity attributes to be null, we can just omit it from config @@ -437,20 +437,20 @@ func appendImportBlockWithIdentity(config teststep.Config, resourceName string, switch v := v.(type) { case bool: - configBuilder.WriteString(fmt.Sprintf(` %q = %t`+"\n", k, v)) + fmt.Fprintf(&configBuilder, ` %q = %t`+"\n", k, v) case []any: var quotedV []string for _, v := range v { quotedV = append(quotedV, fmt.Sprintf(`%q`, v)) } - configBuilder.WriteString(fmt.Sprintf(` %q = [%s]`+"\n", k, strings.Join(quotedV, ", "))) + fmt.Fprintf(&configBuilder, ` %q = [%s]`+"\n", k, strings.Join(quotedV, ", ")) case json.Number: - configBuilder.WriteString(fmt.Sprintf(` %q = %s`+"\n", k, v)) + fmt.Fprintf(&configBuilder, ` %q = %s`+"\n", k, v) case string: - configBuilder.WriteString(fmt.Sprintf(` %q = %q`+"\n", k, v)) + fmt.Fprintf(&configBuilder, ` %q = %q`+"\n", k, v) default: panic(fmt.Sprintf("unexpected type %T for identity value %q", v, k)) diff --git a/helper/resource/teststep_providers.go b/helper/resource/teststep_providers.go index bd9f43b86..d651c931b 100644 --- a/helper/resource/teststep_providers.go +++ b/helper/resource/teststep_providers.go @@ -71,21 +71,21 @@ func (s TestStep) providerConfig(_ context.Context, skipProviderBlock bool, tfVe for name, externalProvider := range s.ExternalProviders { if !skipProviderBlock { - providerBlocks.WriteString(fmt.Sprintf("provider %q {}\n", name)) + fmt.Fprintf(&providerBlocks, "provider %q {}\n", name) } if externalProvider.Source == "" && externalProvider.VersionConstraint == "" { continue } - requiredProviderBlocks.WriteString(fmt.Sprintf(" %s = {\n", name)) + fmt.Fprintf(&requiredProviderBlocks, " %s = {\n", name) if externalProvider.Source != "" { - requiredProviderBlocks.WriteString(fmt.Sprintf(" source = %q\n", externalProvider.Source)) + fmt.Fprintf(&requiredProviderBlocks, " source = %q\n", externalProvider.Source) } if externalProvider.VersionConstraint != "" { - requiredProviderBlocks.WriteString(fmt.Sprintf(" version = %q\n", externalProvider.VersionConstraint)) + fmt.Fprintf(&requiredProviderBlocks, " version = %q\n", externalProvider.VersionConstraint) } requiredProviderBlocks.WriteString(" }\n") @@ -154,30 +154,30 @@ func (s TestStep) providerConfigTestCase(_ context.Context, skipProviderBlock bo // is being used and not the others, but leaving it here just in case // it does have a special purpose that wasn't being unit tested prior. for name := range providerNames { - providerBlocks.WriteString(fmt.Sprintf("provider %q {}\n", name)) + fmt.Fprintf(&providerBlocks, "provider %q {}\n", name) - requiredProviderBlocks.WriteString(fmt.Sprintf(" %s = {\n", name)) + fmt.Fprintf(&requiredProviderBlocks, " %s = {\n", name) requiredProviderBlocks.WriteString(" }\n") } for name, externalProvider := range testCase.ExternalProviders { if !skipProviderBlock { - providerBlocks.WriteString(fmt.Sprintf("provider %q {}\n", name)) + fmt.Fprintf(&providerBlocks, "provider %q {}\n", name) } if externalProvider.Source == "" && externalProvider.VersionConstraint == "" { continue } - requiredProviderBlocks.WriteString(fmt.Sprintf(" %s = {\n", name)) + fmt.Fprintf(&requiredProviderBlocks, " %s = {\n", name) if externalProvider.Source != "" { - requiredProviderBlocks.WriteString(fmt.Sprintf(" source = %q\n", externalProvider.Source)) + fmt.Fprintf(&requiredProviderBlocks, " source = %q\n", externalProvider.Source) } if externalProvider.VersionConstraint != "" { - requiredProviderBlocks.WriteString(fmt.Sprintf(" version = %q\n", externalProvider.VersionConstraint)) + fmt.Fprintf(&requiredProviderBlocks, " version = %q\n", externalProvider.VersionConstraint) } requiredProviderBlocks.WriteString(" }\n") @@ -250,8 +250,8 @@ func addTerraformBlockSource(name, config string) string { var providerBlocks strings.Builder - providerBlocks.WriteString(fmt.Sprintf(" %s = {\n", name)) - providerBlocks.WriteString(fmt.Sprintf(" source = %q\n", getProviderAddr(name))) + fmt.Fprintf(&providerBlocks, " %s = {\n", name) + fmt.Fprintf(&providerBlocks, " source = %q\n", getProviderAddr(name)) providerBlocks.WriteString(" }\n") return providerBlocks.String() diff --git a/terraform/state.go b/terraform/state.go index 68267757a..9d4e917b7 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -803,7 +803,7 @@ func (s *State) String() string { continue } - buf.WriteString(fmt.Sprintf("module.%s:\n", strings.Join(m.Path[1:], "."))) + fmt.Fprintf(&buf, "module.%s:\n", strings.Join(m.Path[1:], ".")) s := bufio.NewScanner(strings.NewReader(mStr)) for s.Scan() { @@ -812,7 +812,7 @@ func (s *State) String() string { text = " " + text } - buf.WriteString(fmt.Sprintf("%s\n", text)) + fmt.Fprintf(&buf, "%s\n", text) } } @@ -1130,10 +1130,10 @@ func (m *ModuleState) String() string { deposedStr = fmt.Sprintf(" (%d deposed)", len(rs.Deposed)) } - buf.WriteString(fmt.Sprintf("%s:%s%s\n", k, taintStr, deposedStr)) - buf.WriteString(fmt.Sprintf(" ID = %s\n", id)) + fmt.Fprintf(&buf, "%s:%s%s\n", k, taintStr, deposedStr) + fmt.Fprintf(&buf, " ID = %s\n", id) if rs.Provider != "" { - buf.WriteString(fmt.Sprintf(" provider = %s\n", rs.Provider)) + fmt.Fprintf(&buf, " provider = %s\n", rs.Provider) } var attributes map[string]string @@ -1153,7 +1153,7 @@ func (m *ModuleState) String() string { for _, ak := range attrKeys { av := attributes[ak] - buf.WriteString(fmt.Sprintf(" %s = %s\n", ak, av)) + fmt.Fprintf(&buf, " %s = %s\n", ak, av) } for idx, t := range rs.Deposed { @@ -1161,13 +1161,13 @@ func (m *ModuleState) String() string { if t.Tainted { taintStr = " (tainted)" } - buf.WriteString(fmt.Sprintf(" Deposed ID %d = %s%s\n", idx+1, t.ID, taintStr)) + fmt.Fprintf(&buf, " Deposed ID %d = %s%s\n", idx+1, t.ID, taintStr) } if len(rs.Dependencies) > 0 { buf.WriteString("\n Dependencies:\n") for _, dep := range rs.Dependencies { - buf.WriteString(fmt.Sprintf(" %s\n", dep)) + fmt.Fprintf(&buf, " %s\n", dep) } } } @@ -1186,9 +1186,9 @@ func (m *ModuleState) String() string { v := m.Outputs[k] switch vTyped := v.Value.(type) { case string: - buf.WriteString(fmt.Sprintf("%s = %s\n", k, vTyped)) + fmt.Fprintf(&buf, "%s = %s\n", k, vTyped) case []interface{}: - buf.WriteString(fmt.Sprintf("%s = %s\n", k, vTyped)) + fmt.Fprintf(&buf, "%s = %s\n", k, vTyped) case map[string]interface{}: var mapKeys []string for key := range vTyped { @@ -1199,11 +1199,11 @@ func (m *ModuleState) String() string { var mapBuf bytes.Buffer mapBuf.WriteString("{") for _, key := range mapKeys { - mapBuf.WriteString(fmt.Sprintf("%s:%s ", key, vTyped[key])) + fmt.Fprintf(&mapBuf, "%s:%s ", key, vTyped[key]) } mapBuf.WriteString("}") - buf.WriteString(fmt.Sprintf("%s = %s\n", k, mapBuf.String())) + fmt.Fprintf(&buf, "%s = %s\n", k, mapBuf.String()) } } } @@ -1438,7 +1438,7 @@ func (s *ResourceState) String() string { defer s.Unlock() var buf bytes.Buffer - buf.WriteString(fmt.Sprintf("Type = %s", s.Type)) + fmt.Fprintf(&buf, "Type = %s", s.Type) return buf.String() } @@ -1748,7 +1748,7 @@ func (s *InstanceState) String() string { return notCreated } - buf.WriteString(fmt.Sprintf("ID = %s\n", s.ID)) + fmt.Fprintf(&buf, "ID = %s\n", s.ID) attributes := s.Attributes attrKeys := make([]string, 0, len(attributes)) @@ -1763,10 +1763,10 @@ func (s *InstanceState) String() string { for _, ak := range attrKeys { av := attributes[ak] - buf.WriteString(fmt.Sprintf("%s = %s\n", ak, av)) + fmt.Fprintf(&buf, "%s = %s\n", ak, av) } - buf.WriteString(fmt.Sprintf("Tainted = %t\n", s.Tainted)) + fmt.Fprintf(&buf, "Tainted = %t\n", s.Tainted) return buf.String() }