Merge branch 'smh-dont-asser-paths' into 'master'

Don't assert relative paths in various tests

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

Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: John Cai <jcai@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Sami Hiltunen <shiltunen@gitlab.com>

4 files changed, 147 insertions, 107 deletions

diff --git a/internal/gitaly/service/operations/branches_test.go b/internal/gitaly/service/operations/branches_test.go
index 970db09b5..4458de296 100644
--- a/internal/gitaly/service/operations/branches_test.go
+++ b/internal/gitaly/service/operations/branches_test.go
@@ -984,51 +984,49 @@ func TestBranchHookOutput(t *testing.T) {
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	testCases := []struct {
-		desc           string
-		hookContent    string
-		output         func(hookPath string) string
-		expectedStderr string
-		expectedStdout string
+		desc                string
+		hookContent         string
+		expectedErrorRegexp string
+		expectedStderr      string
+		expectedStdout      string
 	}{
 		{
-			desc:        "empty stdout and empty stderr",
-			hookContent: "#!/bin/sh\nexit 1",
-			output: func(hookPath string) string {
-				return fmt.Sprintf("executing custom hooks: error executing %q: exit status 1", hookPath)
-			},
+			desc:                "empty stdout and empty stderr",
+			hookContent:         "#!/bin/sh\nexit 1",
+			expectedErrorRegexp: `executing custom hooks: error executing .+: exit status 1`,
 		},
 		{
-			desc:           "empty stdout and some stderr",
-			hookContent:    "#!/bin/sh\necho stderr >&2\nexit 1",
-			output:         func(string) string { return "stderr\n" },
-			expectedStderr: "stderr\n",
+			desc:                "empty stdout and some stderr",
+			hookContent:         "#!/bin/sh\necho stderr >&2\nexit 1",
+			expectedErrorRegexp: "stderr\n",
+			expectedStderr:      "stderr\n",
 		},
 		{
-			desc:           "some stdout and empty stderr",
-			hookContent:    "#!/bin/sh\necho stdout\nexit 1",
-			output:         func(string) string { return "stdout\n" },
-			expectedStdout: "stdout\n",
+			desc:                "some stdout and empty stderr",
+			hookContent:         "#!/bin/sh\necho stdout\nexit 1",
+			expectedErrorRegexp: "stdout\n",
+			expectedStdout:      "stdout\n",
 		},
 		{
-			desc:           "some stdout and some stderr",
-			hookContent:    "#!/bin/sh\necho stdout\necho stderr >&2\nexit 1",
-			output:         func(string) string { return "stderr\n" },
-			expectedStderr: "stderr\n",
-			expectedStdout: "stdout\n",
+			desc:                "some stdout and some stderr",
+			hookContent:         "#!/bin/sh\necho stdout\necho stderr >&2\nexit 1",
+			expectedErrorRegexp: "stderr\n",
+			expectedStderr:      "stderr\n",
+			expectedStdout:      "stdout\n",
 		},
 		{
-			desc:           "whitespace stdout and some stderr",
-			hookContent:    "#!/bin/sh\necho '   '\necho stderr >&2\nexit 1",
-			output:         func(string) string { return "stderr\n" },
-			expectedStderr: "stderr\n",
-			expectedStdout: "   \n",
+			desc:                "whitespace stdout and some stderr",
+			hookContent:         "#!/bin/sh\necho '   '\necho stderr >&2\nexit 1",
+			expectedErrorRegexp: "stderr\n",
+			expectedStderr:      "stderr\n",
+			expectedStdout:      "   \n",
 		},
 		{
-			desc:           "some stdout and whitespace stderr",
-			hookContent:    "#!/bin/sh\necho stdout\necho '   ' >&2\nexit 1",
-			output:         func(string) string { return "stdout\n" },
-			expectedStderr: "   \n",
-			expectedStdout: "stdout\n",
+			desc:                "some stdout and whitespace stderr",
+			hookContent:         "#!/bin/sh\necho stdout\necho '   ' >&2\nexit 1",
+			expectedErrorRegexp: "stdout\n",
+			expectedStderr:      "   \n",
+			expectedStdout:      "stdout\n",
 		},
 	}
 
@@ -1060,8 +1058,7 @@ func TestBranchHookOutput(t *testing.T) {
 					User:       gittest.TestUser,
 				}
 
-				hookFilename := gittest.WriteCustomHook(t, repoPath, hookTestCase.hookName, []byte(testCase.hookContent))
-				expectedError := testCase.output(hookFilename)
+				gittest.WriteCustomHook(t, repoPath, hookTestCase.hookName, []byte(testCase.hookContent))
 
 				_, err := client.UserCreateBranch(ctx, createRequest)
 
@@ -1081,7 +1078,16 @@ func TestBranchHookOutput(t *testing.T) {
 				defer gittest.Exec(t, cfg, "-C", repoPath, "branch", "-d", branchNameInput)
 
 				deleteResponse, err := client.UserDeleteBranch(ctx, deleteRequest)
-				testhelper.RequireGrpcError(t, structerr.NewPermissionDenied("deletion denied by custom hooks: running %s hooks: %s", hookTestCase.hookName, expectedError).WithDetail(
+
+				actualStatus, ok := status.FromError(err)
+				require.True(t, ok)
+
+				statusWithoutMessage := actualStatus.Proto()
+				statusWithoutMessage.Message = "OVERRIDDEN"
+
+				// Assert the message separately as it references the hook path which may change and fail the equality check.
+				require.Regexp(t, fmt.Sprintf(`^rpc error: code = PermissionDenied desc = deletion denied by custom hooks: running %s hooks: %s$`, hookTestCase.hookName, testCase.expectedErrorRegexp), err)
+				testhelper.RequireGrpcError(t, structerr.NewPermissionDenied(statusWithoutMessage.Message).WithDetail(
 					&gitalypb.UserDeleteBranchError{
 						Error: &gitalypb.UserDeleteBranchError_CustomHook{
 							CustomHook: &gitalypb.CustomHookError{
@@ -1091,7 +1097,7 @@ func TestBranchHookOutput(t *testing.T) {
 							},
 						},
 					},
-				), err)
+				), status.ErrorProto(statusWithoutMessage))
 				require.Nil(t, deleteResponse)
 			})
 		}
