testing: gracefully handle subtest failing parent’s T

Don’t panic if a subtest inadvertently calls FailNow
on a parent’s T.  Instead, report the offending subtest
while still reporting the error with the ancestor test and
keep exiting goroutines�.

Note that this implementation has a race if parallel
subtests are failing the parent concurrently.
This is fine:
Calling FailNow on a parent is considered an error
in principle, at the moment, and is reported if it is
detected. Having the race allows the race detector
to detect the error as well.

Fixes #22882

Change-Id: Ifa6d5e55bb88f6bcbb562fc8c99f1f77e320015a
Reviewed-on: https://go-review.googlesource.com/97635
Run-TryBot: Marcel van Lohuizen <[email protected]>
Reviewed-by: Kunpei Sakai <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>

Author: mpvl

src/testing/sub_test.go

@@ -315,6 +315,81 @@ func TestTRun(t *T) {
 		f: func(t *T) {
 			t.Skip()
 		},
+	}, {
+		desc: "subtest calls error on parent",
+		ok:   false,
+		output: `
+--- FAIL: subtest calls error on parent (N.NNs)
+	sub_test.go:NNN: first this
+	sub_test.go:NNN: and now this!
+	sub_test.go:NNN: oh, and this too`,
+		maxPar: 1,
+		f: func(t *T) {
+			t.Errorf("first this")
+			outer := t
+			t.Run("", func(t *T) {
+				outer.Errorf("and now this!")
+			})
+			t.Errorf("oh, and this too")
+		},
+	}, {
+		desc: "subtest calls fatal on parent",
+		ok:   false,
+		output: `
+--- FAIL: subtest calls fatal on parent (N.NNs)
+	sub_test.go:NNN: first this
+	sub_test.go:NNN: and now this!
+    --- FAIL: subtest calls fatal on parent/#00 (N.NNs)
+    	testing.go:NNN: test executed panic(nil) or runtime.Goexit: subtest may have called FailNow on a parent test`,
+		maxPar: 1,
+		f: func(t *T) {
+			outer := t
+			t.Errorf("first this")
+			t.Run("", func(t *T) {
+				outer.Fatalf("and now this!")
+			})
+			t.Errorf("Should not reach here.")
+		},
+	}, {
+		desc: "subtest calls error on ancestor",
+		ok:   false,
+		output: `
+--- FAIL: subtest calls error on ancestor (N.NNs)
+	sub_test.go:NNN: Report to ancestor
+    --- FAIL: subtest calls error on ancestor/#00 (N.NNs)
+    	sub_test.go:NNN: Still do this
+	sub_test.go:NNN: Also do this`,
+		maxPar: 1,
+		f: func(t *T) {
+			outer := t
+			t.Run("", func(t *T) {
+				t.Run("", func(t *T) {
+					outer.Errorf("Report to ancestor")
+				})
+				t.Errorf("Still do this")
+			})
+			t.Errorf("Also do this")
+		},
+	}, {
+		desc: "subtest calls fatal on ancestor",
+		ok:   false,
+		output: `
+--- FAIL: subtest calls fatal on ancestor (N.NNs)
+	sub_test.go:NNN: Nope`,
+		maxPar: 1,
+		f: func(t *T) {
+			outer := t
+			t.Run("", func(t *T) {
+				for i := 0; i < 4; i++ {
+					t.Run("", func(t *T) {
+						outer.Fatalf("Nope")
+					})
+					t.Errorf("Don't do this")
+				}
+				t.Errorf("And neither do this")
+			})
+			t.Errorf("Nor this")
+		},
 	}, {
 		desc:   "panic on goroutine fail after test exit",
 		ok:     false,
@@ -518,8 +593,9 @@ func TestBRun(t *T) {
 }
 
 func makeRegexp(s string) string {
+	s = regexp.QuoteMeta(s)
 	s = strings.Replace(s, ":NNN:", `:\d\d\d:`, -1)
-	s = strings.Replace(s, "(N.NNs)", `\(\d*\.\d*s\)`, -1)
+	s = strings.Replace(s, "N\\.NNs", `\d*\.\d*s`, -1)
 	return s
 }
 

src/testing/testing.go

@@ -718,6 +718,8 @@ type InternalTest struct {
 	F    func(*T)
 }
 
+var errNilPanicOrGoexit = errors.New("test executed panic(nil) or runtime.Goexit")
+
 func tRunner(t *T, fn func(t *T)) {
 	t.runner = callerName(0)
 
@@ -733,8 +735,17 @@ func tRunner(t *T, fn func(t *T)) {
 		t.duration += time.Since(t.start)
 		// If the test panicked, print any test output before dying.
 		err := recover()
+		signal := true
 		if !t.finished && err == nil {
-			err = fmt.Errorf("test executed panic(nil) or runtime.Goexit")
+			err = errNilPanicOrGoexit
+			for p := t.parent; p != nil; p = p.parent {
+				if p.finished {
+					t.Errorf("%v: subtest may have called FailNow on a parent test", err)
+					err = nil
+					signal = false
+					break
+				}
+			}
 		}
 		if err != nil {
 			t.Fail()
@@ -769,7 +780,7 @@ func tRunner(t *T, fn func(t *T)) {
 		if t.parent != nil && atomic.LoadInt32(&t.hasSub) == 0 {
 			t.setRan()
 		}
-		t.signal <- true
+		t.signal <- signal
 	}()
 
 	t.start = time.Now()
@@ -822,7 +833,11 @@ func (t *T) Run(name string, f func(t *T)) bool {
 	// without being preempted, even when their parent is a parallel test. This
 	// may especially reduce surprises if *parallel == 1.
 	go tRunner(t, f)
-	<-t.signal
+	if !<-t.signal {
+		// At this point, it is likely that FailNow was called on one of the
+		// parent tests by one of the subtests. Continue aborting up the chain.
+		runtime.Goexit()
+	}
 	return !t.failed
 }