MM-38016 Fix racy unit test (TestService_AddTopicListener) (#18207)

* remove CreateTestLogger API 
* add missing mockServer.Shutdown

services/remotecluster/mocks_test.go

@@ -5,7 +5,6 @@ package remotecluster
 
 import (
 	"context"
-	"testing"
 
 	"github.com/mattermost/mattermost-server/v6/einterfaces"
 	"github.com/mattermost/mattermost-server/v6/model"
@@ -21,8 +20,8 @@ type mockServer struct {
 	user    *model.User
 }
 
-func newMockServer(t *testing.T, remotes []*model.RemoteCluster) *mockServer {
+func newMockServer(remotes []*model.RemoteCluster) *mockServer {
-	testLogger := mlog.CreateTestLogger(t, nil, mlog.StdAll...)
+	testLogger := mlog.CreateConsoleTestLogger(true, mlog.LvlDebug)
 
 	return &mockServer{
 		remotes: remotes,

services/remotecluster/ping_test.go

@@ -63,7 +63,7 @@ func TestPing(t *testing.T) {
 		}))
 		defer ts.Close()
 
-		mockServer := newMockServer(t, makeRemoteClusters(NumRemotes, ts.URL))
+		mockServer := newMockServer(makeRemoteClusters(NumRemotes, ts.URL))
 		defer mockServer.Shutdown()
 
 		service, err := NewRemoteClusterService(mockServer)
@@ -111,7 +111,7 @@ func TestPing(t *testing.T) {
 		}))
 		defer ts.Close()
 
-		mockServer := newMockServer(t, makeRemoteClusters(NumRemotes, ts.URL))
+		mockServer := newMockServer(makeRemoteClusters(NumRemotes, ts.URL))
 		defer mockServer.Shutdown()
 
 		service, err := NewRemoteClusterService(mockServer)

services/remotecluster/send_test.go

@@ -81,7 +81,7 @@ func TestBroadcastMsg(t *testing.T) {
 		}))
 		defer ts.Close()
 
-		mockServer := newMockServer(t, makeRemoteClusters(NumRemotes, ts.URL))
+		mockServer := newMockServer(makeRemoteClusters(NumRemotes, ts.URL))
 		defer mockServer.Shutdown()
 
 		service, err := NewRemoteClusterService(mockServer)
@@ -138,7 +138,7 @@ func TestBroadcastMsg(t *testing.T) {
 		}))
 		defer ts.Close()
 
-		mockServer := newMockServer(t, makeRemoteClusters(NumRemotes, ts.URL))
+		mockServer := newMockServer(makeRemoteClusters(NumRemotes, ts.URL))
 		defer mockServer.Shutdown()
 
 		service, err := NewRemoteClusterService(mockServer)

services/remotecluster/sendprofileImage_test.go

@@ -101,7 +101,8 @@ func TestService_sendProfileImageToRemote(t *testing.T) {
 
 	provider := testImageProvider{}
 
-	mockServer := newMockServer(t, makeRemoteClusters(NumRemotes, ts.URL))
+	mockServer := newMockServer(makeRemoteClusters(NumRemotes, ts.URL))
+	defer mockServer.Shutdown()
 	mockServer.SetUser(user)
 	service, err := NewRemoteClusterService(mockServer)
 	require.NoError(t, err)

services/remotecluster/service_test.go

@@ -29,7 +29,7 @@ func TestService_AddTopicListener(t *testing.T) {
 		return nil
 	}
 
-	mockServer := newMockServer(t, makeRemoteClusters(NumRemotes, ""))
+	mockServer := newMockServer(makeRemoteClusters(NumRemotes, ""))
 	defer mockServer.Shutdown()
 
 	service, err := NewRemoteClusterService(mockServer)

shared/mlog/tlog.go

@@ -7,40 +7,17 @@ import (
 	"bytes"
 	"io"
 	"os"
-	"strings"
 	"sync"
-	"testing"
 
 	"github.com/mattermost/logr/v2"
 	"github.com/mattermost/logr/v2/formatters"
 	"github.com/mattermost/logr/v2/targets"
 )
 
-// CreateTestLogger creates a logger for unit tests, using the `TB.Log`
+// AddWriterTarget adds a simple io.Writer target to an existing Logger.
-func CreateTestLogger(tb testing.TB, writer io.Writer, levels ...Level) *Logger {
+// The `io.Writer` can be a buffer which is useful for testing.
-	logger, _ := NewLogger()
+// When adding a buffer to collect logs make sure to use `mlog.Buffer` which is
-
+// a thread safe version of `bytes.Buffer`.
-	filter := logr.NewCustomFilter(levels...)
-	formatter := &formatters.Plain{}
-
-	if tb != nil {
-		testtarget := newTestingTarget(tb)
-		if err := logger.log.Logr().AddTarget(testtarget, "_testTB", filter, formatter, 1000); err != nil {
-			tb.Fail()
-			return nil
-		}
-	}
-
-	if writer != nil {
-		target := targets.NewWriterTarget(writer)
-		if err := logger.log.Logr().AddTarget(target, "_testWriter", filter, formatter, 1000); err != nil {
-			tb.Fail()
-			return nil
-		}
-	}
-	return logger
-}
-
 func AddWriterTarget(logger *Logger, w io.Writer, useJSON bool, levels ...Level) error {
 	filter := logr.NewCustomFilter(levels...)
 
@@ -79,45 +56,6 @@ func CreateConsoleTestLogger(useJSON bool, level Level) *Logger {
 	return logger
 }
 
-// testingTarget is a simple log target that writes to the testing log.
-type testingTarget struct {
-	mux sync.Mutex
-	tb  testing.TB
-}
-
-func newTestingTarget(tb testing.TB) *testingTarget {
-	return &testingTarget{
-		tb: tb,
-	}
-}
-
-// Init is called once to initialize the target.
-func (tt *testingTarget) Init() error {
-	return nil
-}
-
-// Write outputs bytes to this file target.
-func (tt *testingTarget) Write(p []byte, rec *logr.LogRec) (int, error) {
-	tt.mux.Lock()
-	defer tt.mux.Unlock()
-
-	if tt.tb != nil {
-		tt.tb.Helper()
-		tt.tb.Log(strings.TrimSpace(string(p)))
-	}
-	return len(p), nil
-}
-
-// Shutdown is called once to free/close any resources.
-// Target queue is already drained when this is called.
-func (tt *testingTarget) Shutdown() error {
-	tt.mux.Lock()
-	defer tt.mux.Unlock()
-
-	tt.tb = nil
-	return nil
-}
-
 // Buffer provides a thread-safe buffer useful for logging to memory in unit tests.
 type Buffer struct {
 	buf bytes.Buffer

web/oauth_test.go

@@ -4,7 +4,6 @@
 package web
 
 import (
-	"bytes"
 	"context"
 	"encoding/base64"
 	"io"
@@ -581,8 +580,7 @@ func TestOAuthComplete_ErrorMessages(t *testing.T) {
 
 	translationFunc := i18n.GetUserTranslations("en")
 	c.AppContext.SetT(translationFunc)
-	buffer := &bytes.Buffer{}
+	c.Logger = mlog.CreateConsoleTestLogger(true, mlog.LvlDebug)
-	c.Logger = mlog.CreateTestLogger(t, buffer, mlog.StdAll...)
 	defer c.Logger.Shutdown()
 	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.GitLabSettings.Enable = true })
 	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableOAuthServiceProvider = true })