address issues from PR comments

Alan4506 · Alan4506 · commit 8b4adbace6ad · 2025-07-08T13:31:20.000-04:00
diff --git a/tests/performance-tests/include/performance-tests/PerformanceTestBase.h b/tests/performance-tests/include/performance-tests/PerformanceTestBase.h
@@ -0,0 +1,33 @@
+/**
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ * SPDX-License-Identifier: Apache-2.0.
+ */
+
+#pragma once
+
+namespace PerformanceTest {
+
+/**
+ * Base class for all performance tests.
+ */
+class PerformanceTestBase {
+ public:
+  virtual ~PerformanceTestBase() = default;
+
+  /**
+   * Initialize resources for the test.
+   */
+  virtual void Setup() = 0;
+
+  /**
+   * Run the performance test operations.
+   */
+  virtual void Run() = 0;
+
+  /**
+   * Clean up resources created during setup.
+   */
+  virtual void TearDown() = 0;
+};
+
+}  // namespace PerformanceTest
diff --git a/tests/performance-tests/include/performance-tests/Utils.h b/tests/performance-tests/include/performance-tests/Utils.h
@@ -5,17 +5,13 @@
 
 #pragma once
 
-#include <aws/core/Aws.h>
 #include <aws/core/utils/StringUtils.h>
 #include <aws/core/utils/UUID.h>
 #include <aws/core/utils/memory/stl/AWSString.h>
 
 #include <algorithm>
 #include <cstddef>
 #include <cstdlib>
-#include <iostream>
-#include <random>
-#include <string>
 
 namespace PerformanceTest {
 namespace Utils {
@@ -37,16 +33,6 @@ static inline Aws::String RandomString(size_t length) {
   return str;
 }
 
-/**
- * Log error messages to stderr.
- * @param service The name of the AWS service
- * @param operation The name of the operation that failed
- * @param message The error message to log
- */
-static inline void LogError(const Aws::String& service, const Aws::String& operation, const Aws::String& message) {
-  std::cerr << "[ERROR] " << service << ":" << operation << " failed: " << message << '\n';
-}
-
 /**
  * Generate a unique identifier using UUID.
  * @return A 10-character lowercase UUID substring
@@ -56,16 +42,5 @@ static inline Aws::String GenerateUniqueId() {
   return Aws::Utils::StringUtils::ToLower(rawUUID.c_str()).substr(0, 10);
 }
 
-/**
- * Get the current SDK version as a formatted string.
- * @return SDK version in the format "major.minor.patch" (e.g., "1.11.0")
- */
-static inline Aws::String GetSDKVersionString() {
-  Aws::SDKOptions::SDKVersion const version;
-  return Aws::Utils::StringUtils::to_string(static_cast<int>(version.major)) + "." +
-         Aws::Utils::StringUtils::to_string(static_cast<int>(version.minor)) + "." +
-         Aws::Utils::StringUtils::to_string(static_cast<int>(version.patch));
-}
-
 }  // namespace Utils
 }  // namespace PerformanceTest
diff --git a/tests/performance-tests/include/performance-tests/services/s3/S3PerformanceTest.h b/tests/performance-tests/include/performance-tests/services/s3/S3PerformanceTest.h
@@ -5,8 +5,10 @@
 
 #pragma once
 
+#include <aws/core/utils/memory/AWSMemory.h>
 #include <aws/core/utils/memory/stl/AWSString.h>
 #include <aws/s3/S3Client.h>
+#include <performance-tests/PerformanceTestBase.h>
 
 #include <cstddef>
 
