From 71f38d266c93bca4b67dbad9c6098f2b366b9ad6 Mon Sep 17 00:00:00 2001 From: Michael Eischer <9106997+MichaelEischer@users.noreply.github.com> Date: Sun, 14 Jun 2026 14:25:00 +0200 Subject: [PATCH] ensure reliable cleanup of test repository (#21880) --- internal/checker/checker_test.go | 34 +++++++------------ internal/repository/checker_test.go | 3 +- .../repository/index/index_parallel_test.go | 3 +- internal/repository/repository_test.go | 6 ++-- internal/repository/testing.go | 6 ++-- internal/test/helpers.go | 23 +++++++------ 6 files changed, 31 insertions(+), 44 deletions(-) diff --git a/internal/checker/checker_test.go b/internal/checker/checker_test.go index c477b14ba..454063876 100644 --- a/internal/checker/checker_test.go +++ b/internal/checker/checker_test.go @@ -73,8 +73,7 @@ func assertOnlyMixedPackHints(t *testing.T, hints []error) { } func TestCheckRepo(t *testing.T) { - repo, _, cleanup := repository.TestFromFixture(t, checkerTestData) - defer cleanup() + repo, _ := repository.TestFromFixture(t, checkerTestData) chkr := checker.New(repo, false) hints, errs := chkr.LoadIndex(context.TODO(), restic.NoopTerminalCounterFactory) @@ -91,8 +90,7 @@ func TestCheckRepo(t *testing.T) { } func TestMissingPack(t *testing.T) { - repo, be, cleanup := repository.TestFromFixture(t, checkerTestData) - defer cleanup() + repo, be := repository.TestFromFixture(t, checkerTestData) packID := restic.TestParseID("657f7fb64f6a854fff6fe9279998ee09034901eded4e6db9bcee0e59745bbce6") test.OK(t, be.Remove(context.TODO(), backend.Handle{Type: restic.PackFile, Name: packID.String()})) @@ -117,8 +115,7 @@ func TestMissingPack(t *testing.T) { } func TestUnreferencedPack(t *testing.T) { - repo, be, cleanup := repository.TestFromFixture(t, checkerTestData) - defer cleanup() + repo, be := repository.TestFromFixture(t, checkerTestData) // index 3f1a only references pack 60e0 packID := "60e0438dcb978ec6860cc1f8c43da648170ee9129af8f650f876bad19f8f788e" @@ -145,8 +142,7 @@ func TestUnreferencedPack(t *testing.T) { } func TestUnreferencedBlobs(t *testing.T) { - repo, be, cleanup := repository.TestFromFixture(t, checkerTestData) - defer cleanup() + repo, be := repository.TestFromFixture(t, checkerTestData) snapshotID := restic.TestParseID("51d249d28815200d59e4be7b3f21a157b864dc343353df9d8e498220c2499b02") test.OK(t, be.Remove(context.TODO(), backend.Handle{Type: restic.SnapshotFile, Name: snapshotID.String()})) @@ -180,8 +176,7 @@ func TestUnreferencedBlobs(t *testing.T) { } func TestModifiedIndex(t *testing.T) { - repo, be, cleanup := repository.TestFromFixture(t, checkerTestData) - defer cleanup() + repo, be := repository.TestFromFixture(t, checkerTestData) done := make(chan struct{}) defer close(done) @@ -220,8 +215,7 @@ func TestModifiedIndex(t *testing.T) { var checkerDuplicateIndexTestData = filepath.Join("testdata", "duplicate-packs-in-index-test-repo.tar.gz") func TestDuplicatePacksInIndex(t *testing.T) { - repo, _, cleanup := repository.TestFromFixture(t, checkerDuplicateIndexTestData) - defer cleanup() + repo, _ := repository.TestFromFixture(t, checkerDuplicateIndexTestData) chkr := checker.New(repo, false) hints, errs := chkr.LoadIndex(context.TODO(), restic.NoopTerminalCounterFactory) @@ -420,8 +414,7 @@ func (r *loadTreesOnceRepository) LoadBlob(ctx context.Context, t restic.BlobTyp } func TestCheckerNoDuplicateTreeDecodes(t *testing.T) { - repo, _, cleanup := repository.TestFromFixture(t, checkerTestData) - defer cleanup() + repo, _ := repository.TestFromFixture(t, checkerTestData) checkRepo := &loadTreesOnceRepository{ Repository: repo, loadedTrees: restic.NewIDSet(), @@ -569,13 +562,12 @@ func TestCheckerBlobTypeConfusion(t *testing.T) { test.Assert(t, delayRepo.Triggered, "delay repository did not trigger") } -func loadBenchRepository(t *testing.B) (*checker.Checker, restic.Repository, func()) { - repo, _, cleanup := repository.TestFromFixture(t, checkerTestData) +func loadBenchRepository(t *testing.B) (*checker.Checker, restic.Repository) { + repo, _ := repository.TestFromFixture(t, checkerTestData) chkr := checker.New(repo, false) hints, errs := chkr.LoadIndex(context.TODO(), restic.NoopTerminalCounterFactory) if len(errs) > 0 { - defer cleanup() t.Fatalf("expected no errors, got %v: %v", len(errs), errs) } @@ -584,12 +576,11 @@ func loadBenchRepository(t *testing.B) (*checker.Checker, restic.Repository, fun t.Fatalf("expected mixed pack hint, got %v", err) } } - return chkr, repo, cleanup + return chkr, repo } func BenchmarkChecker(t *testing.B) { - chkr, _, cleanup := loadBenchRepository(t) - defer cleanup() + chkr, _ := loadBenchRepository(t) t.ResetTimer() @@ -601,8 +592,7 @@ func BenchmarkChecker(t *testing.B) { } func benchmarkSnapshotScaling(t *testing.B, newSnapshots int) { - chkr, repo, cleanup := loadBenchRepository(t) - defer cleanup() + chkr, repo := loadBenchRepository(t) snID := restic.TestParseID("51d249d28815200d59e4be7b3f21a157b864dc343353df9d8e498220c2499b02") sn2, err := data.LoadSnapshot(context.TODO(), repo, snID) diff --git a/internal/repository/checker_test.go b/internal/repository/checker_test.go index ee3e879a8..629e8ace7 100644 --- a/internal/repository/checker_test.go +++ b/internal/repository/checker_test.go @@ -33,8 +33,7 @@ func testWrapCheckPack(ctx context.Context, t *testing.T, repo *Repository, // TestGapInBlobs creates a gap in the blob list by omitting the first entry before passing it to checkPack func TestGapInBlobs(t *testing.T) { - repo, _, cleanup := TestFromFixture(t, checkerTestData) - defer cleanup() + repo, _ := TestFromFixture(t, checkerTestData) err := repo.LoadIndex(context.TODO(), restic.NoopTerminalCounterFactory) rtest.OK(t, err) diff --git a/internal/repository/index/index_parallel_test.go b/internal/repository/index/index_parallel_test.go index 96f1c2a6a..030fb9ea0 100644 --- a/internal/repository/index/index_parallel_test.go +++ b/internal/repository/index/index_parallel_test.go @@ -15,8 +15,7 @@ import ( var repoFixture = filepath.Join("..", "testdata", "test-repo.tar.gz") func TestRepositoryForAllIndexes(t *testing.T) { - repo, _, cleanup := repository.TestFromFixture(t, repoFixture) - defer cleanup() + repo, _ := repository.TestFromFixture(t, repoFixture) expectedIndexIDs := restic.NewIDSet() rtest.OK(t, repo.List(context.TODO(), restic.IndexFile, func(id restic.ID, size int64) error { diff --git a/internal/repository/repository_test.go b/internal/repository/repository_test.go index e9cae679b..63623b660 100644 --- a/internal/repository/repository_test.go +++ b/internal/repository/repository_test.go @@ -319,8 +319,7 @@ func benchmarkLoadUnpacked(b *testing.B, version uint) { var repoFixture = filepath.Join("testdata", "test-repo.tar.gz") func TestRepositoryLoadIndex(t *testing.T) { - repo, _, cleanup := repository.TestFromFixture(t, repoFixture) - defer cleanup() + repo, _ := repository.TestFromFixture(t, repoFixture) rtest.OK(t, repo.LoadIndex(context.TODO(), restic.NoopTerminalCounterFactory)) } @@ -373,8 +372,7 @@ func (be *damageOnceBackend) Load(ctx context.Context, h backend.Handle, length } func TestRepositoryLoadUnpackedRetryBroken(t *testing.T) { - repodir, cleanup := rtest.Env(t, repoFixture) - defer cleanup() + repodir := rtest.Env(t, repoFixture) be, err := local.Open(context.TODO(), local.Config{Path: repodir, Connections: 2}, t.Logf) rtest.OK(t, err) diff --git a/internal/repository/testing.go b/internal/repository/testing.go index 8ca2819f4..944512a54 100644 --- a/internal/repository/testing.go +++ b/internal/repository/testing.go @@ -105,11 +105,11 @@ func TestRepositoryWithVersion(t testing.TB, version uint) (*Repository, restic. return repo, &internalRepository{repo}, be } -func TestFromFixture(t testing.TB, repoFixture string) (*Repository, backend.Backend, func()) { - repodir, cleanup := test.Env(t, repoFixture) +func TestFromFixture(t testing.TB, repoFixture string) (*Repository, backend.Backend) { + repodir := test.Env(t, repoFixture) repo, be := TestOpenLocal(t, repodir) - return repo, be, cleanup + return repo, be } // TestOpenLocal opens a local repository. diff --git a/internal/test/helpers.go b/internal/test/helpers.go index 99d7c49b8..1ca5c151a 100644 --- a/internal/test/helpers.go +++ b/internal/test/helpers.go @@ -137,10 +137,18 @@ func SetupTarTestFixture(t testing.TB, outputDir, tarFile string) { // Env creates a test environment and extracts the repository fixture. // Returned is the repo path and a cleanup function. -func Env(t testing.TB, repoFixture string) (repodir string, cleanup func()) { +func Env(t testing.TB, repoFixture string) string { t.Helper() - tempdir, err := os.MkdirTemp(TestTempDir, "restic-test-env-") - OK(t, err) + + var tempdir string + if TestCleanupTempDirs { + tempdir = t.TempDir() + } else { + var err error + tempdir, err = os.MkdirTemp(TestTempDir, "restic-test-env-") + OK(t, err) + t.Logf("leaving temporary directory %v used for test", tempdir) + } fd, err := os.Open(repoFixture) if err != nil { @@ -150,14 +158,7 @@ func Env(t testing.TB, repoFixture string) (repodir string, cleanup func()) { SetupTarTestFixture(t, tempdir, repoFixture) - return filepath.Join(tempdir, "repo"), func() { - if !TestCleanupTempDirs { - t.Logf("leaving temporary directory %v used for test", tempdir) - return - } - - RemoveAll(t, tempdir) - } + return filepath.Join(tempdir, "repo") } func isFile(fi os.FileInfo) bool {