Merge branch 'pks-testhelper-goroutine-leakage' into 'master'

testhelper: Enable Goroutine leak checks by default

Closes #3790

See merge request gitlab-org/gitaly!3909

21 files changed, 55 insertions, 126 deletions

diff --git a/internal/git/catfile/testhelper_test.go b/internal/git/catfile/testhelper_test.go
index 4479adb85..fa977e099 100644
--- a/internal/git/catfile/testhelper_test.go
+++ b/internal/git/catfile/testhelper_test.go
@@ -2,7 +2,6 @@ package catfile
 
 import (
 	"context"
-	"os"
 	"testing"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/command"
@@ -13,18 +12,7 @@ import (
 )
 
 func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	defer func() {
-		testhelper.MustHaveNoChildProcess()
-		testhelper.MustHaveNoGoroutines()
-	}()
-
-	cleanup := testhelper.Configure()
-	defer cleanup()
-	return m.Run()
+	testhelper.Run(m)
 }
 
 type repoExecutor struct {
diff --git a/internal/git/gitpipe/testhelper_test.go b/internal/git/gitpipe/testhelper_test.go
index b3683135f..8b2532840 100644
--- a/internal/git/gitpipe/testhelper_test.go
+++ b/internal/git/gitpipe/testhelper_test.go
@@ -1,21 +1,11 @@
 package gitpipe
 
 import (
-	"os"
 	"testing"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 )
 
 func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	defer testhelper.MustHaveNoChildProcess()
-
-	cleanup := testhelper.Configure()
-	defer cleanup()
-
-	return m.Run()
+	testhelper.Run(m)
 }
diff --git a/internal/git/gittest/commit_test.go b/internal/git/gittest/commit_test.go
index e96520a14..c3ad815ce 100644
--- a/internal/git/gittest/commit_test.go
+++ b/internal/git/gittest/commit_test.go
@@ -19,6 +19,8 @@ func TestWriteCommit(t *testing.T) {
 	defer cancel()
 
 	batchCache := catfile.NewCache(cfg)
+	defer batchCache.Stop()
+
 	batch, err := batchCache.BatchProcess(ctx, repo)
 	require.NoError(t, err)
 
diff --git a/internal/git/gittest/testhelper_test.go b/internal/git/gittest/testhelper_test.go
index 89d810164..a16e984f6 100644
--- a/internal/git/gittest/testhelper_test.go
+++ b/internal/git/gittest/testhelper_test.go
@@ -12,14 +12,7 @@ import (
 )
 
 func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	defer testhelper.MustHaveNoChildProcess()
-	cleanup := testhelper.Configure()
-	defer cleanup()
-	return m.Run()
+	testhelper.Run(m)
 }
 
 // setup sets up a test configuration and repository. Ideally we'd use our central test helpers to
diff --git a/internal/git/helper_test.go b/internal/git/helper_test.go
index eb7e925fd..f76065121 100644
--- a/internal/git/helper_test.go
+++ b/internal/git/helper_test.go
@@ -1,7 +1,6 @@
 package git
 
 import (
-	"os"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -9,14 +8,7 @@ import (
 )
 
 func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	defer testhelper.MustHaveNoChildProcess()
-	cleanup := testhelper.Configure()
-	defer cleanup()
-	return m.Run()
+	testhelper.Run(m)
 }
 
 func TestValidateRevision(t *testing.T) {
diff --git a/internal/git/housekeeping/testhelper_test.go b/internal/git/housekeeping/testhelper_test.go
index 05d4da7f7..f0afe0397 100644
--- a/internal/git/housekeeping/testhelper_test.go
+++ b/internal/git/housekeeping/testhelper_test.go
@@ -1,18 +1,11 @@
 package housekeeping
 
 import (
-	"os"
 	"testing"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 )
 
 func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	cleanup := testhelper.Configure()
-	defer cleanup()
-	return m.Run()
+	testhelper.Run(m)
 }
diff --git a/internal/git/localrepo/config_test.go b/internal/git/localrepo/config_test.go
index 15c440524..7abc7be08 100644
--- a/internal/git/localrepo/config_test.go
+++ b/internal/git/localrepo/config_test.go
@@ -31,7 +31,10 @@ func setupRepoConfig(t *testing.T) (Config, string) {
 	repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0])
 
 	gitCmdFactory := git.NewExecCommandFactory(cfg)
