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

helper/resource: Remove data source exclusion from ExpectNonEmptyPlan handling #594

Merged

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Sep 25, 2020

Closes #593

Can confirm with this fixes the given acceptance test case while not presenting unexpected errors with other similar data source testing not using ExpectNonEmptyPlan.

… handling

Reference: hashicorp#593

Can confirm with this fixes the given acceptance test case while not presenting unexpected errors with other similar data source testing not using ExpectNonEmptyPlan.
@paddycarver paddycarver added the bug Something isn't working label Sep 30, 2020
Copy link
Member

@kmoe kmoe left a comment

Choose a reason for hiding this comment

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

In the legacy pre-binary test framework, data sources were excluded from the empty plan check that occurs after the final plan in the plan-refresh-plan at the end of each TestStep (https://github.com/hashicorp/terraform-plugin-sdk/blob/v1.6.0/helper/resource/testing_config.go#L148).

This exclusion was added to the test framework in Core v0.7.7 (hashicorp/terraform@031b561), because at that time performing a terraform plan with data sources in the state (e.g. after a refresh) would produce a non-empty plan to "destroy" them. Inclusion of data sources in destroy plans (described as "terrifying" in hashicorp/terraform#14605) was removed in Core v0.10.4 with hashicorp/terraform@83414be. Excluding them from empty plan checks in the test framework was therefore redundant after that point, but the code remained in the test framework long afterwards.

When the legacy test framework was migrated to the new SDK, we mistakenly excluded data sources from all empty plan checks, despite any exclusion having been long redundant. Thank you @bflad for helping clean up this issue!

@ghost
Copy link

ghost commented Oct 31, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ExpectNonEmptyPlan Missing Data Source Differences Between v2.0.1 and v2.0.3
3 participants