diff --git a/internal/gitaly/service/operations/submodules_test.go b/internal/gitaly/service/operations/submodules_test.go
index 221f1b701..06464ee43 100644
--- a/internal/gitaly/service/operations/submodules_test.go
+++ b/internal/gitaly/service/operations/submodules_test.go
@@ -34,11 +34,22 @@ func testUserUpdateSubmodule(t *testing.T, ctx context.Context) {
 	ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx)
 
 	type setupData struct {
-		request          *gitalypb.UserUpdateSubmoduleRequest
-		expectedResponse *gitalypb.UserUpdateSubmoduleResponse
-		verify           func(t *testing.T)
-		commitID         string
-		expectedErr      error
+		request         *gitalypb.UserUpdateSubmoduleRequest
+		requireResponse func(testing.TB, string, *gitalypb.UserUpdateSubmoduleResponse)
+		verify          func(t *testing.T)
+		commitID        string
+		expectedErr     error
+	}
+
+	equalResponse := func(expected *gitalypb.UserUpdateSubmoduleResponse) func(testing.TB, string, *gitalypb.UserUpdateSubmoduleResponse) {
+		return func(tb testing.TB, expectedCommitID string, actual *gitalypb.UserUpdateSubmoduleResponse) {
+			tb.Helper()
+			if expected.BranchUpdate != nil {
+				expected.BranchUpdate.CommitId = expectedCommitID
+			}
+
+			testhelper.ProtoEqual(t, expected, actual)
+		}
 	}
 
 	testCases := []struct {
@@ -72,8 +83,8 @@ func testUserUpdateSubmodule(t *testing.T, ctx context.Context) {
 						Submodule:     []byte("sub"),
 						CommitMessage: []byte("Updating Submodule: sub"),
 					},
-					expectedResponse: &gitalypb.UserUpdateSubmoduleResponse{BranchUpdate: &gitalypb.OperationBranchUpdate{}},
-					commitID:         commitID.String(),
+					requireResponse: equalResponse(&gitalypb.UserUpdateSubmoduleResponse{BranchUpdate: &gitalypb.OperationBranchUpdate{}}),
+					commitID:        commitID.String(),
 				}
 			},
 		},
