diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-04-29 12:11:23 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-05-03 08:43:53 +0300 |
commit | 4d075c784ecd188265d98186c25c116edb781a05 (patch) | |
tree | 020c587e1d419d9b3d8b9d7c600529ec06332207 | |
parent | cc8901bc9389a9f85267fe820acc3777494ea337 (diff) |
commit: Refactor `CheckObjectsExist()` tests to be more extensible
Tests for the `CheckObjectsExist()` are not quite extensible enough to
test various error conditions, most importantly in the context of
chunking. Refactor them.
-rw-r--r-- | internal/gitaly/service/commit/check_objects_exist_test.go | 243 | ||||
-rw-r--r-- | internal/gitaly/service/smarthttp/receive_pack_test.go | 3 |
2 files changed, 170 insertions, 76 deletions
diff --git a/internal/gitaly/service/commit/check_objects_exist_test.go b/internal/gitaly/service/commit/check_objects_exist_test.go index 97fed524f..1be585c02 100644 --- a/internal/gitaly/service/commit/check_objects_exist_test.go +++ b/internal/gitaly/service/commit/check_objects_exist_test.go @@ -1,121 +1,214 @@ package commit import ( + "fmt" "io" "testing" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest" + "gitlab.com/gitlab-org/gitaly/v14/internal/helper" "gitlab.com/gitlab-org/gitaly/v14/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" - "google.golang.org/grpc/codes" ) func TestCheckObjectsExist(t *testing.T) { t.Parallel() ctx := testhelper.Context(t) - cfg, repo, repoPath, client := setupCommitServiceWithRepo(ctx, t, true) + cfg, client := setupCommitService(ctx, t) - // write a few commitIDs we can use - commitID1 := gittest.WriteCommit(t, cfg, repoPath) - commitID2 := gittest.WriteCommit(t, cfg, repoPath) - commitID3 := gittest.WriteCommit(t, cfg, repoPath) + repo, repoPath := gittest.CreateRepository(ctx, t, cfg) - // remove a ref from the repository so we know it doesn't exist - gittest.Exec(t, cfg, "-C", repoPath, "update-ref", "-d", "refs/heads/many_files") + commitID1 := gittest.WriteCommit(t, cfg, repoPath, + gittest.WithBranch("master"), gittest.WithMessage("commit-1"), gittest.WithParents(), + ) + commitID2 := gittest.WriteCommit(t, cfg, repoPath, + gittest.WithBranch("feature"), gittest.WithMessage("commit-2"), gittest.WithParents(commitID1), + ) + commitID3 := gittest.WriteCommit(t, cfg, repoPath, + gittest.WithMessage("commit-3"), gittest.WithParents(commitID1), + ) - nonexistingObject := "abcdefg" - cmd := gittest.NewCommand(t, cfg, "-C", repoPath, "rev-parse", nonexistingObject) - require.Error(t, cmd.Wait(), "ensure the object doesn't exist") - - testCases := []struct { - desc string - input [][]byte - revisionsExistence map[string]bool - returnCode codes.Code + for _, tc := range []struct { + desc string + requests []*gitalypb.CheckObjectsExistRequest + expectedResults map[string]bool + expectedErr error }{ { - desc: "commit ids and refs that exist", - input: [][]byte{ - []byte(commitID1), - []byte("master"), - []byte(commitID2), - []byte(commitID3), - []byte("feature"), + desc: "no requests", + requests: []*gitalypb.CheckObjectsExistRequest{}, + // Ideally, we'd return an invalid-argument error in case there aren't any + // requests. We can't do this though as this would diverge from Praefect's + // behaviour, which always returns `io.EOF`. + expectedResults: map[string]bool{}, + }, + { + desc: "missing repository", + requests: []*gitalypb.CheckObjectsExistRequest{ + { + Revisions: [][]byte{}, + }, + }, + // This is a bug: we don't verify there's a repository. + expectedErr: helper.ErrInvalidArgumentf(`GetStorageByName: no such storage: ""`), + expectedResults: map[string]bool{}, + }, + { + desc: "request without revisions", + requests: []*gitalypb.CheckObjectsExistRequest{ + { + Repository: repo, + }, }, - revisionsExistence: map[string]bool{ - "master": true, - commitID2.String(): true, - commitID3.String(): true, - "feature": true, + expectedResults: map[string]bool{}, + }, + { + desc: "commit ids and refs that exist", + requests: []*gitalypb.CheckObjectsExistRequest{ + { + Repository: repo, + Revisions: [][]byte{ + []byte(commitID1), + []byte("master"), + []byte(commitID2), + []byte(commitID3), + []byte("feature"), + }, + }, }, - returnCode: codes.OK, + // This is a bug: revisions of the first message are not checked. + expectedResults: map[string]bool{}, }, { desc: "ref and objects missing", - input: [][]byte{ - []byte(commitID1), - []byte("master"), - []byte(commitID2), - []byte(commitID3), - []byte("feature"), - []byte("many_files"), - []byte(nonexistingObject), + requests: []*gitalypb.CheckObjectsExistRequest{ + { + Repository: repo, + Revisions: [][]byte{ + []byte(commitID1), + []byte("master"), + []byte(commitID2), + []byte(commitID3), + []byte("feature"), + []byte("refs/does/not/exist"), + }, + }, }, - revisionsExistence: map[string]bool{ - "master": true, - commitID2.String(): true, - commitID3.String(): true, - "feature": true, - "many_files": false, - nonexistingObject: false, + // This is a bug: revisions of the first message are not checked. + expectedResults: map[string]bool{}, + }, + { + desc: "chunked input", + requests: []*gitalypb.CheckObjectsExistRequest{ + { + Repository: repo, + Revisions: [][]byte{ + []byte(commitID1), + }, + }, + { + Revisions: [][]byte{ + []byte(commitID2), + }, + }, + { + Revisions: [][]byte{ + []byte("refs/does/not/exist"), + }, + }, + { + Revisions: [][]byte{ + []byte(commitID3), + }, + }, + }, + // This is a bug: revisions of the first message are not checked. + expectedResults: map[string]bool{ + commitID2.String(): true, + commitID3.String(): true, + "refs/does/not/exist": false, }, - returnCode: codes.OK, }, { - desc: "empty input", - input: [][]byte{}, - returnCode: codes.OK, - revisionsExistence: map[string]bool{}, + desc: "invalid input", + requests: []*gitalypb.CheckObjectsExistRequest{ + { + Repository: repo, + Revisions: [][]byte{ + []byte("-not-a-rev"), + }, + }, + }, + expectedErr: helper.ErrInvalidArgumentf("revision can't start with '-'"), + expectedResults: map[string]bool{}, }, { - desc: "invalid input", - input: [][]byte{[]byte("-not-a-rev")}, - returnCode: codes.InvalidArgument, + desc: "input with whitespace", + requests: []*gitalypb.CheckObjectsExistRequest{ + { + Repository: repo, + Revisions: [][]byte{ + []byte(fmt.Sprintf("%s\n%s", commitID1, commitID2)), + }, + }, + }, + // This is a bug: revisions of the first message are not checked. + expectedErr: nil, + expectedResults: map[string]bool{}, }, - } - - for _, tc := range testCases { + { + desc: "chunked invalid input", + requests: []*gitalypb.CheckObjectsExistRequest{ + { + Repository: repo, + Revisions: [][]byte{ + []byte(commitID1), + }, + }, + { + Revisions: [][]byte{ + []byte("-not-a-rev"), + }, + }, + }, + // This is a bug: we only check that revisions of the first message are + // valid, but don't care about any of the latter ones. + expectedErr: nil, + expectedResults: map[string]bool{ + "-not-a-rev": false, + }, + }, + } { t.Run(tc.desc, func(t *testing.T) { - c, err := client.CheckObjectsExist(ctx) + client, err := client.CheckObjectsExist(ctx) require.NoError(t, err) - require.NoError(t, c.Send( - &gitalypb.CheckObjectsExistRequest{ - Repository: repo, - Revisions: tc.input, - }, - )) - require.NoError(t, c.CloseSend()) + for _, request := range tc.requests { + require.NoError(t, client.Send(request)) + } + require.NoError(t, client.CloseSend()) + results := map[string]bool{} for { - resp, err := c.Recv() - if tc.returnCode != codes.OK { - testhelper.RequireGrpcCode(t, err, tc.returnCode) - break - } else if err != nil { - require.Error(t, err, io.EOF) + var response *gitalypb.CheckObjectsExistResponse + response, err = client.Recv() + if err != nil { break } - actualRevisionsExistence := make(map[string]bool) - for _, revisionExistence := range resp.GetRevisions() { - actualRevisionsExistence[string(revisionExistence.GetName())] = revisionExistence.GetExists() + for _, revision := range response.GetRevisions() { + results[string(revision.GetName())] = revision.GetExists() } - assert.Equal(t, tc.revisionsExistence, actualRevisionsExistence) } + + if tc.expectedErr == nil { + tc.expectedErr = io.EOF + } + + testhelper.RequireGrpcError(t, tc.expectedErr, err) + require.Equal(t, tc.expectedResults, results) }) } } diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go index 54f07fe26..8b56ce272 100644 --- a/internal/gitaly/service/smarthttp/receive_pack_test.go +++ b/internal/gitaly/service/smarthttp/receive_pack_test.go @@ -839,7 +839,8 @@ func TestPostReceive_allRejected(t *testing.T) { ctx context.Context, repo *gitalypb.Repository, pushOptions, env []string, - stdin io.Reader, stdout, stderr io.Writer) error { + stdin io.Reader, stdout, stderr io.Writer, + ) error { return errors.New("not allowed") }, gitalyhook.NopPostReceive, |