Add RemoveState option to TestStep

[vmanilo]

Resolves #85

[vmanilo]

@bflad I've tried to implement 2nd version of your proposal, could you have a look?

[vmanilo]

hey guys, what do you think about this changes?

@bendbennett could you help with review?

[bendbennett]

Hi @vmanilo 👋

I've added the triage label to this PR. We will discuss the changes that you have proposed in an upcoming Team triage meeting.

[frezbo]

anything pending to get this change merged in? Would love to have this to test upgrade suites

[frezbo]

Hi, any plans to get this in?

[bendbennett]

@frezbo apologies but have still to triage this issue. We will provide an update once the team have had a chance to review the proposed changes.

[frezbo]

@frezbo apologies but have still to triage this issue. We will provide an update once the team have had a chance to review the proposed changes.

Thank you ❤️

[Budlee]

Is there a temporary fix for this as I see the upstream is not being triaged yet

[austinvalle]

Hi there @vmanilo 👋🏻 , thanks for submitting this change.

Taking a quick high level look at this PR, we'll need a couple things before we can approve:

• Automated tests to verify the functionality (not all of the testing framework currently has testing, but for new functionality we'll need it to ensure, after merging, we don't introduce any regressions later on)
• Test Step validation mentioned in the original issue (The validation logic can be added here: https://github.com/hashicorp/terraform-plugin-testing/blob/main/helper/resource/teststep_validate.go#L65)

From #85

In this scenario, the following validation should be performed:

• RemoveState is not present in the same TestStep as Config: "not empty", ImportState: true, or RefreshState: true
• RemoveState is not present in the first TestStep of TestCase.Steps, similar to RefreshState

[vmanilo]

hey @austinvalle
thank you for the inputs, I've updated PR

[vmanilo]

hey guys 👋

any updates?

[austinvalle]

Thanks for making those updates @vmanilo ! This is looking good so far, two notes:

• Small suggestions on the PR files related to the Go package docs
• Can you also add a description for this new "test mode" to our website doc? It's a markdown file here:

  terraform-plugin-testing/website/docs/plugin/testing/acceptance-tests/teststep.mdx

  Line 15 in fd9dc5e

  ## Test Modes

[austinvalle]

Suggested change

	// RemoveState list of resource addresses to be removed from state after
	// RemoveState is a list of resource addresses to be removed from state after

[austinvalle]

I think it would make sense to move this definition under the comments mentioning Test modes on line 486. As the intention is for this RemoveState option to be it's own "mode".

Maybe also adding to this doc comment that RemoveState is not meant to be used in the same TestStep with other modes like Config or ImportState?

[austinvalle]

Oh! I forgot, a new changelog entry will also be needed for this. We use a tool called changie, you can read about in our contributing docs. I think this falls into the enhancement category, here is a recent PR with some examples of changelogs.

[vmanilo]

hey @austinvalle 👋

I've addressed your proposals, please take a look

[vmanilo]

guys, when someone has time, please take a look

[austinvalle]

Thanks for making those changes @vmanilo and I appreciate the patience. I made a couple suggestions to change the wording (which I believe I can commit, but wanted to make them as suggestions), but everything else looks good 👍🏻 .

We have a couple other features merging to plugin-testing this week, so I'm hoping to get this merged in alongside or following those.

[vmanilo]

thank you @austinvalle, this wording change looks good to me :)

[austinvalle]

Hi there @vmanilo, sorry about the slow responses, but we've been discussing this PR internally and we've decided that we want to hold off on merging any additional functionality to the TestStep struct in terraform-plugin-testing where we can avoid it. Apologies for the back and forth review process beforehand and for any wasted effort in the development of the PR.

I'll be closing this PR, but will also provide some of our thoughts on state removal in general for testing and what upcoming solutions should help.

---

State removal is/should be a very exceptional behavior for provider testing. For better or worse, most managed resources that operate as singletons support “import on create” like behavior and automatically do state removal behavior rather than destroying anything.

In the near/medium term (current forecast looks like 1.7, so later this year), terraform will support the ability to unmanage resources (e.g. the equivalent of state rm) by putting something in the configuration and it’ll be part of the plan like everything else, similar to the import block introduced in 1.5. This means it would be feasible to write another Config step that applies the state removal without any testing framework changes.

As for the matter of TestStep and terraform-plugin-testing, we are currently exploring refactoring of this interface as developers are trying to deal with the testing framework’s lack of ability to have a “wrapping” configuration/environment for their test steps (examples: #70, #82). As we design this solution we're trying to reduce the amount of functionality that we need to support in TestStep where possible.

Again, apologies for the back and forth on this PR.

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.