@@ -102,8 +113,8 @@ func testUserUpdateSubmodule(t *testing.T, ctx context.Context) {
 						Submodule:     []byte("sub"),
 						CommitMessage: []byte("Updating Submodule: sub"),
 					},
-					expectedResponse: &gitalypb.UserUpdateSubmoduleResponse{BranchUpdate: &gitalypb.OperationBranchUpdate{}},
-					commitID:         commitID.String(),
+					requireResponse: equalResponse(&gitalypb.UserUpdateSubmoduleResponse{BranchUpdate: &gitalypb.OperationBranchUpdate{}}),
+					commitID:        commitID.String(),
 				}
 			},
 		},
@@ -138,8 +149,8 @@ func testUserUpdateSubmodule(t *testing.T, ctx context.Context) {
 						Submodule:     []byte("foo/sub"),
 						CommitMessage: []byte("Updating Submodule: sub"),
 					},
-					expectedResponse: &gitalypb.UserUpdateSubmoduleResponse{BranchUpdate: &gitalypb.OperationBranchUpdate{}},
-					commitID:         commitID.String(),
+					requireResponse: equalResponse(&gitalypb.UserUpdateSubmoduleResponse{BranchUpdate: &gitalypb.OperationBranchUpdate{}}),
+					commitID:        commitID.String(),
 				}
 			},
 		},
@@ -179,8 +190,8 @@ func testUserUpdateSubmodule(t *testing.T, ctx context.Context) {
 						Submodule:     []byte("sub"),
 						CommitMessage: []byte("Updating Submodule: sub"),
 					},
-					expectedResponse: &gitalypb.UserUpdateSubmoduleResponse{BranchUpdate: &gitalypb.OperationBranchUpdate{}},
-					commitID:         commitID.String(),
+					requireResponse: equalResponse(&gitalypb.UserUpdateSubmoduleResponse{BranchUpdate: &gitalypb.OperationBranchUpdate{}}),
+					commitID:        commitID.String(),
 				}
 			},
 		},
@@ -219,8 +230,10 @@ func testUserUpdateSubmodule(t *testing.T, ctx context.Context) {
 						Submodule:     []byte("sub"),
 						CommitMessage: []byte("Updating Submodule: sub"),
 					},
-					expectedResponse: &gitalypb.UserUpdateSubmoduleResponse{
-						PreReceiveError: fmt.Sprintf(`executing custom hooks: error executing "%s/custom_hooks/pre-receive": exit status 1`, repoPath),
+					requireResponse: func(tb testing.TB, expectedCommitID string, response *gitalypb.UserUpdateSubmoduleResponse) {
+						require.Regexp(tb, `^executing custom hooks: error executing ".+/custom_hooks/pre-receive": exit status 1$`, response.GetPreReceiveError())
+						response.PreReceiveError = ""
+						testhelper.ProtoEqual(tb, &gitalypb.UserUpdateSubmoduleResponse{}, response)
 					},
 					commitID: commitID.String(),
 					verify: func(t *testing.T) {
@@ -495,9 +508,9 @@ func testUserUpdateSubmodule(t *testing.T, ctx context.Context) {
 						Submodule:     []byte("foobar"),
 						CommitMessage: []byte("Updating Submodule: sub"),
 					},
-					expectedResponse: &gitalypb.UserUpdateSubmoduleResponse{
+					requireResponse: equalResponse(&gitalypb.UserUpdateSubmoduleResponse{
 						CommitError: "Invalid submodule path",
-					},
+					}),
 					verify: func(t *testing.T) {},
 				}
 			},
