From 332ea1f233891e45bd274e714ab8e4e11c79dc95 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 14 Sep 2021 09:47:24 -0700 Subject: [PATCH] backend/local: TestLocal_plan_context_error to fail terraform.NewContext The original intent of this test was to verify that we properly release the state lock if terraform.NewContext fails. This was in response to a bug in an earlier version of Terraform where that wasn't true. In the recent refactoring that made terraform.NewContext no longer responsible for provider constraint/checksum verification, this test began testing a failed plan operation instead, which left the error return path from terraform.NewContext untested. An invalid parallelism value is the one remaining case where terraform.NewContext can return an error, so as a localized fix for this test I've switched it to just intentionally set an invalid parallelism value. This is still not ideal because it's still testing an implementation detail, but I've at least left a comment inline to try to be clearer about what the goal is here so that we can respond in a more appropriate way if future changes cause this test to fail again. In the long run I'd like to move this last remaining check out to be the responsibility of the CLI layer, with terraform.NewContext either just assuming the value correct or panicking when it isn't, but the handling of this CLI option is currently rather awkwardly spread across the command and backend packages so we'll save that refactoring for a later date. --- internal/backend/local/backend_plan_test.go | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/internal/backend/local/backend_plan_test.go b/internal/backend/local/backend_plan_test.go index 0e7992111..7d5b92b16 100644 --- a/internal/backend/local/backend_plan_test.go +++ b/internal/backend/local/backend_plan_test.go @@ -119,9 +119,24 @@ func TestLocal_plan_context_error(t *testing.T) { b, cleanup := TestLocal(t) defer cleanup() + // This is an intentionally-invalid value to make terraform.NewContext fail + // when b.Operation calls it. + // NOTE: This test was originally using a provider initialization failure + // as its forced error condition, but terraform.NewContext is no longer + // responsible for checking that. Invalid parallelism is the last situation + // where terraform.NewContext can return error diagnostics, and arguably + // we should be validating this argument at the UI layer anyway, so perhaps + // in future we'll make terraform.NewContext never return errors and then + // this test will become redundant, because its purpose is specifically + // to test that we properly unlock the state if terraform.NewContext + // returns an error. + if b.ContextOpts == nil { + b.ContextOpts = &terraform.ContextOpts{} + } + b.ContextOpts.Parallelism = -1 + op, configCleanup, done := testOperationPlan(t, "./testdata/plan") defer configCleanup() - op.PlanRefresh = true // we coerce a failure in Context() by omitting the provider schema run, err := b.Operation(context.Background(), op) @@ -136,7 +151,7 @@ func TestLocal_plan_context_error(t *testing.T) { // the backend should be unlocked after a run assertBackendStateUnlocked(t, b) - if got, want := done(t).Stderr(), "failed to read schema for test_instance.foo in registry.terraform.io/hashicorp/test"; !strings.Contains(got, want) { + if got, want := done(t).Stderr(), "Error: Invalid parallelism value"; !strings.Contains(got, want) { t.Fatalf("unexpected error output:\n%s\nwant: %s", got, want) } }