From 2560469bc79cdb19230d60cded747e1002317adc Mon Sep 17 00:00:00 2001 From: Doug Lauder Date: Wed, 18 Aug 2021 13:44:04 -0400 Subject: [PATCH] MM-38016 Fix racy unit test (TestService_AddTopicListener) (#18207) * remove CreateTestLogger API * add missing mockServer.Shutdown --- services/remotecluster/mocks_test.go | 5 +- services/remotecluster/ping_test.go | 4 +- services/remotecluster/send_test.go | 4 +- .../remotecluster/sendprofileImage_test.go | 3 +- services/remotecluster/service_test.go | 2 +- shared/mlog/tlog.go | 70 ++----------------- web/oauth_test.go | 4 +- 7 files changed, 14 insertions(+), 78 deletions(-) diff --git a/services/remotecluster/mocks_test.go b/services/remotecluster/mocks_test.go index 4f102b5ac2..f23dade315 100644 --- a/services/remotecluster/mocks_test.go +++ b/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 { - testLogger := mlog.CreateTestLogger(t, nil, mlog.StdAll...) +func newMockServer(remotes []*model.RemoteCluster) *mockServer { + testLogger := mlog.CreateConsoleTestLogger(true, mlog.LvlDebug) return &mockServer{ remotes: remotes, diff --git a/services/remotecluster/ping_test.go b/services/remotecluster/ping_test.go index bb985e730c..7254f60f98 100644 --- a/services/remotecluster/ping_test.go +++ b/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) diff --git a/services/remotecluster/send_test.go b/services/remotecluster/send_test.go index a132bdfb91..1df582cb29 100644 --- a/services/remotecluster/send_test.go +++ b/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) diff --git a/services/remotecluster/sendprofileImage_test.go b/services/remotecluster/sendprofileImage_test.go index 6f433383cf..dc32748ffb 100644 --- a/services/remotecluster/sendprofileImage_test.go +++ b/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) diff --git a/services/remotecluster/service_test.go b/services/remotecluster/service_test.go index 856439ba3a..1e9bbd5b63 100644 --- a/services/remotecluster/service_test.go +++ b/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) diff --git a/shared/mlog/tlog.go b/shared/mlog/tlog.go index 89efe303cd..ef8f6016a0 100644 --- a/shared/mlog/tlog.go +++ b/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` -func CreateTestLogger(tb testing.TB, writer io.Writer, levels ...Level) *Logger { - logger, _ := NewLogger() - - 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 -} - +// AddWriterTarget adds a simple io.Writer target to an existing Logger. +// The `io.Writer` can be a buffer which is useful for testing. +// When adding a buffer to collect logs make sure to use `mlog.Buffer` which is +// a thread safe version of `bytes.Buffer`. 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 diff --git a/web/oauth_test.go b/web/oauth_test.go index ab4266bcc0..e69a516e7e 100644 --- a/web/oauth_test.go +++ b/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.CreateTestLogger(t, buffer, mlog.StdAll...) + c.Logger = mlog.CreateConsoleTestLogger(true, mlog.LvlDebug) 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 })