From c0cf314c5722a4f7ec0a1f355d23fa2511344989 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 8 May 2020 15:40:18 -0700 Subject: Fix race conditions in tests Calling logrus hook.LastEntry() can lead to race conditions. Use AllEntries instead: https://github.com/sirupsen/logrus/blob/60c74ad9be0d874af0ab0daef6ab07c5c5911f0d/hooks/test/test.go#L77 Closes https://gitlab.com/gitlab-org/gitlab-shell/-/issues/450 --- client/client_test.go | 66 ++++++++++++----------- internal/command/receivepack/gitalycall_test.go | 10 ++-- internal/command/uploadarchive/gitalycall_test.go | 9 ++-- internal/command/uploadpack/gitalycall_test.go | 10 ++-- 4 files changed, 51 insertions(+), 44 deletions(-) diff --git a/client/client_test.go b/client/client_test.go index 6c4268e..5e852dc 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -123,11 +123,12 @@ func testSuccessfulGet(t *testing.T, client *GitlabNetClient) { assert.NoError(t, err) assert.Equal(t, string(responseBody), "Hello") - assert.Equal(t, 1, len(hook.Entries)) - assert.Equal(t, logrus.InfoLevel, hook.LastEntry().Level) - assert.Contains(t, hook.LastEntry().Message, "method=GET") - assert.Contains(t, hook.LastEntry().Message, "status=200") - assert.Contains(t, hook.LastEntry().Message, "Finished HTTP request") + entries := hook.AllEntries() + assert.Equal(t, 1, len(entries)) + assert.Equal(t, logrus.InfoLevel, entries[0].Level) + assert.Contains(t, entries[0].Message, "method=GET") + assert.Contains(t, entries[0].Message, "status=200") + assert.Contains(t, entries[0].Message, "Finished HTTP request") }) } @@ -146,11 +147,12 @@ func testSuccessfulPost(t *testing.T, client *GitlabNetClient) { assert.NoError(t, err) assert.Equal(t, "Echo: {\"key\":\"value\"}", string(responseBody)) - assert.Equal(t, 1, len(hook.Entries)) - assert.Equal(t, logrus.InfoLevel, hook.LastEntry().Level) - assert.Contains(t, hook.LastEntry().Message, "method=POST") - assert.Contains(t, hook.LastEntry().Message, "status=200") - assert.Contains(t, hook.LastEntry().Message, "Finished HTTP request") + entries := hook.AllEntries() + assert.Equal(t, 1, len(entries)) + assert.Equal(t, logrus.InfoLevel, entries[0].Level) + assert.Contains(t, entries[0].Message, "method=POST") + assert.Contains(t, entries[0].Message, "status=200") + assert.Contains(t, entries[0].Message, "Finished HTTP request") }) } @@ -161,11 +163,12 @@ func testMissing(t *testing.T, client *GitlabNetClient) { assert.EqualError(t, err, "Internal API error (404)") assert.Nil(t, response) - assert.Equal(t, 1, len(hook.Entries)) - assert.Equal(t, logrus.InfoLevel, hook.LastEntry().Level) - assert.Contains(t, hook.LastEntry().Message, "method=GET") - assert.Contains(t, hook.LastEntry().Message, "status=404") - assert.Contains(t, hook.LastEntry().Message, "Internal API error") + entries := hook.AllEntries() + assert.Equal(t, 1, len(entries)) + assert.Equal(t, logrus.InfoLevel, entries[0].Level) + assert.Contains(t, entries[0].Message, "method=GET") + assert.Contains(t, entries[0].Message, "status=404") + assert.Contains(t, entries[0].Message, "Internal API error") }) t.Run("Missing error for POST", func(t *testing.T) { @@ -174,11 +177,12 @@ func testMissing(t *testing.T, client *GitlabNetClient) { assert.EqualError(t, err, "Internal API error (404)") assert.Nil(t, response) - assert.Equal(t, 1, len(hook.Entries)) - assert.Equal(t, logrus.InfoLevel, hook.LastEntry().Level) - assert.Contains(t, hook.LastEntry().Message, "method=POST") - assert.Contains(t, hook.LastEntry().Message, "status=404") - assert.Contains(t, hook.LastEntry().Message, "Internal API error") + entries := hook.AllEntries() + assert.Equal(t, 1, len(entries)) + assert.Equal(t, logrus.InfoLevel, entries[0].Level) + assert.Contains(t, entries[0].Message, "method=POST") + assert.Contains(t, entries[0].Message, "status=404") + assert.Contains(t, entries[0].Message, "Internal API error") }) } @@ -204,11 +208,12 @@ func testBrokenRequest(t *testing.T, client *GitlabNetClient) { assert.EqualError(t, err, "Internal API unreachable") assert.Nil(t, response) - assert.Equal(t, 1, len(hook.Entries)) - assert.Equal(t, logrus.InfoLevel, hook.LastEntry().Level) - assert.Contains(t, hook.LastEntry().Message, "method=GET") - assert.NotContains(t, hook.LastEntry().Message, "status=") - assert.Contains(t, hook.LastEntry().Message, "Internal API unreachable") + entries := hook.AllEntries() + assert.Equal(t, 1, len(entries)) + assert.Equal(t, logrus.InfoLevel, entries[0].Level) + assert.Contains(t, entries[0].Message, "method=GET") + assert.NotContains(t, entries[0].Message, "status=") + assert.Contains(t, entries[0].Message, "Internal API unreachable") }) t.Run("Broken request for POST", func(t *testing.T) { @@ -218,11 +223,12 @@ func testBrokenRequest(t *testing.T, client *GitlabNetClient) { assert.EqualError(t, err, "Internal API unreachable") assert.Nil(t, response) - assert.Equal(t, 1, len(hook.Entries)) - assert.Equal(t, logrus.InfoLevel, hook.LastEntry().Level) - assert.Contains(t, hook.LastEntry().Message, "method=POST") - assert.NotContains(t, hook.LastEntry().Message, "status=") - assert.Contains(t, hook.LastEntry().Message, "Internal API unreachable") + entries := hook.AllEntries() + assert.Equal(t, 1, len(entries)) + assert.Equal(t, logrus.InfoLevel, entries[0].Level) + assert.Contains(t, entries[0].Message, "method=POST") + assert.NotContains(t, entries[0].Message, "status=") + assert.Contains(t, entries[0].Message, "Internal API unreachable") }) } diff --git a/internal/command/receivepack/gitalycall_test.go b/internal/command/receivepack/gitalycall_test.go index dd75176..fd062b2 100644 --- a/internal/command/receivepack/gitalycall_test.go +++ b/internal/command/receivepack/gitalycall_test.go @@ -2,7 +2,6 @@ package receivepack import ( "bytes" - "strings" "testing" "github.com/sirupsen/logrus" @@ -43,7 +42,10 @@ func TestReceivePack(t *testing.T) { require.NoError(t, err) require.Equal(t, "ReceivePack: "+userId+" "+repo, output.String()) - require.Equal(t, logrus.InfoLevel, hook.LastEntry().Level) - require.True(t, strings.Contains(hook.LastEntry().Message, "executing git command")) - require.True(t, strings.Contains(hook.LastEntry().Message, "command=git-receive-pack")) + + entries := hook.AllEntries() + require.Equal(t, 2, len(entries)) + require.Equal(t, logrus.InfoLevel, entries[1].Level) + require.Contains(t, entries[1].Message, "executing git command") + require.Contains(t, entries[1].Message, "command=git-receive-pack") } diff --git a/internal/command/uploadarchive/gitalycall_test.go b/internal/command/uploadarchive/gitalycall_test.go index 95274ee..8b60e11 100644 --- a/internal/command/uploadarchive/gitalycall_test.go +++ b/internal/command/uploadarchive/gitalycall_test.go @@ -2,7 +2,6 @@ package uploadarchive import ( "bytes" - "strings" "testing" "github.com/sirupsen/logrus" @@ -43,7 +42,9 @@ func TestUploadPack(t *testing.T) { require.NoError(t, err) require.Equal(t, "UploadArchive: "+repo, output.String()) - require.Equal(t, logrus.InfoLevel, hook.LastEntry().Level) - require.True(t, strings.Contains(hook.LastEntry().Message, "executing git command")) - require.True(t, strings.Contains(hook.LastEntry().Message, "command=git-upload-archive")) + entries := hook.AllEntries() + require.Equal(t, 2, len(entries)) + require.Equal(t, logrus.InfoLevel, entries[1].Level) + require.Contains(t, entries[1].Message, "executing git command") + require.Contains(t, entries[1].Message, "command=git-upload-archive") } diff --git a/internal/command/uploadpack/gitalycall_test.go b/internal/command/uploadpack/gitalycall_test.go index 4421d41..cf3e621 100644 --- a/internal/command/uploadpack/gitalycall_test.go +++ b/internal/command/uploadpack/gitalycall_test.go @@ -2,11 +2,8 @@ package uploadpack import ( "bytes" - "strings" "testing" - "github.com/sirupsen/logrus" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -44,9 +41,10 @@ func TestUploadPack(t *testing.T) { require.NoError(t, err) require.Equal(t, "UploadPack: "+repo, output.String()) - require.Equal(t, logrus.InfoLevel, hook.LastEntry().Level) - require.True(t, strings.Contains(hook.LastEntry().Message, "executing git command")) - require.True(t, strings.Contains(hook.LastEntry().Message, "command=git-upload-pack")) + entries := hook.AllEntries() + assert.Equal(t, 2, len(entries)) + require.Contains(t, entries[1].Message, "executing git command") + require.Contains(t, entries[1].Message, "command=git-upload-pack") for k, v := range map[string]string{ "gitaly-feature-cache_invalidator": "true", -- cgit v1.2.1