-	repo := New(gitCmdFactory, catfile.NewCache(cfg), repoProto, cfg)
+	catfileCache := catfile.NewCache(cfg)
+	t.Cleanup(catfileCache.Stop)
+
+	repo := New(gitCmdFactory, catfileCache, repoProto, cfg)
 
 	return repo.Config(), repoPath
 }
diff --git a/internal/git/localrepo/objects_test.go b/internal/git/localrepo/objects_test.go
index d1d797955..53c420d6b 100644
--- a/internal/git/localrepo/objects_test.go
+++ b/internal/git/localrepo/objects_test.go
@@ -35,7 +35,9 @@ func setupRepo(t *testing.T, bare bool) (*Repo, string) {
 	}
 
 	gitCmdFactory := git.NewExecCommandFactory(cfg)
-	return New(gitCmdFactory, catfile.NewCache(cfg), repoProto, cfg), repoPath
+	catfileCache := catfile.NewCache(cfg)
+	t.Cleanup(catfileCache.Stop)
+	return New(gitCmdFactory, catfileCache, repoProto, cfg), repoPath
 }
 
 type ReaderFunc func([]byte) (int, error)
diff --git a/internal/git/localrepo/refs_test.go b/internal/git/localrepo/refs_test.go
index 3d3fdcd32..6bdc9a7e1 100644
--- a/internal/git/localrepo/refs_test.go
+++ b/internal/git/localrepo/refs_test.go
@@ -242,9 +242,12 @@ func TestRepo_GetRemoteReferences(t *testing.T) {
 	annotatedTagOID := text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "rev-parse", "annotated-tag"))
 
 	gitCmdFactory := git.NewExecCommandFactory(cfg)
+	catfileCache := catfile.NewCache(cfg)
+	defer catfileCache.Stop()
+
 	repo := New(
 		gitCmdFactory,
-		catfile.NewCache(cfg),
+		catfileCache,
 		&gitalypb.Repository{StorageName: "default", RelativePath: filepath.Join(relativePath, ".git")},
 		cfg,
 	)
diff --git a/internal/git/localrepo/remote_test.go b/internal/git/localrepo/remote_test.go
index 6b165f0c9..547f0cf90 100644
--- a/internal/git/localrepo/remote_test.go
+++ b/internal/git/localrepo/remote_test.go
@@ -37,7 +37,10 @@ func setupRepoRemote(t *testing.T, bare bool) (Remote, string) {
 	}
 
 	gitCmdFactory := git.NewExecCommandFactory(cfg)
