From 2b73527b433c0ab2bc67bbf5c98989fca84c3b9b Mon Sep 17 00:00:00 2001 From: ashmrtn <3891298+ashmrtn@users.noreply.github.com> Date: Thu, 20 Jul 2023 17:29:17 -0700 Subject: [PATCH] refactor(general): Cleaner error checking in retention tests (#3164) * More robust error comparisons in retention tests Update tests for retention to use `ErrorIs` checks instead of comparing error messages. * Use `require.NoError` in retention tests Minor cleanup to reduce branches in code by using `require.NoError` instead of if-blocks and `t.Fatal`. --- internal/blobtesting/object_locking_map.go | 4 ++- repo/blob/storage_extend_test.go | 22 +++++-------- repo/maintenance/blob_retain_test.go | 37 +++++++++------------- 3 files changed, 26 insertions(+), 37 deletions(-) diff --git a/internal/blobtesting/object_locking_map.go b/internal/blobtesting/object_locking_map.go index 9276d2fb9..2f507b44c 100644 --- a/internal/blobtesting/object_locking_map.go +++ b/internal/blobtesting/object_locking_map.go @@ -14,6 +14,8 @@ "github.com/kopia/kopia/repo/blob" ) +var ErrBlobLocked = errors.New("cannot alter object before retention period expires") + type entry struct { value []byte mtime time.Time @@ -60,7 +62,7 @@ func (s *objectLockingMap) getLatestForMutationLocked(id blob.ID) (*entry, error } if !e.retentionTime.IsZero() && e.retentionTime.After(s.timeNow()) { - return nil, errors.New("cannot alter object before retention period expires") + return nil, errors.WithStack(ErrBlobLocked) } return e, nil diff --git a/repo/blob/storage_extend_test.go b/repo/blob/storage_extend_test.go index 1022c18a0..3f48c2287 100644 --- a/repo/blob/storage_extend_test.go +++ b/repo/blob/storage_extend_test.go @@ -6,7 +6,9 @@ "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/kopia/kopia/internal/blobtesting" "github.com/kopia/kopia/internal/cache" "github.com/kopia/kopia/internal/faketime" "github.com/kopia/kopia/internal/repotesting" @@ -46,9 +48,7 @@ func (s *formatSpecificTestSuite) TestExtendBlobRetention(t *testing.T) { env.RepositoryWriter.Flush(ctx) blobsBefore, err := blob.ListAllBlobs(ctx, env.RepositoryWriter.BlobStorage(), "") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) if got, want := len(blobsBefore), 4; got != want { t.Fatalf("unexpected number of blobs after writing: %v", blobsBefore) @@ -59,37 +59,31 @@ func (s *formatSpecificTestSuite) TestExtendBlobRetention(t *testing.T) { // Verify that file is locked _, err = st.TouchBlob(ctx, blobsBefore[lastBlobIdx].BlobID, time.Hour) - assert.EqualErrorf(t, err, "cannot alter object before retention period expires", "Altering locked object should fail") + assert.ErrorIs(t, err, blobtesting.ErrBlobLocked, "Altering locked object should fail") ta.Advance(7 * 24 * time.Hour) // Verify that file is unlocked _, err = st.TouchBlob(ctx, blobsBefore[lastBlobIdx].BlobID, time.Hour) - if err != nil { - t.Fatalf("Altering expired object failed") - } + require.NoError(t, err, "Altering expired object failed") // Relock blob err = env.RepositoryWriter.BlobStorage().ExtendBlobRetention(ctx, blobsBefore[lastBlobIdx].BlobID, blob.ExtendOptions{ RetentionMode: blob.Governance, RetentionPeriod: 2 * time.Hour, }) - if err != nil { - t.Fatalf("Extending Retention time failed, got err: %v", err) - } + require.NoErrorf(t, err, "Extending Retention time failed, got err: %v", err) // Verify Lock period ta.Advance(1 * time.Hour) _, err = st.TouchBlob(ctx, blobsBefore[lastBlobIdx].BlobID, time.Hour) - assert.EqualErrorf(t, err, "cannot alter object before retention period expires", "Altering locked object should fail") + assert.ErrorIs(t, err, blobtesting.ErrBlobLocked, "Altering locked object should fail") ta.Advance(2 * time.Hour) _, err = st.TouchBlob(ctx, blobsBefore[lastBlobIdx].BlobID, time.Hour) - if err != nil { - t.Fatalf("Altering expired object failed") - } + require.NoError(t, err, "Altering expired object failed") } func (s *formatSpecificTestSuite) TestExtendBlobRetentionUnsupported(t *testing.T) { diff --git a/repo/maintenance/blob_retain_test.go b/repo/maintenance/blob_retain_test.go index 9dcc4653d..e72fb819a 100644 --- a/repo/maintenance/blob_retain_test.go +++ b/repo/maintenance/blob_retain_test.go @@ -6,7 +6,9 @@ "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/kopia/kopia/internal/blobtesting" "github.com/kopia/kopia/internal/cache" "github.com/kopia/kopia/internal/faketime" "github.com/kopia/kopia/internal/repotesting" @@ -45,9 +47,7 @@ func (s *formatSpecificTestSuite) TestExtendBlobRetentionTime(t *testing.T) { env.RepositoryWriter.Flush(ctx) blobsBefore, err := blob.ListAllBlobs(ctx, env.RepositoryWriter.BlobStorage(), "") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) if got, want := len(blobsBefore), 4; got != want { t.Fatalf("unexpected number of blobs after writing: %v", blobsBefore) @@ -58,17 +58,15 @@ func (s *formatSpecificTestSuite) TestExtendBlobRetentionTime(t *testing.T) { ta.Advance(7 * 24 * time.Hour) - if _, err = st.TouchBlob(ctx, blobsBefore[lastBlobIdx].BlobID, time.Hour); err != nil { - t.Fatalf("Altering expired object failed") - } + _, err = st.TouchBlob(ctx, blobsBefore[lastBlobIdx].BlobID, time.Hour) + require.NoError(t, err, "Altering expired object failed") // extend retention time of all blobs - if _, err = maintenance.ExtendBlobRetentionTime(ctx, env.RepositoryWriter, maintenance.ExtendBlobRetentionTimeOptions{}); err != nil { - t.Fatal(err) - } + _, err = maintenance.ExtendBlobRetentionTime(ctx, env.RepositoryWriter, maintenance.ExtendBlobRetentionTimeOptions{}) + require.NoError(t, err) _, err = st.TouchBlob(ctx, blobsBefore[lastBlobIdx].BlobID, time.Hour) - assert.EqualErrorf(t, err, "cannot alter object before retention period expires", "Altering locked object should fail") + assert.ErrorIs(t, err, blobtesting.ErrBlobLocked, "Altering locked object should fail") } func (s *formatSpecificTestSuite) TestExtendBlobRetentionTimeDisabled(t *testing.T) { @@ -95,9 +93,7 @@ func (s *formatSpecificTestSuite) TestExtendBlobRetentionTimeDisabled(t *testing env.RepositoryWriter.Flush(ctx) blobsBefore, err := blob.ListAllBlobs(ctx, env.RepositoryWriter.BlobStorage(), "") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) if got, want := len(blobsBefore), 4; got != want { t.Fatalf("unexpected number of blobs after writing: %v", blobsBefore) @@ -108,16 +104,13 @@ func (s *formatSpecificTestSuite) TestExtendBlobRetentionTimeDisabled(t *testing ta.Advance(7 * 24 * time.Hour) - if _, err = st.TouchBlob(ctx, blobsBefore[lastBlobIdx].BlobID, time.Hour); err != nil { - t.Fatalf("Altering expired object failed") - } + _, err = st.TouchBlob(ctx, blobsBefore[lastBlobIdx].BlobID, time.Hour) + require.NoError(t, err, "Altering expired object failed") // extend retention time of all blobs - if _, err = maintenance.ExtendBlobRetentionTime(ctx, env.RepositoryWriter, maintenance.ExtendBlobRetentionTimeOptions{}); err != nil { - t.Fatal(err) - } + _, err = maintenance.ExtendBlobRetentionTime(ctx, env.RepositoryWriter, maintenance.ExtendBlobRetentionTimeOptions{}) + require.NoError(t, err) - if _, err = st.TouchBlob(ctx, blobsBefore[lastBlobIdx].BlobID, time.Hour); err != nil { - t.Fatalf("Altering expired object failed") - } + _, err = st.TouchBlob(ctx, blobsBefore[lastBlobIdx].BlobID, time.Hour) + require.NoError(t, err, "Altering expired object failed") }