From 5defa2e52ce73df51d54d9088ff3e1e781e9d424 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Thu, 26 Nov 2020 12:05:26 +0100 Subject: [PATCH 1/2] testing: Add testify/suite to remove boilerplate This change introduces the suite utilities in one test to showcase how we can use it to remove boilerplate from our test suite. As first iteration the setup and teardown of the repository was choosen, though it can be expanded trivially to include the service server and client. As with many things, there's a tradeoff when executing tests. For automated testing `make test` will work as expected, though `go test -run=` will not work as easily as before. Currently I don't see a way around that, as the regexp support for the `run` argument requires a match on the suite name too. `-m` does work. With the small commit we've got now, upon merging we'd devise a strategy to include this throughout the Gitaly code base. Though it must be noted that this can be done piece meal. Additionally the output changes slightly with this change ``` === RUN TestRepositoryService === RUN TestRepositoryService/TestGetInfoAttributesExisting --- PASS: TestRepositoryService (0.02s) --- PASS: TestRepositoryService/TestGetInfoAttributesExisting (0.02s) --- PASS: TestRepositoryTestSuite/TestGetInfoAttributesExisting (0.02s) ``` --- .../repository/info_attributes_test.go | 39 ++++++++----------- .../gitaly/service/repository/suite_test.go | 38 ++++++++++++++++++ 2 files changed, 54 insertions(+), 23 deletions(-) create mode 100644 internal/gitaly/service/repository/suite_test.go diff --git a/internal/gitaly/service/repository/info_attributes_test.go b/internal/gitaly/service/repository/info_attributes_test.go index bafb9f6c9f0..3a682c9c1fc 100644 --- a/internal/gitaly/service/repository/info_attributes_test.go +++ b/internal/gitaly/service/repository/info_attributes_test.go @@ -5,7 +5,6 @@ import ( "io/ioutil" "os" "path/filepath" - "testing" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" @@ -14,62 +13,56 @@ import ( "gitlab.com/gitlab-org/gitaly/streamio" ) -func TestGetInfoAttributesExisting(t *testing.T) { +func (suite *RepositoryServiceTestSuite) TestGetInfoAttributesExisting() { locator := config.NewLocator(config.Config) - serverSocketPath, stop := runRepoServer(t, locator) + serverSocketPath, stop := runRepoServer(suite.T(), locator) defer stop() - client, conn := newRepositoryClient(t, serverSocketPath) + client, conn := newRepositoryClient(suite.T(), serverSocketPath) defer conn.Close() - testRepo, repoPath, cleanupFn := testhelper.NewTestRepo(t) - defer cleanupFn() - - infoPath := filepath.Join(repoPath, "info") + infoPath := filepath.Join(suite.repositoryPath, "info") os.MkdirAll(infoPath, 0755) buffSize := streamio.WriteBufferSize + 1 data := bytes.Repeat([]byte("*.pbxproj binary\n"), buffSize) attrsPath := filepath.Join(infoPath, "attributes") err := ioutil.WriteFile(attrsPath, data, 0644) - require.NoError(t, err) + require.NoError(suite.T(), err) - request := &gitalypb.GetInfoAttributesRequest{Repository: testRepo} + request := &gitalypb.GetInfoAttributesRequest{Repository: suite.repository} testCtx, cancelCtx := testhelper.Context() defer cancelCtx() stream, err := client.GetInfoAttributes(testCtx, request) - require.NoError(t, err) + require.NoError(suite.T(), err) receivedData, err := ioutil.ReadAll(streamio.NewReader(func() ([]byte, error) { response, err := stream.Recv() return response.GetAttributes(), err })) - require.NoError(t, err) - require.Equal(t, data, receivedData) + require.NoError(suite.T(), err) + require.Equal(suite.T(), data, receivedData) } -func TestGetInfoAttributesNonExisting(t *testing.T) { +func (suite *RepositoryServiceTestSuite) TestGetInfoAttributesNonExisting() { locator := config.NewLocator(config.Config) - serverSocketPath, stop := runRepoServer(t, locator) + serverSocketPath, stop := runRepoServer(suite.T(), locator) defer stop() - client, conn := newRepositoryClient(t, serverSocketPath) + client, conn := newRepositoryClient(suite.T(), serverSocketPath) defer conn.Close() - testRepo, _, cleanupFn := testhelper.NewTestRepo(t) - defer cleanupFn() - - request := &gitalypb.GetInfoAttributesRequest{Repository: testRepo} + request := &gitalypb.GetInfoAttributesRequest{Repository: suite.repository} testCtx, cancelCtx := testhelper.Context() defer cancelCtx() response, err := client.GetInfoAttributes(testCtx, request) - require.NoError(t, err) + require.NoError(suite.T(), err) message, err := response.Recv() - require.NoError(t, err) + require.NoError(suite.T(), err) - require.Empty(t, message.GetAttributes()) + require.Empty(suite.T(), message.GetAttributes()) } diff --git a/internal/gitaly/service/repository/suite_test.go b/internal/gitaly/service/repository/suite_test.go new file mode 100644 index 00000000000..3954b0d1999 --- /dev/null +++ b/internal/gitaly/service/repository/suite_test.go @@ -0,0 +1,38 @@ +package repository + +import ( + "testing" + + "github.com/stretchr/testify/suite" + "gitlab.com/gitlab-org/gitaly/internal/testhelper" + "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" +) + +type RepositoryServiceTestSuite struct { + // This include a T() function to access *testing.T + suite.Suite + + repository *gitalypb.Repository + repositoryPath string + repoCleanupFn func() +} + +// Setup the repository service server and testing repository +// before each test +func (suite *RepositoryServiceTestSuite) SetupTest() { + testRepo, repoPath, cleanupFn := testhelper.NewTestRepo(suite.T()) + + suite.repository = testRepo + suite.repositoryPath = repoPath + suite.repoCleanupFn = cleanupFn +} + +func (suite *RepositoryServiceTestSuite) TearDownTest() { + suite.repoCleanupFn() +} + +// In order for 'go test' to run this suite, we need to create +// a normal test function and pass our suite to suite.Run +func TestRepositoryService(t *testing.T) { + suite.Run(t, new(RepositoryServiceTestSuite)) +} -- GitLab From ed59266cf276b47e12b713b0003879050e06e3e7 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Thu, 26 Nov 2020 15:32:27 +0100 Subject: [PATCH 2/2] testing: Deduplicate server and client setup Continuing the work of the parent commit, this change deduplicates the server and client setup. As such the tests have very little boilerplate left. The next item is probably the context.Context. Visually the code is less cluttered now, and less seems to be going on. --- .../repository/info_attributes_test.go | 34 ++++++------------- .../gitaly/service/repository/suite_test.go | 19 +++++++++++ .../service/repository/testhelper_test.go | 7 +++- 3 files changed, 36 insertions(+), 24 deletions(-) diff --git a/internal/gitaly/service/repository/info_attributes_test.go b/internal/gitaly/service/repository/info_attributes_test.go index 3a682c9c1fc..984906469be 100644 --- a/internal/gitaly/service/repository/info_attributes_test.go +++ b/internal/gitaly/service/repository/info_attributes_test.go @@ -6,20 +6,13 @@ import ( "os" "path/filepath" - "github.com/stretchr/testify/require" - "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/streamio" ) func (suite *RepositoryServiceTestSuite) TestGetInfoAttributesExisting() { - locator := config.NewLocator(config.Config) - serverSocketPath, stop := runRepoServer(suite.T(), locator) - defer stop() - - client, conn := newRepositoryClient(suite.T(), serverSocketPath) - defer conn.Close() + require := suite.Require() infoPath := filepath.Join(suite.repositoryPath, "info") os.MkdirAll(infoPath, 0755) @@ -28,41 +21,36 @@ func (suite *RepositoryServiceTestSuite) TestGetInfoAttributesExisting() { data := bytes.Repeat([]byte("*.pbxproj binary\n"), buffSize) attrsPath := filepath.Join(infoPath, "attributes") err := ioutil.WriteFile(attrsPath, data, 0644) - require.NoError(suite.T(), err) + require.NoError(err) request := &gitalypb.GetInfoAttributesRequest{Repository: suite.repository} testCtx, cancelCtx := testhelper.Context() defer cancelCtx() - stream, err := client.GetInfoAttributes(testCtx, request) - require.NoError(suite.T(), err) + stream, err := suite.client.GetInfoAttributes(testCtx, request) + require.NoError(err) receivedData, err := ioutil.ReadAll(streamio.NewReader(func() ([]byte, error) { response, err := stream.Recv() return response.GetAttributes(), err })) - require.NoError(suite.T(), err) - require.Equal(suite.T(), data, receivedData) + require.NoError(err) + require.Equal(data, receivedData) } func (suite *RepositoryServiceTestSuite) TestGetInfoAttributesNonExisting() { - locator := config.NewLocator(config.Config) - serverSocketPath, stop := runRepoServer(suite.T(), locator) - defer stop() - - client, conn := newRepositoryClient(suite.T(), serverSocketPath) - defer conn.Close() + require := suite.Require() request := &gitalypb.GetInfoAttributesRequest{Repository: suite.repository} testCtx, cancelCtx := testhelper.Context() defer cancelCtx() - response, err := client.GetInfoAttributes(testCtx, request) - require.NoError(suite.T(), err) + response, err := suite.client.GetInfoAttributes(testCtx, request) + require.NoError(err) message, err := response.Recv() - require.NoError(suite.T(), err) + require.NoError(err) - require.Empty(suite.T(), message.GetAttributes()) + require.Empty(message.GetAttributes()) } diff --git a/internal/gitaly/service/repository/suite_test.go b/internal/gitaly/service/repository/suite_test.go index 3954b0d1999..0805b9d8f64 100644 --- a/internal/gitaly/service/repository/suite_test.go +++ b/internal/gitaly/service/repository/suite_test.go @@ -4,8 +4,10 @@ import ( "testing" "github.com/stretchr/testify/suite" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" + "google.golang.org/grpc" ) type RepositoryServiceTestSuite struct { @@ -15,6 +17,11 @@ type RepositoryServiceTestSuite struct { repository *gitalypb.Repository repositoryPath string repoCleanupFn func() + + srv *testhelper.TestServer + + client gitalypb.RepositoryServiceClient + conn *grpc.ClientConn } // Setup the repository service server and testing repository @@ -25,9 +32,21 @@ func (suite *RepositoryServiceTestSuite) SetupTest() { suite.repository = testRepo suite.repositoryPath = repoPath suite.repoCleanupFn = cleanupFn + + suite.srv = buildTestingServer(suite.T(), config.Config, config.NewLocator(config.Config)) + + req := suite.Require() + req.NoError(suite.srv.Start()) + + cl, conn := newRepositoryClient(suite.T(), "unix://"+suite.srv.Socket()) + suite.client = cl + suite.conn = conn } func (suite *RepositoryServiceTestSuite) TearDownTest() { + suite.conn.Close() + suite.srv.Stop() + suite.repoCleanupFn() } diff --git a/internal/gitaly/service/repository/testhelper_test.go b/internal/gitaly/service/repository/testhelper_test.go index 9095225ff9d..6b91fe4790c 100644 --- a/internal/gitaly/service/repository/testhelper_test.go +++ b/internal/gitaly/service/repository/testhelper_test.go @@ -96,7 +96,7 @@ func newSecureRepoClient(t *testing.T, serverSocketPath string, pool *x509.CertP var NewSecureRepoClient = newSecureRepoClient -func runRepoServerWithConfig(t *testing.T, cfg config.Cfg, locator storage.Locator, opts ...testhelper.TestServerOpt) (string, func()) { +func buildTestingServer(t *testing.T, cfg config.Cfg, locator storage.Locator, opts ...testhelper.TestServerOpt) *testhelper.TestServer { streamInt := []grpc.StreamServerInterceptor{ mcache.StreamInvalidator(dcache.LeaseKeyer{}, protoregistry.GitalyProtoPreregistered), } @@ -109,6 +109,11 @@ func runRepoServerWithConfig(t *testing.T, cfg config.Cfg, locator storage.Locat gitalypb.RegisterRepositoryServiceServer(srv.GrpcServer(), NewServer(cfg, RubyServer, locator, config.Config.GitalyInternalSocketPath())) reflection.Register(srv.GrpcServer()) + return srv +} + +func runRepoServerWithConfig(t *testing.T, cfg config.Cfg, locator storage.Locator, opts ...testhelper.TestServerOpt) (string, func()) { + srv := buildTestingServer(t, cfg, locator, opts...) require.NoError(t, srv.Start()) return "unix://" + srv.Socket(), srv.Stop -- GitLab