Merge branch 'smh-dont-assert-repo-path' into 'master'

Don't assert repository paths in errors in tests

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6023

Merged-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Reviewed-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Sami Hiltunen <shiltunen@gitlab.com>

5 files changed, 135 insertions, 52 deletions

diff --git a/internal/gitaly/service/ref/find_refs_by_oid_test.go b/internal/gitaly/service/ref/find_refs_by_oid_test.go
index 00a8b2481..cbeb5275d 100644
--- a/internal/gitaly/service/ref/find_refs_by_oid_test.go
+++ b/internal/gitaly/service/ref/find_refs_by_oid_test.go
@@ -125,13 +125,19 @@ func TestFindRefsByOID_failure(t *testing.T) {
 	ctx := testhelper.Context(t)
 	cfg, client := setupRefServiceWithoutRepo(t)
 
+	equalError := func(t *testing.T, expected error) func(error) {
+		return func(actual error) {
+			testhelper.RequireGrpcError(t, expected, actual)
+		}
+	}
+
 	testCases := []struct {
 		desc  string
-		setup func(t *testing.T) (*gitalypb.FindRefsByOIDRequest, error)
+		setup func(t *testing.T) (*gitalypb.FindRefsByOIDRequest, func(error))
 	}{
 		{
 			desc: "no ref exists for OID",
-			setup: func(t *testing.T) (*gitalypb.FindRefsByOIDRequest, error) {
+			setup: func(t *testing.T) (*gitalypb.FindRefsByOIDRequest, func(error)) {
 				repo, repoPath := gittest.CreateRepository(t, ctx, cfg)
 				oid := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("no ref exists for OID"))
 
@@ -143,7 +149,7 @@ func TestFindRefsByOID_failure(t *testing.T) {
 		},
 		{
 			desc: "repository is corrupted",
-			setup: func(t *testing.T) (*gitalypb.FindRefsByOIDRequest, error) {
+			setup: func(t *testing.T) (*gitalypb.FindRefsByOIDRequest, func(error)) {
 				repo, repoPath := gittest.CreateRepository(t, ctx, cfg)
 				oid := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("no ref exists for OID"))
 				gittest.Exec(t, cfg, "-C", repoPath, "update-ref", "refs/heads/corrupted-repo-branch", oid.String())
@@ -153,15 +159,20 @@ func TestFindRefsByOID_failure(t *testing.T) {
 				return &gitalypb.FindRefsByOIDRequest{
 						Repository: repo,
 						Oid:        oid.String(),
-					}, testhelper.WithInterceptedMetadata(
-						structerr.NewFailedPrecondition("%w: %q does not exist", storage.ErrRepositoryNotValid, "objects"),
-						"repository_path", repoPath,
-					)
+					}, func(actual error) {
+						testhelper.RequireStatusWithErrorMetadataRegexp(t,
+							structerr.NewFailedPrecondition("%w: %q does not exist", storage.ErrRepositoryNotValid, "objects"),
+							actual,
+							map[string]string{
+								"repository_path": ".+",
+							},
+						)
+					}
 			},
 		},
 		{
 			desc: "repository is missing",
-			setup: func(t *testing.T) (*gitalypb.FindRefsByOIDRequest, error) {
+			setup: func(t *testing.T) (*gitalypb.FindRefsByOIDRequest, func(error)) {
 				relativePath := gittest.NewRepositoryName(t)
 
 				return &gitalypb.FindRefsByOIDRequest{
@@ -170,14 +181,14 @@ func TestFindRefsByOID_failure(t *testing.T) {
 							RelativePath: relativePath,
 						},
 						Oid: strings.Repeat("a", gittest.DefaultObjectHash.EncodedLen()),
-					}, testhelper.ToInterceptedMetadata(
+					}, equalError(t, testhelper.ToInterceptedMetadata(
 						structerr.New("%w", storage.NewRepositoryNotFoundError(cfg.Storages[0].Name, relativePath)),
-					)
+					))
 			},
 		},
 		{
 			desc: "oid is not a commit",
-			setup: func(t *testing.T) (*gitalypb.FindRefsByOIDRequest, error) {
+			setup: func(t *testing.T) (*gitalypb.FindRefsByOIDRequest, func(error)) {
 				repo, repoPath := gittest.CreateRepository(t, ctx, cfg)
 				oid := gittest.WriteBlob(t, cfg, repoPath, []byte("the blob"))
 
@@ -189,7 +200,7 @@ func TestFindRefsByOID_failure(t *testing.T) {
 		},
 		{
 			desc: "oid prefix too short",
-			setup: func(t *testing.T) (*gitalypb.FindRefsByOIDRequest, error) {
+			setup: func(t *testing.T) (*gitalypb.FindRefsByOIDRequest, func(error)) {
 				repo, repoPath := gittest.CreateRepository(t, ctx, cfg)
 				oid := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("oid prefix too short"))
 				gittest.Exec(t, cfg, "-C", repoPath, "update-ref", "refs/heads/short-oid", oid.String())
@@ -197,7 +208,7 @@ func TestFindRefsByOID_failure(t *testing.T) {
 				return &gitalypb.FindRefsByOIDRequest{
 					Repository: repo,
 					Oid:        oid.String()[:2],
-				}, structerr.NewInvalidArgument("for-each-ref pipeline command: exit status 129")
+				}, equalError(t, structerr.NewInvalidArgument("for-each-ref pipeline command: exit status 129"))
 			},
 		},
 	}
@@ -208,11 +219,16 @@ func TestFindRefsByOID_failure(t *testing.T) {
 		t.Run(tc.desc, func(t *testing.T) {
 			t.Parallel()
 
-			request, expectedErr := tc.setup(t)
+			request, requireError := tc.setup(t)
 
 			response, err := client.FindRefsByOID(ctx, request)
 			require.Empty(t, response.GetRefs())
-			testhelper.RequireGrpcError(t, expectedErr, err)
+			if requireError != nil {
+				requireError(err)
+				return
+			}
+
+			require.NoError(t, err)
 		})
 	}
 }
diff --git a/internal/gitaly/service/repository/config_test.go b/internal/gitaly/service/repository/config_test.go
index 80f85689f..91f357938 100644
--- a/internal/gitaly/service/repository/config_test.go
+++ b/internal/gitaly/service/repository/config_test.go
@@ -15,7 +15,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/v16/streamio"
 	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
 func TestGetConfig(t *testing.T) {
@@ -71,7 +70,8 @@ func TestGetConfig(t *testing.T) {
 		require.NoError(t, os.Remove(configPath))
 
 		config, err := getConfig(t, client, repo)
-		testhelper.RequireGrpcError(t, status.Errorf(codes.NotFound, "opening gitconfig: open %s: no such file or directory", configPath), err)
+		testhelper.RequireGrpcCode(t, err, codes.NotFound)
+		require.Regexp(t, "^rpc error: code = NotFound desc = opening gitconfig: open .+/config: no such file or directory$", err.Error())
 		require.Equal(t, "", config)
 	})
 
diff --git a/internal/gitaly/service/repository/fsck_test.go b/internal/gitaly/service/repository/fsck_test.go
index c7401ed9a..1f69f4009 100644
--- a/internal/gitaly/service/repository/fsck_test.go
+++ b/internal/gitaly/service/repository/fsck_test.go
@@ -1,7 +1,6 @@
 package repository
 
 import (
-	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
@@ -22,10 +21,14 @@ func TestFsck(t *testing.T) {
 	ctx := testhelper.Context(t)
 	cfg, client := setupRepositoryServiceWithoutRepo(t)
 
+	equalResponse := func(tb testing.TB, expected *gitalypb.FsckResponse) func(*gitalypb.FsckResponse) {
+		return func(actual *gitalypb.FsckResponse) { testhelper.ProtoEqual(tb, expected, actual) }
+	}
+
 	type setupData struct {
-		repo             *gitalypb.Repository
-		expectedErr      error
-		expectedResponse *gitalypb.FsckResponse
+		repo            *gitalypb.Repository
+		expectedErr     error
+		requireResponse func(*gitalypb.FsckResponse)
 	}
 
 	for _, tc := range []struct {
@@ -47,8 +50,8 @@ func TestFsck(t *testing.T) {
 				repo, _ := gittest.CreateRepository(t, ctx, cfg)
 
 				return setupData{
-					repo:             repo,
-					expectedResponse: &gitalypb.FsckResponse{},
+					repo:            repo,
+					requireResponse: equalResponse(t, &gitalypb.FsckResponse{}),
 				}
 			},
 		},
@@ -59,8 +62,8 @@ func TestFsck(t *testing.T) {
 				gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
 
 				return setupData{
-					repo:             repo,
-					expectedResponse: &gitalypb.FsckResponse{},
+					repo:            repo,
+					requireResponse: equalResponse(t, &gitalypb.FsckResponse{}),
 				}
 			},
 		},
@@ -76,9 +79,8 @@ func TestFsck(t *testing.T) {
 
 				return setupData{
 					repo: repo,
-					expectedResponse: &gitalypb.FsckResponse{
-						//nolint:gitaly-linters
-						Error: []byte(fmt.Sprintf("fatal: not a git repository: '%s'\n", repoPath)),
+					requireResponse: func(actual *gitalypb.FsckResponse) {
+						require.Regexp(t, "^fatal: not a git repository: '.+'\n$", string(actual.Error))
 					},
 				}
 			},
@@ -103,9 +105,9 @@ func TestFsck(t *testing.T) {
 
 				return setupData{
 					repo: repo,
-					expectedResponse: &gitalypb.FsckResponse{
+					requireResponse: equalResponse(t, &gitalypb.FsckResponse{
 						Error: []byte(expectedErr),
-					},
+					}),
 				}
 			},
 		},
@@ -120,8 +122,8 @@ func TestFsck(t *testing.T) {
 				gittest.WriteBlob(t, cfg, repoPath, []byte("content"))
 
 				return setupData{
-					repo:             repo,
-					expectedResponse: &gitalypb.FsckResponse{},
+					repo:            repo,
+					requireResponse: equalResponse(t, &gitalypb.FsckResponse{}),
 				}
 			},
 		},
@@ -150,9 +152,9 @@ func TestFsck(t *testing.T) {
 
 				return setupData{
 					repo: repo,
-					expectedResponse: &gitalypb.FsckResponse{
+					requireResponse: equalResponse(t, &gitalypb.FsckResponse{
 						Error: []byte(expectedErr),
-					},
+					}),
 				}
 			},
 		},
@@ -167,8 +169,13 @@ func TestFsck(t *testing.T) {
 			response, err := client.Fsck(ctx, &gitalypb.FsckRequest{
 				Repository: setupData.repo,
 			})
-			testhelper.RequireGrpcError(t, setupData.expectedErr, err)
-			testhelper.ProtoEqual(t, setupData.expectedResponse, response)
+			if setupData.expectedErr != nil {
+				testhelper.RequireGrpcError(t, setupData.expectedErr, err)
+				return
+			}
+
+			require.NoError(t, err)
+			setupData.requireResponse(response)
 		})
 	}
 }
diff --git a/internal/gitaly/service/repository/object_format_test.go b/internal/gitaly/service/repository/object_format_test.go
index 11ddb12b6..77da79547 100644
--- a/internal/gitaly/service/repository/object_format_test.go
+++ b/internal/gitaly/service/repository/object_format_test.go
@@ -1,7 +1,6 @@
 package repository
 
 import (
-	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
@@ -22,9 +21,16 @@ func TestObjectFormat(t *testing.T) {
 	ctx := testhelper.Context(t)
 	cfg, client := setupRepositoryServiceWithoutRepo(t)
 
+	equalError := func(tb testing.TB, expected error) func(error) {
+		return func(actual error) {
+			tb.Helper()
+			testhelper.RequireGrpcError(tb, expected, actual)
+		}
+	}
+
 	type setupData struct {
 		request          *gitalypb.ObjectFormatRequest
-		expectedErr      error
+		requireError     func(error)
 		expectedResponse *gitalypb.ObjectFormatResponse
 	}
 
@@ -36,8 +42,8 @@ func TestObjectFormat(t *testing.T) {
 			desc: "unset repository",
 			setup: func(t *testing.T) setupData {
 				return setupData{
-					request:     &gitalypb.ObjectFormatRequest{},
-					expectedErr: structerr.NewInvalidArgument("%w", storage.ErrRepositoryNotSet),
+					request:      &gitalypb.ObjectFormatRequest{},
+					requireError: equalError(t, structerr.NewInvalidArgument("%w", storage.ErrRepositoryNotSet)),
 				}
 			},
 		},
@@ -50,7 +56,7 @@ func TestObjectFormat(t *testing.T) {
 							RelativePath: "path",
 						},
 					},
-					expectedErr: structerr.NewInvalidArgument("%w", storage.ErrStorageNotSet),
+					requireError: equalError(t, structerr.NewInvalidArgument("%w", storage.ErrStorageNotSet)),
 				}
 			},
 		},
