From 1aef8477fffe8ef7a758de461b9667b50f8be6bd Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Thu, 23 Feb 2023 11:53:24 -0500 Subject: hook: Remove require calls from helper goroutines As mentioned in the previous commit, `require` calls `t.FailNow()`, which cannot safely be used in helper goroutines. Convert ``TestServer_PackObjectsHook_separateContext` to use `assert` outside of the main goroutine. --- internal/gitaly/service/hook/pack_objects_test.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'internal/gitaly/service') diff --git a/internal/gitaly/service/hook/pack_objects_test.go b/internal/gitaly/service/hook/pack_objects_test.go index a4b562240..99e941e26 100644 --- a/internal/gitaly/service/hook/pack_objects_test.go +++ b/internal/gitaly/service/hook/pack_objects_test.go @@ -16,6 +16,7 @@ import ( "github.com/prometheus/client_golang/prometheus/testutil" "github.com/sirupsen/logrus" "github.com/sirupsen/logrus/hooks/test" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v15/internal/git" "gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest" @@ -149,8 +150,8 @@ func testServerPackObjectsHookSeparateContextWithRuntimeDir(t *testing.T, ctx co go func() { defer wg.Done() _, err := client1.PackObjectsHookWithSidechannel(ctx1, req) - testhelper.RequireGrpcCode(t, err, codes.Canceled) - require.NoError(t, wt1.Wait()) + testhelper.AssertGrpcCode(t, err, codes.Canceled) + assert.NoError(t, wt1.Wait()) }() // Call 2: this is a normal call with the same request as call 1 @@ -189,8 +190,8 @@ func testServerPackObjectsHookSeparateContextWithRuntimeDir(t *testing.T, ctx co go func() { defer wg.Done() _, err := client2.PackObjectsHookWithSidechannel(ctx2, req) - require.NoError(t, err) - require.NoError(t, wt2.Wait()) + assert.NoError(t, err) + assert.NoError(t, wt2.Wait()) }() close(start1) -- cgit v1.2.3 From ba14537e5412e53695cc3f9c41068c60d03856d1 Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Thu, 23 Feb 2023 12:00:00 -0500 Subject: hooks: Fix flaky test on MacOS We have observed intermittent failures on `TestServer_PackObjectsHook_separateContext` on our MacOS runners. The error occurs ~10% of the time on MacOS, but has been completely unreproducible on Linux. This appears to be a long-standing issue with FreeBSD, dating back to 2005[0], which MacOS has inherited. Unix domain sockets that are closed after a partial read may return `ENOTCONN`, instead of `EPIPE` as seen on Linux. This is exactly the scenario performed in this test. Using the original reproduction script provided in the linked bug report, the `ENOTCONN` issue occurs ~10% of the time, matching Gitaly's failure rate. Given that this functionality works as expected on Linux and the OS itself is what is non-deterministic, the simplest fix is to accept either `EPIPE` or `ENOTCONN` when on MacOS. We are forced to check for the expected error string instead of `errors.Is(err, syscall.ENOTCONN)` as the syscall information is lost when returned over gRPC to the test program. [0] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=79138 --- internal/gitaly/service/hook/pack_objects_test.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) (limited to 'internal/gitaly/service') diff --git a/internal/gitaly/service/hook/pack_objects_test.go b/internal/gitaly/service/hook/pack_objects_test.go index 99e941e26..8d1344983 100644 --- a/internal/gitaly/service/hook/pack_objects_test.go +++ b/internal/gitaly/service/hook/pack_objects_test.go @@ -8,6 +8,7 @@ import ( "fmt" "io" "net" + "runtime" "strings" "sync" "testing" @@ -150,7 +151,17 @@ func testServerPackObjectsHookSeparateContextWithRuntimeDir(t *testing.T, ctx co go func() { defer wg.Done() _, err := client1.PackObjectsHookWithSidechannel(ctx1, req) - testhelper.AssertGrpcCode(t, err, codes.Canceled) + + if runtime.GOOS == "darwin" { + assert.Contains(t, []codes.Code{codes.Canceled, codes.Internal}, status.Code(err)) + + if status.Code(err) == codes.Internal { + assert.Contains(t, err.Error(), "write: socket is not connected") + } + } else { + testhelper.AssertGrpcCode(t, err, codes.Canceled) + } + assert.NoError(t, wt1.Wait()) }() -- cgit v1.2.3