@@ -527,9 +540,9 @@ func testUserUpdateSubmodule(t *testing.T, ctx context.Context) {
 						Submodule:     []byte("foobar/does/not/exist"),
 						CommitMessage: []byte("Updating Submodule: sub"),
 					},
-					expectedResponse: &gitalypb.UserUpdateSubmoduleResponse{
+					requireResponse: equalResponse(&gitalypb.UserUpdateSubmoduleResponse{
 						CommitError: "Invalid submodule path",
-					},
+					}),
 					verify: func(t *testing.T) {},
 				}
 			},
@@ -558,9 +571,9 @@ func testUserUpdateSubmodule(t *testing.T, ctx context.Context) {
 						Submodule:     []byte("sub"),
 						CommitMessage: []byte("Updating Submodule: sub"),
 					},
-					expectedResponse: &gitalypb.UserUpdateSubmoduleResponse{
+					requireResponse: equalResponse(&gitalypb.UserUpdateSubmoduleResponse{
 						CommitError: fmt.Sprintf("The submodule sub is already at %s", subCommitID),
-					},
+					}),
 					verify: func(t *testing.T) {},
 				}
 			},
@@ -587,9 +600,9 @@ func testUserUpdateSubmodule(t *testing.T, ctx context.Context) {
 						Submodule:     []byte("VERSION"),
 						CommitMessage: []byte("Updating Submodule: sub"),
 					},
-					expectedResponse: &gitalypb.UserUpdateSubmoduleResponse{
+					requireResponse: equalResponse(&gitalypb.UserUpdateSubmoduleResponse{
 						CommitError: "Invalid submodule path",
-					},
+					}),
 					verify: func(t *testing.T) {},
 				}
 			},
@@ -610,9 +623,9 @@ func testUserUpdateSubmodule(t *testing.T, ctx context.Context) {
 						Submodule:     []byte("foobar"),
 						CommitMessage: []byte("Updating Submodule: sub"),
 					},
-					expectedResponse: &gitalypb.UserUpdateSubmoduleResponse{
+					requireResponse: equalResponse(&gitalypb.UserUpdateSubmoduleResponse{
 						CommitError: "Repository is empty",
-					},
+					}),
 					verify: func(t *testing.T) {},
 				}
 			},
@@ -643,8 +656,8 @@ func testUserUpdateSubmodule(t *testing.T, ctx context.Context) {
 						CommitMessage:  []byte("Updating Submodule: sub"),
 						ExpectedOldOid: expectedOldOID.String(),
 					},
-					expectedResponse: &gitalypb.UserUpdateSubmoduleResponse{BranchUpdate: &gitalypb.OperationBranchUpdate{}},
-					commitID:         commitID.String(),
+					requireResponse: equalResponse(&gitalypb.UserUpdateSubmoduleResponse{BranchUpdate: &gitalypb.OperationBranchUpdate{}}),
+					commitID:        commitID.String(),
 				}
 			},
 		},
@@ -746,9 +759,9 @@ func testUserUpdateSubmodule(t *testing.T, ctx context.Context) {
 						CommitMessage:  []byte("Updating Submodule: sub"),
 						ExpectedOldOid: firstCommit.String(),
 					},
-					expectedResponse: &gitalypb.UserUpdateSubmoduleResponse{
+					requireResponse: equalResponse(&gitalypb.UserUpdateSubmoduleResponse{
 						CommitError: "Could not update refs/heads/master. Please refresh and try again.",
-					},
+					}),
 					commitID: commitID.String(),
 					verify:   func(t *testing.T) {},
 				}
