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

Tests are executed 5 times each time #194

Closed
gerbil opened this issue Sep 29, 2023 · 5 comments
Closed

Tests are executed 5 times each time #194

gerbil opened this issue Sep 29, 2023 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@gerbil
Copy link

gerbil commented Sep 29, 2023

terraform-plugin-testing version

v1.5.1

Relevant provider source code

package opswatProvider

import (
	"github.com/hashicorp/terraform-plugin-testing/helper/resource"
	"regexp"
	"testing"
)

func TestAccFileSyncDataSource(t *testing.T) {
	resource.Test(t, resource.TestCase{
		ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
		Steps: []resource.TestStep{
			// Read testing
			{
				Config: providerConfig + configFileSyncDataSource,
				Check: resource.ComposeTestCheckFunc(
					// Verify value returned
					resource.TestMatchResourceAttr("data.opswat_file_sync.test", "timeout", regexp.MustCompile("[0-9]+")),
				),
			},
		},
	})
}

const configFileSyncDataSource = `
data "opswat_file_sync" "test" {}
output "opswat_file_sync" {
  value = data.opswat_file_sync.test
}
`

Terraform Configuration Files

data "XXX" "test" {}
output XXX" {
  value = data.XXX.test
}

Expected Behavior

Test should be executed only once

Actual Behavior

Test was executed 5 times (based on fmt.Println in GetGlobalSync() func)

$ TF_ACC=1 go test ./internal/test/ -v
=== RUN TestAccFileSyncDataSource
request URL: https://xxx/admin/config/file/sync
request URL: https://xxx/admin/config/file/sync
request URL: https://xxx/admin/config/file/sync
request URL: https://xxx/admin/config/file/sync
request URL: https://xxx/admin/config/file/sync
--- PASS: TestAccFileSyncDataSource (21.23s)
PASS
ok terraform-provider-opswat/internal/test 23.468s

Steps to Reproduce

git clone https://github.com/gerbil/terraform-provider-opswat
$ TF_ACC=1 go test ./internal/test/ -v

@gerbil gerbil added the bug Something isn't working label Sep 29, 2023
@gerbil
Copy link
Author

gerbil commented Sep 29, 2023

Btw applicable only to datasource tests

@bflad bflad self-assigned this Nov 22, 2023
@bflad
Copy link
Contributor

bflad commented Nov 22, 2023

Hi @gerbil 👋 Thank you for submitting this issue.

This testing logic behavior is certainly confusing as the testing logic currently abstracts a few different Terraform commands per test step. It is not that the test itself is running multiple times, but the data source read is being invoked multiple times. This design choice was mainly to cover provider developer gotchas that were present in older versions of Terraform itself and due to unfortunate behaviors caused simply by using the prior provider development SDK, terraform-plugin-sdk. Essentially a Config* test step today runs the following commands: refresh, plan -refresh=false, apply -refresh=false, plan -refresh=false, plan. Modern versions of Terraform may be seeing the data source being referenced in the output value and forcing the data source refresh. You could try removing the output to see if it reduces the number of data source read invocations, as you can just directly reference data source attributes for value assertions.

One thing we could potentially do right now is remove the explicit refresh command invocations, since Terraform's recommendations and implementation details have shifted to recommend planning with refresh directly. We might also be able to introduce some special configuration-based heuristics to see if a configuration contains no managed resources. Given that the current behavior is potentially relied on by existing provider testing, we would need to be careful to not break those tests though. In the next major version, we may look at paring down or removing those abstractions so its more obvious what the testing logic is going to do as well.

Thanks again for the report.

bflad added a commit that referenced this issue Nov 29, 2023
…e state shimming when possible (#223)

Reference: hashicorp/terraform-plugin-sdk#515
Reference: #194
Reference: #222
Reference: https://developer.hashicorp.com/terraform/cli/commands/refresh

This change replaces the usage of explicit refresh commands by instead calling the next command with the relevant refresh flags. Separating the refresh from plan was previously done with the idea that the testing logic may introduce further assertion hooks based on that explicit refresh. Over time though, the Terraform recommendations for refreshing and planning have solidified around always recommending to use a plan with behavior changing flags as necessary. Even the Terraform CLI implementation of the refresh command itself has been shifted to call a plan with special flags.

This should reduce testing times decently as it means that the testing logic will skip spinning up and tearing down any relevant providers two times per `TestStep` and whatever time would be associated with Terraform CLI loading for those commands. Comparing the testing performed across this Go module:

Prior: `TF_ACC=1 go test -count=1 ./...  111.07s user 59.37s system 233% cpu 1:12.99 total`
Now: `TF_ACC=1 go test -count=1 ./...  90.81s user 48.25s system 229% cpu 1:00.54 total`

The separated refresh also prevented `PreApply` plan checks from catching certain situations because the refreshed state could cause the plan to be missing changes.
@bflad
Copy link
Contributor

bflad commented Dec 26, 2023

Hi again @gerbil 👋

terraform-plugin-testing v1.6.0 removed separate terraform refresh invocations so you should notice less refresh calls to your data source. As mentioned, removing the explicit output configuration block might also help skip calls to your data source with certain versions of Terraform.

Beyond this, any more changes within the testing logic to help with this would likely be fairly complex to implement since it means parsing Terraform configurations, such as detecting data source only configurations. Are you okay with the current situation and explanations to close this?

@bflad bflad added the waiting-response Issues or pull requests waiting for an external response label Dec 26, 2023
@gerbil
Copy link
Author

gerbil commented Dec 27, 2023

Sure, thank you!

@gerbil gerbil closed this as completed Dec 27, 2023
@github-actions github-actions bot removed the waiting-response Issues or pull requests waiting for an external response label Dec 27, 2023
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 27, 2024
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

No branches or pull requests

2 participants