make flumetest.Start() concurrency safe

Author: ansel1

Makefile

@@ -31,7 +31,7 @@ fmt:
 	go fmt $(PACKAGES)
 
 test:
-	go test $(BUILD_FLAGS) $(PACKAGES)
+	go test -race $(BUILD_FLAGS) $(PACKAGES)
 
 cover: builddir
 	# runs go test in each package one at a time, generating coverage profiling

flumetest/flumetest.go

@@ -23,6 +23,7 @@ import (
 	"io/ioutil"
 	"os"
 	"strconv"
+	"sync"
 	"testing"
 )
 
@@ -87,12 +88,50 @@ func MustSetDefaults() {
 // Be sure to call the returned function at the end of the test to reset the log
 // output to its original setting.
 func Start(t testing.TB) func() {
+	// delegate to an inner method which is testable
+	return start(t)
+}
+
+type lockedBuf struct {
+	buf *bytes.Buffer
+	sync.Mutex
+}
+
+func (l *lockedBuf) Write(p []byte) (n int, err error) {
+	l.Lock()
+	defer l.Unlock()
+	return l.buf.Write(p)
+}
+
+func (l *lockedBuf) Len() int {
+	l.Lock()
+	defer l.Unlock()
+	return l.buf.Len()
+}
+
+func (l *lockedBuf) String() string {
+	l.Lock()
+	defer l.Unlock()
+	return l.buf.String()
+}
+
+type testingTB interface {
+	Failed() bool
+	Log(args ...interface{})
+}
+
+func start(t testingTB) func() {
 	var revert func()
 	if Verbose {
 		revert = flume.SetOut(flume.LogFuncWriter(t.Log, true))
 	} else {
-		buf := bytes.NewBuffer(nil)
-		revertOut := flume.SetOut(buf)
+		// need to use a synchronized version of buf, since
+		// logs may be written on other goroutines than this one,
+		// and bytes.Buffer is not concurrent safe.
+		buf := lockedBuf{
+			buf: bytes.NewBuffer(nil),
+		}
+		revertOut := flume.SetOut(&buf)
 		revert = func() {
 			revertOut()
 			// make sure that if the test panics or fails, we dump the logs

flumetest/flumetest_test.go

@@ -1,17 +1,78 @@
 package flumetest
 
 import (
+	"fmt"
 	"github.com/gemalto/flume"
+	"github.com/stretchr/testify/assert"
+	"sync"
 	"testing"
 )
 
 func init() {
 	MustSetDefaults()
 }
 
-func TestStart(t *testing.T) {
-	defer Start(t)()
+type mockT struct {
+	failed  bool
+	lastLog string
+	sync.Mutex
+}
+
+func (m *mockT) Failed() bool {
+	m.Lock()
+	defer m.Unlock()
+	return m.failed
+}
 
+func (m *mockT) Log(args ...interface{}) {
+	m.Lock()
+	defer m.Unlock()
+	m.lastLog = fmt.Sprint(args...)
+}
+
+func TestStart(t *testing.T) {
 	var log = flume.New("TestStart")
-	log.Info("Hi", "color", "red", "size", 5, "multilinevalue")
+
+	fakeTestRun := func(succeed bool) string {
+		m := mockT{
+			failed: !succeed,
+		}
+		finish := start(&m)
+
+		log.Info("Hi", "color", "red")
+
+		finish()
+		m.Lock()
+		defer m.Unlock()
+		return m.lastLog
+	}
+	assert.Empty(t, fakeTestRun(true), "should not have logged, because the test didn't fail")
+	assert.Contains(t, fakeTestRun(false), "color:red", "should have logged since test failed")
+
+	// this test is meant to trigger the race detector if we're not synchronizing correctly on the message
+	// buffer
+	m := mockT{
+		failed: true,
+	}
+	finish := start(&m)
+
+	var wg sync.WaitGroup
+	wg.Add(1)
+	stop := make(chan struct{})
+	go func() {
+		wg.Done()
+		for {
+			select {
+			case <-stop:
+				return
+			default:
+				log.Info("logging on a different goroutine, for race detector")
+			}
+		}
+
+	}()
+	wg.Wait()
+	finish()
+	stop <- struct{}{}
+
 }