Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding RefreshState test step #1070

Merged
merged 14 commits into from Oct 12, 2022
Merged

Adding RefreshState test step #1070

merged 14 commits into from Oct 12, 2022

Conversation

bendbennett
Copy link
Contributor

Closes: #1069

Copy link
Contributor

@gdavison gdavison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick response, @bendbennett. It looks good. One thing I'd prefer to see is that the RefreshOnly tests could run a full TestCheckFunc.

// RefreshStateCheck checks state following `terraform refresh`. It
// should be used to verify that the state has the expected resources,
// IDs, and attributes.
RefreshStateCheck RefreshStateCheckFunc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the use case I'm picturing, I'd prefer to be able to reuse the existing ecosystem of TestCheckFuncs after the refresh. This would allow tests to run against the whole refreshed state.

t.Fatalf("Error getting state: %s", err)
}

if step.Config == "" {
Copy link
Member

@bflad bflad Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this TestStep mode worry about a configuration? What happens if the configuration changes during a refresh step from a prior step? Here are the potential issues I can see here with a new/updated configuration:

  • Changes which providers are defined. If the existing state has a resource with a provider which is no longer presented properly, then an error could be raised.
  • Has an invalid provider configuration, which raises a new error.
  • Has an invalid resource configuration, which raises a new error.
  • Has an updated resource configuration, which wouldn't be reflected during the refresh and causes checks to potentially have unexpected results compared to the configuration.

The prior TestStep with a configuration would've already caught the first few and I think rather than introduce the potential for these classes of issues, we can avoid this by not resetting the working directory and its configuration. The validation that a refresh step is not the first step can be implemented in the testcase_validate.go and/or teststep_validate.go files, which runs prior to executing any of the test steps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have refactored accordingly.

err = runProviderCommand(ctx, t, func() error {
return wd.Init(ctx)
}, wd, providers)
if err != nil {
t.Fatalf("Error running init: %s", err)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the above, I'm not sure we should intentionally run init again, given the potential issues it could cause.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have refactored.

if err != nil {
t.Fatalf("Error getting state: %s", err)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should refresh TestStep run plan after the refresh to check for unexpected plan differences? If so, it'll need that Terraform command added and check against ExpectNonEmptyPlan if those differences are expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have added a plan following the check and the check against ExpectNonEmptyPlan.


// RefreshState, if true, will test the functionality of `terraform
// refresh` by refreshing the state.
RefreshState bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should consider what website documentation we can add around this. It'd also be good to denote here and in the website that its current intention is refresh, (potentially) plan, and optionally running checks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field should also document that it cannot be the first TestStep and any other potential restrictions or contradictory definitions (such as ImportState + RefreshState for example)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the documentation. In terms of the website docs, where do you think they should be added? The information around ImportState is in /plugin/sdkv2/resources. As we're just discussing testing here should be add a new section or add/amend an area of the website docs that already exist?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think https://www.terraform.io/plugin/sdkv2/testing/acceptance-tests/teststep is okay enough for now. Ideally that page would probably be broken up by test mode, but adding information in the current information architecture should be okay. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have added a couple of lines. Let me know if you think this needs to be expanded.

bendbennett added a commit to hashicorp/terraform-provider-tls that referenced this pull request Oct 6, 2022
@bflad bflad added enhancement New feature or request subsystem/tests Issues and feature requests related to the testing framework. labels Oct 10, 2022
@bflad bflad added this to the v2.24.0 milestone Oct 10, 2022
helper/resource/testing.go Outdated Show resolved Hide resolved
helper/resource/testing.go Outdated Show resolved Hide resolved
helper/resource/testing_new_refresh_state.go Show resolved Hide resolved
helper/resource/testing_new_refresh_state.go Outdated Show resolved Hide resolved
helper/resource/testing_new_refresh_state.go Outdated Show resolved Hide resolved
.changelog/1070.txt Outdated Show resolved Hide resolved
{
Config: `resource "random_password" "test" { }`,
Check: TestCheckResourceAttr("random_password.test", "special", "false"),
ExpectNonEmptyPlan: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: It'd be nice if the provider resource actually converged after the apply without an unexpected plan difference so it didn't look like this sort of post-RefreshState ExpectNonEmptyPlan was required, but no worries if this is difficult to work out. I think we may be able to reset the time during the Create function by explicitly calling something like:

// Ensure test resource can converge after apply.
setTimeForTest(time.Now())
return nil

But I'm not sure if that's the least confusing option there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Have used PreConfig to reset time for TestStep to avoid state mutation in ReadContext and subsequent diff generation by CustomizeDiff.

bflad
bflad approved these changes Oct 11, 2022
Copy link
Member

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 🚀

helper/resource/teststep_validate_test.go Show resolved Hide resolved
An acceptance test’s mode is implicitly determined by the fields provided in the
`TestStep` definition. The applicable fields are defined in the [TestStep
Reference API](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-sdk/helper/resource#TestStep).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was tempted to suggest adding "Enable with XXX: true." in the above import and refresh descriptions but this wording here would probably make things more confusing. Maybe we can circle around to this page at a later time.

@bendbennett bendbennett merged commit 1dba057 into main Oct 12, 2022
5 checks passed
@bendbennett bendbennett deleted the bendbennett/issues-1069 branch October 12, 2022 07:08
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request subsystem/tests Issues and feature requests related to the testing framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Add RefreshOnly acceptance test step type
3 participants