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

Steve Moyer created

refactor(809): guarantee test cleanup

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/add_test.go                     | 10 +++---
commands/env_testing.go                  | 10 -----
commands/rm_test.go                      |  2 
commands/select/select_test.go           |  3 -
commands/user_create_test.go             | 16 +++++----
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                 | 33 ++++----------------
repository/gogit_testing.go              | 42 +++++++++++++++++++------
repository/mock_repo_test.go             |  5 +-
repository/repo_testing.go               | 28 +---------------
tests/read_bugs_test.go                  |  6 +--
23 files changed, 91 insertions(+), 140 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/add_test.go 🔗

@@ -7,10 +7,10 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func newTestEnvUserAndBug(t *testing.T) (*testEnv, string, string) {
+func newTestEnvAndBug(t *testing.T) (*testEnv, string) {
 	t.Helper()
 
-	testEnv, userID := newTestEnvAndUser(t)
+	testEnv, _ := newTestEnvAndUser(t)
 	opts := addOptions{
 		title:          "this is a bug title",
 		message:        "this is a bug message",
@@ -23,10 +23,10 @@ func newTestEnvUserAndBug(t *testing.T) (*testEnv, string, string) {
 	bugID := strings.Split(testEnv.out.String(), " ")[0]
 	testEnv.out.Reset()
 
-	return testEnv, userID, bugID
+	return testEnv, bugID
 }
 
 func TestAdd(t *testing.T) {
-	_, _, user := newTestEnvUserAndBug(t)
-	require.Regexp(t, "^[0-9A-Fa-f]{7}$", user)
+	_, bugID := newTestEnvAndBug(t)
+	require.Regexp(t, "^[0-9A-Fa-f]{7}$", bugID)
 }

commands/env_testing.go 🔗

@@ -12,20 +12,13 @@ import (
 
 type testEnv struct {
 	env *Env
-	cwd string
 	out *bytes.Buffer
 }
 
 func newTestEnv(t *testing.T) *testEnv {
 	t.Helper()
 
-	cwd := t.TempDir()
-
-	repo, err := repository.InitGoGitRepo(cwd, gitBugNamespace)
-	require.NoError(t, err)
-	t.Cleanup(func() {
-		require.NoError(t, repo.Close())
-	})
+	repo := repository.CreateGoGitTestRepo(t, false)
 
 	buf := new(bytes.Buffer)
 
@@ -42,7 +35,6 @@ func newTestEnv(t *testing.T) *testEnv {
 			out:     out{Writer: buf},
 			err:     out{Writer: buf},
 		},
-		cwd: cwd,
 		out: buf,
 	}
 }

commands/rm_test.go 🔗

@@ -7,7 +7,7 @@ import (
 )
 
 func TestRm(t *testing.T) {
-	testEnv, _, bugID := newTestEnvUserAndBug(t)
+	testEnv, bugID := newTestEnvAndBug(t)
 
 	exp := "bug " + bugID + " removed\n"
 

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)

commands/user_create_test.go 🔗

@@ -1,21 +1,25 @@
 package commands
 
 import (
-	"path/filepath"
 	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 )
 
+const (
+	testUserName  = "John Doe"
+	testUserEmail = "jdoe@example.com"
+)
+
 func newTestEnvAndUser(t *testing.T) (*testEnv, string) {
 	t.Helper()
 
 	testEnv := newTestEnv(t)
 
 	opts := createUserOptions{
-		name:           "John Doe",
-		email:          "jdoe@example.com",
+		name:           testUserName,
+		email:          testUserEmail,
 		avatarURL:      "",
 		nonInteractive: true,
 	}
@@ -29,8 +33,6 @@ func newTestEnvAndUser(t *testing.T) (*testEnv, string) {
 }
 
 func TestUserCreateCommand(t *testing.T) {
-	testEnv, userID := newTestEnvAndUser(t)
-
-	require.FileExists(t, filepath.Join(testEnv.cwd, ".git", "refs", "identities", userID))
-	require.FileExists(t, filepath.Join(testEnv.cwd, ".git", "git-bug", "identity-cache"))
+	_, userID := newTestEnvAndUser(t)
+	require.Regexp(t, "[0-9a-f]{64}", userID)
 }

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 🔗

@@ -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")

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
+}

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)
 }

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)

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()