-
Notifications
You must be signed in to change notification settings - Fork 232
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
Conversation
There was a problem hiding this 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:
helper/resource/testing_new.go
Outdated
} | ||
// If we never checked an id-only refresh, it is a failure. | ||
// TODO KEM: why is this here? does this ever happen? | ||
// if idRefresh { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
One minor aspect that needs to be covered is how to providers that call into our |
aac6d05
to
587988b
Compare
helper/resource/testing.go
Outdated
@@ -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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
bf62c82
to
e9ad188
Compare
510b2bd
to
0c01113
Compare
6b27e70
to
51aaecd
Compare
a097a6f
to
df3f5c9
Compare
159877f
to
851c59c
Compare
ded6b68
to
a731935
Compare
0ebc7b4
to
d94d731
Compare
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>
d94d731
to
02ed754
Compare
There was a problem hiding this 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!
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.
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.
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. |
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 followingTestMain
function toprovider_test.go
: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 checksA 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, theDisableBinaryDriver
boolean property can be used to use the legacy test driver for individualTestCase
s.Multi-provider testing
Providers that require resources from other providers during acceptance tests have two options:
terraform.d/plugins/$GOOS_$GOARCH/$provider_name
, relative to the provider source code directory root. The environment variableTF_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