@@ -772,9 +785,9 @@ func testUserUpdateSubmodule(t *testing.T, ctx context.Context) {
 
 			// If there is no verification function, lets do the default verification of
 			// checking if the submodule was updated correctly in the main repo.
+			var expectedCommitID string
 			if setupData.verify == nil {
-				newCommitID := text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "rev-parse", string(setupData.request.Branch)))
-				setupData.expectedResponse.BranchUpdate.CommitId = newCommitID
+				expectedCommitID = text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "rev-parse", string(setupData.request.Branch)))
 
 				entry := gittest.Exec(t, cfg, "-C", repoPath, "ls-tree", "-z", fmt.Sprintf("%s^{tree}:", response.BranchUpdate.CommitId), tc.subPath)
 				parser := localrepo.NewParser(bytes.NewReader(entry), git.ObjectHashSHA1)
@@ -786,7 +799,12 @@ func testUserUpdateSubmodule(t *testing.T, ctx context.Context) {
 				setupData.verify(t)
 			}
 
-			testhelper.ProtoEqual(t, setupData.expectedResponse, response)
+			if setupData.requireResponse != nil {
+				setupData.requireResponse(t, expectedCommitID, response)
+				return
+			}
+
+			require.Nil(t, response)
 		})
 	}
 }
diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go
index ba8a9f756..157398b45 100644
--- a/internal/gitaly/service/operations/tags_test.go
+++ b/internal/gitaly/service/operations/tags_test.go
@@ -1427,51 +1427,49 @@ func TestTagHookOutput(t *testing.T) {
 	ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx)
 
 	for _, tc := range []struct {
-		desc           string
-		hookContent    string
-		expectedStdout string
-		expectedStderr string
-		expectedErr    func(hookPath string) string
+		desc                string
+		hookContent         string
+		expectedStdout      string
+		expectedStderr      string
+		expectedErrorRegexp string
 	}{
 		{
-			desc:        "empty stdout and empty stderr",
-			hookContent: "#!/bin/sh\nexit 1",
-			expectedErr: func(hookPath string) string {
-				return fmt.Sprintf("executing custom hooks: error executing %q: exit status 1", hookPath)
-			},
+			desc:                "empty stdout and empty stderr",
+			hookContent:         "#!/bin/sh\nexit 1",
+			expectedErrorRegexp: `^executing custom hooks: error executing .+: exit status 1$`,
 		},
 		{
-			desc:           "empty stdout and some stderr",
-			hookContent:    "#!/bin/sh\necho stderr >&2\nexit 1",
-			expectedStderr: "stderr\n",
-			expectedErr:    func(string) string { return "stderr\n" },
+			desc:                "empty stdout and some stderr",
+			hookContent:         "#!/bin/sh\necho stderr >&2\nexit 1",
+			expectedStderr:      "stderr\n",
+			expectedErrorRegexp: `^stderr\n$`,
 		},
 		{
-			desc:           "some stdout and empty stderr",
-			hookContent:    "#!/bin/sh\necho stdout\nexit 1",
-			expectedStdout: "stdout\n",
-			expectedErr:    func(string) string { return "stdout\n" },
+			desc:                "some stdout and empty stderr",
+			hookContent:         "#!/bin/sh\necho stdout\nexit 1",
+			expectedStdout:      "stdout\n",
+			expectedErrorRegexp: `^stdout\n$`,
 		},
 		{
-			desc:           "some stdout and some stderr",
-			hookContent:    "#!/bin/sh\necho stdout\necho stderr >&2\nexit 1",
-			expectedStdout: "stdout\n",
-			expectedStderr: "stderr\n",
-			expectedErr:    func(string) string { return "stderr\n" },
+			desc:                "some stdout and some stderr",
+			hookContent:         "#!/bin/sh\necho stdout\necho stderr >&2\nexit 1",
+			expectedStdout:      "stdout\n",
+			expectedStderr:      "stderr\n",
+			expectedErrorRegexp: `^stderr\n$`,
 		},
 		{
-			desc:           "whitespace stdout and some stderr",
-			hookContent:    "#!/bin/sh\necho '   '\necho stderr >&2\nexit 1",
-			expectedStdout: "   \n",
-			expectedStderr: "stderr\n",
-			expectedErr:    func(string) string { return "stderr\n" },
+			desc:                "whitespace stdout and some stderr",
+			hookContent:         "#!/bin/sh\necho '   '\necho stderr >&2\nexit 1",
+			expectedStdout:      "   \n",
+			expectedStderr:      "stderr\n",
+			expectedErrorRegexp: `^stderr\n$`,
 		},
 		{
-			desc:           "some stdout and whitespace stderr",
-			hookContent:    "#!/bin/sh\necho stdout\necho '   ' >&2\nexit 1",
-			expectedStdout: "stdout\n",
-			expectedStderr: "   \n",
-			expectedErr:    func(string) string { return "stdout\n" },
+			desc:                "some stdout and whitespace stderr",
+			hookContent:         "#!/bin/sh\necho stdout\necho '   ' >&2\nexit 1",
+			expectedStdout:      "stdout\n",
+			expectedStderr:      "   \n",
+			expectedErrorRegexp: `^stdout\n$`,
 		},
 	} {
 		for _, hookTC := range []struct {
@@ -1497,7 +1495,7 @@ func TestTagHookOutput(t *testing.T) {
 				commitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch(git.DefaultBranch))
 				gittest.WriteTag(t, cfg, repoPath, "to-be-deleted", commitID.Revision())
 
-				hookFilename := gittest.WriteCustomHook(t, repoPath, hookTC.hook, []byte(tc.hookContent))
+				gittest.WriteCustomHook(t, repoPath, hookTC.hook, []byte(tc.hookContent))
 
 				t.Run("UserCreateTag", func(t *testing.T) {
 					t.Parallel()
@@ -1532,9 +1530,11 @@ func TestTagHookOutput(t *testing.T) {
 						User:       gittest.TestUser,
 					})
 					require.NoError(t, err)
-					testhelper.ProtoEqual(t, &gitalypb.UserDeleteTagResponse{
-						PreReceiveError: tc.expectedErr(hookFilename),
-					}, response)
+
+					preReceiveErr := response.PreReceiveError
+					response.PreReceiveError = ""
+					testhelper.ProtoEqual(t, &gitalypb.UserDeleteTagResponse{}, response)
+					require.Regexp(t, tc.expectedErrorRegexp, preReceiveErr)
 				})
 			})
 		}