@@ -17,46 +19,32 @@ namespace S3 {
  * Configuration for S3 performance test cases.
  */
 struct TestCase {
-  Aws::String sizeLabel;
+  const char* sizeLabel;
   size_t sizeBytes;
-  Aws::String bucketTypeLabel;
+  const char* bucketTypeLabel;
 };
 
 /**
- * Runs S3 performance test by creating a bucket, performing PutObject and GetObject operations,
- * then cleaning up resources.
- * @param s3 S3 client instance to use for operations
- * @param config Test configuration containing object size and bucket type parameters
- * @param availabilityZoneId Availability zone ID for S3 Express buckets
- * @param iterations Number of put/get operations to perform
+ * S3 performance test implementation.
+ * Tests PutObject and GetObject operations with different payload sizes and bucket types.
  */
-void RunTest(Aws::S3::S3Client& s3, const TestCase& config, const Aws::String& availabilityZoneId, int iterations = 3);
-
-/**
- * Create S3 bucket for testing.
- * @param s3 S3 client instance
- * @param config Test configuration
- * @param availabilityZoneId Availability zone ID for S3 Express buckets
- * @return Bucket name if successful, empty string if failed
- */
-Aws::String SetupBucket(Aws::S3::S3Client& s3, const TestCase& config, const Aws::String& availabilityZoneId);
-
-/**
- * Run PutObject and GetObject operations.
- * @param s3 S3 client instance
- * @param bucketName Name of the bucket to use
- * @param config Test configuration
- * @param iterations Number of operations to perform
- */
-void RunOperations(Aws::S3::S3Client& s3, const Aws::String& bucketName, const TestCase& config, int iterations);
+class S3PerformanceTest : public PerformanceTestBase {
+ public:
+  S3PerformanceTest(const Aws::String& region, const TestCase& config, const Aws::String& availabilityZoneId, int iterations = 3);
+
+  void Setup() override;
+  void TearDown() override;
+  void Run() override;
+
+ private:
+  const TestCase& m_config;
+  const Aws::String m_region;
+  const Aws::String m_availabilityZoneId;
+  const int m_iterations;
+  Aws::UniquePtr<Aws::S3::S3Client> m_s3;
+  Aws::String m_bucketName;
+};
 
-/**
- * Clean up S3 resources (objects and bucket).
- * @param s3 S3 client instance
- * @param bucketName Name of the bucket to clean up
- * @param iterations Number of objects to delete
- */
-void CleanupResources(Aws::S3::S3Client& s3, const Aws::String& bucketName, int iterations);
 }  // namespace S3
 }  // namespace Services
 }  // namespace PerformanceTest
diff --git a/tests/performance-tests/include/performance-tests/services/s3/S3TestConfig.h b/tests/performance-tests/include/performance-tests/services/s3/S3TestConfig.h
@@ -5,21 +5,25 @@
 
 #pragma once
 