@@ -63,7 +69,7 @@ func TestObjectFormat(t *testing.T) {
 							StorageName: cfg.Storages[0].Name,
 						},
 					},
-					expectedErr: structerr.NewInvalidArgument("%w", storage.ErrRepositoryPathNotSet),
+					requireError: equalError(t, structerr.NewInvalidArgument("%w", storage.ErrRepositoryPathNotSet)),
 				}
 			},
 		},
@@ -77,9 +83,9 @@ func TestObjectFormat(t *testing.T) {
 							RelativePath: "nonexistent.git",
 						},
 					},
-					expectedErr: testhelper.ToInterceptedMetadata(
+					requireError: equalError(t, testhelper.ToInterceptedMetadata(
 						structerr.New("%w", storage.NewRepositoryNotFoundError(cfg.Storages[0].Name, "nonexistent.git")),
-					),
+					)),
 				}
 			},
 		},
@@ -138,13 +144,15 @@ func TestObjectFormat(t *testing.T) {
 					request: &gitalypb.ObjectFormatRequest{
 						Repository: repoProto,
 					},
-					expectedErr: testhelper.WithInterceptedMetadata(
-						structerr.NewInternal("detecting object hash: reading object format: exit status 128"),
-						"stderr",
-						fmt.Sprintf("error: invalid value for 'extensions.objectformat': 'blake2b'\n"+
-							"fatal: bad config line 5 in file %s\n", filepath.Join(repoPath, "config"),
-						),
-					),
+					requireError: func(actual error) {
+						testhelper.RequireStatusWithErrorMetadataRegexp(t,
+							structerr.NewInternal("detecting object hash: reading object format: exit status 128"),
+							actual,
+							map[string]string{
+								"stderr": "^error: invalid value for 'extensions.objectformat': 'blake2b'\nfatal: bad config line 5 in file .+/config\n$",
+							},
+						)
+					},
 				}
 			},
 		},
