refactor(809): eliminate need to defer CleanupTestRepos()

Steve Moyer created 3 years ago

Change summary

api/graphql/graphql_test.go              |  3 -
api/http/git_file_handlers_test.go       |  3 -
bridge/github/export_test.go             |  6 +---
bridge/github/import_integration_test.go |  3 -
bridge/github/import_test.go             |  3 -
bridge/gitlab/export_test.go             |  6 +---
bridge/gitlab/import_test.go             |  3 -
bug/operation_test.go                    |  3 -
cache/repo_cache_test.go                 | 18 +++++---------
commands/env_testing.go                  |  2 +
commands/select/select_test.go           |  3 -
entity/dag/common_test.go                |  6 ++--
entity/dag/entity_actions_test.go        | 12 +++------
identity/identity_actions_test.go        |  3 -
identity/identity_test.go                |  7 ++---
repository/gogit_test.go                 |  2 
repository/gogit_testing.go              | 31 +++++++++++++++++--------
repository/mock_repo_test.go             |  5 +--
repository/repo_testing.go               | 28 ++--------------------
tests/read_bugs_test.go                  |  6 +---
20 files changed, 60 insertions(+), 93 deletions(-)

Detailed changes

api/graphql/graphql_test.go

@@ -14,8 +14,7 @@ import (
 )
 
 func TestQueries(t *testing.T) {
-	repo := repository.CreateGoGitTestRepo(false)
-	defer repository.CleanupTestRepos(repo)
+	repo := repository.CreateGoGitTestRepo(t, false)
 
 	random_bugs.FillRepoWithSeed(repo, 10, 42)
 

api/http/git_file_handlers_test.go

@@ -19,8 +19,7 @@ import (
 )
 
 func TestGitFileHandlers(t *testing.T) {
-	repo := repository.CreateGoGitTestRepo(false)
-	defer repository.CleanupTestRepos(repo)
+	repo := repository.CreateGoGitTestRepo(t, false)
 
 	mrc := cache.NewMultiRepoCache()
 	repoCache, err := mrc.RegisterDefaultRepository(repo)

bridge/github/export_test.go

@@ -137,8 +137,7 @@ func TestGithubPushPull(t *testing.T) {
 	}
 
 	// create repo backend
-	repo := repository.CreateGoGitTestRepo(false)
-	defer repository.CleanupTestRepos(repo)
+	repo := repository.CreateGoGitTestRepo(t, false)
 
 	backend, err := cache.NewRepoCache(repo)
 	require.NoError(t, err)
@@ -213,8 +212,7 @@ func TestGithubPushPull(t *testing.T) {
 
 	fmt.Printf("test repository exported in %f seconds\n", time.Since(start).Seconds())
 
-	repoTwo := repository.CreateGoGitTestRepo(false)
-	defer repository.CleanupTestRepos(repoTwo)
+	repoTwo := repository.CreateGoGitTestRepo(t, false)
 
 	// create a second backend
 	backendTwo, err := cache.NewRepoCache(repoTwo)

bridge/github/import_integration_test.go

@@ -32,8 +32,7 @@ func TestGithubImporterIntegration(t *testing.T) {
 	importer.client = &rateLimitHandlerClient{sc: clientMock}
 
 	// arrange
-	repo := repository.CreateGoGitTestRepo(false)
-	defer repository.CleanupTestRepos(repo)
+	repo := repository.CreateGoGitTestRepo(t, false)
 	backend, err := cache.NewRepoCache(repo)
 	require.NoError(t, err)
 	defer backend.Close()

bridge/github/import_test.go

@@ -24,8 +24,7 @@ func TestGithubImporter(t *testing.T) {
 		t.Skip("Env var GITHUB_TOKEN_PRIVATE missing")
 	}
 
-	repo := repository.CreateGoGitTestRepo(false)
-	defer repository.CleanupTestRepos(repo)
+	repo := repository.CreateGoGitTestRepo(t, false)
 
 	backend, err := cache.NewRepoCache(repo)
 	require.NoError(t, err)

bridge/gitlab/export_test.go

@@ -142,8 +142,7 @@ func TestGitlabPushPull(t *testing.T) {
 	}
 
 	// create repo backend
-	repo := repository.CreateGoGitTestRepo(false)
-	defer repository.CleanupTestRepos(repo)
+	repo := repository.CreateGoGitTestRepo(t, false)
 
 	backend, err := cache.NewRepoCache(repo)
 	require.NoError(t, err)
@@ -215,8 +214,7 @@ func TestGitlabPushPull(t *testing.T) {
 
 	fmt.Printf("test repository exported in %f seconds\n", time.Since(start).Seconds())
 
-	repoTwo := repository.CreateGoGitTestRepo(false)
-	defer repository.CleanupTestRepos(repoTwo)
+	repoTwo := repository.CreateGoGitTestRepo(t, false)
 
 	// create a second backend
 	backendTwo, err := cache.NewRepoCache(repoTwo)

bridge/gitlab/import_test.go

@@ -29,8 +29,7 @@ func TestGitlabImport(t *testing.T) {
 		t.Skip("Env var GITLAB_PROJECT_ID missing")
 	}
 
-	repo := repository.CreateGoGitTestRepo(false)
-	defer repository.CleanupTestRepos(repo)
+	repo := repository.CreateGoGitTestRepo(t, false)
 
 	backend, err := cache.NewRepoCache(repo)
 	require.NoError(t, err)

bug/operation_test.go

@@ -92,8 +92,7 @@ func TestMetadata(t *testing.T) {
 }
 
 func TestID(t *testing.T) {
-	repo := repository.CreateGoGitTestRepo(false)
-	defer repository.CleanupTestRepos(repo)
+	repo := repository.CreateGoGitTestRepo(t, false)
 
 	repos := []repository.ClockedRepo{
 		repository.NewMockRepo(),

cache/repo_cache_test.go

@@ -14,8 +14,7 @@ import (
 )
 
 func TestCache(t *testing.T) {
-	repo := repository.CreateGoGitTestRepo(false)
-	defer repository.CleanupTestRepos(repo)
+	repo := repository.CreateGoGitTestRepo(t, false)
 
 	cache, err := NewRepoCache(repo)
 	require.NoError(t, err)
@@ -112,8 +111,7 @@ func TestCache(t *testing.T) {
 }
 
 func TestCachePushPull(t *testing.T) {
-	repoA, repoB, remote := repository.SetupGoGitReposAndRemote()
-	defer repository.CleanupTestRepos(repoA, repoB, remote)
+	repoA, repoB, _ := repository.SetupGoGitReposAndRemote(t)
 
 	cacheA, err := NewRepoCache(repoA)
 	require.NoError(t, err)
@@ -171,10 +169,9 @@ func TestCachePushPull(t *testing.T) {
 }
 
 func TestRemove(t *testing.T) {
-	repo := repository.CreateGoGitTestRepo(false)
-	remoteA := repository.CreateGoGitTestRepo(true)
-	remoteB := repository.CreateGoGitTestRepo(true)
-	defer repository.CleanupTestRepos(repo, remoteA, remoteB)
+	repo := repository.CreateGoGitTestRepo(t, false)
+	remoteA := repository.CreateGoGitTestRepo(t, true)
+	remoteB := repository.CreateGoGitTestRepo(t, true)
 
 	err := repo.AddRemote("remoteA", remoteA.GetLocalRemote())
 	require.NoError(t, err)
@@ -220,7 +217,7 @@ func TestRemove(t *testing.T) {
 }
 
 func TestCacheEviction(t *testing.T) {
-	repo := repository.CreateGoGitTestRepo(false)
+	repo := repository.CreateGoGitTestRepo(t, false)
 	repoCache, err := NewRepoCache(repo)
 	require.NoError(t, err)
 	repoCache.setCacheSize(2)
@@ -287,8 +284,7 @@ func TestLongDescription(t *testing.T) {
 
 	text := strings.Repeat("x", 65536)
 
-	repo := repository.CreateGoGitTestRepo(false)
-	defer repository.CleanupTestRepos(repo)
+	repo := repository.CreateGoGitTestRepo(t, false)
 
 	backend, err := NewRepoCache(repo)
 	require.NoError(t, err)

commands/env_testing.go

@@ -21,6 +21,8 @@ func newTestEnv(t *testing.T) *testEnv {
 
 	cwd := t.TempDir()
 
+	// r := repository.CreateGoGitTestRepo(t, false) // TODO
+
 	repo, err := repository.InitGoGitRepo(cwd, gitBugNamespace)
 	require.NoError(t, err)
 	t.Cleanup(func() {

commands/select/select_test.go

@@ -11,8 +11,7 @@ import (
 )
 
 func TestSelect(t *testing.T) {
-	repo := repository.CreateGoGitTestRepo(false)
-	defer repository.CleanupTestRepos(repo)
+	repo := repository.CreateGoGitTestRepo(t, false)
 
 	repoCache, err := cache.NewRepoCache(repo)
 	require.NoError(t, err)

entity/dag/common_test.go

@@ -103,9 +103,9 @@ func makeTestContext() (repository.ClockedRepo, identity.Interface, identity.Int
 }
 
 func makeTestContextRemote(t *testing.T) (repository.ClockedRepo, repository.ClockedRepo, repository.ClockedRepo, identity.Interface, identity.Interface, identity.Resolver, Definition) {
-	repoA := repository.CreateGoGitTestRepo(false)
-	repoB := repository.CreateGoGitTestRepo(false)
-	remote := repository.CreateGoGitTestRepo(true)
+	repoA := repository.CreateGoGitTestRepo(t, false)
+	repoB := repository.CreateGoGitTestRepo(t, false)
+	remote := repository.CreateGoGitTestRepo(t, true)
 
 	err := repoA.AddRemote("remote", remote.GetLocalRemote())
 	require.NoError(t, err)

entity/dag/entity_actions_test.go

@@ -24,8 +24,7 @@ func allEntities(t testing.TB, bugs <-chan StreamedEntity) []*Entity {
 }
 
 func TestEntityPushPull(t *testing.T) {
-	repoA, repoB, remote, id1, id2, resolver, def := makeTestContextRemote(t)
-	defer repository.CleanupTestRepos(repoA, repoB, remote)
+	repoA, repoB, _, id1, id2, resolver, def := makeTestContextRemote(t)
 
 	// A --> remote --> B
 	e := New(def)
@@ -61,8 +60,7 @@ func TestEntityPushPull(t *testing.T) {
 }
 
 func TestListLocalIds(t *testing.T) {
-	repoA, repoB, remote, id1, id2, resolver, def := makeTestContextRemote(t)
-	defer repository.CleanupTestRepos(repoA, repoB, remote)
+	repoA, repoB, _, id1, id2, resolver, def := makeTestContextRemote(t)
 
 	// A --> remote --> B
 	e := New(def)
@@ -206,8 +204,7 @@ func assertNotEqualRefs(t *testing.T, repoA, repoB repository.RepoData, prefix s
 }
 
 func TestMerge(t *testing.T) {
-	repoA, repoB, remote, id1, id2, resolver, def := makeTestContextRemote(t)
-	defer repository.CleanupTestRepos(repoA, repoB, remote)
+	repoA, repoB, _, id1, id2, resolver, def := makeTestContextRemote(t)
 
 	// SCENARIO 1
 	// if the remote Entity doesn't exist locally, it's created
@@ -387,8 +384,7 @@ func TestMerge(t *testing.T) {
 }
 
 func TestRemove(t *testing.T) {
-	repoA, repoB, remote, id1, _, resolver, def := makeTestContextRemote(t)
-	defer repository.CleanupTestRepos(repoA, repoB, remote)
+	repoA, _, _, id1, _, resolver, def := makeTestContextRemote(t)
 
 	e := New(def)
 	e.Append(newOp1(id1, "foo"))

identity/identity_actions_test.go

@@ -9,8 +9,7 @@ import (
 )
 
 func TestIdentityPushPull(t *testing.T) {
-	repoA, repoB, remote := repository.SetupGoGitReposAndRemote()
-	defer repository.CleanupTestRepos(repoA, repoB, remote)
+	repoA, repoB, _ := repository.SetupGoGitReposAndRemote(t)
 
 	identity1, err := NewIdentity(repoA, "name1", "email1")
 	require.NoError(t, err)

identity/identity_test.go

@@ -245,10 +245,9 @@ func TestJSON(t *testing.T) {
 }
 
 func TestIdentityRemove(t *testing.T) {
-	repo := repository.CreateGoGitTestRepo(false)
-	remoteA := repository.CreateGoGitTestRepo(true)
-	remoteB := repository.CreateGoGitTestRepo(true)
-	defer repository.CleanupTestRepos(repo, remoteA, remoteB)
+	repo := repository.CreateGoGitTestRepo(t, false)
+	remoteA := repository.CreateGoGitTestRepo(t, true)
+	remoteB := repository.CreateGoGitTestRepo(t, true)
 
 	err := repo.AddRemote("remoteA", remoteA.GetLocalRemote())
 	require.NoError(t, err)

repository/gogit_test.go

@@ -71,7 +71,7 @@ func TestNewGoGitRepo(t *testing.T) {
 }
 
 func TestGoGitRepo(t *testing.T) {
-	RepoTest(t, CreateGoGitTestRepo, CleanupTestRepos)
+	RepoTest(t, CreateGoGitTestRepo)
 }
 
 func TestGoGitRepo_Indexes(t *testing.T) {

repository/gogit_testing.go

@@ -1,21 +1,26 @@
 package repository
 
 import (
-	"io/ioutil"
 	"log"
+	"testing"
 
 	"github.com/99designs/keyring"
 )
 
+type TestingT interface {
+	Cleanup(func())
+	Helper()
+	TempDir() string
+}
+
 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 TestingT, bare bool) TestedRepo {
+	t.Helper()
+
+	dir := t.TempDir()
 
 	var creator func(string, string) (*GoGitRepo, error)
 
@@ -30,6 +35,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 +54,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 {

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 TestingT, bare bool) TestedRepo { return NewMockRepo() }
 
-	RepoTest(t, creator, cleaner)
+	RepoTest(t, creator)
 }

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 TestingT, 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)

tests/read_bugs_test.go

@@ -9,8 +9,7 @@ import (
 )
 
 func TestReadBugs(t *testing.T) {
-	repo := repository.CreateGoGitTestRepo(false)
-	defer repository.CleanupTestRepos(repo)
+	repo := repository.CreateGoGitTestRepo(t, false)
 
 	random_bugs.FillRepoWithSeed(repo, 15, 42)
 
@@ -23,8 +22,7 @@ func TestReadBugs(t *testing.T) {
 }
 
 func benchmarkReadBugs(bugNumber int, t *testing.B) {
-	repo := repository.CreateGoGitTestRepo(false)
-	defer repository.CleanupTestRepos(repo)
+	repo := repository.CreateGoGitTestRepo(t, false)
 
 	random_bugs.FillRepoWithSeed(repo, bugNumber, 42)
 	t.ResetTimer()