From eea5de6213c6d0c362dd651a8a2904d896554e46 Mon Sep 17 00:00:00 2001
From: Jay Pipes <jaypipes@gmail.com>
Date: Tue, 8 Aug 2023 10:04:41 -0400
Subject: [PATCH] call t.Error() from sub-tests not Scenario.Run()

This patch addresses a couple related problems, all with the evaluation
of testing.T failures. When `testing.T.Run()` is executed, a new
goroutine is spawned with a new `testing.T` pointer. This specific
goroutine's `testing.T` pointer needs to have *its* `testing.T.Error()`
method called in order for that sub-test to be marked failed. We were
erroneously calling `testing.T.Error()` within the `Scenario.Run()`
method instead of inside the `Spec.Eval()` method, which resulted in the
test scenario being marked failed instead of the individual test unit.

We address the exec plugin's `Spec.Eval()` in this patch to call
`testing.T.Error()` on any assertion failure however additional patches
are coming for the http and kube plugins.

Finally, I made a change to the `gdterrors.TimeoutExceeded()` function
to allow for an assertion failure message to be supplied to the error
producer, making it easier for folks to see "this test assertion failed
to succeed before a timeout of (duration)".

Addresses Issue #8

Signed-off-by: Jay Pipes <jaypipes@gmail.com>
---
 errors/failure.go   | 11 +++++++++--
 plugin/exec/eval.go |  9 +++++++--
 scenario/run.go     | 10 +---------
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/errors/failure.go b/errors/failure.go
index 9272662..a16d42c 100644
--- a/errors/failure.go
+++ b/errors/failure.go
@@ -31,8 +31,15 @@ var (
 )
 
 // TimeoutExceeded returns an ErrTimeoutExceeded when a test's execution
-// exceeds a timeout length.
-func TimeoutExceeded(duration string) error {
+// exceeds a timeout length. The optional failure parameter indicates a failed
+// assertion that occurred before a timeout was reached.
+func TimeoutExceeded(duration string, failure error) error {
+	if failure != nil {
+		return fmt.Errorf(
+			"%w: timed out waiting for assertion to succeed (%s)",
+			failure, duration,
+		)
+	}
 	return fmt.Errorf("%s (%s)", ErrTimeoutExceeded, duration)
 }
 
diff --git a/plugin/exec/eval.go b/plugin/exec/eval.go
index 0b9c25d..68fef54 100644
--- a/plugin/exec/eval.go
+++ b/plugin/exec/eval.go
@@ -71,6 +71,11 @@ func (s *Spec) Eval(ctx context.Context, t *testing.T) *result.Result {
 		eerr, _ := err.(*exec.ExitError)
 		ec = eerr.ExitCode()
 	}
-	assertions := newAssertions(s.Assert, ec, outbuf, errbuf)
-	return result.New(result.WithFailures(assertions.Failures()...))
+	a := newAssertions(s.Assert, ec, outbuf, errbuf)
+	if !a.OK() {
+		for _, fail := range a.Failures() {
+			t.Error(fail)
+		}
+	}
+	return result.New(result.WithFailures(a.Failures()...))
 }
diff --git a/scenario/run.go b/scenario/run.go
index f2d1be8..c1fec8a 100644
--- a/scenario/run.go
+++ b/scenario/run.go
@@ -83,6 +83,7 @@ func (s *Scenario) Run(ctx context.Context, t *testing.T) error {
 			res := spec.Eval(specCtx, t)
 			if res.HasRuntimeError() {
 				rterr = res.RuntimeError()
+				t.Fatal(rterr)
 				break
 			}
 			// Results can have arbitrary run data stored in them and we
@@ -91,15 +92,6 @@ func (s *Scenario) Run(ctx context.Context, t *testing.T) error {
 			if res.HasData() {
 				ctx = gdtcontext.StorePriorRun(ctx, res.Data())
 			}
-			for _, failure := range res.Failures() {
-				if gdtcontext.TimedOut(specCtx, failure) {
-					if to != nil && !to.Expected {
-						t.Fatal(gdterrors.TimeoutExceeded(to.After))
-					}
-				} else {
-					t.Fatal(failure)
-				}
-			}
 			if wait != nil && wait.After != "" {
 				debug.Println(ctx, t, "wait: %s after", wait.After)
 				time.Sleep(wait.AfterDuration())