Enable TF_SCHEMA_PANIC_ON_ERROR Only During Acceptance Testing

[bflad]

SDK version

v2.0.0-rc.1

Use-cases

For a long time, the acceptance testing framework has accepted the TF_SCHEMA_PANIC_ON_ERROR environment variable as a way to tease out state setting and drift detection issues in resources where the (*helper/schema.ResourceData).Set() call is not error checked, e.g.

func ExampleThingRead(d *schema.ResourceData, meta interface{}) error {
  // ...
  d.Set("attr", someInvalidValue)
  // ...
}

In the upcoming SDK v2, the value has been flipped on by default, which is great for catching provider bugs, but unfortunately at the potential expense of very poor user experience. When a resource panics, it makes the resource practically unusable (since this affects refresh) and potentially can leave the user's Terraform state in an errant state (depending on how Terraform CLI handles the panic). Considering that it can be fairly easy for larger providers to miss failing acceptance testing during merges and before releases, this can cause a lot of issues to be submitted and an immediate need to cut a bug fix release. Asking users to run with TF_SCHEMA_PANIC_ON_ERROR disabled could be problematic in its own right.

Proposal

Consider guarding the TF_SCHEMA_PANIC_ON_ERROR behavior to only be enabled by default in the acceptance testing framework. For example, acceptance testing today requires TF_ACC=1 for helper/resource.Test() / helper/resource.ParallelTest() so that could potentially be used.

References

• TF_SCHEMA_PANIC_ON_ERROR on by default #344

[bflad]

I wonder, instead of panicking at all, if maybe the acceptance testing framework could instead capture this issue, allow the testing to continue as normal, then report it at the end of the TestStep as an error?

--- FAIL: TestAccAWSLightsailInstance_basic (43.56s)
    testing.go:630: Error setting state for resource attribute during testing.
        These errors prevent Terraform from saving computed attributes and
        properly performing drift detection on configured values.

        Error: 1 error(s) found:
          * resource: aws_lightsail_instance: ram_size: '' expected type 'int', got unconvertible type 'string'

Panics will likely leave dangling testing resources.

[paultyng]

I think panic is the correct behavior for now, but I do think adding recover at least to the testing framework is a reasonable follow-on issue. I'll write it up, as it could give a bit better test runs for unrelated tests.

[paultyng]

Oh sorry, I misunderstood what you were saying. I think the thing is we would need some hook down in ResourceData.Set that understood this was a test and could call the appropriate output method. I don't think ResourceData has access to the current context.Context, but that may be something to explore.

[ghost]

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.