provisioners/local-exec: Detect errors in "stop" test

Previously this test was just assuming that the provisioner run would
succeed and only requiring that it run for more than 50ms before exiting.
That meant that it could potentially false-positive succeed if the
provisioner happened to return an error but take more than 50ms to do so.

Now we'll test for failure before we ask the provisioner to stop, which
narrows the false-positive window. This still isn't completely robust
because we don't have any way to test whether the provisioner failed due
to being canceled or for some other reason. The error message returned on
cancellation varies depending on what state the provisioner was in when
it got the cancellation message, so it's not currently feasible to write
a robust check that would definitely distinguish between the expected error
vs. unexpected errors.

Signed-off-by: Martin Atkins <mart@degeneration.co.uk>

internal/builtin/provisioners/local-exec/resource_provisioner_test.go

@@ -10,6 +10,7 @@ import (
 	"os"
 	"runtime"
 	"strings"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -67,26 +68,34 @@ func TestResourceProvider_stop(t *testing.T) {
 	}
 
 	doneCh := make(chan struct{})
-	startTime := time.Now()
+	var provisionerResp atomic.Pointer[provisioners.ProvisionResourceResponse]
 	go func() {
 		defer close(doneCh)
-		// The functionality of p.Apply is tested in TestResourceProvider_Apply.
-		// Because p.Apply is called in a goroutine, trying to t.Fatal() on its
-		// result would be ignored or would cause a panic if the parent goroutine
-		// has already completed.
-		_ = p.ProvisionResource(provisioners.ProvisionResourceRequest{
+		resp := p.ProvisionResource(provisioners.ProvisionResourceRequest{
 			Config:   c,
 			UIOutput: output,
 		})
+		provisionerResp.Store(&resp)
 	}()
 
-	mustExceed := (50 * time.Millisecond)
+	mustExceed := (250 * time.Millisecond)
 	select {
 	case <-doneCh:
-		t.Fatalf("expected to finish sometime after %s finished in %s", mustExceed, time.Since(startTime))
+		// If doneCh is closed here then provisionerResp will have been
+		// set to a non-nil pointer and so we'll catch it in the
+		// if statement immediately below.
 	case <-time.After(mustExceed):
 		t.Logf("correctly took longer than %s", mustExceed)
 	}
+	if resp := provisionerResp.Load(); resp != nil {
+		// This catches a potential misleading outcome where the provisioner
+		// exits early due to an error but does so slow enough that it would
+		// pass the minimum time check above.
+		if resp.Diagnostics.HasErrors() {
+			t.Fatalf("provisioner failed: %s", resp.Diagnostics.Err())
+		}
+		t.Fatalf("provisioner responded with success before we asked it to stop")
+	}
 
 	// Stop it
 	stopTime := time.Now()
@@ -102,6 +111,11 @@ func TestResourceProvider_stop(t *testing.T) {
 	case <-time.After(finishWithin):
 		t.Fatalf(maxTempl, finishWithin, time.Since(stopTime))
 	}
+
+	// Our background goroutine _must_ eventually exit before we consider
+	// the test to be done.
+	for range doneCh {
+	}
 }
 
 func TestResourceProvider_ApplyCustomInterpreter(t *testing.T) {