From d0f2fba0ce4b859852eb7492a4302ad4924fa71d Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Thu, 23 Feb 2023 11:41:07 -0500 Subject: testhelper: Add AssertGrpcCode helper function Most of our testhelper functions use the `require` family of assertions, which internally use use `t.FailNow()`. However, this is not safe for use goroutines other than the one actually running the test function. Calling it from a helper goroutine will not determinstically stop the test and may lead to spurious failures from the race detector. Add an `AssertGrpcCode` function that uses `assert`, which can safely be called from a helper goroutine. --- internal/testhelper/grpc.go | 11 +++++++++++ 1 file changed, 11 insertions(+) (limited to 'internal') diff --git a/internal/testhelper/grpc.go b/internal/testhelper/grpc.go index 8cf96b326..3815bc6e3 100644 --- a/internal/testhelper/grpc.go +++ b/internal/testhelper/grpc.go @@ -6,6 +6,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/grpc" "google.golang.org/grpc/codes" @@ -50,6 +51,16 @@ func RequireGrpcCode(tb testing.TB, err error, expectedCode codes.Code) { require.Equal(tb, expectedCode, status.Code()) } +// AssertGrpcCode asserts that the error has the expected gRPC status code. +func AssertGrpcCode(tb testing.TB, err error, expectedCode codes.Code) { + tb.Helper() + + assert.Error(tb, err) + status, ok := status.FromError(err) + assert.True(tb, ok) + assert.Equal(tb, expectedCode, status.Code()) +} + // RequireGrpcError asserts that expected and actual gRPC errors are equal. Comparing gRPC errors // directly with `require.Equal()` will not typically work correct. func RequireGrpcError(tb testing.TB, expected, actual error) { -- cgit v1.2.3 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') 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') 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