terraform-plugin-sdk icon indicating copy to clipboard operation
terraform-plugin-sdk copied to clipboard

ExpectError is ignored for test case where destroy action expected to have error

Open IronSavior opened this issue 5 years ago • 14 comments

Maybe I'm doing this wrong?

SDK version

{
  "Path": "github.com/hashicorp/terraform-plugin-sdk/v2",
  "Version": "v2.0.2"
}

Relevant provider source code

// In provider resource DELETE function:
_, err := client.Remove(ctx, req)
if err != nil {
	logger.WithError(err).Error("could not remove")
	return diag.FromErr(err) // <-- Code path under test
}
// In test case:
client.On("Remove", mock.Anything, req).Return(nil, &err) // Set up mock client to return error

resource.UnitTest(t, resource.TestCase{
	Providers: testAccProviders,
	Steps: []resource.TestStep{{
		Config:      loadFixtureString("testdata/%s.tf", t.Name()),
		ExpectError: regexp.MustCompile(`.*`), // <-- should match any error at all
		Check: resource.ComposeAggregateTestCheckFunc(
			// irrelevant
		),
	}},
})

Debug Output

Test output:

    testing_new.go:22: WARNING: destroy failed, so remote objects may still exist and be subject to billing
    testing_new.go:22: failed to destroy: 
        Error: could not remove

Expected Behavior

Test should pass because the delete operation is expected to fail

Actual Behavior

Test fails because the resource is not destroyed.

Steps to Reproduce

Create an apply/destroy test case where the destroy is guaranteed to fail.

References

https://github.com/hashicorp/terraform-plugin-sdk/issues/347 - Except their problem was during resource creation.

IronSavior avatar Oct 09 '20 20:10 IronSavior

I am hitting a similar issue. I want to test a case where the attempted deletion of a resource with dependencies should fail. The apply does fail with the expected error, but then the test itself errors out due to dangling resources:

    resource_launchdarkly_feature_flag_test.go:1027: Step 3/3 error: Error running apply: exit status 1
        
        Error: flag "prereq" in project "tx10w6a1r8" cannot be deleted: 400 Bad Request: {"code":"invalid_request","message":"A flag that is a prerequisite of other flags cannot be archived"}
        
        
    testing_new.go:70: Error running post-test destroy, there may be dangling resources: exit status 1
        
        Error: failed to delete flag "prereq" from project "tx10w6a1r8": 404 Not Found: {"message":"Unknown project key tx10w6a1r8"}

Are there any updates on this? Is there a way around this that anyone knows of?

sloloris avatar Sep 27 '21 15:09 sloloris

It's not a solution but a workaround. Try adding a step at the end which successfully creates the resources and it should work.

resource.UnitTest(t, resource.TestCase{
	Providers: testAccProviders,
	Steps: []resource.TestStep{{
		Config:      loadFixtureString("testdata/%s.tf", t.Name()),
		ExpectError: regexp.MustCompile(`.*`), // <-- should match any error at all
		Check: resource.ComposeAggregateTestCheckFunc(
			// irrelevant
		),
                {
                    Config: SomeFuncThatSuccessfullyCreatesResource(resourceArgss),
                },
	}},
})

AniketKariya avatar Apr 18 '22 09:04 AniketKariya

Hmm. . . @AniketKariya that didn't work for me.

matthewcummings avatar Jun 03 '22 01:06 matthewcummings

I have a use case where destroys should only succeed/be allowed if the resource specifies force_destroy = true. It looks like it won't work as is:

https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/resource/testing_new.go#L24

matthewcummings avatar Jun 03 '22 01:06 matthewcummings

Hasn't been merged yet but I have a PR to handle this in a way similar to ExpectError: https://github.com/hashicorp/terraform-plugin-sdk/pull/976

matthewcummings avatar Jun 04 '22 15:06 matthewcummings

Hi folks 👋 The testing framework generally expects tests to always destroy successfully at the end of a TestCase to prevent dangling infrastructure and manual resource cleanup.

Does setting up the testing using a Destroy: true configured TestStep help?

resource.TestCase{
  // ... other fields ...
  Steps: []resource.TestStep{
    {
      Config: "# config that successfully creates resource, but requires forced destruction",
      // ... potentially other fields ...
    },
    {
      Config: "# config that should unsuccessfully destroy resource, potentially same as above Config",
      Destroy: true,
      ExpectError: regexp.MustCompile(/*...*/),
      // ... potentially other fields ...
    },
    {
      Config: "# config that successfully applies forced destruction settings to resource so it can be destroyed by TestCase"
      // ... potentially other fields ...
    },
  },
}

It should also be possible to set Config: "", if that is slightly clearer for signaling the desire to destroy all resources amidst the steps.

bflad avatar Jun 14 '22 23:06 bflad

@bflad that makes perfect sense, nonetheless it's perfectly normal for a provider to fail when deleting resources, intentionally, for any number of reasons. I spent some time looking into this (existing providers) and I came to the conclusion that people just don't have tests for such cases, which is not ideal.

Destroy: true doesn't help for my use case.

So let's flip this back to you - are you saying that you will not add/allow functionality which can handle (expected) failed deletes? I'm all for simplicity but in my case, being able to validate this behavior is desirable.

matthewcummings avatar Jun 15 '22 00:06 matthewcummings

Hi @matthewcummings and @IronSavior - something I think we need to make sure we check: have you tried to have a TestStep where Destroy: true and ExpectError: regexp is set?

Destroy, under the hood, changes drastically what the TestStep does: instead of terraform plan followed by terraform apply, it will do terraform plan -destroy followed by terraform apply. Effectively exercising the deletion of a resource.

Are you saying that you tested that, and found that in case of error, ExpectError would not actually be matched against it?

detro avatar Jun 15 '22 12:06 detro

Hi @detro yes, I have tried that. It doesn't work, when the post test destroy runs, if there's an error, there's no current way to handle it, see https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/resource/testing_new.go#L32

matthewcummings avatar Jun 19 '22 14:06 matthewcummings

@detro any update on this?

matthewcummings avatar Jun 24 '22 00:06 matthewcummings

@detro checking again. . . any updates here?

matthewcummings avatar Jul 24 '22 00:07 matthewcummings

I have similar issue. I want to let provider user to explicitly set enabled = false to fully destroy the resource since the resource must be stop before it is deleting.

I have traced sdk source code, just like @matthewcummings found. Calling CheckDestory function is after runProviderCommand function calling, so there is no way to catch the error return from runProviderCommand and write custom error handling for it.

Although I have acknowledged that there are some resource will still left on tfstate, I want to execute sweepers to clean up the resource after acceptance test.

https://github.com/hashicorp/terraform-plugin-sdk/blob/ed85ac47867d82e7d1ae802b3b0be02333609ece/helper/resource/testing_new.go#L21-L34

titaneric avatar Mar 07 '23 09:03 titaneric