Ensure git tag tests and other create test repos in tmpdir

There are a few places where tests appear to reuse testing repos which
causes random CI failures.

This PR simply changes these tests to ensure that cloning always happens
into new temporary directories.

Fix #18444

Signed-off-by: Andrew Thornton <[email protected]>

Author: zeripath

modules/git/commit_info_test.go

@@ -21,17 +21,21 @@ const (
 	benchmarkReposDir = "benchmark/repos/"
 )
 
-func cloneRepo(url, dir, name string) (string, error) {
-	repoDir := filepath.Join(dir, name)
-	if _, err := os.Stat(repoDir); err == nil {
-		return repoDir, nil
+func cloneRepo(url, name string) (string, error) {
+	repoDir, err := os.MkdirTemp("", name)
+	if err != nil {
+		return "", err
 	}
-	return repoDir, Clone(DefaultContext, url, repoDir, CloneRepoOptions{
+	if err := Clone(DefaultContext, url, repoDir, CloneRepoOptions{
 		Mirror:  false,
 		Bare:    false,
 		Quiet:   true,
 		Timeout: 5 * time.Minute,
-	})
+	}); err != nil {
+		_ = util.RemoveAll(repoDir)
+		return "", err
+	}
+	return repoDir, nil
 }
 
 func testGetCommitsInfo(t *testing.T, repo1 *Repository) {
@@ -61,20 +65,35 @@ func testGetCommitsInfo(t *testing.T, repo1 *Repository) {
 	}
 	for _, testCase := range testCases {
 		commit, err := repo1.GetCommit(testCase.CommitID)
-		assert.NoError(t, err)
+		if err != nil {
+			assert.NoError(t, err, "Unable to get commit: %s from testcase due to error: %v", testCase.CommitID, err)
+			// no point trying to do anything else for this test.
+			continue
+		}
 		assert.NotNil(t, commit)
 		assert.NotNil(t, commit.Tree)
 		assert.NotNil(t, commit.Tree.repo)
 
 		tree, err := commit.Tree.SubTree(testCase.Path)
+		if err != nil {
+			assert.NoError(t, err, "Unable to get subtree: %s of commit: %s from testcase due to error: %v", testCase.Path, testCase.CommitID, err)
+			// no point trying to do anything else for this test.
+			continue
+		}
+
 		assert.NotNil(t, tree, "tree is nil for testCase CommitID %s in Path %s", testCase.CommitID, testCase.Path)
 		assert.NotNil(t, tree.repo, "repo is nil for testCase CommitID %s in Path %s", testCase.CommitID, testCase.Path)
 
-		assert.NoError(t, err)
 		entries, err := tree.ListEntries()
-		assert.NoError(t, err)
-		commitsInfo, treeCommit, err := entries.GetCommitsInfo(context.Background(), commit, testCase.Path, nil)
-		assert.NoError(t, err)
+		if err != nil {
+			assert.NoError(t, err, "Unable to get entries of subtree: %s in commit: %s from testcase due to error: %v", testCase.Path, testCase.CommitID, err)
+			// no point trying to do anything else for this test.
+			continue
+		}
+
+		// FIXME: Context.TODO() - if graceful has started we should use its Shutdown context otherwise use install signals in TestMain.
+		commitsInfo, treeCommit, err := entries.GetCommitsInfo(context.TODO(), commit, testCase.Path, nil)
+		assert.NoError(t, err, "Unable to get commit information for entries of subtree: %s in commit: %s from testcase due to error: %v", testCase.Path, testCase.CommitID, err)
 		if err != nil {
 			t.FailNow()
 		}
@@ -100,40 +119,52 @@ func TestEntries_GetCommitsInfo(t *testing.T) {
 
 	testGetCommitsInfo(t, bareRepo1)
 
-	clonedPath, err := cloneRepo(bareRepo1Path, testReposDir, "repo1_TestEntries_GetCommitsInfo")
-	assert.NoError(t, err)
+	clonedPath, err := cloneRepo(bareRepo1Path, "repo1_TestEntries_GetCommitsInfo")
+	if err != nil {
+		assert.NoError(t, err)
+	}
 	defer util.RemoveAll(clonedPath)
 	clonedRepo1, err := OpenRepository(clonedPath)
-	assert.NoError(t, err)
+	if err != nil {
+		assert.NoError(t, err)
+	}
 	defer clonedRepo1.Close()
 
 	testGetCommitsInfo(t, clonedRepo1)
 }
 
 func BenchmarkEntries_GetCommitsInfo(b *testing.B) {
-	benchmarks := []struct {
+	type benchmarkType struct {
 		url  string
 		name string
-	}{
+	}
+
+	benchmarks := []benchmarkType{
 		{url: "https://github.com/go-gitea/gitea.git", name: "gitea"},
 		{url: "https://github.com/ethantkoenig/manyfiles.git", name: "manyfiles"},
 		{url: "https://github.com/moby/moby.git", name: "moby"},
 		{url: "https://github.com/golang/go.git", name: "go"},
 		{url: "https://github.com/torvalds/linux.git", name: "linux"},
 	}
-	for _, benchmark := range benchmarks {
+
+	doBenchmark := func(benchmark benchmarkType) {
 		var commit *Commit
 		var entries Entries
 		var repo *Repository
-		if repoPath, err := cloneRepo(benchmark.url, benchmarkReposDir, benchmark.name); err != nil {
+		repoPath, err := cloneRepo(benchmark.url, benchmark.name)
+		if err != nil {
 			b.Fatal(err)
-		} else if repo, err = OpenRepository(repoPath); err != nil {
+		}
+		defer util.RemoveAll(repoPath)
+
+		if repo, err = OpenRepository(repoPath); err != nil {
 			b.Fatal(err)
-		} else if commit, err = repo.GetBranchCommit("master"); err != nil {
-			repo.Close()
+		}
+		defer repo.Close()
+
+		if commit, err = repo.GetBranchCommit("master"); err != nil {
 			b.Fatal(err)
 		} else if entries, err = commit.Tree.ListEntries(); err != nil {
-			repo.Close()
 			b.Fatal(err)
 		}
 		entries.Sort()
@@ -146,6 +177,9 @@ func BenchmarkEntries_GetCommitsInfo(b *testing.B) {
 				}
 			}
 		})
-		repo.Close()
+	}
+
+	for _, benchmark := range benchmarks {
+		doBenchmark(benchmark)
 	}
 }

modules/git/repo_compare_test.go

@@ -17,17 +17,33 @@ import (
 
 func TestGetFormatPatch(t *testing.T) {
 	bareRepo1Path := filepath.Join(testReposDir, "repo1_bare")
-	clonedPath, err := cloneRepo(bareRepo1Path, testReposDir, "repo1_TestGetFormatPatch")
+	clonedPath, err := cloneRepo(bareRepo1Path, "repo1_TestGetFormatPatch")
+	if err != nil {
+		assert.NoError(t, err)
+		return
+	}
 	defer util.RemoveAll(clonedPath)
-	assert.NoError(t, err)
+
 	repo, err := OpenRepository(clonedPath)
+	if err != nil {
+		assert.NoError(t, err)
+		return
+	}
 	defer repo.Close()
-	assert.NoError(t, err)
+
 	rd := &bytes.Buffer{}
 	err = repo.GetPatch("8d92fc95^", "8d92fc95", rd)
-	assert.NoError(t, err)
+	if err != nil {
+		assert.NoError(t, err)
+		return
+	}
+
 	patchb, err := io.ReadAll(rd)
-	assert.NoError(t, err)
+	if err != nil {
+		assert.NoError(t, err)
+		return
+	}
+
 	patch := string(patchb)
 	assert.Regexp(t, "^From 8d92fc95", patch)
 	assert.Contains(t, patch, "Subject: [PATCH] Add file2.txt")
@@ -37,17 +53,25 @@ func TestReadPatch(t *testing.T) {
 	// Ensure we can read the patch files
 	bareRepo1Path := filepath.Join(testReposDir, "repo1_bare")
 	repo, err := OpenRepository(bareRepo1Path)
+	if err != nil {
+		assert.NoError(t, err)
+		return
+	}
 	defer repo.Close()
-	assert.NoError(t, err)
 	// This patch doesn't exist
 	noFile, err := repo.ReadPatchCommit(0)
 	assert.Error(t, err)
+
 	// This patch is an empty one (sometimes it's a 404)
 	noCommit, err := repo.ReadPatchCommit(1)
 	assert.Error(t, err)
+
 	// This patch is legit and should return a commit
 	oldCommit, err := repo.ReadPatchCommit(2)
-	assert.NoError(t, err)
+	if err != nil {
+		assert.NoError(t, err)
+		return
+	}
 
 	assert.Empty(t, noFile)
 	assert.Empty(t, noCommit)
@@ -58,23 +82,45 @@ func TestReadPatch(t *testing.T) {
 func TestReadWritePullHead(t *testing.T) {
 	// Ensure we can write SHA1 head corresponding to PR and open them
 	bareRepo1Path := filepath.Join(testReposDir, "repo1_bare")
-	repo, err := OpenRepository(bareRepo1Path)
-	assert.NoError(t, err)
+
+	// As we are writing we should clone the repository first
+	clonedPath, err := cloneRepo(bareRepo1Path, "TestReadWritePullHead")
+	if err != nil {
+		assert.NoError(t, err)
+		return
+	}
+	defer util.RemoveAll(clonedPath)
+
+	repo, err := OpenRepository(clonedPath)
+	if err != nil {
+		assert.NoError(t, err)
+		return
+	}
 	defer repo.Close()
+
 	// Try to open non-existing Pull
 	_, err = repo.GetRefCommitID(PullPrefix + "0/head")
 	assert.Error(t, err)
+
 	// Write a fake sha1 with only 40 zeros
 	newCommit := "feaf4ba6bc635fec442f46ddd4512416ec43c2c2"
 	err = repo.SetReference(PullPrefix+"1/head", newCommit)
-	assert.NoError(t, err)
-	// Remove file after the test
-	defer func() {
-		_ = repo.RemoveReference(PullPrefix + "1/head")
-	}()
+	if err != nil {
+		assert.NoError(t, err)
+		return
+	}
+
 	// Read the file created
 	headContents, err := repo.GetRefCommitID(PullPrefix + "1/head")
-	assert.NoError(t, err)
+	if err != nil {
+		assert.NoError(t, err)
+		return
+	}
+
 	assert.Len(t, string(headContents), 40)
 	assert.True(t, string(headContents) == newCommit)
+
+	// Remove file after the test
+	err = repo.RemoveReference(PullPrefix + "1/head")
+	assert.NoError(t, err)
 }