@@ -156,7 +164,12 @@ func TestObjectFormat(t *testing.T) {
 
 			setupData := tc.setup(t)
 			response, err := client.ObjectFormat(ctx, setupData.request)
-			testhelper.RequireGrpcError(t, setupData.expectedErr, err)
+			if setupData.requireError != nil {
+				setupData.requireError(err)
+				return
+			}
+
+			require.NoError(t, err)
 			testhelper.ProtoEqual(t, setupData.expectedResponse, response)
 		})
 	}
diff --git a/internal/testhelper/grpc.go b/internal/testhelper/grpc.go
index 375b54efc..f60110566 100644
--- a/internal/testhelper/grpc.go
+++ b/internal/testhelper/grpc.go
@@ -72,6 +72,42 @@ func RequireGrpcError(tb testing.TB, expected, actual error) {
 	ProtoEqual(tb, status.Convert(expected).Proto(), status.Convert(actual).Proto())
 }
 
+// RequireStatusWithErrorMetadataRegexp asserts that expected and actual error match each other. Both are expected to
+// be status errors. The error metadata in the status is matched against regular expressions defined in expectedMetadata.
+// expectedMetadata is keyed by error metadata key, and the value is a regex string that the value is expected to match.
+//
+// This method is useful when the error metadata contains values that should not be asserted for equality like changing paths.
+// If the metadata is expected to be equal, use an equality assertion instead.
+func RequireStatusWithErrorMetadataRegexp(tb testing.TB, expected, actual error, expectedMetadata map[string]string) {
+	tb.Helper()
+
+	actualStatus, ok := status.FromError(actual)
+	require.True(tb, ok, "actual was not a status: %+v", actual)
+
+	actualDetails := actualStatus.Details()
+
+	actualWithoutMetadata := actualStatus.Proto()
+	actualWithoutMetadata.Details = nil
+	RequireGrpcError(tb, expected, status.ErrorProto(actualWithoutMetadata))
+
+	actualKeys := make([]string, 0, len(actualDetails))
+	for _, detail := range actualDetails {
+		actualKeys = append(actualKeys, string(detail.(*testproto.ErrorMetadata).Key))
+	}
+
+	expectedKeys := make([]string, 0, len(expectedMetadata))
+	for key := range expectedMetadata {
+		expectedKeys = append(expectedKeys, key)
+	}
+
+	require.ElementsMatch(tb, expectedKeys, actualKeys, "actual metadata keys don't match expected")
+
+	for _, detail := range actualDetails {
+		metadata := detail.(*testproto.ErrorMetadata)
+		require.Regexp(tb, expectedMetadata[string(metadata.Key)], string(metadata.Value), "metadata key %q's value didn't match expected", metadata.Key)
+	}
+}
+
 // MergeOutgoingMetadata merges provided metadata-s and returns context with resulting value.
 func MergeOutgoingMetadata(ctx context.Context, md ...metadata.MD) context.Context {
 	ctxmd, ok := metadata.FromOutgoingContext(ctx)
@@ -97,6 +133,17 @@ func MergeIncomingMetadata(ctx context.Context, md ...metadata.MD) context.Conte
 // metadata into structured errors via the StructErrUnaryInterceptor and StructErrStreamInterceptor so that we can
 // test that metadata has been set as expected on the client-side of a gRPC call.
 func WithInterceptedMetadata(err structerr.Error, key string, value any) structerr.Error {
+	if key == "relative_path" {
+		// There are a number of tests that assert the returned error metadata for equality.
+		// The relative path might be rewritten before the handler which leads to the equality
+		// checks failing. Override the returned relative path so the actual values are not asserted,
+		// just that the key is present.
+		//
+		// This is not really an ideal fix as this overrides a value magically. To remove this, we'll
+		// have to adjust every test that is asserting the relative paths to not do so.
+		value = "OVERRIDDEN_BY_TESTHELPER"
+	}
+
 	return err.WithDetail(&testproto.ErrorMetadata{
 		Key:   []byte(key),
 		Value: []byte(fmt.Sprintf("%v", value)),