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

Add ExpectedDiffChanges to assert expected diff change type in acceptance test steps #329

Closed

Conversation

pdecat
Copy link
Contributor

@pdecat pdecat commented Feb 19, 2020

This implements @bflad 's solution detailed in #222.

Should also resolve hashicorp/terraform-plugin-testing#61 which sounds exactly the same to me.

Tested with hashicorp/terraform-provider-kubernetes#769

All resource unit tests are disabled, I tried to enable them but get:

panic: runtime error: invalid memory address or nil pointer dereference                                                                                                                  
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xef6001]                                                                                                                   
                                                                                                                                                                                         
goroutine 107 [running]:                                                                                                                                                                 
github.com/hashicorp/terraform-plugin-sdk/internal/helper/plugin.(*GRPCProviderServer).getProviderSchemaBlock(...)                                                                       
        /home/patrick/go/src/github.com/hashicorp/terraform-plugin-sdk/internal/helper/plugin/grpc_provider.go:76                                                                        
github.com/hashicorp/terraform-plugin-sdk/internal/helper/plugin.(*GRPCProviderServer).GetSchema(0x0, 0x159b7a0, 0xc0001f8390, 0xc0001b0e80, 0x0, 0xc0001f8390, 0xc00007da80)            
        /home/patrick/go/src/github.com/hashicorp/terraform-plugin-sdk/internal/helper/plugin/grpc_provider.go:55 +0x91                                                                  
github.com/hashicorp/terraform-plugin-sdk/internal/tfplugin5._Provider_GetSchema_Handler(0x124b600, 0x0, 0x159b7a0, 0xc0001f8390, 0xc0001ea720, 0x0, 0x159b7a0, 0xc0001f8390, 0x0, 0x0)  
        /home/patrick/go/src/github.com/hashicorp/terraform-plugin-sdk/internal/tfplugin5/tfplugin5.pb.go:3045 +0x217                                                                    
google.golang.org/grpc.(*Server).processUnaryRPC(0xc0000f6160, 0x15aa1a0, 0xc00009fb00, 0xc0002d6100, 0xc0003a7a10, 0x1dc6e80, 0x0, 0x0, 0x0)                                            
        /home/patrick/go/pkg/mod/google.golang.org/grpc@v1.23.0/server.go:995 +0x460                                                                                                     
google.golang.org/grpc.(*Server).handleStream(0xc0000f6160, 0x15aa1a0, 0xc00009fb00, 0xc0002d6100, 0x0)                                                                                  
        /home/patrick/go/pkg/mod/google.golang.org/grpc@v1.23.0/server.go:1275 +0xd97                                                                                                    
google.golang.org/grpc.(*Server).serveStreams.func1.1(0xc0000361c0, 0xc0000f6160, 0x15aa1a0, 0xc00009fb00, 0xc0002d6100)                                                                 
        /home/patrick/go/pkg/mod/google.golang.org/grpc@v1.23.0/server.go:710 +0xbb                                                                                                      
created by google.golang.org/grpc.(*Server).serveStreams.func1                                                                                                                           
        /home/patrick/go/pkg/mod/google.golang.org/grpc@v1.23.0/server.go:708 +0xa1                                                                                                      
FAIL    github.com/hashicorp/terraform-plugin-sdk/helper/resource       0.894s                                                                                                           
FAIL                                                                                                                                                                                     

@pdecat
Copy link
Contributor Author

pdecat commented Feb 19, 2020

Changing kubernetes/resource_kubernetes_job_test.go like this:

# git diff kubernetes/resource_kubernetes_job_test.go
diff --git kubernetes/resource_kubernetes_job_test.go kubernetes/resource_kubernetes_job_test.go
index 851a34c8..f0af36f8 100644
--- kubernetes/resource_kubernetes_job_test.go
+++ kubernetes/resource_kubernetes_job_test.go
@@ -63,7 +63,7 @@ func TestAccKubernetesJob_basic(t *testing.T) {
                        },
                        {
                                Config:              testAccKubernetesJobConfig_recreated_selector(name),
-                               ExpectedDiffChanges: map[string]terraform.DiffChangeType{"kubernetes_job.test": terraform.DiffDestroyCreate},
+                               ExpectedDiffChanges: map[string]terraform.DiffChangeType{"kubernetes_job.test": terraform.DiffUpdate},
                                Check: resource.ComposeAggregateTestCheckFunc(
                                        testAccCheckKubernetesJobExists("kubernetes_job.test", &conf),
                                        resource.TestCheckResourceAttr("kubernetes_job.test", "metadata.0.name", name),

Triggers:

make testacc TEST=./kubernetes TESTARGS='-run=TestAccKubernetesJob -count=1'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./kubernetes -v -run=TestAccKubernetesJob -count=1 -timeout 120m
go: finding k8s.io/client-go v12.0.0+incompatible
go: finding k8s.io/client-go v12.0.0+incompatible
go: finding github.com/terraform-providers/terraform-provider-google v2.17.0+incompatible
go: finding github.com/terraform-providers/terraform-provider-google v2.17.0+incompatible
go: finding github.com/terraform-providers/terraform-provider-aws v2.32.0+incompatible
go: finding github.com/terraform-providers/terraform-provider-aws v2.32.0+incompatible
=== RUN   TestAccKubernetesJob_basic
--- FAIL: TestAccKubernetesJob_basic (21.64s)
    testing.go:660: Step 2 error: Unexpected diff change type for 'kubernetes_job.test', expected DiffUpdate, got DeleteThenCreate
FAIL
FAIL    github.com/terraform-providers/terraform-provider-kubernetes/kubernetes 21.679s
FAIL
make: *** [GNUmakefile:17: testacc] Error 1

kmoe and others added 15 commits March 13, 2020 17:03
auto-configure providers during binary acctests
Borrows some core code from 23acb02cb846fd002e7a0b52904ee88019b33211
Update to tfplugin 5.2, map new protocol fields (v1)
The gofmtcheck.sh script uses the gofmt -s flag for reporting simplication issues, so the fixing command must also include this flag.
Makefile: Update fmt target to include gofmt -s flag
@pdecat pdecat force-pushed the acctests-expected-diff-change-type branch from 49fd8a4 to ff160e9 Compare April 1, 2020 08:02
tmeckel and others added 12 commits April 11, 2020 16:33
…k to their old positions as requested by review in helper/schema/schema.go
RequiredWith schema check and changes in validation for optional schema fields
Co-Authored-By: appilon <apilon@hashicorp.com>
Co-Authored-By: appilon <apilon@hashicorp.com>
Co-Authored-By: appilon <apilon@hashicorp.com>
Co-Authored-By: kmoe <5575356+kmoe@users.noreply.github.com>
Add package docs for binary test driver
…n-no-state

Binary testing: omit test cleanup when state is empty
@pdecat pdecat force-pushed the acctests-expected-diff-change-type branch from ff160e9 to 17dfe9c Compare April 22, 2020 05:26
@appilon
Copy link
Contributor

appilon commented Apr 22, 2020

Hello @pdecat, the plugin SDK separating into its own repository unfortunately required duplicating a significant amount of Terraform core code into this repository, as the acceptance test framework used to be driven by this "emulated" version of Terraform that was internalized. As of V1.7 of the SDK we released a new binary based solution #262 that aims to decouple core code from living in this repo (of course in V1 this is opt in, so the current artifacts still exist). The binary solution was shimmed to allow nearly all existing acceptance tests to still pass. In V2 of the SDK, the binary based solution will be the only driver for acceptance tests (and many thousands of lines of code removed).

This related issue/comment has a bit more context.

We sincerely appreciate your interest and investment in enhancing the testing suite, but unfortunately we aren't investing in the old "driver" anymore. We will be ramping up communications around the new driver and V2 very shortly. Our apologies for not getting the message out sooner. We will make note of this enhancement though and keep it as a datapoint for enhancements to the binary driver and future versions of the SDK.

@pdecat
Copy link
Contributor Author

pdecat commented Apr 22, 2020

Hi @appilon, thanks for the update!

Just to clarify, does that mean I can try to submit an equivalent PR on the new upcoming driver,
or that I should just wait for this feature to be implemented by core contributors?

@appilon
Copy link
Contributor

appilon commented Apr 22, 2020

@pdecat We would definitely be open to reviewing a proposed enhancement to the acceptance test framework with the binary driver as the backing implementation.

You can look at the version2 branch as a starting place. Please note though we are close to beginning alpha testing and making V2 the mainline of development (overtake master). So you might want to hold off requesting review until master has settled over the next few weeks, but you could certainly PoC against version2.

There is some info on the upcoming V2 on the forum

To try and shed a little extra context though, Terraform 0.12+ has switched to a new wire protocol and architecture. The SDK as it is manages to shim itself to talk with TF 0.12, however it's become clear to us it's not a great fit. The acceptance test framework as it is currently uses these legacy systems and types (ie terraform.State) the SDK actually isn't responsible for diffing at all but sort of fakes it so existing providers are still compatible with TF 0.12.

So I wanted to give the heads up that we might be hesitant on enhancements leaning into these types, hard to really say until we see a proposed PR, but I wanted to give the heads up so we don't waste another round of your time if your time is precious on this feature. We sincerely appreciate your contributions to our community 😃 .

@appilon
Copy link
Contributor

appilon commented Apr 22, 2020

@pdecat ☝️ I updated the last message with some important extra context, hope you saw it.

@pdecat
Copy link
Contributor Author

pdecat commented Apr 22, 2020

Got that, thanks again!

@appilon
Copy link
Contributor

appilon commented Apr 23, 2020

We have officially moved version2 over to master as the new mainline. Backports and releases of V1 will happen off of a new branch v1-maint. I am closing this PR for maintenance, apologies for any inconvenience.

@ghost
Copy link

ghost commented May 24, 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 May 24, 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.

Core FR: Check that only an 'update' occurred in a test
6 participants