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

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

Open
IronSavior opened this issue Oct 9, 2020 · 14 comments
Assignees
Labels
bug Something isn't working subsystem/tests Issues and feature requests related to the testing framework.

Comments

@IronSavior
Copy link
Contributor

IronSavior commented Oct 9, 2020

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

#347 - Except their problem was during resource creation.

@IronSavior IronSavior added the bug Something isn't working label Oct 9, 2020
@paddycarver paddycarver added the subsystem/tests Issues and feature requests related to the testing framework. label Dec 17, 2020
@sloloris
Copy link

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?

@AniketKariya
Copy link

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),
                },
	}},
})

@matthewcummings
Copy link

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

@matthewcummings
Copy link

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
Copy link

Hasn't been merged yet but I have a PR to handle this in a way similar to ExpectError: #976

@bflad bflad self-assigned this Jun 14, 2022
@bflad
Copy link
Contributor

bflad commented Jun 14, 2022

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 bflad added the waiting-response Issues or pull requests waiting for an external response label Jun 14, 2022
@matthewcummings
Copy link

@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.

@detro
Copy link
Contributor

detro commented Jun 15, 2022

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?

@matthewcummings
Copy link

matthewcummings commented Jun 19, 2022

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
Copy link

@detro any update on this?

@matthewcummings
Copy link

@detro checking again. . . any updates here?

@bflad bflad removed the waiting-response Issues or pull requests waiting for an external response label Oct 26, 2022
@titaneric
Copy link

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.

err := runProviderCommand(ctx, t, func() error {
return wd.Destroy(ctx)
}, wd, providers)
if err != nil {
return err
}
if c.CheckDestroy != nil {
logging.HelperResourceTrace(ctx, "Using TestCase CheckDestroy")
logging.HelperResourceDebug(ctx, "Calling TestCase CheckDestroy")
if err := c.CheckDestroy(statePreDestroy); err != nil {
return err
}

@gmarciani
Copy link

Hey there, any update on this?

@TomerHeber
Copy link

This is an old thread - but there is a way to make it work:
(similar to what was suggested above).

resource.TestCase{
  Steps: []resource.TestStep{
    {
      Config: {config that creates the resource},
      // ... potentially other fields .
    },
    {
      Config: {exactly the same config as above},
      Destroy: true, {this is important}
      ExpectError: {your expected error},
    },

Destroy will be called twice.
First call - destroy should return the expected error.
Second call - destroy must be successful (mock it accordingly).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working subsystem/tests Issues and feature requests related to the testing framework.
Projects
None yet
Development

No branches or pull requests

10 participants