Merge pull request #817 from MichaelMure/refactor/guarantee-test-cleanup

refactor(809): guarantee test cleanup

4 files changed, 44 insertions, 64 deletions

diff --git a/repository/gogit_test.go b/repository/gogit_test.go
index c376de22..a3de0a03 100644
--- a/repository/gogit_test.go
+++ b/repository/gogit_test.go
@@ -1,8 +1,6 @@
 package repository
 
 import (
-	"io/ioutil"
-	"os"
 	"path"
 	"path/filepath"
 	"testing"
@@ -13,26 +11,14 @@ import (
 
 func TestNewGoGitRepo(t *testing.T) {
 	// Plain
-	plainRoot, err := ioutil.TempDir("", "")
-	require.NoError(t, err)
-	t.Cleanup(func() {
-		require.NoError(t, os.RemoveAll(plainRoot))
-	})
-
-	plainRepo, err := InitGoGitRepo(plainRoot, namespace)
-	require.NoError(t, err)
+	plainRepo := CreateGoGitTestRepo(t, false)
+	plainRoot := goGitRepoDir(t, plainRepo)
 	require.NoError(t, plainRepo.Close())
 	plainGitDir := filepath.Join(plainRoot, ".git")
 
 	// Bare
-	bareRoot, err := ioutil.TempDir("", "")
-	require.NoError(t, err)
-	t.Cleanup(func() {
-		require.NoError(t, os.RemoveAll(bareRoot))
-	})
-
-	bareRepo, err := InitBareGoGitRepo(bareRoot, namespace)
-	require.NoError(t, err)
+	bareRepo := CreateGoGitTestRepo(t, true)
+	bareRoot := goGitRepoDir(t, bareRepo)
 	require.NoError(t, bareRepo.Close())
 	bareGitDir := bareRoot
 
@@ -71,17 +57,12 @@ func TestNewGoGitRepo(t *testing.T) {
 }
 
 func TestGoGitRepo(t *testing.T) {
-	RepoTest(t, CreateGoGitTestRepo, CleanupTestRepos)
+	RepoTest(t, CreateGoGitTestRepo)
 }
 
 func TestGoGitRepo_Indexes(t *testing.T) {
-	plainRoot := t.TempDir()
-
-	repo, err := InitGoGitRepo(plainRoot, namespace)
-	require.NoError(t, err)
-	t.Cleanup(func() {
-		require.NoError(t, repo.Close())
-	})
+	repo := CreateGoGitTestRepo(t, false)
+	plainRoot := goGitRepoDir(t, repo)
 
 	// Can create indices
 	indexA, err := repo.GetBleveIndex("a")
diff --git a/repository/gogit_testing.go b/repository/gogit_testing.go
index 7647c711..afbb917f 100644
--- a/repository/gogit_testing.go
+++ b/repository/gogit_testing.go
@@ -1,8 +1,10 @@
 package repository
 
 import (
-	"io/ioutil"
 	"log"
+	"path/filepath"
+	"strings"
+	"testing"
 
 	"github.com/99designs/keyring"
 )
@@ -11,11 +13,10 @@ const namespace = "git-bug"
 
 // This is intended for testing only
 
-func CreateGoGitTestRepo(bare bool) TestedRepo {
-	dir, err := ioutil.TempDir("", "")
-	if err != nil {
-		log.Fatal(err)
-	}
+func CreateGoGitTestRepo(t testing.TB, bare bool) TestedRepo {
+	t.Helper()
+
+	dir := t.TempDir()
 
 	var creator func(string, string) (*GoGitRepo, error)
 
@@ -30,6 +31,10 @@ func CreateGoGitTestRepo(bare bool) TestedRepo {
 		log.Fatal(err)
 	}
 
+	t.Cleanup(func() {
+		repo.Close()
+	})
+
 	config := repo.LocalConfig()
 	if err := config.StoreString("user.name", "testuser"); err != nil {
 		log.Fatal("failed to set user.name for test repository: ", err)
@@ -45,10 +50,12 @@ func CreateGoGitTestRepo(bare bool) TestedRepo {
 	}
 }
 
-func SetupGoGitReposAndRemote() (repoA, repoB, remote TestedRepo) {
-	repoA = CreateGoGitTestRepo(false)
-	repoB = CreateGoGitTestRepo(false)
-	remote = CreateGoGitTestRepo(true)
+func SetupGoGitReposAndRemote(t *testing.T) (repoA, repoB, remote TestedRepo) {
+	t.Helper()
+
+	repoA = CreateGoGitTestRepo(t, false)
+	repoB = CreateGoGitTestRepo(t, false)
+	remote = CreateGoGitTestRepo(t, true)
 
 	err := repoA.AddRemote("origin", remote.GetLocalRemote())
 	if err != nil {
@@ -62,3 +69,18 @@ func SetupGoGitReposAndRemote() (repoA, repoB, remote TestedRepo) {
 
 	return repoA, repoB, remote
 }
+
+func goGitRepoDir(t *testing.T, repo TestedRepo) string {
+	t.Helper()
+
+	dir := repo.GetLocalRemote()
+	if strings.HasSuffix(dir, ".git") {
+		dir, _ = filepath.Split(dir)
+	}
+
+	if dir[len(dir)-1] == filepath.Separator {
+		dir = dir[:len(dir)-1]
+	}
+
+	return dir
+}
diff --git a/repository/mock_repo_test.go b/repository/mock_repo_test.go
index 12851a80..66fad7b6 100644
--- a/repository/mock_repo_test.go
+++ b/repository/mock_repo_test.go
@@ -5,8 +5,7 @@ import (
 )
 
 func TestMockRepo(t *testing.T) {
-	creator := func(bare bool) TestedRepo { return NewMockRepo() }
-	cleaner := func(repos ...Repo) {}
+	creator := func(t testing.TB, bare bool) TestedRepo { return NewMockRepo() }
 
-	RepoTest(t, creator, cleaner)
+	RepoTest(t, creator)
 }
diff --git a/repository/repo_testing.go b/repository/repo_testing.go
index 0db585cd..5d51d23f 100644
--- a/repository/repo_testing.go
+++ b/repository/repo_testing.go
@@ -1,7 +1,6 @@
 package repository
 
 import (
-	"log"
 	"math/rand"
 	"testing"
 
@@ -14,37 +13,16 @@ import (
 // TODO: add tests for RepoBleve
 // TODO: add tests for RepoStorage
 
-func CleanupTestRepos(repos ...Repo) {
-	var firstErr error
-	for _, repo := range repos {
-		if repo, ok := repo.(TestedRepo); ok {
-			err := repo.EraseFromDisk()
-			if err != nil {
-				log.Println(err)
-				if firstErr == nil {
-					firstErr = err
-				}
-			}
-		}
-	}
-
-	if firstErr != nil {
-		log.Fatal(firstErr)
-	}
-}
-
-type RepoCreator func(bare bool) TestedRepo
-type RepoCleaner func(repos ...Repo)
+type RepoCreator func(t testing.TB, bare bool) TestedRepo
 
 // Test suite for a Repo implementation
-func RepoTest(t *testing.T, creator RepoCreator, cleaner RepoCleaner) {
+func RepoTest(t *testing.T, creator RepoCreator) {
 	for bare, name := range map[bool]string{
 		false: "Plain",
 		true:  "Bare",
 	} {
 		t.Run(name, func(t *testing.T) {
-			repo := creator(bare)
-			defer cleaner(repo)
+			repo := creator(t, bare)
 
 			t.Run("Data", func(t *testing.T) {
 				RepoDataTest(t, repo)