-	return New(gitCmdFactory, catfile.NewCache(cfg), repoProto, cfg).Remote(), repoPath
+	catfileCache := catfile.NewCache(cfg)
+	t.Cleanup(catfileCache.Stop)
+
+	return New(gitCmdFactory, catfileCache, repoProto, cfg).Remote(), repoPath
 }
 
 func TestRepo_Remote(t *testing.T) {
diff --git a/internal/git/localrepo/repo.go b/internal/git/localrepo/repo.go
index 08c996b18..af869a6bb 100644
--- a/internal/git/localrepo/repo.go
+++ b/internal/git/localrepo/repo.go
@@ -37,7 +37,9 @@ func New(gitCmdFactory git.CommandFactory, catfileCache catfile.Cache, repo repo
 // dependencies ad-hoc from the given config.
 func NewTestRepo(t testing.TB, cfg config.Cfg, repo repository.GitRepo) *Repo {
 	gitCmdFactory := git.NewExecCommandFactory(cfg)
-	return New(gitCmdFactory, catfile.NewCache(cfg), repo, cfg)
+	catfileCache := catfile.NewCache(cfg)
+	t.Cleanup(catfileCache.Stop)
+	return New(gitCmdFactory, catfileCache, repo, cfg)
 }
 
 // Path returns the on-disk path of the repository.
diff --git a/internal/git/localrepo/repo_test.go b/internal/git/localrepo/repo_test.go
index 8993471e0..c456e21b3 100644
--- a/internal/git/localrepo/repo_test.go
+++ b/internal/git/localrepo/repo_test.go
@@ -20,7 +20,9 @@ func TestRepo(t *testing.T) {
 	gittest.TestRepository(t, cfg, func(t testing.TB, pbRepo *gitalypb.Repository) git.Repository {
 		t.Helper()
 		gitCmdFactory := git.NewExecCommandFactory(cfg)
-		return New(gitCmdFactory, catfile.NewCache(cfg), pbRepo, cfg)
+		catfileCache := catfile.NewCache(cfg)
+		t.Cleanup(catfileCache.Stop)
+		return New(gitCmdFactory, catfileCache, pbRepo, cfg)
 	})
 }
 
@@ -28,7 +30,9 @@ func TestRepo_Path(t *testing.T) {
 	t.Run("valid repository", func(t *testing.T) {
 		cfg, repoProto, repoPath := testcfg.BuildWithRepo(t)
 		gitCmdFactory := git.NewExecCommandFactory(cfg)
-		repo := New(gitCmdFactory, catfile.NewCache(cfg), repoProto, cfg)
+		catfileCache := catfile.NewCache(cfg)
+		t.Cleanup(catfileCache.Stop)
+		repo := New(gitCmdFactory, catfileCache, repoProto, cfg)
 
 		path, err := repo.Path()
 		require.NoError(t, err)
@@ -38,7 +42,9 @@ func TestRepo_Path(t *testing.T) {
 	t.Run("deleted repository", func(t *testing.T) {
 		cfg, repoProto, repoPath := testcfg.BuildWithRepo(t)
 		gitCmdFactory := git.NewExecCommandFactory(cfg)
-		repo := New(gitCmdFactory, catfile.NewCache(cfg), repoProto, cfg)
+		catfileCache := catfile.NewCache(cfg)
+		t.Cleanup(catfileCache.Stop)
+		repo := New(gitCmdFactory, catfileCache, repoProto, cfg)
 
 		require.NoError(t, os.RemoveAll(repoPath))
 
@@ -49,7 +55,9 @@ func TestRepo_Path(t *testing.T) {
 	t.Run("non-git repository", func(t *testing.T) {
 		cfg, repoProto, repoPath := testcfg.BuildWithRepo(t)
 		gitCmdFactory := git.NewExecCommandFactory(cfg)
-		repo := New(gitCmdFactory, catfile.NewCache(cfg), repoProto, cfg)
+		catfileCache := catfile.NewCache(cfg)
+		t.Cleanup(catfileCache.Stop)
+		repo := New(gitCmdFactory, catfileCache, repoProto, cfg)
 
 		// Recreate the repository as a simple empty directory to simulate
 		// that the repository is in a partially-created state.
diff --git a/internal/git/localrepo/testhelper_test.go b/internal/git/localrepo/testhelper_test.go
index 224d1566b..38486d705 100644
--- a/internal/git/localrepo/testhelper_test.go
+++ b/internal/git/localrepo/testhelper_test.go
@@ -1,19 +1,11 @@
 package localrepo
 
 import (
-	"os"
 	"testing"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 )
 
 func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	defer testhelper.MustHaveNoChildProcess()
-	cleanup := testhelper.Configure()
-	defer cleanup()
-	return m.Run()
+	testhelper.Run(m)
 }
diff --git a/internal/git/objectpool/clone_test.go b/internal/git/objectpool/clone_test.go
index 0720170a0..f3689ef24 100644
--- a/internal/git/objectpool/clone_test.go
+++ b/internal/git/objectpool/clone_test.go
@@ -23,11 +23,14 @@ func setupObjectPool(t *testing.T) (*ObjectPool, *gitalypb.Repository) {
 	cfg, repo, _ := testcfg.BuildWithRepo(t)
 	gitCommandFactory := git.NewExecCommandFactory(cfg)
 
+	catfileCache := catfile.NewCache(cfg)
+	t.Cleanup(catfileCache.Stop)
+
 	pool, err := NewObjectPool(
 		cfg,
 		config.NewLocator(cfg),
 		gitCommandFactory,
-		catfile.NewCache(cfg),
+		catfileCache,
 		transaction.NewManager(cfg, backchannel.NewRegistry()),
 		repo.GetStorageName(),
 		gittest.NewObjectPoolName(t),
diff --git a/internal/git/objectpool/testhelper_test.go b/internal/git/objectpool/testhelper_test.go
index b252b37c1..bf79617d8 100644
--- a/internal/git/objectpool/testhelper_test.go
+++ b/internal/git/objectpool/testhelper_test.go
@@ -1,7 +1,6 @@
 package objectpool
 
 import (
-	"os"
 	"testing"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/hooks"
@@ -9,13 +8,8 @@ import (
 )
 
 func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	defer testhelper.MustHaveNoChildProcess()
-	cleanup := testhelper.Configure()
-	defer cleanup()
-	hooks.Override = "/"
-	return m.Run()
+	testhelper.Run(m, testhelper.WithSetup(func() error {
+		hooks.Override = "/"
+		return nil
+	}))
 }
diff --git a/internal/git/packfile/packfile_test.go b/internal/git/packfile/packfile_test.go
index c8d412827..ecf56563b 100644
--- a/internal/git/packfile/packfile_test.go
+++ b/internal/git/packfile/packfile_test.go
@@ -1,7 +1,6 @@
 package packfile_test
 
 import (
-	"os"
 	"path/filepath"
 	"testing"
 
@@ -13,14 +12,7 @@ import (
 )
 
 func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	defer testhelper.MustHaveNoChildProcess()
-	cleanup := testhelper.Configure()
-	defer cleanup()
-	return m.Run()
+	testhelper.Run(m)
 }
 
 func TestList(t *testing.T) {
diff --git a/internal/git/quarantine/testhelper_test.go b/internal/git/quarantine/testhelper_test.go
index 7ad131635..38d896d42 100644
--- a/internal/git/quarantine/testhelper_test.go
+++ b/internal/git/quarantine/testhelper_test.go
@@ -1,21 +1,11 @@
 package quarantine
 
 import (
-	"os"
 	"testing"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 )
 
 func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	defer testhelper.MustHaveNoChildProcess()
-
-	cleanup := testhelper.Configure()
-	defer cleanup()
-
-	return m.Run()
+	testhelper.Run(m)
 }
diff --git a/internal/git/remoterepo/helper_test.go b/internal/git/remoterepo/helper_test.go
index 555b5445d..619ff5b16 100644
--- a/internal/git/remoterepo/helper_test.go
+++ b/internal/git/remoterepo/helper_test.go
@@ -1,19 +1,11 @@
 package remoterepo
 
 import (
-	"os"
 	"testing"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 )
 
 func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	defer testhelper.MustHaveNoChildProcess()
-	cleanup := testhelper.Configure()
-	defer cleanup()
-	return m.Run()
+	testhelper.Run(m)
 }
diff --git a/internal/git/remoterepo/repository_test.go b/internal/git/remoterepo/repository_test.go
index ddc63fb11..5bb233fdc 100644
--- a/internal/git/remoterepo/repository_test.go
+++ b/internal/git/remoterepo/repository_test.go
@@ -32,6 +32,7 @@ func TestRepository(t *testing.T) {
 			deps.GetTxManager(),
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
+			deps.GetConnsPool(),
 		))
 		gitalypb.RegisterCommitServiceServer(srv, commit.NewServer(
 			deps.GetCfg(),
diff --git a/internal/git/stats/testhelper_test.go b/internal/git/stats/testhelper_test.go
index ce080ff27..b842dc949 100644
--- a/internal/git/stats/testhelper_test.go
+++ b/internal/git/stats/testhelper_test.go
@@ -1,19 +1,11 @@
 package stats
 
 import (
-	"os"
 	"testing"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 )
 
 func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	defer testhelper.MustHaveNoChildProcess()
-	cleanup := testhelper.Configure()
-	defer cleanup()
-	return m.Run()
+	testhelper.Run(m)
 }
diff --git a/internal/git/updateref/updateref_test.go b/internal/git/updateref/updateref_test.go
index c2cf6c0d6..17c87fd16 100644
--- a/internal/git/updateref/updateref_test.go
+++ b/internal/git/updateref/updateref_test.go
@@ -3,7 +3,6 @@ package updateref
 import (
 	"context"
 	"fmt"
-	"os"
 	"strings"
 	"testing"
 
@@ -17,15 +16,10 @@ import (
 )
 
 func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	defer testhelper.MustHaveNoChildProcess()
-	cleanup := testhelper.Configure()
-	defer cleanup()
-	hooks.Override = "/"
-	return m.Run()
+	testhelper.Run(m, testhelper.WithSetup(func() error {
+		hooks.Override = "/"
+		return nil
+	}))
 }
 
 func setupUpdater(t *testing.T, ctx context.Context) (config.Cfg, *localrepo.Repo, *Updater) {