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

Binary acceptance test driver #262

Merged
merged 4 commits into from
Feb 12, 2020
Merged

Binary acceptance test driver #262

merged 4 commits into from
Feb 12, 2020

Conversation

kmoe
Copy link
Member

@kmoe kmoe commented Nov 26, 2019

This PR adds a new optional test driver for Terraform provider acceptance tests that runs the Terraform CLI.

To minimise work for provider developers at this stage, the API follows as closely as possible the functionality of the legacy test driver, minimising changes needed to acceptance test code.

Since we are targeting a 1.x.x release of the provider SDK, no breaking changes will be made, and the legacy test driver will only be removed in version 2.0.0.

Usage

The new test driver must be enabled by initialising the test helper in the TestMain() function in all provider packages that run acceptance tests. Most providers have only one package.

Import the new "github.com/hashicorp/terraform-plugin-sdk/acctest" package and add the following TestMain function to provider_test.go:

func TestMain(m *testing.M) {
	acctest.UseBinaryDriver("provider_name", Provider)
	resource.TestMain(m)
}

Some providers already have a TestMain() defined, usually for the purpose of enabling test sweepers (as described in Extending Terraform: Sweepers). These additional occurrences should be removed.

Known issue: TypeSet attribute checks

A limitation of the binary acceptance test driver is that attribute checks addressing TypeSet elements will fail.

These attribute checks are rarely used in the legacy test framework, as the hashcode used for addressing Set elements in the flatmap representation must be calculated during the test. Please see #196.

A follow-up improvement to the SDK type system will fix this issue and provide an idiomatic way to check complex type attributes.

Opt-out per TestCase

Initialising the binary test helper using acctest.UseBinaryDriver() causes all tests to be run using the new binary driver. Until SDK version 2.0.0, the DisableBinaryDriver boolean property can be used to use the legacy test driver for individual TestCases.

Multi-provider testing

Providers that require resources from other providers during acceptance tests have two options:

  • Standard provider discovery mechanism. During acceptance test runs, Terraform CLI will attempt to locate providers on the local filesystem, and will download any missing providers in the usual way. This is the default behaviour.
  • Binaries in the provider repo. Relying on the Terraform CLI provider discovery mechanism can be avoided by including auxiliary provider binaries in terraform.d/plugins/$GOOS_$GOARCH/$provider_name, relative to the provider source code directory root. The environment variable TF_ACC_PROVIDER_ROOT_DIR must be set to the path of the source code directory root to use this feature. Providers can be included in binary or zip format.

It is no longer necessary to import other Terraform providers as Go modules: these imports should be removed.

Further examples will be added to this section as I raise PRs to enable the binary test driver in HashiCorp-owned providers.

Closes #13

Copy link
Contributor

@appilon appilon left a comment

Choose a reason for hiding this comment

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

I think you accidentally committed vendor

Fortunately looks like an easy fix just remove the last commit:

2f0ac66

plugin/client.go Outdated Show resolved Hide resolved
}
// If we never checked an id-only refresh, it is a failure.
// TODO KEM: why is this here? does this ever happen?
// if idRefresh {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know if any of "the big providers" even use this? What value does it provide, I'm not sure it's a modern practice I would be okay if we made providers "delete" those tests or rewrite them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copying some data here from a conversation elsewhere:

There are 806 occurrences of ID refresh tests in terraform-providers providers:

$ grep -r --exclude-dir="vendor" --include="*_test.go" "IDRefresh" * | wc -l
806

and more than half of these are in "big" providers:

$ grep -r --exclude-dir="vendor" --include="*_test.go" "IDRefresh" * | grep -e aws -e azure -e google -e kubernetes -e oracle | wc -l
454

@appilon
Copy link
Contributor

appilon commented Nov 26, 2019

One minor aspect that needs to be covered is how to providers that call into our resource.TestMain() can get setup with the new framework. It is used to run sweepers (maybe this is what you were referring to on the todo list)

@kmoe kmoe force-pushed the acctests-poc-wip-kem branch 2 times, most recently from aac6d05 to 587988b Compare December 4, 2019 20:28
@@ -559,6 +559,12 @@ func Test(t TestT, c TestCase) {
providers[name] = p
}

