Merge branch 'pks-protobuf-changes' into 'master'

Future-proof our use of Protobufs in tests

See merge request gitlab-org/gitaly!2299

14 files changed, 49 insertions, 27 deletions

diff --git a/cmd/praefect/subcmd_enable_writes_test.go b/cmd/praefect/subcmd_enable_writes_test.go
index 179a980f1..201ae6907 100644
--- a/cmd/praefect/subcmd_enable_writes_test.go
+++ b/cmd/praefect/subcmd_enable_writes_test.go
@@ -4,9 +4,9 @@ import (
 	"context"
 	"testing"
 
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/config"
+	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 )
 
@@ -37,7 +37,7 @@ func TestEnableWritesSubcommand(t *testing.T) {
 			args: []string{"-virtual-storage=passed-storage"},
 			enableWritesFunc: func(t testing.TB) EnableWritesFunc {
 				return func(_ context.Context, req *gitalypb.EnableWritesRequest) (*gitalypb.EnableWritesResponse, error) {
-					assert.Equal(t, &gitalypb.EnableWritesRequest{
+					testhelper.ProtoEqual(t, &gitalypb.EnableWritesRequest{
 						VirtualStorage: "passed-storage",
 					}, req)
 					return &gitalypb.EnableWritesResponse{}, nil
diff --git a/internal/praefect/protoregistry/protoregistry_test.go b/internal/praefect/protoregistry/protoregistry_test.go
index 3f898211e..f0db8b48a 100644
--- a/internal/praefect/protoregistry/protoregistry_test.go
+++ b/internal/praefect/protoregistry/protoregistry_test.go
@@ -7,6 +7,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/protoregistry"
+	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 )
 
@@ -192,7 +193,7 @@ func TestRequestFactory(t *testing.T) {
 	pb, err := mInfo.UnmarshalRequestProto([]byte{})
 	require.NoError(t, err)
 
-	require.Exactly(t, &gitalypb.RepositoryExistsRequest{}, pb)
+	testhelper.ProtoEqual(t, &gitalypb.RepositoryExistsRequest{}, pb)
 }
 
 func TestMethodInfoScope(t *testing.T) {
diff --git a/internal/praefect/replicator_test.go b/internal/praefect/replicator_test.go
index 5c1e67670..bee946d78 100644
--- a/internal/praefect/replicator_test.go
+++ b/internal/praefect/replicator_test.go
@@ -10,7 +10,7 @@ import (
 	"testing"
 	"time"
 
-	"github.com/stretchr/testify/assert"
+	"github.com/golang/protobuf/proto"
 	"github.com/stretchr/testify/require"
 	gitalyauth "gitlab.com/gitlab-org/gitaly/auth"
 	gitaly_config "gitlab.com/gitlab-org/gitaly/internal/config"
@@ -261,15 +261,15 @@ func TestPropagateReplicationJob(t *testing.T) {
 	require.NoError(t, err)
 
 	primaryRepository := &gitalypb.Repository{StorageName: primaryStorage, RelativePath: repositoryRelativePath}
-	expectedPrimaryGcReq := gitalypb.GarbageCollectRequest{
+	expectedPrimaryGcReq := &gitalypb.GarbageCollectRequest{
 		Repository:   primaryRepository,
 		CreateBitmap: true,
 	}
-	expectedPrimaryRepackFullReq := gitalypb.RepackFullRequest{
+	expectedPrimaryRepackFullReq := &gitalypb.RepackFullRequest{
 		Repository:   primaryRepository,
 		CreateBitmap: false,
 	}
-	expectedPrimaryRepackIncrementalReq := gitalypb.RepackIncrementalRequest{
+	expectedPrimaryRepackIncrementalReq := &gitalypb.RepackIncrementalRequest{
 		Repository: primaryRepository,
 	}
 
@@ -300,36 +300,36 @@ func TestPropagateReplicationJob(t *testing.T) {
 }
 
 type mockRepositoryServer struct {
-	gcChan, repackFullChan, repackIncrChan chan interface{}
+	gcChan, repackFullChan, repackIncrChan chan proto.Message
 
 	gitalypb.UnimplementedRepositoryServiceServer
 }
 
 func newMockRepositoryServer() *mockRepositoryServer {
 	return &mockRepositoryServer{
-		gcChan:         make(chan interface{}),
-		repackFullChan: make(chan interface{}),
-		repackIncrChan: make(chan interface{}),
+		gcChan:         make(chan proto.Message),
+		repackFullChan: make(chan proto.Message),
+		repackIncrChan: make(chan proto.Message),
 	}
 }
 
 func (m *mockRepositoryServer) GarbageCollect(ctx context.Context, in *gitalypb.GarbageCollectRequest) (*gitalypb.GarbageCollectResponse, error) {
 	go func() {
-		m.gcChan <- *in
+		m.gcChan <- in
 	}()
 	return &gitalypb.GarbageCollectResponse{}, nil
 }
 
 func (m *mockRepositoryServer) RepackFull(ctx context.Context, in *gitalypb.RepackFullRequest) (*gitalypb.RepackFullResponse, error) {
 	go func() {
-		m.repackFullChan <- *in
+		m.repackFullChan <- in
 	}()
 	return &gitalypb.RepackFullResponse{}, nil
 }
 
 func (m *mockRepositoryServer) RepackIncremental(ctx context.Context, in *gitalypb.RepackIncrementalRequest) (*gitalypb.RepackIncrementalResponse, error) {
 	go func() {
-		m.repackIncrChan <- *in
+		m.repackIncrChan <- in
 	}()
 	return &gitalypb.RepackIncrementalResponse{}, nil
 }
@@ -351,12 +351,12 @@ func runMockRepositoryServer(t *testing.T) (*mockRepositoryServer, string, func(
 	return mockServer, "unix://" + serverSocketPath, server.Stop
 }
 
-func waitForRequest(t *testing.T, ch chan interface{}, expected interface{}, timeout time.Duration) {
+func waitForRequest(t *testing.T, ch chan proto.Message, expected proto.Message, timeout time.Duration) {
 	timer := time.NewTimer(timeout)
 	defer timer.Stop()
 	select {
 	case req := <-ch:
-		assert.Equal(t, expected, req)
+		testhelper.ProtoEqual(t, expected, req)
 		close(ch)
 	case <-timer.C:
 		t.Fatal("timed out")
diff --git a/internal/rubyserver/balancer/balancer_test.go b/internal/rubyserver/balancer/balancer_test.go
index a83f79caa..269a37ece 100644
--- a/internal/rubyserver/balancer/balancer_test.go
+++ b/internal/rubyserver/balancer/balancer_test.go
@@ -183,6 +183,8 @@ type action struct {
 }
 
 type testClientConn struct {
+	resolver.ClientConn
+
 	addrUpdates   [][]resolver.Address
 	configUpdates []string
 	mu            sync.Mutex
diff --git a/internal/service/blob/get_blobs_test.go b/internal/service/blob/get_blobs_test.go
index 8ecfcb696..f6a52a5f3 100644
--- a/internal/service/blob/get_blobs_test.go
+++ b/internal/service/blob/get_blobs_test.go
@@ -126,7 +126,7 @@ func TestSuccessfulGetBlobsRequest(t *testing.T) {
 					expectedBlob.Data = expectedBlob.Data[:limit]
 				}
 
-				require.Equal(t, expectedBlob, receivedBlob)
+				testhelper.ProtoEqual(t, expectedBlob, receivedBlob)
 			}
 		})
 	}
diff --git a/internal/service/commit/commit_messages_test.go b/internal/service/commit/commit_messages_test.go
index 7d6b97e0c..e57602867 100644
--- a/internal/service/commit/commit_messages_test.go
+++ b/internal/service/commit/commit_messages_test.go
@@ -51,7 +51,9 @@ func TestSuccessfulGetCommitMessagesRequest(t *testing.T) {
 	}
 	fetchedMessages := readAllMessagesFromClient(t, c)
 
-	require.Equal(t, expectedMessages, fetchedMessages)
+	require.Len(t, fetchedMessages, len(expectedMessages))
+	testhelper.ProtoEqual(t, expectedMessages[0], fetchedMessages[0])
+	testhelper.ProtoEqual(t, expectedMessages[1], fetchedMessages[1])
 }
 
 func TestFailedGetCommitMessagesRequest(t *testing.T) {
diff --git a/internal/service/commit/commit_signatures_test.go b/internal/service/commit/commit_signatures_test.go
index 11ae70b56..086b2a885 100644
--- a/internal/service/commit/commit_signatures_test.go
+++ b/internal/service/commit/commit_signatures_test.go
@@ -69,7 +69,10 @@ func TestSuccessfulGetCommitSignaturesRequest(t *testing.T) {
 
 	fetchedSignatures := readAllSignaturesFromClient(t, c)
 
-	require.Equal(t, expectedSignatures, fetchedSignatures)
+	require.Len(t, fetchedSignatures, len(expectedSignatures))
+	for i, expected := range expectedSignatures {
+		testhelper.ProtoEqual(t, expected, fetchedSignatures[i])
+	}
 }
 
 func TestFailedGetCommitSignaturesRequest(t *testing.T) {
diff --git a/internal/service/operations/merge_test.go b/internal/service/operations/merge_test.go
index bb5ef67aa..ea2a5997e 100644
--- a/internal/service/operations/merge_test.go
+++ b/internal/service/operations/merge_test.go
@@ -219,7 +219,7 @@ func TestFailedMergeConcurrentUpdate(t *testing.T) {
 
 	secondResponse, err := mergeBidi.Recv()
 	require.NoError(t, err, "receive second response")
-	require.Equal(t, *secondResponse, gitalypb.UserMergeBranchResponse{}, "response should be empty")
+	testhelper.ProtoEqual(t, secondResponse, &gitalypb.UserMergeBranchResponse{})
 
 	commit, err := gitlog.GetCommit(ctx, testRepo, mergeBranchName)
 	require.NoError(t, err, "get commit after RPC finished")
@@ -319,7 +319,7 @@ func TestSuccessfulUserFFBranchRequest(t *testing.T) {
 
 	resp, err := client.UserFFBranch(ctx, request)
 	require.NoError(t, err)
-	require.Equal(t, expectedResponse, resp)
+	testhelper.ProtoEqual(t, expectedResponse, resp)
 	newBranchHead := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", branchName)
 	require.Equal(t, commitID, text.ChompBytes(newBranchHead), "branch head not updated")
 }
diff --git a/internal/service/ref/tag_messages_test.go b/internal/service/ref/tag_messages_test.go
index 8c512c912..16dac869b 100644
--- a/internal/service/ref/tag_messages_test.go
+++ b/internal/service/ref/tag_messages_test.go
@@ -51,7 +51,9 @@ func TestSuccessfulGetTagMessagesRequest(t *testing.T) {
 	require.NoError(t, err)
 
 	fetchedMessages := readAllMessagesFromClient(t, c)
-	require.Equal(t, expectedMessages, fetchedMessages)
+	require.Len(t, fetchedMessages, len(expectedMessages))
+	testhelper.ProtoEqual(t, expectedMessages[0], fetchedMessages[0])
+	testhelper.ProtoEqual(t, expectedMessages[1], fetchedMessages[1])
 }
 
 func TestFailedGetTagMessagesRequest(t *testing.T) {
diff --git a/internal/service/repository/create_from_snapshot_test.go b/internal/service/repository/create_from_snapshot_test.go
index 912432759..f91f04638 100644
--- a/internal/service/repository/create_from_snapshot_test.go
+++ b/internal/service/repository/create_from_snapshot_test.go
@@ -93,7 +93,7 @@ func TestCreateRepositoryFromSnapshotSuccess(t *testing.T) {
 	rsp, err := createFromSnapshot(t, req)
 
 	require.NoError(t, err)
-	require.Equal(t, rsp, &gitalypb.CreateRepositoryFromSnapshotResponse{})
+	testhelper.ProtoEqual(t, rsp, &gitalypb.CreateRepositoryFromSnapshotResponse{})
 
 	require.DirExists(t, repoPath)
 	for _, entry := range entries {
diff --git a/internal/service/repository/snapshot_test.go b/internal/service/repository/snapshot_test.go
index c36af632c..8659b00b0 100644
--- a/internal/service/repository/snapshot_test.go
+++ b/internal/service/repository/snapshot_test.go
@@ -245,7 +245,7 @@ func copyRepoUsingSnapshot(t *testing.T, source *gitalypb.Repository) (*gitalypb
 
 	rsp, err := createFromSnapshot(t, createRepoReq)
 	require.NoError(t, err)
-	require.Equal(t, rsp, &gitalypb.CreateRepositoryFromSnapshotResponse{})
+	testhelper.ProtoEqual(t, rsp, &gitalypb.CreateRepositoryFromSnapshotResponse{})
 	return repoCopy, repoCopyPath, cleanupCopy
 }
 
diff --git a/internal/service/wiki/find_file_test.go b/internal/service/wiki/find_file_test.go
index 1ff13871f..eea64cc52 100644
--- a/internal/service/wiki/find_file_test.go
+++ b/internal/service/wiki/find_file_test.go
@@ -144,7 +144,7 @@ func TestSuccessfulWikiFindFileRequest(t *testing.T) {
 				receivedResponse.RawData = nil
 			}
 
-			require.Equal(t, expectedResponse, receivedResponse, "mismatched response")
+			testhelper.ProtoEqual(t, expectedResponse, receivedResponse)
 			if len(expectedResponse.Name) > 0 {
 				require.Equal(t, testCase.expectedContent, receivedContent, "mismatched content")
 			}
diff --git a/internal/service/wiki/write_page_test.go b/internal/service/wiki/write_page_test.go
index 37b1ce86d..f9ef38d87 100644
--- a/internal/service/wiki/write_page_test.go
+++ b/internal/service/wiki/write_page_test.go
@@ -155,7 +155,7 @@ func TestFailedWikiWritePageDueToDuplicatePage(t *testing.T) {
 	require.NoError(t, err)
 
 	expectedResponse := &gitalypb.WikiWritePageResponse{DuplicateError: []byte("Cannot write //Installing-Gitaly.md, found //Installing-Gitaly.md.")}
-	require.Equal(t, expectedResponse, response, "mismatched response")
+	testhelper.ProtoEqual(t, expectedResponse, response)
 }
 
 func TestFailedWikiWritePageInPathDueToDuplicatePage(t *testing.T) {
@@ -200,7 +200,7 @@ func TestFailedWikiWritePageInPathDueToDuplicatePage(t *testing.T) {
 	require.NoError(t, err)
 
 	expectedResponse := &gitalypb.WikiWritePageResponse{DuplicateError: []byte("Cannot write foo/Installing-Gitaly.md, found foo/Installing-Gitaly.md.")}
-	require.Equal(t, expectedResponse, response, "mismatched response")
+	testhelper.ProtoEqual(t, expectedResponse, response)
 }
 
 func TestFailedWikiWritePageDueToValidations(t *testing.T) {
diff --git a/internal/testhelper/proto.go b/internal/testhelper/proto.go
new file mode 100644
index 000000000..95a895071
--- /dev/null
+++ b/internal/testhelper/proto.go
@@ -0,0 +1,12 @@
+package testhelper
+
+import (
+	"testing"
+
+	"github.com/golang/protobuf/proto"
+	"github.com/stretchr/testify/require"
+)
+
+func ProtoEqual(t testing.TB, expected proto.Message, actual proto.Message) {
+	require.True(t, proto.Equal(expected, actual), "proto messages not equal\nexpected: %v\ngot:      %v", expected, actual)
+}