From 518e0ed37117a431a75ab8e617807d1e5716cbad Mon Sep 17 00:00:00 2001 From: Doug Lauder Date: Thu, 15 Apr 2021 10:49:33 -0400 Subject: [PATCH] fix racy ping test (#17400) - ensure no logging is attempted after unit test is completed by explicitly shutting down the mock server, and ensuring no reference to testing.T is held. --- services/remotecluster/mocks_test.go | 52 ++++++++++++++++++++++---- services/remotecluster/ping_test.go | 5 ++- services/remotecluster/send_test.go | 4 ++ services/remotecluster/service_test.go | 2 + 4 files changed, 54 insertions(+), 9 deletions(-) diff --git a/services/remotecluster/mocks_test.go b/services/remotecluster/mocks_test.go index 5607828ff7..8f02fc1ab6 100644 --- a/services/remotecluster/mocks_test.go +++ b/services/remotecluster/mocks_test.go @@ -6,6 +6,7 @@ package remotecluster import ( "fmt" "strings" + "sync" "testing" "go.uber.org/zap/zapcore" @@ -51,34 +52,69 @@ func (ms *mockServer) GetStore() store.Store { storeMock.On("RemoteCluster").Return(remoteClusterStoreMock) return storeMock } +func (ms *mockServer) Shutdown() { ms.logger.Shutdown() } type mockLogger struct { - t *testing.T + t *testing.T + mux sync.Mutex } func (ml *mockLogger) IsLevelEnabled(level mlog.LogLevel) bool { return true } func (ml *mockLogger) Debug(s string, flds ...mlog.Field) { - ml.t.Log("debug", s, fieldsToStrings(flds)) + ml.mux.Lock() + defer ml.mux.Unlock() + if ml.t != nil { + ml.t.Log("debug", s, fieldsToStrings(flds)) + } } func (ml *mockLogger) Info(s string, flds ...mlog.Field) { - ml.t.Log("info", s, fieldsToStrings(flds)) + ml.mux.Lock() + defer ml.mux.Unlock() + if ml.t != nil { + ml.t.Log("info", s, fieldsToStrings(flds)) + } } func (ml *mockLogger) Warn(s string, flds ...mlog.Field) { - ml.t.Log("warn", s, fieldsToStrings(flds)) + ml.mux.Lock() + defer ml.mux.Unlock() + if ml.t != nil { + ml.t.Log("warn", s, fieldsToStrings(flds)) + } } func (ml *mockLogger) Error(s string, flds ...mlog.Field) { - ml.t.Log("error", s, fieldsToStrings(flds)) + ml.mux.Lock() + defer ml.mux.Unlock() + if ml.t != nil { + ml.t.Log("error", s, fieldsToStrings(flds)) + } } func (ml *mockLogger) Critical(s string, flds ...mlog.Field) { - ml.t.Log("crit", s, fieldsToStrings(flds)) + ml.mux.Lock() + defer ml.mux.Unlock() + if ml.t != nil { + ml.t.Log("crit", s, fieldsToStrings(flds)) + } } func (ml *mockLogger) Log(level mlog.LogLevel, s string, flds ...mlog.Field) { - ml.t.Log(level.Name, s, fieldsToStrings(flds)) + ml.mux.Lock() + defer ml.mux.Unlock() + if ml.t != nil { + ml.t.Log(level.Name, s, fieldsToStrings(flds)) + } } func (ml *mockLogger) LogM(levels []mlog.LogLevel, s string, flds ...mlog.Field) { - ml.t.Log(levelsToString(levels), s, fieldsToStrings(flds)) + ml.mux.Lock() + defer ml.mux.Unlock() + if ml.t != nil { + ml.t.Log(levelsToString(levels), s, fieldsToStrings(flds)) + } +} +func (ml *mockLogger) Shutdown() { + ml.mux.Lock() + defer ml.mux.Unlock() + ml.t = nil } func levelsToString(levels []mlog.LogLevel) string { diff --git a/services/remotecluster/ping_test.go b/services/remotecluster/ping_test.go index f12ae6af53..a0ba1ea673 100644 --- a/services/remotecluster/ping_test.go +++ b/services/remotecluster/ping_test.go @@ -23,7 +23,6 @@ const ( ) func TestPing(t *testing.T) { - t.Skip("MM-34785") disablePing = false t.Run("No error", func(t *testing.T) { @@ -65,6 +64,8 @@ func TestPing(t *testing.T) { defer ts.Close() mockServer := newMockServer(t, makeRemoteClusters(NumRemotes, ts.URL)) + defer mockServer.Shutdown() + service, err := NewRemoteClusterService(mockServer) require.NoError(t, err) @@ -111,6 +112,8 @@ func TestPing(t *testing.T) { defer ts.Close() mockServer := newMockServer(t, makeRemoteClusters(NumRemotes, ts.URL)) + defer mockServer.Shutdown() + service, err := NewRemoteClusterService(mockServer) require.NoError(t, err) diff --git a/services/remotecluster/send_test.go b/services/remotecluster/send_test.go index 27b9b20e1e..115f281ea0 100644 --- a/services/remotecluster/send_test.go +++ b/services/remotecluster/send_test.go @@ -82,6 +82,8 @@ func TestBroadcastMsg(t *testing.T) { defer ts.Close() mockServer := newMockServer(t, makeRemoteClusters(NumRemotes, ts.URL)) + defer mockServer.Shutdown() + service, err := NewRemoteClusterService(mockServer) require.NoError(t, err) @@ -137,6 +139,8 @@ func TestBroadcastMsg(t *testing.T) { defer ts.Close() mockServer := newMockServer(t, makeRemoteClusters(NumRemotes, ts.URL)) + defer mockServer.Shutdown() + service, err := NewRemoteClusterService(mockServer) require.NoError(t, err) diff --git a/services/remotecluster/service_test.go b/services/remotecluster/service_test.go index bc8e157fc2..2895d6c475 100644 --- a/services/remotecluster/service_test.go +++ b/services/remotecluster/service_test.go @@ -30,6 +30,8 @@ func TestService_AddTopicListener(t *testing.T) { } mockServer := newMockServer(t, makeRemoteClusters(NumRemotes, "")) + defer mockServer.Shutdown() + service, err := NewRemoteClusterService(mockServer) require.NoError(t, err)