Remove test data after test completion

Fix tests that doesn't clean up after run.
Unused code removed: `satisfyConfigValidation`,`cwd`.
Global Config changed to generate `BinDir` instead of
`ConfigureGitalyHooksBinary` and  `ConfigureGitalySSH`.
Deferred cleanup will report about errors.
`testhelper.TempDir` simplified as there is no much sense
in `prefix` parameter.

16 files changed, 55 insertions, 94 deletions

diff --git a/internal/cache/walker_test.go b/internal/cache/walker_test.go
index 50e4a55c5..ef7ba95f9 100644
--- a/internal/cache/walker_test.go
+++ b/internal/cache/walker_test.go
@@ -104,8 +104,6 @@ func setupDiskCacheWalker(t testing.TB) func() {
 		},
 	}
 
-	satisfyConfigValidation(tmpPath)
-
 	cleanup := func() {
 		config.Config.Storages = oldStorages
 		require.NoError(t, os.RemoveAll(tmpPath))
@@ -114,39 +112,6 @@ func setupDiskCacheWalker(t testing.TB) func() {
 	return cleanup
 }
 
-// satisfyConfigValidation puts garbage values in the config file to satisfy
-// validation
-func satisfyConfigValidation(tmpPath string) error {
-	config.Config.ListenAddr = "meow"
-
-	if err := os.MkdirAll(filepath.Join(tmpPath, "hooks"), 0700); err != nil {
-		return err
-	}
-	if err := os.MkdirAll(filepath.Join(tmpPath, "git-hooks"), 0700); err != nil {
-		return err
-	}
-
-	for _, filePath := range []string{
-		filepath.Join("ruby", "git-hooks", "pre-receive"),
-		filepath.Join("ruby", "git-hooks", "post-receive"),
-		filepath.Join("ruby", "git-hooks", "update"),
-	} {
-		if err := ioutil.WriteFile(filepath.Join(tmpPath, filePath), nil, 0755); err != nil {
-			return err
-		}
-	}
-	config.Config.GitlabShell = config.GitlabShell{
-		Dir: filepath.Join(tmpPath, "gitlab-shell"),
-	}
-	config.Config.Ruby = config.Ruby{
-		Dir: filepath.Join(tmpPath, "ruby"),
-	}
-
-	config.Config.BinDir = filepath.Join(tmpPath, "bin")
-
-	return nil
-}
-
 func pollCountersUntil(t testing.TB, expectRemovals int) {
 	// poll injected mock prometheus counters until expected events occur
 	timeout := time.After(time.Second)
diff --git a/internal/config/config.go b/internal/config/config.go
index 0f041f2a9..79c244a12 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -319,8 +319,7 @@ func (c Cfg) Storage(storageName string) (Storage, bool) {
 func validateBinDir() error {
 	if err := validateIsDirectory(Config.BinDir, "bin_dir"); err != nil {
 		log.WithError(err).Warn("Gitaly bin directory is not configured")
-		// TODO this must become a fatal error
-		return nil
+		return err
 	}
 
 	var err error
diff --git a/internal/git/dirs_test.go b/internal/git/dirs_test.go
index ae73b912a..e0bb87e0c 100644
--- a/internal/git/dirs_test.go
+++ b/internal/git/dirs_test.go
@@ -50,7 +50,7 @@ func TestObjectDirsNoAlternates(t *testing.T) {
 }
 
 func TestObjectDirsOutsideStorage(t *testing.T) {
-	tmp, clean := testhelper.TempDir(t, "")
+	tmp, clean := testhelper.TempDir(t)
 	defer clean()
 
 	storageRoot := filepath.Join(tmp, "storage-root")
diff --git a/internal/praefect/info_service_test.go b/internal/praefect/info_service_test.go
index b35769bd4..5ee9d96cd 100644
--- a/internal/praefect/info_service_test.go
+++ b/internal/praefect/info_service_test.go
@@ -44,7 +44,7 @@ func TestInfoService_RepositoryReplicas(t *testing.T) {
 		gconfig.Config.Storages = storages
 	}(gconfig.Config.Storages)
 
-	tempDir, cleanupTempDir := testhelper.TempDir(t, "praefect-test")
+	tempDir, cleanupTempDir := testhelper.TempDir(t)
 	defer cleanupTempDir()
 
 	for _, node := range conf.VirtualStorages[0].Nodes {
diff --git a/internal/praefect/replicator_test.go b/internal/praefect/replicator_test.go
index 0a2830a4e..f4d7c7bfc 100644
--- a/internal/praefect/replicator_test.go
+++ b/internal/praefect/replicator_test.go
@@ -767,7 +767,6 @@ func newRepositoryClient(t *testing.T, serverSocketPath string) (gitalypb.Reposi
 var RubyServer = &rubyserver.Server{}
 
 func TestMain(m *testing.M) {
-	testhelper.ConfigureGitalySSH()
 	testhelper.Configure()
 	os.Exit(testMain(m))
 }
@@ -783,6 +782,8 @@ func testMain(m *testing.M) int {
 		log.Fatal(err)
 	}
 
+	testhelper.ConfigureGitalySSH()
+
 	if err := RubyServer.Start(); err != nil {
 		log.Fatal(err)
 	}
diff --git a/internal/praefect/server_test.go b/internal/praefect/server_test.go
index f0f3fac88..62f095c3d 100644
--- a/internal/praefect/server_test.go
+++ b/internal/praefect/server_test.go
@@ -560,13 +560,17 @@ func TestRepoRename(t *testing.T) {
 	require.NoError(t, err)
 	require.True(t, resp.GetExists(), "repo with new name must exist")
 	require.DirExists(t, expNewPath0, "must be renamed on secondary from %q to %q", path0, expNewPath0)
+	defer func() { require.NoError(t, os.RemoveAll(expNewPath0)) }()
 
 	// the renaming of the repo on the secondary servers is not deterministic
 	// since it relies on eventually consistent replication
 	pollUntilRemoved(t, path1, time.After(10*time.Second))
 	require.DirExists(t, expNewPath1, "must be renamed on secondary from %q to %q", path1, expNewPath1)
+	defer func() { require.NoError(t, os.RemoveAll(expNewPath1)) }()
+
 	pollUntilRemoved(t, path2, time.After(10*time.Second))
 	require.DirExists(t, expNewPath2, "must be renamed on secondary from %q to %q", path2, expNewPath2)
+	defer func() { require.NoError(t, os.RemoveAll(expNewPath2)) }()
 }
 
 func tempStoragePath(t testing.TB) string {
diff --git a/internal/safe/file_writer_test.go b/internal/safe/file_writer_test.go
index 08b498f0b..308e3eb7a 100644
--- a/internal/safe/file_writer_test.go
+++ b/internal/safe/file_writer_test.go
@@ -15,7 +15,7 @@ import (
 )
 
 func TestFile(t *testing.T) {
-	dir, cleanup := testhelper.TempDir(t, t.Name())
+	dir, cleanup := testhelper.TempDir(t)
 	defer cleanup()
 
 	filePath := filepath.Join(dir, "test_file_contents")
@@ -41,7 +41,7 @@ func TestFile(t *testing.T) {
 }
 
 func TestFileRace(t *testing.T) {
-	dir, cleanup := testhelper.TempDir(t, t.Name())
+	dir, cleanup := testhelper.TempDir(t)
 	defer cleanup()
 
 	filePath := filepath.Join(dir, "test_file_contents")
@@ -68,7 +68,7 @@ func TestFileRace(t *testing.T) {
 }
 
 func TestFileCloseBeforeCommit(t *testing.T) {
-	dir, cleanup := testhelper.TempDir(t, t.Name())
+	dir, cleanup := testhelper.TempDir(t)
 	defer cleanup()
 
 	dstPath := filepath.Join(dir, "safety_meow")
@@ -87,7 +87,7 @@ func TestFileCloseBeforeCommit(t *testing.T) {
 }
 
 func TestFileCommitBeforeClose(t *testing.T) {
-	dir, cleanup := testhelper.TempDir(t, t.Name())
+	dir, cleanup := testhelper.TempDir(t)
 	defer cleanup()
 
 	dstPath := filepath.Join(dir, "safety_meow")
diff --git a/internal/service/repository/create_test.go b/internal/service/repository/create_test.go
index 889784043..9fe052bb5 100644
--- a/internal/service/repository/create_test.go
+++ b/internal/service/repository/create_test.go
@@ -35,6 +35,7 @@ func TestCreateRepositorySuccess(t *testing.T) {
 	req := &gitalypb.CreateRepositoryRequest{Repository: repo}
 	_, err = client.CreateRepository(ctx, req)
 	require.NoError(t, err)
+	defer func() { require.NoError(t, os.RemoveAll(repoDir)) }()
 
 	fi, err := os.Stat(repoDir)
 	require.NoError(t, err)
diff --git a/internal/service/repository/fetch_test.go b/internal/service/repository/fetch_test.go
index 1f586c4d4..9257f1a90 100644
--- a/internal/service/repository/fetch_test.go
+++ b/internal/service/repository/fetch_test.go
@@ -32,8 +32,11 @@ func TestFetchSourceBranchSourceRepositorySuccess(t *testing.T) {
 	md := testhelper.GitalyServersMetadata(t, serverSocketPath)
 	ctx := metadata.NewOutgoingContext(ctxOuter, md)
 
-	targetRepo, _ := newTestRepo(t, "fetch-source-target.git")
-	sourceRepo, sourcePath := newTestRepo(t, "fetch-source-source.git")
+	targetRepo, _, cleanup := newTestRepo(t, "fetch-source-target.git")
+	defer cleanup()
+
+	sourceRepo, sourcePath, cleanup := newTestRepo(t, "fetch-source-source.git")
+	defer cleanup()
 
 	sourceBranch := "fetch-source-branch-test-branch"
 	newCommitID := testhelper.CreateCommit(t, sourcePath, sourceBranch, nil)
@@ -68,7 +71,8 @@ func TestFetchSourceBranchSameRepositorySuccess(t *testing.T) {
 	md := testhelper.GitalyServersMetadata(t, serverSocketPath)
 	ctx := metadata.NewOutgoingContext(ctxOuter, md)
 
-	repo, repoPath := newTestRepo(t, "fetch-source-source.git")
+	repo, repoPath, cleanup := newTestRepo(t, "fetch-source-source.git")
+	defer cleanup()
 
 	sourceBranch := "fetch-source-branch-test-branch"
 	newCommitID := testhelper.CreateCommit(t, repoPath, sourceBranch, nil)
@@ -103,8 +107,11 @@ func TestFetchSourceBranchBranchNotFound(t *testing.T) {
 	md := testhelper.GitalyServersMetadata(t, serverSocketPath)
 	ctx := metadata.NewOutgoingContext(ctxOuter, md)
 
-	targetRepo, _ := newTestRepo(t, "fetch-source-target.git")
-	sourceRepo, _ := newTestRepo(t, "fetch-source-source.git")
+	targetRepo, _, cleanup := newTestRepo(t, "fetch-source-target.git")
+	defer cleanup()
+
+	sourceRepo, _, cleanup := newTestRepo(t, "fetch-source-source.git")
+	defer cleanup()
 
 	sourceBranch := "does-not-exist"
 	targetRef := "refs/tmp/fetch-source-branch-test"
@@ -168,7 +175,7 @@ func TestFetchFullServerRequiresAuthentication(t *testing.T) {
 	testhelper.RequireGrpcError(t, err, codes.Unauthenticated)
 }
 
-func newTestRepo(t *testing.T, relativePath string) (*gitalypb.Repository, string) {
+func newTestRepo(t *testing.T, relativePath string) (*gitalypb.Repository, string, func()) {
 	_, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
@@ -180,7 +187,7 @@ func newTestRepo(t *testing.T, relativePath string) (*gitalypb.Repository, strin
 	require.NoError(t, os.RemoveAll(repoPath))
 	testhelper.MustRunCommand(t, nil, "git", "clone", "--bare", testRepoPath, repoPath)
 
-	return repo, repoPath
+	return repo, repoPath, func() { require.NoError(t, os.RemoveAll(repoPath)) }
 }
 
 func runFullServer(t *testing.T) (*grpc.Server, string) {
diff --git a/internal/service/repository/fork_test.go b/internal/service/repository/fork_test.go
index e04ef8c06..7f603823b 100644
--- a/internal/service/repository/fork_test.go
+++ b/internal/service/repository/fork_test.go
@@ -86,6 +86,7 @@ func TestSuccessfulCreateForkRequest(t *testing.T) {
 
 			_, err = client.CreateFork(ctx, req)
 			require.NoError(t, err)
+			defer func() { require.NoError(t, os.RemoveAll(forkedRepoPath)) }()
 
 			testhelper.MustRunCommand(t, nil, "git", "-C", forkedRepoPath, "fsck")
 
diff --git a/internal/service/repository/redirecting_test_server_test.go b/internal/service/repository/redirecting_test_server_test.go
index d9dee1840..f5a200488 100644
--- a/internal/service/repository/redirecting_test_server_test.go
+++ b/internal/service/repository/redirecting_test_server_test.go
@@ -38,7 +38,7 @@ func StartRedirectingTestServer() (*RedirectingTestServerState, *httptest.Server
 }
 
 func TestRedirectingServerRedirects(t *testing.T) {
-	dir, cleanup := testhelper.TempDir(t, t.Name())
+	dir, cleanup := testhelper.TempDir(t)
 	defer cleanup()
 
 	httpServerState, redirectingServer := StartRedirectingTestServer()
diff --git a/internal/service/repository/rename_test.go b/internal/service/repository/rename_test.go
index 4edf51f92..2727f243a 100644
--- a/internal/service/repository/rename_test.go
+++ b/internal/service/repository/rename_test.go
@@ -1,7 +1,7 @@
 package repository
 
 import (
-	"path/filepath"
+	"os"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -21,12 +21,7 @@ func TestRenameRepositorySuccess(t *testing.T) {
 	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
-	tempDir, cleanupTempDir := testhelper.TempDir(t, t.Name())
-	defer cleanupTempDir()
-
-	destinationPath := filepath.Join(tempDir, "a", "new", "location")
-
-	req := &gitalypb.RenameRepositoryRequest{Repository: testRepo, RelativePath: destinationPath}
+	req := &gitalypb.RenameRepositoryRequest{Repository: testRepo, RelativePath: "a-new-location"}
 
 	ctx, cancel := testhelper.Context()
 	defer cancel()
@@ -34,9 +29,10 @@ func TestRenameRepositorySuccess(t *testing.T) {
 	_, err := client.RenameRepository(ctx, req)
 	require.NoError(t, err)
 
-	newDirectory, err := helper.GetPath(&gitalypb.Repository{StorageName: "default", RelativePath: destinationPath})
+	newDirectory, err := helper.GetPath(&gitalypb.Repository{StorageName: "default", RelativePath: req.RelativePath})
 	require.NoError(t, err)
 	require.DirExists(t, newDirectory)
+	defer func() { require.NoError(t, os.RemoveAll(newDirectory)) }()
 
 	require.True(t, helper.IsGitDirectory(newDirectory), "moved Git repository has been corrupted")
 
diff --git a/internal/service/repository/replicate_test.go b/internal/service/repository/replicate_test.go
index 2dd9a8fbf..6e452d396 100644
--- a/internal/service/repository/replicate_test.go
+++ b/internal/service/repository/replicate_test.go
@@ -25,7 +25,7 @@ import (
 )
 
 func TestReplicateRepository(t *testing.T) {
-	tmpPath, cleanup := testhelper.TempDir(t, t.Name())
+	tmpPath, cleanup := testhelper.TempDir(t)
 	defer cleanup()
 
 	replicaPath := filepath.Join(tmpPath, "replica")
@@ -197,7 +197,7 @@ func TestReplicateRepositoryInvalidArguments(t *testing.T) {
 }
 
 func TestReplicateRepository_BadRepository(t *testing.T) {
-	tmpPath, cleanup := testhelper.TempDir(t, t.Name())
+	tmpPath, cleanup := testhelper.TempDir(t)
 	defer cleanup()
 
 	replicaPath := filepath.Join(tmpPath, "replica")
@@ -254,7 +254,7 @@ func TestReplicateRepository_BadRepository(t *testing.T) {
 }
 
 func TestReplicateRepository_FailedFetchInternalRemote(t *testing.T) {
-	tmpPath, cleanup := testhelper.TempDir(t, t.Name())
+	tmpPath, cleanup := testhelper.TempDir(t)
 	defer cleanup()
 
 	replicaPath := filepath.Join(tmpPath, "replica")
diff --git a/internal/service/ssh/testhelper_test.go b/internal/service/ssh/testhelper_test.go
index b9a437039..8ef0b490d 100644
--- a/internal/service/ssh/testhelper_test.go
+++ b/internal/service/ssh/testhelper_test.go
@@ -23,7 +23,6 @@ const (
 var (
 	testRepo      *gitalypb.Repository
 	gitalySSHPath string
-	cwd           string
 )
 
 func TestMain(m *testing.M) {
@@ -35,7 +34,6 @@ func testMain(m *testing.M) int {
 	defer testhelper.MustHaveNoChildProcess()
 
 	hooks.Override = "/"
-	cwd = mustGetCwd()
 
 	err := os.RemoveAll(testPath)
 	if err != nil {
@@ -51,14 +49,6 @@ func testMain(m *testing.M) int {
 	return m.Run()
 }
 
-func mustGetCwd() string {
-	wd, err := os.Getwd()
-	if err != nil {
-		log.Panic(err)
-	}
-	return wd
-}
-
 func runSSHServer(t *testing.T, serverOpts ...ServerOpt) (string, func()) {
 	srv := testhelper.NewServer(t, nil, nil)
 
diff --git a/internal/testhelper/hook_env.go b/internal/testhelper/hook_env.go
index 8f5356d26..519df694a 100644
--- a/internal/testhelper/hook_env.go
+++ b/internal/testhelper/hook_env.go
@@ -2,11 +2,11 @@ package testhelper
 
 import (
 	"io/ioutil"
+	"log"
 	"os"
 	"path"
 	"path/filepath"
 
-	log "github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/git/hooks"
@@ -37,11 +37,8 @@ env | grep -e ^GIT -e ^GL_ > `+hookOutputFile+"\n"), 0755))
 
 // ConfigureGitalyHooksBinary builds gitaly-hooks command for tests
 func ConfigureGitalyHooksBinary() {
-	var err error
-
-	config.Config.BinDir, err = filepath.Abs("testdata/gitaly-libexec")
-	if err != nil {
-		log.Fatal(err)
+	if config.Config.BinDir == "" {
+		log.Fatal("config.Config.BinDir must be set")
 	}
 
 	goBuildArgs := []string{
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index d0003e1e1..8eb2dc411 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -68,6 +68,14 @@ func Configure() {
 
 		config.Config.InternalSocketDir = dir
 
+		if err := os.MkdirAll("testdata/gitaly-libexec", 0755); err != nil {
+			log.Fatal(err)
+		}
+		config.Config.BinDir, err = filepath.Abs("testdata/gitaly-libexec")
+		if err != nil {
+			log.Fatal(err)
+		}
+
 		for _, f := range []func() error{
 			ConfigureRuby,
 			ConfigureGit,
@@ -448,7 +456,7 @@ func initRepo(t TB, bare bool) (*gitalypb.Repository, string, func()) {
 		repo.RelativePath = path.Join(repo.RelativePath, ".git")
 	}
 
-	return repo, repoPath, func() { os.RemoveAll(repoPath) }
+	return repo, repoPath, func() { require.NoError(t, os.RemoveAll(repoPath)) }
 }
 
 // NewTestRepo creates a bare copy of the test repository.
@@ -481,7 +489,7 @@ func cloneTestRepo(t TB, bare bool) (repo *gitalypb.Repository, repoPath string,
 
 	MustRunCommand(t, nil, "git", append(args, testRepoPath, repoPath)...)
 
-	return repo, repoPath, func() { os.RemoveAll(repoPath) }
+	return repo, repoPath, func() { require.NoError(t, os.RemoveAll(repoPath)) }
 }
 
 // AddWorktreeArgs returns git command arguments for adding a worktree at the
@@ -497,11 +505,8 @@ func AddWorktree(t TB, repoPath string, worktreeName string) {
 
 // ConfigureGitalySSH configures the gitaly-ssh command for tests
 func ConfigureGitalySSH() {
-	var err error
-
-	config.Config.BinDir, err = filepath.Abs("testdata/gitaly-libexec")
-	if err != nil {
-		log.Fatal(err)
+	if config.Config.BinDir == "" {
+		log.Fatal("config.Config.BinDir must be set")
 	}
 
 	goBuildArgs := []string{
@@ -562,21 +567,16 @@ func CreateLooseRef(t TB, repoPath, refName string) {
 }
 
 // TempDir is a wrapper around ioutil.TempDir that provides a cleanup function.
-// The returned temp directory will be created in the directory specified by
-// environment variable TEST_TEMP_DIR_PATH. If that variable is unset, the
-// relative folder "./testdata/tmp" to this source file will be used.
-func TempDir(t TB, prefix string) (string, func() error) {
+func TempDir(t TB) (string, func()) {
 	_, currentFile, _, ok := runtime.Caller(0)
 	if !ok {
 		log.Fatal("Could not get caller info")
 	}
 
 	rootTmpDir := path.Join(path.Dir(currentFile), "testdata/tmp")
-	dirPath, err := ioutil.TempDir(rootTmpDir, prefix)
+	tmpDir, err := ioutil.TempDir(rootTmpDir, "")
 	require.NoError(t, err)
-	return dirPath, func() error {
-		return os.RemoveAll(dirPath)
-	}
+	return tmpDir, func() { require.NoError(t, os.RemoveAll(tmpDir)) }
 }
 
 // GitObjectMustExist is a test assertion that fails unless the git repo in repoPath contains sha