From ed5d49340538ae39946dee61b8b7d566c8873336 Mon Sep 17 00:00:00 2001 From: Eng Zer Jun Date: Fri, 8 Dec 2023 10:33:35 +0800 Subject: [PATCH] refactor: apply suggestions from code review This commit apply the following suggestions: 1. https://github.com/distribution/distribution/pull/4185#discussion_r1419694460 2. https://github.com/distribution/distribution/pull/4185#discussion_r1419697921 3. https://github.com/distribution/distribution/pull/4185#discussion_r1419699112 4. https://github.com/distribution/distribution/pull/4185#discussion_r1419702609 Signed-off-by: Eng Zer Jun --- registry/storage/driver/azure/azure_test.go | 40 +--- .../storage/driver/filesystem/driver_test.go | 35 +-- registry/storage/driver/gcs/gcs_test.go | 65 ++---- .../storage/driver/inmemory/driver_test.go | 27 +-- .../middleware/cloudfront/middleware_test.go | 6 +- registry/storage/driver/s3-aws/s3_test.go | 66 ++---- .../storage/driver/testsuites/benchsuites.go | 163 -------------- .../storage/driver/testsuites/testsuites.go | 211 ++++++++++++++++-- 8 files changed, 255 insertions(+), 358 deletions(-) delete mode 100644 registry/storage/driver/testsuites/benchsuites.go diff --git a/registry/storage/driver/azure/azure_test.go b/registry/storage/driver/azure/azure_test.go index c92f98c6..9182cd9f 100644 --- a/registry/storage/driver/azure/azure_test.go +++ b/registry/storage/driver/azure/azure_test.go @@ -2,7 +2,6 @@ package azure import ( "context" - "fmt" "math/rand" "os" "strings" @@ -10,7 +9,6 @@ import ( storagedriver "github.com/distribution/distribution/v3/registry/storage/driver" "github.com/distribution/distribution/v3/registry/storage/driver/testsuites" - "github.com/stretchr/testify/suite" ) const ( @@ -22,7 +20,7 @@ const ( ) var azureDriverConstructor func() (storagedriver.StorageDriver, error) -var skipCheck func() string +var skipCheck func(tb testing.TB) func init() { var ( @@ -69,39 +67,23 @@ func init() { } // Skip Azure storage driver tests if environment variable parameters are not provided - skipCheck = func() string { + skipCheck = func(tb testing.TB) { + tb.Helper() + if len(missing) > 0 { - return fmt.Sprintf("Must set %s environment variables to run Azure tests", strings.Join(missing, ", ")) + tb.Skipf("Must set %s environment variables to run Azure tests", strings.Join(missing, ", ")) } - return "" } } -func newDriverSuite() *testsuites.DriverSuite { - return testsuites.NewDriverSuite(azureDriverConstructor, skipCheck) -} - func TestAzureDriverSuite(t *testing.T) { - suite.Run(t, newDriverSuite()) + skipCheck(t) + testsuites.Driver(t, azureDriverConstructor) } func BenchmarkAzureDriverSuite(b *testing.B) { - benchsuite := testsuites.NewDriverBenchmarkSuite(newDriverSuite()) - benchsuite.Suite.SetupSuite() - b.Cleanup(benchsuite.Suite.TearDownSuite) - - b.Run("PutGetEmptyFiles", benchsuite.BenchmarkPutGetEmptyFiles) - b.Run("PutGet1KBFiles", benchsuite.BenchmarkPutGet1KBFiles) - b.Run("PutGet1MBFiles", benchsuite.BenchmarkPutGet1MBFiles) - b.Run("PutGet1GBFiles", benchsuite.BenchmarkPutGet1GBFiles) - b.Run("StreamEmptyFiles", benchsuite.BenchmarkStreamEmptyFiles) - b.Run("Stream1KBFiles", benchsuite.BenchmarkStream1KBFiles) - b.Run("Stream1MBFiles", benchsuite.BenchmarkStream1MBFiles) - b.Run("Stream1GBFiles", benchsuite.BenchmarkStream1GBFiles) - b.Run("List5Files", benchsuite.BenchmarkList5Files) - b.Run("List50Files", benchsuite.BenchmarkList50Files) - b.Run("Delete5Files", benchsuite.BenchmarkDelete5Files) - b.Run("Delete50Files", benchsuite.BenchmarkDelete50Files) + skipCheck(b) + testsuites.BenchDriver(b, azureDriverConstructor) } var letterRunes = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ") @@ -115,9 +97,7 @@ func randStringRunes(n int) string { } func TestCommitAfterMove(t *testing.T) { - if skipCheck() != "" { - t.Skip(skipCheck()) - } + skipCheck(t) driver, err := azureDriverConstructor() if err != nil { diff --git a/registry/storage/driver/filesystem/driver_test.go b/registry/storage/driver/filesystem/driver_test.go index 7a13cd3c..e236aa47 100644 --- a/registry/storage/driver/filesystem/driver_test.go +++ b/registry/storage/driver/filesystem/driver_test.go @@ -6,45 +6,24 @@ import ( storagedriver "github.com/distribution/distribution/v3/registry/storage/driver" "github.com/distribution/distribution/v3/registry/storage/driver/testsuites" - "github.com/stretchr/testify/suite" ) -func newDriverSuite(tb testing.TB) *testsuites.DriverSuite { +func newDriverConstructor(tb testing.TB) testsuites.DriverConstructor { root := tb.TempDir() - drvr, err := FromParameters(map[string]interface{}{ - "rootdirectory": root, - }) - if err != nil { - panic(err) + return func() (storagedriver.StorageDriver, error) { + return FromParameters(map[string]interface{}{ + "rootdirectory": root, + }) } - - return testsuites.NewDriverSuite(func() (storagedriver.StorageDriver, error) { - return drvr, nil - }, testsuites.NeverSkip) } func TestFilesystemDriverSuite(t *testing.T) { - suite.Run(t, newDriverSuite(t)) + testsuites.Driver(t, newDriverConstructor(t)) } func BenchmarkFilesystemDriverSuite(b *testing.B) { - benchsuite := testsuites.NewDriverBenchmarkSuite(newDriverSuite(b)) - benchsuite.Suite.SetupSuite() - b.Cleanup(benchsuite.Suite.TearDownSuite) - - b.Run("PutGetEmptyFiles", benchsuite.BenchmarkPutGetEmptyFiles) - b.Run("PutGet1KBFiles", benchsuite.BenchmarkPutGet1KBFiles) - b.Run("PutGet1MBFiles", benchsuite.BenchmarkPutGet1MBFiles) - b.Run("PutGet1GBFiles", benchsuite.BenchmarkPutGet1GBFiles) - b.Run("StreamEmptyFiles", benchsuite.BenchmarkStreamEmptyFiles) - b.Run("Stream1KBFiles", benchsuite.BenchmarkStream1KBFiles) - b.Run("Stream1MBFiles", benchsuite.BenchmarkStream1MBFiles) - b.Run("Stream1GBFiles", benchsuite.BenchmarkStream1GBFiles) - b.Run("List5Files", benchsuite.BenchmarkList5Files) - b.Run("List50Files", benchsuite.BenchmarkList50Files) - b.Run("Delete5Files", benchsuite.BenchmarkDelete5Files) - b.Run("Delete50Files", benchsuite.BenchmarkDelete50Files) + testsuites.BenchDriver(b, newDriverConstructor(b)) } func TestFromParametersImpl(t *testing.T) { diff --git a/registry/storage/driver/gcs/gcs_test.go b/registry/storage/driver/gcs/gcs_test.go index 55d333d7..cdbb0f33 100644 --- a/registry/storage/driver/gcs/gcs_test.go +++ b/registry/storage/driver/gcs/gcs_test.go @@ -10,7 +10,6 @@ import ( "github.com/distribution/distribution/v3/internal/dcontext" storagedriver "github.com/distribution/distribution/v3/registry/storage/driver" "github.com/distribution/distribution/v3/registry/storage/driver/testsuites" - "github.com/stretchr/testify/suite" "golang.org/x/oauth2" "golang.org/x/oauth2/google" "google.golang.org/api/googleapi" @@ -19,7 +18,7 @@ import ( var ( gcsDriverConstructor func(rootDirectory string) (storagedriver.StorageDriver, error) - skipGCS func() string + skipCheck func(tb testing.TB) ) func init() { @@ -27,15 +26,12 @@ func init() { credentials := os.Getenv("GOOGLE_APPLICATION_CREDENTIALS") // Skip GCS storage driver tests if environment variable parameters are not provided - skipGCS = func() string { - if bucket == "" || credentials == "" { - return "The following environment variables must be set to enable these tests: REGISTRY_STORAGE_GCS_BUCKET, GOOGLE_APPLICATION_CREDENTIALS" - } - return "" - } + skipCheck = func(tb testing.TB) { + tb.Helper() - if skipGCS() != "" { - return + if bucket == "" || credentials == "" { + tb.Skip("The following environment variables must be set to enable these tests: REGISTRY_STORAGE_GCS_BUCKET, GOOGLE_APPLICATION_CREDENTIALS") + } } jsonKey, err := os.ReadFile(credentials) @@ -87,42 +83,27 @@ func init() { } } -func newDriverSuite(tb testing.TB) *testsuites.DriverSuite { +func newDriverConstructor(tb testing.TB) testsuites.DriverConstructor { root := tb.TempDir() - return testsuites.NewDriverSuite(func() (storagedriver.StorageDriver, error) { + return func() (storagedriver.StorageDriver, error) { return gcsDriverConstructor(root) - }, skipGCS) + } } -func TestGcsDriverSuite(t *testing.T) { - suite.Run(t, newDriverSuite(t)) +func TestGCSDriverSuite(t *testing.T) { + skipCheck(t) + testsuites.Driver(t, newDriverConstructor(t)) } func BenchmarkGcsDriverSuite(b *testing.B) { - benchsuite := testsuites.NewDriverBenchmarkSuite(newDriverSuite(b)) - benchsuite.Suite.SetupSuite() - b.Cleanup(benchsuite.Suite.TearDownSuite) - - b.Run("PutGetEmptyFiles", benchsuite.BenchmarkPutGetEmptyFiles) - b.Run("PutGet1KBFiles", benchsuite.BenchmarkPutGet1KBFiles) - b.Run("PutGet1MBFiles", benchsuite.BenchmarkPutGet1MBFiles) - b.Run("PutGet1GBFiles", benchsuite.BenchmarkPutGet1GBFiles) - b.Run("StreamEmptyFiles", benchsuite.BenchmarkStreamEmptyFiles) - b.Run("Stream1KBFiles", benchsuite.BenchmarkStream1KBFiles) - b.Run("Stream1MBFiles", benchsuite.BenchmarkStream1MBFiles) - b.Run("Stream1GBFiles", benchsuite.BenchmarkStream1GBFiles) - b.Run("List5Files", benchsuite.BenchmarkList5Files) - b.Run("List50Files", benchsuite.BenchmarkList50Files) - b.Run("Delete5Files", benchsuite.BenchmarkDelete5Files) - b.Run("Delete50Files", benchsuite.BenchmarkDelete50Files) + skipCheck(b) + testsuites.BenchDriver(b, newDriverConstructor(b)) } // Test Committing a FileWriter without having called Write func TestCommitEmpty(t *testing.T) { - if skipGCS() != "" { - t.Skip(skipGCS()) - } + skipCheck(t) validRoot := t.TempDir() @@ -163,9 +144,7 @@ func TestCommitEmpty(t *testing.T) { // Test Committing a FileWriter after having written exactly // defaultChunksize bytes. func TestCommit(t *testing.T) { - if skipGCS() != "" { - t.Skip(skipGCS()) - } + skipCheck(t) validRoot := t.TempDir() @@ -209,9 +188,7 @@ func TestCommit(t *testing.T) { } func TestRetry(t *testing.T) { - if skipGCS() != "" { - t.Skip(skipGCS()) - } + skipCheck(t) assertError := func(expected string, observed error) { observedMsg := "" @@ -246,9 +223,7 @@ func TestRetry(t *testing.T) { } func TestEmptyRootList(t *testing.T) { - if skipGCS() != "" { - t.Skip(skipGCS()) - } + skipCheck(t) validRoot := t.TempDir() @@ -303,9 +278,7 @@ func TestEmptyRootList(t *testing.T) { // TestMoveDirectory checks that moving a directory returns an error. func TestMoveDirectory(t *testing.T) { - if skipGCS() != "" { - t.Skip(skipGCS()) - } + skipCheck(t) validRoot := t.TempDir() diff --git a/registry/storage/driver/inmemory/driver_test.go b/registry/storage/driver/inmemory/driver_test.go index 17e1d662..b1ede2c3 100644 --- a/registry/storage/driver/inmemory/driver_test.go +++ b/registry/storage/driver/inmemory/driver_test.go @@ -5,35 +5,16 @@ import ( storagedriver "github.com/distribution/distribution/v3/registry/storage/driver" "github.com/distribution/distribution/v3/registry/storage/driver/testsuites" - "github.com/stretchr/testify/suite" ) -func newDriverSuite() *testsuites.DriverSuite { - inmemoryDriverConstructor := func() (storagedriver.StorageDriver, error) { - return New(), nil - } - return testsuites.NewDriverSuite(inmemoryDriverConstructor, testsuites.NeverSkip) +func newDriverConstructor() (storagedriver.StorageDriver, error) { + return New(), nil } func TestInMemoryDriverSuite(t *testing.T) { - suite.Run(t, newDriverSuite()) + testsuites.Driver(t, newDriverConstructor) } func BenchmarkInMemoryDriverSuite(b *testing.B) { - benchsuite := testsuites.NewDriverBenchmarkSuite(newDriverSuite()) - benchsuite.Suite.SetupSuite() - b.Cleanup(benchsuite.Suite.TearDownSuite) - - b.Run("PutGetEmptyFiles", benchsuite.BenchmarkPutGetEmptyFiles) - b.Run("PutGet1KBFiles", benchsuite.BenchmarkPutGet1KBFiles) - b.Run("PutGet1MBFiles", benchsuite.BenchmarkPutGet1MBFiles) - b.Run("PutGet1GBFiles", benchsuite.BenchmarkPutGet1GBFiles) - b.Run("StreamEmptyFiles", benchsuite.BenchmarkStreamEmptyFiles) - b.Run("Stream1KBFiles", benchsuite.BenchmarkStream1KBFiles) - b.Run("Stream1MBFiles", benchsuite.BenchmarkStream1MBFiles) - b.Run("Stream1GBFiles", benchsuite.BenchmarkStream1GBFiles) - b.Run("List5Files", benchsuite.BenchmarkList5Files) - b.Run("List50Files", benchsuite.BenchmarkList50Files) - b.Run("Delete5Files", benchsuite.BenchmarkDelete5Files) - b.Run("Delete50Files", benchsuite.BenchmarkDelete50Files) + testsuites.BenchDriver(b, newDriverConstructor) } diff --git a/registry/storage/driver/middleware/cloudfront/middleware_test.go b/registry/storage/driver/middleware/cloudfront/middleware_test.go index 65299b17..9d1e11a2 100644 --- a/registry/storage/driver/middleware/cloudfront/middleware_test.go +++ b/registry/storage/driver/middleware/cloudfront/middleware_test.go @@ -4,14 +4,14 @@ import ( "context" "os" "testing" + + "github.com/stretchr/testify/require" ) func TestNoConfig(t *testing.T) { options := make(map[string]interface{}) _, err := newCloudFrontStorageMiddleware(context.Background(), nil, options) - if err == nil || err.Error() != "no baseurl provided" { - t.Fatalf(`expected error "no baseurl provided", got: %v`, err) - } + require.ErrorContains(t, err, "no baseurl provided") } func TestCloudFrontStorageMiddlewareGenerateKey(t *testing.T) { diff --git a/registry/storage/driver/s3-aws/s3_test.go b/registry/storage/driver/s3-aws/s3_test.go index 4eda118e..ea11f872 100644 --- a/registry/storage/driver/s3-aws/s3_test.go +++ b/registry/storage/driver/s3-aws/s3_test.go @@ -16,8 +16,6 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/s3" - "github.com/stretchr/testify/suite" - "github.com/distribution/distribution/v3/internal/dcontext" storagedriver "github.com/distribution/distribution/v3/registry/storage/driver" "github.com/distribution/distribution/v3/registry/storage/driver/testsuites" @@ -25,7 +23,7 @@ import ( var ( s3DriverConstructor func(rootDirectory, storageClass string) (*Driver, error) - skipS3 func() string + skipCheck func(tb testing.TB) ) func init() { @@ -142,49 +140,35 @@ func init() { } // Skip S3 storage driver tests if environment variable parameters are not provided - skipS3 = func() string { + skipCheck = func(tb testing.TB) { + tb.Helper() + if accessKey == "" || secretKey == "" || region == "" || bucket == "" || encrypt == "" { - return "Must set AWS_ACCESS_KEY, AWS_SECRET_KEY, AWS_REGION, S3_BUCKET, and S3_ENCRYPT to run S3 tests" + tb.Skip("Must set AWS_ACCESS_KEY, AWS_SECRET_KEY, AWS_REGION, S3_BUCKET, and S3_ENCRYPT to run S3 tests") } - return "" } } -func newDriverSuite(tb testing.TB) *testsuites.DriverSuite { +func newDriverConstructor(tb testing.TB) testsuites.DriverConstructor { root := tb.TempDir() - return testsuites.NewDriverSuite(func() (storagedriver.StorageDriver, error) { + return func() (storagedriver.StorageDriver, error) { return s3DriverConstructor(root, s3.StorageClassStandard) - }, skipS3) + } } func TestS3DriverSuite(t *testing.T) { - suite.Run(t, newDriverSuite(t)) + skipCheck(t) + testsuites.Driver(t, newDriverConstructor(t)) } func BenchmarkS3DriverSuite(b *testing.B) { - benchsuite := testsuites.NewDriverBenchmarkSuite(newDriverSuite(b)) - benchsuite.Suite.SetupSuite() - b.Cleanup(benchsuite.Suite.TearDownSuite) - - b.Run("PutGetEmptyFiles", benchsuite.BenchmarkPutGetEmptyFiles) - b.Run("PutGet1KBFiles", benchsuite.BenchmarkPutGet1KBFiles) - b.Run("PutGet1MBFiles", benchsuite.BenchmarkPutGet1MBFiles) - b.Run("PutGet1GBFiles", benchsuite.BenchmarkPutGet1GBFiles) - b.Run("StreamEmptyFiles", benchsuite.BenchmarkStreamEmptyFiles) - b.Run("Stream1KBFiles", benchsuite.BenchmarkStream1KBFiles) - b.Run("Stream1MBFiles", benchsuite.BenchmarkStream1MBFiles) - b.Run("Stream1GBFiles", benchsuite.BenchmarkStream1GBFiles) - b.Run("List5Files", benchsuite.BenchmarkList5Files) - b.Run("List50Files", benchsuite.BenchmarkList50Files) - b.Run("Delete5Files", benchsuite.BenchmarkDelete5Files) - b.Run("Delete50Files", benchsuite.BenchmarkDelete50Files) + skipCheck(b) + testsuites.BenchDriver(b, newDriverConstructor(b)) } func TestEmptyRootList(t *testing.T) { - if skipS3() != "" { - t.Skip(skipS3()) - } + skipCheck(t) validRoot := t.TempDir() rootedDriver, err := s3DriverConstructor(validRoot, s3.StorageClassStandard) @@ -228,9 +212,7 @@ func TestEmptyRootList(t *testing.T) { } func TestStorageClass(t *testing.T) { - if skipS3() != "" { - t.Skip(skipS3()) - } + skipCheck(t) rootDir := t.TempDir() contents := []byte("contents") @@ -292,9 +274,7 @@ func TestStorageClass(t *testing.T) { } func TestDelete(t *testing.T) { - if skipS3() != "" { - t.Skip(skipS3()) - } + skipCheck(t) rootDir := t.TempDir() @@ -493,9 +473,7 @@ func TestDelete(t *testing.T) { } func TestWalk(t *testing.T) { - if skipS3() != "" { - t.Skip(skipS3()) - } + skipCheck(t) rootDir := t.TempDir() @@ -734,9 +712,7 @@ func TestWalk(t *testing.T) { } func TestOverThousandBlobs(t *testing.T) { - if skipS3() != "" { - t.Skip(skipS3()) - } + skipCheck(t) rootDir := t.TempDir() standardDriver, err := s3DriverConstructor(rootDir, s3.StorageClassStandard) @@ -762,9 +738,7 @@ func TestOverThousandBlobs(t *testing.T) { } func TestMoveWithMultipartCopy(t *testing.T) { - if skipS3() != "" { - t.Skip(skipS3()) - } + skipCheck(t) rootDir := t.TempDir() d, err := s3DriverConstructor(rootDir, s3.StorageClassStandard) @@ -815,9 +789,7 @@ func TestMoveWithMultipartCopy(t *testing.T) { } func TestListObjectsV2(t *testing.T) { - if skipS3() != "" { - t.Skip(skipS3()) - } + skipCheck(t) rootDir := t.TempDir() d, err := s3DriverConstructor(rootDir, s3.StorageClassStandard) diff --git a/registry/storage/driver/testsuites/benchsuites.go b/registry/storage/driver/testsuites/benchsuites.go deleted file mode 100644 index eed55b90..00000000 --- a/registry/storage/driver/testsuites/benchsuites.go +++ /dev/null @@ -1,163 +0,0 @@ -package testsuites - -import ( - "bytes" - "context" - "io" - "path" - "testing" -) - -type DriverBenchmarkSuite struct { - Suite *DriverSuite -} - -func NewDriverBenchmarkSuite(ds *DriverSuite) *DriverBenchmarkSuite { - return &DriverBenchmarkSuite{Suite: ds} -} - -// BenchmarkPutGetEmptyFiles benchmarks PutContent/GetContent for 0B files -func (s *DriverBenchmarkSuite) BenchmarkPutGetEmptyFiles(b *testing.B) { - s.benchmarkPutGetFiles(b, 0) -} - -// BenchmarkPutGet1KBFiles benchmarks PutContent/GetContent for 1KB files -func (s *DriverBenchmarkSuite) BenchmarkPutGet1KBFiles(b *testing.B) { - s.benchmarkPutGetFiles(b, 1024) -} - -// BenchmarkPutGet1MBFiles benchmarks PutContent/GetContent for 1MB files -func (s *DriverBenchmarkSuite) BenchmarkPutGet1MBFiles(b *testing.B) { - s.benchmarkPutGetFiles(b, 1024*1024) -} - -// BenchmarkPutGet1GBFiles benchmarks PutContent/GetContent for 1GB files -func (s *DriverBenchmarkSuite) BenchmarkPutGet1GBFiles(b *testing.B) { - s.benchmarkPutGetFiles(b, 1024*1024*1024) -} - -func (s *DriverBenchmarkSuite) benchmarkPutGetFiles(b *testing.B, size int64) { - b.SetBytes(size) - parentDir := randomPath(8) - defer func() { - b.StopTimer() - // nolint:errcheck - s.Suite.StorageDriver.Delete(s.Suite.ctx, firstPart(parentDir)) - }() - - for i := 0; i < b.N; i++ { - filename := path.Join(parentDir, randomPath(32)) - err := s.Suite.StorageDriver.PutContent(s.Suite.ctx, filename, randomContents(size)) - s.Suite.Require().NoError(err) - - _, err = s.Suite.StorageDriver.GetContent(s.Suite.ctx, filename) - s.Suite.Require().NoError(err) - } -} - -// BenchmarkStreamEmptyFiles benchmarks Writer/Reader for 0B files -func (s *DriverBenchmarkSuite) BenchmarkStreamEmptyFiles(b *testing.B) { - s.benchmarkStreamFiles(b, 0) -} - -// BenchmarkStream1KBFiles benchmarks Writer/Reader for 1KB files -func (s *DriverBenchmarkSuite) BenchmarkStream1KBFiles(b *testing.B) { - s.benchmarkStreamFiles(b, 1024) -} - -// BenchmarkStream1MBFiles benchmarks Writer/Reader for 1MB files -func (s *DriverBenchmarkSuite) BenchmarkStream1MBFiles(b *testing.B) { - s.benchmarkStreamFiles(b, 1024*1024) -} - -// BenchmarkStream1GBFiles benchmarks Writer/Reader for 1GB files -func (s *DriverBenchmarkSuite) BenchmarkStream1GBFiles(b *testing.B) { - s.benchmarkStreamFiles(b, 1024*1024*1024) -} - -func (s *DriverBenchmarkSuite) benchmarkStreamFiles(b *testing.B, size int64) { - b.SetBytes(size) - parentDir := randomPath(8) - defer func() { - b.StopTimer() - // nolint:errcheck - s.Suite.StorageDriver.Delete(s.Suite.ctx, firstPart(parentDir)) - }() - - for i := 0; i < b.N; i++ { - filename := path.Join(parentDir, randomPath(32)) - writer, err := s.Suite.StorageDriver.Writer(s.Suite.ctx, filename, false) - s.Suite.Require().NoError(err) - written, err := io.Copy(writer, bytes.NewReader(randomContents(size))) - s.Suite.Require().NoError(err) - s.Suite.Require().Equal(size, written) - - err = writer.Commit(context.Background()) - s.Suite.Require().NoError(err) - err = writer.Close() - s.Suite.Require().NoError(err) - - rc, err := s.Suite.StorageDriver.Reader(s.Suite.ctx, filename, 0) - s.Suite.Require().NoError(err) - rc.Close() - } -} - -// BenchmarkList5Files benchmarks List for 5 small files -func (s *DriverBenchmarkSuite) BenchmarkList5Files(b *testing.B) { - s.benchmarkListFiles(b, 5) -} - -// BenchmarkList50Files benchmarks List for 50 small files -func (s *DriverBenchmarkSuite) BenchmarkList50Files(b *testing.B) { - s.benchmarkListFiles(b, 50) -} - -func (s *DriverBenchmarkSuite) benchmarkListFiles(b *testing.B, numFiles int64) { - parentDir := randomPath(8) - defer func() { - b.StopTimer() - // nolint:errcheck - s.Suite.StorageDriver.Delete(s.Suite.ctx, firstPart(parentDir)) - }() - - for i := int64(0); i < numFiles; i++ { - err := s.Suite.StorageDriver.PutContent(s.Suite.ctx, path.Join(parentDir, randomPath(32)), nil) - s.Suite.Require().NoError(err) - } - - b.ResetTimer() - for i := 0; i < b.N; i++ { - files, err := s.Suite.StorageDriver.List(s.Suite.ctx, parentDir) - s.Suite.Require().NoError(err) - s.Suite.Require().Equal(numFiles, int64(len(files))) - } -} - -// BenchmarkDelete5Files benchmarks Delete for 5 small files -func (s *DriverBenchmarkSuite) BenchmarkDelete5Files(b *testing.B) { - s.benchmarkDeleteFiles(b, 5) -} - -// BenchmarkDelete50Files benchmarks Delete for 50 small files -func (s *DriverBenchmarkSuite) BenchmarkDelete50Files(b *testing.B) { - s.benchmarkDeleteFiles(b, 50) -} - -func (s *DriverBenchmarkSuite) benchmarkDeleteFiles(b *testing.B, numFiles int64) { - for i := 0; i < b.N; i++ { - parentDir := randomPath(8) - defer s.Suite.deletePath(firstPart(parentDir)) - - b.StopTimer() - for j := int64(0); j < numFiles; j++ { - err := s.Suite.StorageDriver.PutContent(s.Suite.ctx, path.Join(parentDir, randomPath(32)), nil) - s.Suite.Require().NoError(err) - } - b.StartTimer() - - // This is the operation we're benchmarking - err := s.Suite.StorageDriver.Delete(s.Suite.ctx, firstPart(parentDir)) - s.Suite.Require().NoError(err) - } -} diff --git a/registry/storage/driver/testsuites/testsuites.go b/registry/storage/driver/testsuites/testsuites.go index 8eef05ac..cd98832a 100644 --- a/registry/storage/driver/testsuites/testsuites.go +++ b/registry/storage/driver/testsuites/testsuites.go @@ -28,14 +28,6 @@ func init() { _, _ = crand.Read(randomBytes) // always returns len(randomBytes) and nil error } -// SkipCheck is a function used to determine if a test suite should be skipped. -// If a SkipCheck returns a non-empty skip reason, the suite is skipped with -// the given reason. -type SkipCheck func() (reason string) - -// NeverSkip is a default SkipCheck which never skips the suite. -var NeverSkip SkipCheck = func() string { return "" } - // DriverConstructor is a function which returns a new // storagedriver.StorageDriver. type DriverConstructor func() (storagedriver.StorageDriver, error) @@ -45,30 +37,25 @@ type DriverConstructor func() (storagedriver.StorageDriver, error) type DriverTeardown func() error // DriverSuite is a [suite.Suite] test suite designed to test a -// storagedriver.StorageDriver. The intended way to create a DriverSuite is -// with [NewDriverSuite]. +// storagedriver.StorageDriver. type DriverSuite struct { suite.Suite Constructor DriverConstructor Teardown DriverTeardown - SkipCheck storagedriver.StorageDriver ctx context.Context } -func NewDriverSuite(driverConstructor DriverConstructor, skipCheck SkipCheck) *DriverSuite { - return &DriverSuite{ +// Driver runs [DriverSuite] for the given [DriverConstructor]. +func Driver(t *testing.T, driverConstructor DriverConstructor) { + suite.Run(t, &DriverSuite{ Constructor: driverConstructor, - SkipCheck: skipCheck, ctx: context.Background(), - } + }) } // SetupSuite implements [suite.SetupAllSuite] interface. func (suite *DriverSuite) SetupSuite() { - if reason := suite.SkipCheck(); reason != "" { - suite.T().Skip(reason) - } d, err := suite.Constructor() suite.Require().NoError(err) suite.StorageDriver = d @@ -479,6 +466,11 @@ func (suite *DriverSuite) TestReadNonexistentStream() { // TestWriteZeroByteStreamThenAppend tests if zero byte file handling works for append to a Stream func (suite *DriverSuite) TestWriteZeroByteStreamThenAppend() { + if suite.StorageDriver.Name() == "s3aws" { + // See https://github.com/distribution/distribution/pull/4185#discussion_r1419721968 + suite.T().Skip("S3 multipart uploads require at least 1 chunk (>0B)") + } + filename := randomPath(32) defer suite.deletePath(firstPart(filename)) chunkSize := int64(32) @@ -537,6 +529,11 @@ func (suite *DriverSuite) TestWriteZeroByteStreamThenAppend() { // TestWriteZeroByteContentThenAppend tests if zero byte file handling works for append to PutContent func (suite *DriverSuite) TestWriteZeroByteContentThenAppend() { + if suite.StorageDriver.Name() == "s3aws" { + // See https://github.com/distribution/distribution/pull/4185#discussion_r1419721968 + suite.T().Skip("S3 multipart uploads require at least 1 chunk (>0B)") + } + filename := randomPath(32) defer suite.deletePath(firstPart(filename)) chunkSize := int64(32) @@ -1087,6 +1084,184 @@ func (suite *DriverSuite) TestConcurrentFileStreams() { // c.Assert(misswrites, check.Not(check.Equals), 1024) // } +type DriverBenchmarkSuite struct { + Suite *DriverSuite +} + +func BenchDriver(b *testing.B, driverConstructor DriverConstructor) { + benchsuite := &DriverBenchmarkSuite{ + Suite: &DriverSuite{ + Constructor: driverConstructor, + ctx: context.Background(), + }, + } + benchsuite.Suite.SetupSuite() + b.Cleanup(benchsuite.Suite.TearDownSuite) + + b.Run("PutGetEmptyFiles", benchsuite.BenchmarkPutGetEmptyFiles) + b.Run("PutGet1KBFiles", benchsuite.BenchmarkPutGet1KBFiles) + b.Run("PutGet1MBFiles", benchsuite.BenchmarkPutGet1MBFiles) + b.Run("PutGet1GBFiles", benchsuite.BenchmarkPutGet1GBFiles) + b.Run("StreamEmptyFiles", benchsuite.BenchmarkStreamEmptyFiles) + b.Run("Stream1KBFiles", benchsuite.BenchmarkStream1KBFiles) + b.Run("Stream1MBFiles", benchsuite.BenchmarkStream1MBFiles) + b.Run("Stream1GBFiles", benchsuite.BenchmarkStream1GBFiles) + b.Run("List5Files", benchsuite.BenchmarkList5Files) + b.Run("List50Files", benchsuite.BenchmarkList50Files) + b.Run("Delete5Files", benchsuite.BenchmarkDelete5Files) + b.Run("Delete50Files", benchsuite.BenchmarkDelete50Files) +} + +func NewDriverBenchmarkSuite(ds *DriverSuite) *DriverBenchmarkSuite { + return &DriverBenchmarkSuite{Suite: ds} +} + +// BenchmarkPutGetEmptyFiles benchmarks PutContent/GetContent for 0B files +func (s *DriverBenchmarkSuite) BenchmarkPutGetEmptyFiles(b *testing.B) { + s.benchmarkPutGetFiles(b, 0) +} + +// BenchmarkPutGet1KBFiles benchmarks PutContent/GetContent for 1KB files +func (s *DriverBenchmarkSuite) BenchmarkPutGet1KBFiles(b *testing.B) { + s.benchmarkPutGetFiles(b, 1024) +} + +// BenchmarkPutGet1MBFiles benchmarks PutContent/GetContent for 1MB files +func (s *DriverBenchmarkSuite) BenchmarkPutGet1MBFiles(b *testing.B) { + s.benchmarkPutGetFiles(b, 1024*1024) +} + +// BenchmarkPutGet1GBFiles benchmarks PutContent/GetContent for 1GB files +func (s *DriverBenchmarkSuite) BenchmarkPutGet1GBFiles(b *testing.B) { + s.benchmarkPutGetFiles(b, 1024*1024*1024) +} + +func (s *DriverBenchmarkSuite) benchmarkPutGetFiles(b *testing.B, size int64) { + b.SetBytes(size) + parentDir := randomPath(8) + defer func() { + b.StopTimer() + // nolint:errcheck + s.Suite.StorageDriver.Delete(s.Suite.ctx, firstPart(parentDir)) + }() + + for i := 0; i < b.N; i++ { + filename := path.Join(parentDir, randomPath(32)) + err := s.Suite.StorageDriver.PutContent(s.Suite.ctx, filename, randomContents(size)) + s.Suite.Require().NoError(err) + + _, err = s.Suite.StorageDriver.GetContent(s.Suite.ctx, filename) + s.Suite.Require().NoError(err) + } +} + +// BenchmarkStreamEmptyFiles benchmarks Writer/Reader for 0B files +func (s *DriverBenchmarkSuite) BenchmarkStreamEmptyFiles(b *testing.B) { + s.benchmarkStreamFiles(b, 0) +} + +// BenchmarkStream1KBFiles benchmarks Writer/Reader for 1KB files +func (s *DriverBenchmarkSuite) BenchmarkStream1KBFiles(b *testing.B) { + s.benchmarkStreamFiles(b, 1024) +} + +// BenchmarkStream1MBFiles benchmarks Writer/Reader for 1MB files +func (s *DriverBenchmarkSuite) BenchmarkStream1MBFiles(b *testing.B) { + s.benchmarkStreamFiles(b, 1024*1024) +} + +// BenchmarkStream1GBFiles benchmarks Writer/Reader for 1GB files +func (s *DriverBenchmarkSuite) BenchmarkStream1GBFiles(b *testing.B) { + s.benchmarkStreamFiles(b, 1024*1024*1024) +} + +func (s *DriverBenchmarkSuite) benchmarkStreamFiles(b *testing.B, size int64) { + b.SetBytes(size) + parentDir := randomPath(8) + defer func() { + b.StopTimer() + // nolint:errcheck + s.Suite.StorageDriver.Delete(s.Suite.ctx, firstPart(parentDir)) + }() + + for i := 0; i < b.N; i++ { + filename := path.Join(parentDir, randomPath(32)) + writer, err := s.Suite.StorageDriver.Writer(s.Suite.ctx, filename, false) + s.Suite.Require().NoError(err) + written, err := io.Copy(writer, bytes.NewReader(randomContents(size))) + s.Suite.Require().NoError(err) + s.Suite.Require().Equal(size, written) + + err = writer.Commit(context.Background()) + s.Suite.Require().NoError(err) + err = writer.Close() + s.Suite.Require().NoError(err) + + rc, err := s.Suite.StorageDriver.Reader(s.Suite.ctx, filename, 0) + s.Suite.Require().NoError(err) + rc.Close() + } +} + +// BenchmarkList5Files benchmarks List for 5 small files +func (s *DriverBenchmarkSuite) BenchmarkList5Files(b *testing.B) { + s.benchmarkListFiles(b, 5) +} + +// BenchmarkList50Files benchmarks List for 50 small files +func (s *DriverBenchmarkSuite) BenchmarkList50Files(b *testing.B) { + s.benchmarkListFiles(b, 50) +} + +func (s *DriverBenchmarkSuite) benchmarkListFiles(b *testing.B, numFiles int64) { + parentDir := randomPath(8) + defer func() { + b.StopTimer() + // nolint:errcheck + s.Suite.StorageDriver.Delete(s.Suite.ctx, firstPart(parentDir)) + }() + + for i := int64(0); i < numFiles; i++ { + err := s.Suite.StorageDriver.PutContent(s.Suite.ctx, path.Join(parentDir, randomPath(32)), nil) + s.Suite.Require().NoError(err) + } + + b.ResetTimer() + for i := 0; i < b.N; i++ { + files, err := s.Suite.StorageDriver.List(s.Suite.ctx, parentDir) + s.Suite.Require().NoError(err) + s.Suite.Require().Equal(numFiles, int64(len(files))) + } +} + +// BenchmarkDelete5Files benchmarks Delete for 5 small files +func (s *DriverBenchmarkSuite) BenchmarkDelete5Files(b *testing.B) { + s.benchmarkDeleteFiles(b, 5) +} + +// BenchmarkDelete50Files benchmarks Delete for 50 small files +func (s *DriverBenchmarkSuite) BenchmarkDelete50Files(b *testing.B) { + s.benchmarkDeleteFiles(b, 50) +} + +func (s *DriverBenchmarkSuite) benchmarkDeleteFiles(b *testing.B, numFiles int64) { + for i := 0; i < b.N; i++ { + parentDir := randomPath(8) + defer s.Suite.deletePath(firstPart(parentDir)) + + b.StopTimer() + for j := int64(0); j < numFiles; j++ { + err := s.Suite.StorageDriver.PutContent(s.Suite.ctx, path.Join(parentDir, randomPath(32)), nil) + s.Suite.Require().NoError(err) + } + b.StartTimer() + + // This is the operation we're benchmarking + err := s.Suite.StorageDriver.Delete(s.Suite.ctx, firstPart(parentDir)) + s.Suite.Require().NoError(err) + } +} + func (suite *DriverSuite) testFileStreams(size int64) { tf, err := os.CreateTemp("", "tf") suite.Require().NoError(err)