if acctest.TestHelper != nil {
// inject providers for ImportStateVerify
RunLegacyTest(t.(*testing.T), c, providers)
Copy link
Contributor

@appilon appilon Dec 4, 2019

Choose a reason for hiding this comment

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

the providers map is something I managed to cut as cruft in the version2 branch, wondering if we can get away without it (so I don't need to restore it in that branch)? Can't remember if I authored this comment but seems like its lingering around for ImportStateVerify

Copy link
Member

@radeksimko radeksimko Jan 10, 2020

Choose a reason for hiding this comment

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

I suppose we can keep this in when merging into master (as opt-in) and then remove it in version2 branch. This would align with the risk profile you set by removing it in version2 I expect?

We could add a comment here to remember to do it after merging?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree we should remove the conditional check that acts as an "opt-in" in version2. Unfortunately it's the map passed to RunLegacyTest called providers that I cut from version2 and once this is merged to master I will need to rebase version2 and restore it. Unless of course we trace providers through RunLegacyTest and find a way to remove it.

It's not a big deal I'm sure we will get the merge conflicts solved one way or another.

return nil
}

func testIDRefresh(c TestCase, t *testing.T, wd *tftest.WorkingDir, step TestStep, r *terraform.ResourceState) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

amazing that you reverse engineered whatever this was? I have never written an "IDRefresh" test myself and I don't think its covered in the extend docs.

@kmoe kmoe force-pushed the acctests-poc-wip-kem branch 3 times, most recently from bf62c82 to e9ad188 Compare December 19, 2019 21:05
@appilon appilon changed the title [DO NOT MERGE] Binary acceptance testing framework Binary acceptance testing framework Jan 10, 2020
@kmoe kmoe force-pushed the acctests-poc-wip-kem branch 3 times, most recently from 6b27e70 to 51aaecd Compare January 13, 2020 15:12
@kmoe kmoe force-pushed the acctests-poc-wip-kem branch 2 times, most recently from a097a6f to df3f5c9 Compare January 23, 2020 16:47
@kmoe kmoe changed the title Binary acceptance testing framework Binary acceptance test driver Jan 29, 2020
@kmoe kmoe force-pushed the acctests-poc-wip-kem branch 2 times, most recently from 159877f to 851c59c Compare January 31, 2020 12:32
go.mod Outdated Show resolved Hide resolved
@kmoe kmoe force-pushed the acctests-poc-wip-kem branch 2 times, most recently from ded6b68 to a731935 Compare February 4, 2020 14:00
kmoe and others added 3 commits February 10, 2020 22:35
Add hashicorp/terraform-plugin-test, zclconf/go-cty, and hashicorp-terraform-json.
Co-authored-by: Katy Moe <katy@katy.moe>
Co-authored-by: Alex Pilon <apilon@hashicorp.com>
Co-authored-by: Radek Simko <radek.simko@gmail.com>
Copy link
Contributor

@appilon appilon left a comment

Choose a reason for hiding this comment

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

The beautiful new binary driver is over my head at this point. It's insulated from causing much harm though as it's still opt in! So I say let's give it a shot!

helper/resource/testing_new_config.go Show resolved Hide resolved
@kmoe kmoe merged commit 3595c17 into master Feb 12, 2020
@kmoe kmoe deleted the acctests-poc-wip-kem branch February 12, 2020 18:05
kmoe added a commit to hashicorp/terraform-provider-random that referenced this pull request Feb 17, 2020
This commit enables the new binary test driver introduced in SDK v1.7.0 for all acceptance tests in this provider. No test results should change.

Please see hashicorp/terraform-plugin-sdk#262 for more details about this test driver.
kmoe added a commit to hashicorp/terraform-provider-random that referenced this pull request Mar 5, 2020
This commit enables the new binary test driver introduced in SDK v1.7.0 for all acceptance tests in this provider. No test results should change.

Please see hashicorp/terraform-plugin-sdk#262 for more details about this test driver.
@ghost
Copy link

ghost commented Mar 14, 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 and limited conversation to collaborators Mar 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow provider discovery as part of testing framework
3 participants