From f6ac643f64df50205772702fe3ea749c40108b03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 17 Dec 2020 17:08:26 +0100 Subject: [PATCH 1/3] testhelper: split MustRunCommand into that and MayFailRunCommand Why can't you just use exec.Command? Well, it's annoying to switch between the two calling conventions, and as a follow-up commit to this shows it's more obvious what you intend. I.e. you'd use MayFailRunCommand to cleanup after something that normally cleans up after itself, but on failure might not (and would otherwise ruin things for the next test). This is useful for tests of the form (pseudocode): for each test: testhelper.MustRunCommand(git tag some-name) defer exec.Command(git -d some-name) ret = UserDeleteTag(some-name) // maybe this just dies [sanity check ret] In this case usually everything's fine and we don't need the exec.Command, but if UserDeleteTag() dies we not only fail the test that died, but also the next test as it expected to be able to setup "some-name" again. Yes we could have unique refs or whatever in every test, or we could eliminate the need to ever use MayFailRunCommand if all "defer" code did exhaustive "do I need to do this teardown?" checks before doing anything. But it's more idiomatic and less verbose to just always do the cleanup unconditionally. --- internal/testhelper/testhelper.go | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go index d36ff70f017..6cfca0210de 100644 --- a/internal/testhelper/testhelper.go +++ b/internal/testhelper/testhelper.go @@ -87,8 +87,7 @@ func GitalyServersMetadata(t testing.TB, serverSocketPath string) metadata.MD { return metadata.Pairs("gitaly-servers", base64.StdEncoding.EncodeToString(gitalyServersJSON)) } -// MustRunCommand runs a command with an optional standard input and returns the standard output, or fails. -func MustRunCommand(t testing.TB, stdin io.Reader, name string, args ...string) []byte { +func testRunCommandInternal(t testing.TB, failOk bool, stdin io.Reader, name string, args ...string) []byte { if t != nil { t.Helper() } @@ -111,7 +110,7 @@ func MustRunCommand(t testing.TB, stdin io.Reader, name string, args ...string) } output, err := cmd.Output() - if err != nil { + if err != nil && !failOk { stderr := err.(*exec.ExitError).Stderr if t == nil { log.Print(name, args) @@ -127,6 +126,21 @@ func MustRunCommand(t testing.TB, stdin io.Reader, name string, args ...string) return output } +// MustRunCommand runs a command with an optional standard input and returns the standard output, or fails. +func MustRunCommand(t testing.TB, stdin io.Reader, name string, args ...string) []byte { + return testRunCommandInternal(t, false, stdin, name, args...) +} + +// MayFailRunCommand is MustRunCommand which ignores failures. +// +// Useful e.g. for normally redundant teardown if a test doesn't die, +// but which should run in a "defer" block to cleanup things for the +// next test if the current test were to die in the middle of its own +// setup/testing/teardown. +func MayFailRunCommand(t testing.TB, stdin io.Reader, name string, args ...string) []byte { + return testRunCommandInternal(t, true, stdin, name, args...) +} + // GetTemporaryGitalySocketFileName will return a unique, useable socket file name func GetTemporaryGitalySocketFileName() string { if testDirectory == "" { -- GitLab From 57b6a2fa32f530726a7a9621c680648b1f2aeb60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 17 Dec 2020 16:52:07 +0100 Subject: [PATCH 2/3] UserCreateTag tests: move to testhelper.MustRunCommand from exec.Command() Use this helper instead which'll die on error. Fixes up code I added in 5896fdaf4 (UserCreateTag tests: test nested annotated tags, 2020-12-08). Suggested at https://gitlab.com/gitlab-org/gitaly/-/merge_requests/2942#note_469176310 --- internal/gitaly/service/operations/tags_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go index ccd03e157a8..e39618c4e4f 100644 --- a/internal/gitaly/service/operations/tags_test.go +++ b/internal/gitaly/service/operations/tags_test.go @@ -239,7 +239,7 @@ func testSuccessfulUserCreateTagRequest(t *testing.T, ctx context.Context) { require.NoError(t, err, "error from calling RPC") require.Empty(t, response.PreReceiveError, "PreReceiveError must be empty, signalling the push was accepted") - defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "tag", "-d", inputTagName).Run() + defer testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", "-d", inputTagName) responseOk := &gitalypb.UserCreateTagResponse{ Tag: testCase.expectedTag, @@ -360,7 +360,7 @@ func TestSuccessfulUserCreateTagRequestToNonCommit(t *testing.T) { Tag: testCase.expectedTag, } response, err := client.UserCreateTag(ctx, request) - defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "tag", "-d", inputTagName).Run() + defer testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", "-d", inputTagName) // Fake up *.Id for annotated tags if len(testCase.expectedTag.Id) == 0 { @@ -450,7 +450,7 @@ func TestSuccessfulUserCreateTagNestedTags(t *testing.T) { Message: []byte(tagMessage), } response, err := client.UserCreateTag(ctx, request) - defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "tag", "-d", tagName).Run() + defer testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", "-d", tagName) require.NoError(t, err) createdID := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", tagName) @@ -571,7 +571,7 @@ func TestUserCreateTagsuccessfulCreationOfPrefixedTag(t *testing.T) { for _, testCase := range testCases { t.Run(testCase.desc, func(t *testing.T) { - defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "tag", "-d", testCase.tagNameInput).Run() + defer testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", "-d", testCase.tagNameInput) request := &gitalypb.UserCreateTagRequest{ Repository: testRepo, @@ -635,7 +635,7 @@ func testSuccessfulGitHooksForUserCreateTagRequest(t *testing.T, ctx context.Con for _, hookName := range GitlabHooks { t.Run(hookName, func(t *testing.T) { - defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "tag", "-d", tagName).Run() + defer testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", "-d", tagName) hookOutputTempPath, cleanup := testhelper.WriteEnvToCustomHook(t, testRepoPath, hookName) defer cleanup() @@ -732,7 +732,7 @@ func testFailedUserDeleteTagDueToHooks(t *testing.T, ctx context.Context) { tagNameInput := "to-be-deleted-soon-tag" testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", tagNameInput) - defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "tag", "-d", tagNameInput).Run() + defer testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", "-d", tagNameInput) request := &gitalypb.UserDeleteTagRequest{ Repository: testRepo, @@ -976,7 +976,7 @@ func testTagHookOutput(t *testing.T, ctx context.Context) { require.False(t, createResponse.Exists) require.Equal(t, testCase.output, createResponse.PreReceiveError) - defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "tag", "-d", tagNameInput).Run() + defer testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", "-d", tagNameInput) testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", tagNameInput) deleteResponse, err := client.UserDeleteTag(ctx, deleteRequest) -- GitLab From cf9efae8603490980f1cf15c288659955d2120fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 18 Dec 2020 21:35:28 +0100 Subject: [PATCH 3/3] UserDeleteTag tests: use testhelper.MayFailRunCommand() As noted in the commit that introduced the MayFailRunCommand() function a couple of commits back it's useful for these cases where we might die in the middle of N number of tests, so we'd like the tag deleted for the next test. Otherwise failures would bleed across tests. --- internal/gitaly/service/operations/tags_test.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go index e39618c4e4f..d90d42d769e 100644 --- a/internal/gitaly/service/operations/tags_test.go +++ b/internal/gitaly/service/operations/tags_test.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "io/ioutil" - "os/exec" "path/filepath" "testing" @@ -38,7 +37,7 @@ func testSuccessfulUserDeleteTagRequest(t *testing.T, ctx context.Context) { tagNameInput := "to-be-deleted-soon-tag" - defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "tag", "-d", tagNameInput).Run() + defer testhelper.MayFailRunCommand(t, nil, "git", "-C", testRepoPath, "tag", "-d", tagNameInput) testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", tagNameInput) @@ -73,7 +72,6 @@ func testSuccessfulGitHooksForUserDeleteTagRequest(t *testing.T, ctx context.Con defer cleanupFn() tagNameInput := "to-be-déleted-soon-tag" - defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "tag", "-d", tagNameInput).Run() request := &gitalypb.UserDeleteTagRequest{ Repository: testRepo, @@ -84,6 +82,7 @@ func testSuccessfulGitHooksForUserDeleteTagRequest(t *testing.T, ctx context.Con for _, hookName := range GitlabHooks { t.Run(hookName, func(t *testing.T) { testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", tagNameInput) + defer testhelper.MayFailRunCommand(t, nil, "git", "-C", testRepoPath, "tag", "-d", tagNameInput) hookOutputTempPath, cleanup := testhelper.WriteEnvToCustomHook(t, testRepoPath, hookName) defer cleanup() @@ -523,7 +522,7 @@ func testUserDeleteTagsuccessfulDeletionOfPrefixedTag(t *testing.T, ctx context. for _, testCase := range testCases { t.Run(testCase.desc, func(t *testing.T) { testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", testCase.tagNameInput, testCase.tagCommit) - defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "tag", "-d", testCase.tagNameInput).Run() + defer testhelper.MayFailRunCommand(t, nil, "git", "-C", testRepoPath, "tag", "-d", testCase.tagNameInput) request := &gitalypb.UserDeleteTagRequest{ Repository: testRepo, -- GitLab