diff --git a/internal/gitaly/service/operations/update_branches_test.go b/internal/gitaly/service/operations/update_branches_test.go
index 53fd736a6..6a38d6206 100644
--- a/internal/gitaly/service/operations/update_branches_test.go
+++ b/internal/gitaly/service/operations/update_branches_test.go
@@ -5,6 +5,7 @@ package operations
 import (
 	"crypto/sha1"
 	"fmt"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -224,7 +225,22 @@ func TestFailedUserUpdateBranchDueToHooks(t *testing.T) {
 		response, err := client.UserUpdateBranch(ctx, request)
 		require.NoError(t, err)
 		require.Contains(t, response.PreReceiveError, "GL_USERNAME="+gittest.TestUser.GlUsername)
-		require.Contains(t, response.PreReceiveError, "PWD="+repoPath)
+
+		var gitDir, pwd string
+		for _, env := range strings.Split(response.PreReceiveError, " ") {
+			components := strings.Split(env, "=")
+			if components[0] == "GIT_DIR" {
+				gitDir = components[1]
+			}
+
+			if components[0] == "PWD" {
+				pwd = components[1]
+			}
+		}
+
+		// Assert that the working directory of the hook is the repository's root directory.
+		require.NotEmpty(t, gitDir)
+		require.Equal(t, gitDir, pwd)
 
 		responseOk := &gitalypb.UserUpdateBranchResponse{
 			PreReceiveError: response.PreReceiveError,