-#include <aws/core/utils/memory/stl/AWSSet.h>
-#include <aws/core/utils/memory/stl/AWSString.h>
-#include <aws/core/utils/memory/stl/AWSVector.h>
+#include <performance-tests/services/s3/S3PerformanceTest.h>
+
+#include <array>
+#include <set>
 
 namespace PerformanceTest {
 namespace Services {
 namespace S3 {
 namespace TestConfig {
-const Aws::Set<Aws::String> TestOperations = {"PutObject", "GetObject"};
+const std::set<const char*> TestOperations = {"PutObject", "GetObject"};
 
-const Aws::Vector<TestCase> TestMatrix = {{"8KB", 8 * 1024, "s3-standard"},    {"64KB", 64 * 1024, "s3-standard"},
-                                          {"1MB", 1024 * 1024, "s3-standard"}, {"8KB", 8 * 1024, "s3-express"},
-                                          {"64KB", 64 * 1024, "s3-express"},   {"1MB", 1024 * 1024, "s3-express"}};
+const std::array<TestCase, 6> TestMatrix = {{{"8KB", 8 * 1024, "s3-standard"},
+                                             {"64KB", 64 * 1024, "s3-standard"},
+                                             {"1MB", 1024 * 1024, "s3-standard"},
+                                             {"8KB", 8 * 1024, "s3-express"},
+                                             {"64KB", 64 * 1024, "s3-express"},
+                                             {"1MB", 1024 * 1024, "s3-express"}}};
 
-const Aws::String OutputFilename = "s3-performance-test-results.json";
+const char* OutputFilename = "s3-performance-test-results.json";
 }  // namespace TestConfig
 }  // namespace S3
 }  // namespace Services
diff --git a/tests/performance-tests/src/services/s3/S3PerformanceTest.cpp b/tests/performance-tests/src/services/s3/S3PerformanceTest.cpp
@@ -3,7 +3,10 @@
  * SPDX-License-Identifier: Apache-2.0.
  */
 
+#include <aws/core/client/ClientConfiguration.h>
 #include <aws/core/utils/StringUtils.h>
+#include <aws/core/utils/logging/LogMacros.h>
+#include <aws/core/utils/memory/AWSMemory.h>
 #include <aws/core/utils/memory/stl/AWSAllocator.h>
 #include <aws/core/utils/memory/stl/AWSString.h>
 #include <aws/core/utils/memory/stl/AWSStringStream.h>
@@ -21,92 +24,96 @@
 #include <performance-tests/Utils.h>
 #include <performance-tests/services/s3/S3PerformanceTest.h>
 
-void PerformanceTest::Services::S3::RunTest(Aws::S3::S3Client& s3, const TestCase& config, const Aws::String& availabilityZoneId,
-                                            int iterations) {
-  auto bucketName = SetupBucket(s3, config, availabilityZoneId);
-  if (bucketName.empty()) {
-    return;
-  }
+#include <cstring>
 
-  RunOperations(s3, bucketName, config, iterations);
-  CleanupResources(s3, bucketName, iterations);
-}
+PerformanceTest::Services::S3::S3PerformanceTest::S3PerformanceTest(const Aws::String& region, const TestCase& config,
+                                                                    const Aws::String& availabilityZoneId, int iterations)
+    : m_config(config), m_region(region), m_availabilityZoneId(availabilityZoneId), m_iterations(iterations) {}
+
+void PerformanceTest::Services::S3::S3PerformanceTest::Setup() {
+  Aws::Client::ClientConfiguration cfg;
+  cfg.region = m_region;
+  m_s3 = Aws::MakeUnique<Aws::S3::S3Client>("S3PerformanceTest", cfg);
 
-Aws::String PerformanceTest::Services::S3::SetupBucket(Aws::S3::S3Client& s3, const TestCase& config,
-                                                       const Aws::String& availabilityZoneId) {
-  Aws::String bucketName;
   Aws::S3::Model::CreateBucketRequest cbr;
   Aws::String const bucketId = PerformanceTest::Utils::GenerateUniqueId();
 
-  if (config.bucketTypeLabel == "s3-express") {
-    bucketName = "perf-express-" + bucketId + "--" + availabilityZoneId + "--x-s3";
-    cbr.SetBucket(bucketName);
+  if (strcmp(m_config.bucketTypeLabel, "s3-express") == 0) {
+    m_bucketName = "perf-express-" + bucketId + "--" + m_availabilityZoneId + "--x-s3";
+    cbr.SetBucket(m_bucketName);
     Aws::S3::Model::CreateBucketConfiguration bucketConfig;
     bucketConfig.SetLocation(
-        Aws::S3::Model::LocationInfo().WithType(Aws::S3::Model::LocationType::AvailabilityZone).WithName(availabilityZoneId));
+        Aws::S3::Model::LocationInfo().WithType(Aws::S3::Model::LocationType::AvailabilityZone).WithName(m_availabilityZoneId));
 
     bucketConfig.SetBucket(Aws::S3::Model::BucketInfo()
                                .WithType(Aws::S3::Model::BucketType::Directory)
                                .WithDataRedundancy(Aws::S3::Model::DataRedundancy::SingleAvailabilityZone));
 
     cbr.SetCreateBucketConfiguration(bucketConfig);
   } else {
-    bucketName = "perf-standard-" + bucketId;
-    cbr.SetBucket(bucketName);
+    m_bucketName = "perf-standard-" + bucketId;
+    cbr.SetBucket(m_bucketName);
   }
 
-  auto createOutcome = s3.CreateBucket(cbr);
+  auto createOutcome = m_s3->CreateBucket(cbr);
   if (!createOutcome.IsSuccess()) {
-    PerformanceTest::Utils::LogError("S3", "CreateBucket", createOutcome.GetError().GetMessage());
-    return "";
+    AWS_LOG_ERROR("PerformanceTest", ("S3:CreateBucket failed: " + createOutcome.GetError().GetMessage()).c_str());
+    m_bucketName.clear();
   }
-
-  return bucketName;
 }
 
-void PerformanceTest::Services::S3::RunOperations(Aws::S3::S3Client& s3, const Aws::String& bucketName, const TestCase& config,
-                                                  int iterations) {
-  const auto randomPayload = PerformanceTest::Utils::RandomString(config.sizeBytes);
+void PerformanceTest::Services::S3::S3PerformanceTest::Run() {
+  if (m_bucketName.empty()) {
+    AWS_LOG_ERROR("PerformanceTest", "S3:Run - Bucket setup failed, skipping test");
+    return;
+  }
+
+  const auto randomPayload = PerformanceTest::Utils::RandomString(m_config.sizeBytes);
 
   // Run PutObject multiple times
-  for (int i = 0; i < iterations; i++) {
+  for (int i = 0; i < m_iterations; i++) {
     auto stream = Aws::MakeShared<Aws::StringStream>("PerfStream");
     *stream << randomPayload;
 
     Aws::S3::Model::PutObjectRequest por;
-    por.WithBucket(bucketName).WithKey("test-object-" + Aws::Utils::StringUtils::to_string(i)).SetBody(stream);
-    por.SetAdditionalCustomHeaderValue("test-dimension-size", config.sizeLabel);
-    por.SetAdditionalCustomHeaderValue("test-dimension-bucket-type", config.bucketTypeLabel);
-    auto putOutcome = s3.PutObject(por);
+    por.WithBucket(m_bucketName).WithKey("test-object-" + Aws::Utils::StringUtils::to_string(i)).SetBody(stream);
+    por.SetAdditionalCustomHeaderValue("test-dimension-size", m_config.sizeLabel);
+    por.SetAdditionalCustomHeaderValue("test-dimension-bucket-type", m_config.bucketTypeLabel);
+    auto putOutcome = m_s3->PutObject(por);
     if (!putOutcome.IsSuccess()) {
-      PerformanceTest::Utils::LogError("S3", "PutObject", putOutcome.GetError().GetMessage());
+      AWS_LOG_ERROR("PerformanceTest", ("S3:PutObject failed: " + putOutcome.GetError().GetMessage()).c_str());
     }
   }
 
   // Run GetObject multiple times
-  for (int i = 0; i < iterations; i++) {
+  for (int i = 0; i < m_iterations; i++) {
     Aws::S3::Model::GetObjectRequest gor;
-    gor.WithBucket(bucketName).WithKey("test-object-" + Aws::Utils::StringUtils::to_string(i));
-    gor.SetAdditionalCustomHeaderValue("test-dimension-size", config.sizeLabel);
-    gor.SetAdditionalCustomHeaderValue("test-dimension-bucket-type", config.bucketTypeLabel);
-    auto getOutcome = s3.GetObject(gor);
+    gor.WithBucket(m_bucketName).WithKey("test-object-" + Aws::Utils::StringUtils::to_string(i));
+    gor.SetAdditionalCustomHeaderValue("test-dimension-size", m_config.sizeLabel);
+    gor.SetAdditionalCustomHeaderValue("test-dimension-bucket-type", m_config.bucketTypeLabel);
+    auto getOutcome = m_s3->GetObject(gor);
     if (!getOutcome.IsSuccess()) {
-      PerformanceTest::Utils::LogError("S3", "GetObject", getOutcome.GetError().GetMessage());
+      AWS_LOG_ERROR("PerformanceTest", ("S3:GetObject failed: " + getOutcome.GetError().GetMessage()).c_str());
     }
   }
 }
 
-void PerformanceTest::Services::S3::CleanupResources(Aws::S3::S3Client& s3, const Aws::String& bucketName, int iterations) {
-  for (int i = 0; i < iterations; i++) {
-    auto deleteObjectOutcome = s3.DeleteObject(
-        Aws::S3::Model::DeleteObjectRequest().WithBucket(bucketName).WithKey("test-object-" + Aws::Utils::StringUtils::to_string(i)));
+void PerformanceTest::Services::S3::S3PerformanceTest::TearDown() {
+  if (m_bucketName.empty()) {
+    AWS_LOG_ERROR("PerformanceTest", "S3:TearDown - No bucket to clean up, setup likely failed");
+    return;
+  }
+
+  for (int i = 0; i < m_iterations; i++) {
+    auto deleteObjectOutcome = m_s3->DeleteObject(
+        Aws::S3::Model::DeleteObjectRequest().WithBucket(m_bucketName).WithKey("test-object-" + Aws::Utils::StringUtils::to_string(i)));
     if (!deleteObjectOutcome.IsSuccess()) {
-      PerformanceTest::Utils::LogError("S3", "DeleteObject", deleteObjectOutcome.GetError().GetMessage());
+      AWS_LOG_ERROR("PerformanceTest", ("S3:DeleteObject failed: " + deleteObjectOutcome.GetError().GetMessage()).c_str());
     }
   }
 
-  auto deleteBucketOutcome = s3.DeleteBucket(Aws::S3::Model::DeleteBucketRequest().WithBucket(bucketName));
+  auto deleteBucketOutcome = m_s3->DeleteBucket(Aws::S3::Model::DeleteBucketRequest().WithBucket(m_bucketName));
   if (!deleteBucketOutcome.IsSuccess()) {
-    PerformanceTest::Utils::LogError("S3", "DeleteBucket", deleteBucketOutcome.GetError().GetMessage());
+    AWS_LOG_ERROR("PerformanceTest", ("S3:DeleteBucket failed: " + deleteBucketOutcome.GetError().GetMessage()).c_str());
   }
 }
diff --git a/tests/performance-tests/src/services/s3/main.cpp b/tests/performance-tests/src/services/s3/main.cpp
