testscript: fix error handling in setup

The new FailNow logic introduced in #192 had a flaw: it did not correctly handle failures in the setup code, as observed in #185. This PR fixes that omission.
rogpeppe · Jan 17, 2023 · 592d274 · 592d274
1 parent 9957a52
commit 592d274
Show file tree

Hide file tree

Showing 2 changed files with 70 additions and 44 deletions.
diff --git a/testscript/testscript.go b/testscript/testscript.go
@@ -375,6 +375,11 @@ type backgroundCmd struct {
 // setup sets up the test execution temporary directory and environment.
 // It returns the comment section of the txtar archive.
 func (ts *TestScript) setup() string {
+	defer catchFailNow(func() {
+		// There's been a failure in setup; fail immediately regardless
+		// of the ContinueOnError flag.
+		ts.t.FailNow()
+	})
 	ts.workdir = filepath.Join(ts.testTempDir, "script-"+ts.name)
 
 	// Establish a temporary directory in workdir, but use a prefix that ensures
@@ -575,19 +580,11 @@ func (ts *TestScript) run() {
 	}
 }
 
-var failNow = errors.New("fail now!")
-
 func (ts *TestScript) runLine(line string) (runOK bool) {
-	defer func() {
-		e := recover()
-		if e == nil {
-			return
-		}
-		if e != failNow {
-			panic(e)
-		}
+	defer catchFailNow(func() {
 		runOK = false
-	}()
+	})
+
 	// Parse input line. Ignore blanks entirely.
 	args := ts.parse(line)
 	if len(args) == 0 {
@@ -678,6 +675,21 @@ func (ts *TestScript) applyScriptUpdates() {
 	ts.Logf("%s updated", ts.file)
 }
 
+var failNow = errors.New("fail now!")
+
+// catchFailNow catches any panic from Fatalf and calls
+// f if it did so. It must be called in a defer.
+func catchFailNow(f func()) {
+	e := recover()
+	if e == nil {
+		return
+	}
+	if e != failNow {
+		panic(e)
+	}
+	f()
+}
+
 // condition reports whether the given condition is satisfied.
 func (ts *TestScript) condition(cond string) (bool, error) {
 	switch {

diff --git a/testscript/testscript_test.go b/testscript/testscript_test.go
@@ -135,6 +135,31 @@ func TestEnv(t *testing.T) {
 	}
 }
 
+func TestSetupFailure(t *testing.T) {
+	dir := t.TempDir()
+	if err := os.WriteFile(filepath.Join(dir, "foo.txt"), nil, 0o666); err != nil {
+		t.Fatal(err)
+	}
+	ft := &fakeT{}
+	func() {
+		defer catchAbort()
+		RunT(ft, Params{
+			Dir: dir,
+			Setup: func(*Env) error {
+				return fmt.Errorf("some failure")
+			},
+		})
+	}()
+	if !ft.failed {
+		t.Fatal("test should have failed because of setup failure")
+	}
+
+	want := regexp.MustCompile(`^FAIL: .*: some failure\n$`)
+	if got := ft.log.String(); !want.MatchString(got) {
+		t.Fatalf("expected msg to match `%v`; got:\n%q", want, got)
+	}
+}
+
 func TestScripts(t *testing.T) {
 	// TODO set temp directory.
 	testDeferCount := 0
@@ -197,15 +222,9 @@ func TestScripts(t *testing.T) {
 					ts.Fatalf("testscript [-v] [-continue] [-update] [-explicit-exec] <dir>")
 				}
 				dir := fset.Arg(0)
-				t := &fakeT{ts: ts, verbose: *fVerbose}
+				t := &fakeT{verbose: *fVerbose}
 				func() {
-					defer func() {
-						if err := recover(); err != nil {
-							if err != errAbort {
-								panic(err)
-							}
-						}
-					}()
+					defer catchAbort()
 					RunT(t, Params{
 						Dir:                 ts.MkAbs(dir),
 						UpdateScripts:       *fUpdate,
@@ -219,13 +238,13 @@ func TestScripts(t *testing.T) {
 				}()
 				ts.stdout = strings.Replace(t.log.String(), ts.workdir, "$WORK", -1)
 				if neg {
-					if len(t.failMsgs) == 0 {
+					if !t.failed {
 						ts.Fatalf("testscript unexpectedly succeeded")
 					}
 					return
 				}
-				if len(t.failMsgs) > 0 {
-					ts.Fatalf("testscript unexpectedly failed with errors: %q", t.failMsgs)
+				if t.failed {
+					ts.Fatalf("testscript unexpectedly failed with errors: %q", &t.log)
 				}
 			},
 		},
@@ -310,24 +329,21 @@ func TestWorkdirRoot(t *testing.T) {
 func TestBadDir(t *testing.T) {
 	ft := new(fakeT)
 	func() {
-		defer func() {
-			if err := recover(); err != nil {
-				if err != errAbort {
-					panic(err)
-				}
-			}
-		}()
+		defer catchAbort()
 		RunT(ft, Params{
 			Dir: "thiswillnevermatch",
 		})
 	}()
-	wantCount := 1
-	if got := len(ft.failMsgs); got != wantCount {
-		t.Fatalf("expected %v fail message; got %v", wantCount, got)
+	want := regexp.MustCompile(`no txtar nor txt scripts found in dir thiswillnevermatch`)
+	if got := ft.log.String(); !want.MatchString(got) {
+		t.Fatalf("expected msg to match `%v`; got:\n%v", want, got)
 	}
-	wantMsg := regexp.MustCompile(`no txtar nor txt scripts found in dir thiswillnevermatch`)
-	if got := ft.failMsgs[0]; !wantMsg.MatchString(got) {
-		t.Fatalf("expected msg to match `%v`; got:\n%v", wantMsg, got)
+}
+
+// catchAbort catches the panic raised by fakeT.FailNow.
+func catchAbort() {
+	if err := recover(); err != nil && err != errAbort {
+		panic(err)
 	}
 }
 
@@ -398,11 +414,9 @@ func waitFile(ts *TestScript, neg bool, args []string) {
 }
 
 type fakeT struct {
-	ts       *TestScript
-	log      bytes.Buffer
-	failMsgs []string
-	verbose  bool
-	failed   bool
+	log     strings.Builder
+	verbose bool
+	failed  bool
 }
 
 var errAbort = errors.New("abort test")
@@ -412,9 +426,8 @@ func (t *fakeT) Skip(args ...interface{}) {
 }
 
 func (t *fakeT) Fatal(args ...interface{}) {
-	t.failed = true
-	t.failMsgs = append(t.failMsgs, fmt.Sprint(args...))
-	panic(errAbort)
+	t.Log(args...)
+	t.FailNow()
 }
 
 func (t *fakeT) Parallel() {}
@@ -424,7 +437,8 @@ func (t *fakeT) Log(args ...interface{}) {
 }
 
 func (t *fakeT) FailNow() {
-	t.Fatal("failed")
+	t.failed = true
+	panic(errAbort)
 }
 
 func (t *fakeT) Run(name string, f func(T)) {