Adding RefreshState test step #1070

bendbennett · 2022-09-29T06:19:59Z

Closes: #1069

gdavison

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.

gdavison · 2022-09-29T18:17:54Z

helper/resource/testing.go

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


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.

bflad · 2022-10-05T16:20:54Z

helper/resource/testing_new_refresh_state.go

+		t.Fatalf("Error getting state: %s", err)
+	}
+
+	if step.Config == "" {


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.

Have refactored accordingly.

bflad · 2022-10-05T16:21:28Z

helper/resource/testing_new_refresh_state.go

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


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

Have refactored.

bflad · 2022-10-05T16:25:13Z

helper/resource/testing_new_refresh_state.go

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


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.

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

bflad · 2022-10-05T16:26:54Z

helper/resource/testing.go

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


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.

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)

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?

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. 👍

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

…e-init and adding plan following refresh (#1069)

…fresh (#1069)

…lan added in hashicorp/terraform-plugin-sdk#1070 (#268)

helper/resource/testing.go

helper/resource/testing_new_refresh_state.go

.changelog/1070.txt

bflad · 2022-10-10T18:31:04Z

helper/resource/teststep_providers_test.go

+			{
+				Config:             `resource "random_password" "test" { }`,
+				Check:              TestCheckResourceAttr("random_password.test", "special", "false"),
+				ExpectNonEmptyPlan: true,


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.

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

Co-authored-by: Brian Flad <bflad417@gmail.com>

…nfig or destroy in a test step (#1069)

… and result in a diff (#1069)

bflad

Looks good to me 🚀

helper/resource/teststep_validate_test.go

bflad · 2022-10-11T22:50:38Z

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

 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).


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.

Co-authored-by: Brian Flad <bflad417@gmail.com>

…valid (#1069)

github-actions · 2022-11-12T02:30:09Z

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.

bendbennett added 2 commits September 29, 2022 07:18

Adding RefreshState test step (#1069)

f9a2ca4

Adding CHANGELOG entry (#1069)

204d99e

bendbennett marked this pull request as ready for review September 29, 2022 06:23

bendbennett requested a review from a team as a code owner September 29, 2022 06:23

bendbennett mentioned this pull request Sep 29, 2022

Update certificate ready_for_renewal during refresh? hashicorp/terraform-provider-tls#278

Merged

gdavison reviewed Sep 29, 2022

View reviewed changes

Running TestCheckFunc with refreshed state (#1069)

468b72d

bendbennett requested a review from gdavison October 5, 2022 11:05

bflad reviewed Oct 5, 2022

View reviewed changes

bendbennett added 4 commits October 6, 2022 10:09

Adding validation to check conditions for RefreshState (#1069)

a5fad91

Expanding comments on RefreshState (#1069)

e17526a

Removing option to override config during refresh testing, removing r…

51342f5

…e-init and adding plan following refresh (#1069)

Adding test coverage for to verify expect non-empty plan following re…

2f726cb

…fresh (#1069)

bendbennett added a commit to hashicorp/terraform-provider-tls that referenced this pull request Oct 6, 2022

Modifying refresh tests to use the option to check expect non-empty p…

5cf8851

…lan added in hashicorp/terraform-plugin-sdk#1070 (#268)

bflad added enhancement New feature or request subsystem/tests Issues and feature requests related to the testing framework. labels Oct 10, 2022

bflad added this to the v2.24.0 milestone Oct 10, 2022

bflad reviewed Oct 10, 2022

View reviewed changes

bendbennett and others added 5 commits October 11, 2022 07:51

Apply suggestions from code review

0f220c8

Co-authored-by: Brian Flad <bflad417@gmail.com>

Adding validation to verify that refresh state is not present with co…

4be309c

…nfig or destroy in a test step (#1069)

Reset time during test step so that ReadContext does not mutate state…

034a8b7

… and result in a diff (#1069)

Updating website docs (#1069)

bc221f7

Merge branch 'main' into bendbennett/issues-1069

830445e

bflad approved these changes Oct 11, 2022

View reviewed changes

bendbennett and others added 2 commits October 12, 2022 07:58

Apply suggestions from code review

9ec2d62

Co-authored-by: Brian Flad <bflad417@gmail.com>

Test to verify that setting config and refresh state together is not …

5be3380

…valid (#1069)

bendbennett merged commit 1dba057 into main Oct 12, 2022

bendbennett deleted the bendbennett/issues-1069 branch October 12, 2022 07:08

github-actions bot locked as resolved and limited conversation to collaborators Nov 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding RefreshState test step #1070

Adding RefreshState test step #1070

bendbennett commented Sep 29, 2022

gdavison left a comment

gdavison Sep 29, 2022

bflad Oct 5, 2022 •

edited

Loading

bendbennett Oct 6, 2022

bflad Oct 5, 2022

bendbennett Oct 6, 2022

bflad Oct 5, 2022

bendbennett Oct 6, 2022

bflad Oct 5, 2022

bflad Oct 5, 2022

bendbennett Oct 6, 2022

bflad Oct 10, 2022

bendbennett Oct 11, 2022

bflad Oct 10, 2022

bendbennett Oct 11, 2022

bflad left a comment

bflad Oct 11, 2022

github-actions bot commented Nov 12, 2022

Adding RefreshState test step #1070

Adding RefreshState test step #1070

Conversation

bendbennett commented Sep 29, 2022

gdavison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 12, 2022

bflad Oct 5, 2022 •

edited

Loading