testing: don't be silent if a test's goroutine fails a test after tes…

…t exits

Fixes #15654

Change-Id: I9bdaa9b76d480d75f24d95f0235efd4a79e3593e
Reviewed-on: https://go-review.googlesource.com/23320
Reviewed-by: Russ Cox <[email protected]>
Run-TryBot: Marcel van Lohuizen <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Joe Tsai <[email protected]>

src/testing/sub_test.go

@@ -307,6 +307,27 @@ func TestTRun(t *T) {
 		f: func(t *T) {
 			t.Skip()
 		},
+	}, {
+		desc:   "panic on goroutine fail after test exit",
+		ok:     false,
+		maxPar: 4,
+		f: func(t *T) {
+			ch := make(chan bool)
+			t.Run("", func(t *T) {
+				go func() {
+					<-ch
+					defer func() {
+						if r := recover(); r == nil {
+							realTest.Errorf("expected panic")
+						}
+						ch <- true
+					}()
+					t.Errorf("failed after success")
+				}()
+			})
+			ch <- true
+			<-ch
+		},
 	}}
 	for _, tc := range testCases {
 		ctx := newTestContext(tc.maxPar, newMatcher(regexp.MatchString, "", ""))

src/testing/testing.go

@@ -196,13 +196,14 @@ var (
 // common holds the elements common between T and B and
 // captures common methods such as Errorf.
 type common struct {
-	mu       sync.RWMutex // guards output and failed
+	mu       sync.RWMutex // guards output, failed, and done.
 	output   []byte       // Output generated by test or benchmark.
 	w        io.Writer    // For flushToParent.
 	chatty   bool         // A copy of the chatty flag.
 	failed   bool         // Test or benchmark has failed.
 	skipped  bool         // Test of benchmark has been skipped.
-	finished bool
+	finished bool         // Test function has completed.
+	done     bool         // Test is finished and all subtests have completed.
 
 	parent   *common
 	level    int       // Nesting depth of test or benchmark.
@@ -351,6 +352,10 @@ func (c *common) Fail() {
 	}
 	c.mu.Lock()
 	defer c.mu.Unlock()
+	// c.done needs to be locked to synchronize checks to c.done in parent tests.
+	if c.done {
+		panic("Fail in goroutine after " + c.name + " has completed")
+	}
 	c.failed = true
 }
 
@@ -540,6 +545,9 @@ func tRunner(t *T, fn func(t *T)) {
 		}
 		t.report() // Report after all subtests have finished.
 
+		// Do not lock t.done to allow race detector to detect race in case
+		// the user does not appropriately synchronizes a goroutine.
+		t.done = true
 		t.signal <- true
 	}()