Rename S3ClientWrapper to S3ClientTestWrapper for clarity

cpp/arcticdb/storage/s3/s3_client_wrapper.cpp

@@ -19,15 +19,15 @@ using namespace object_store_utils;
 
 namespace s3 {
 
-std::optional<Aws::S3::S3Error> S3ClientWrapper::has_failure_trigger(const std::string& bucket_name) const {
-    bool static_failures_enabled = ConfigsMap::instance()->get_int("S3ClientWrapper.EnableFailures", 0) == 1;
+std::optional<Aws::S3::S3Error> S3ClientTestWrapper::has_failure_trigger(const std::string& bucket_name) const {
+    bool static_failures_enabled = ConfigsMap::instance()->get_int("S3ClientTestWrapper.EnableFailures", 0) == 1;
     // Check if mock failures are enabled
     if (!static_failures_enabled) {
         return std::nullopt;
     }
 
     // Get target buckets (if not set or "all", affects all buckets)
-    auto failure_buckets_str = ConfigsMap::instance()->get_string("S3ClientWrapper.FailureBucket", "all");
+    auto failure_buckets_str = ConfigsMap::instance()->get_string("S3ClientTestWrapper.FailureBucket", "all");
 
     if (failure_buckets_str != "all") {
         // Split the comma-separated bucket names and check if current bucket is in the list
@@ -52,8 +52,8 @@ std::optional<Aws::S3::S3Error> S3ClientWrapper::has_failure_trigger(const std::
     }
 
     // Get error configuration
-    auto error_code = ConfigsMap::instance()->get_int("S3ClientWrapper.ErrorCode", static_cast<int>(Aws::S3::S3Errors::NETWORK_CONNECTION));
-    auto retryable = ConfigsMap::instance()->get_int("S3ClientWrapper.ErrorRetryable", 0) == 1;
+    auto error_code = ConfigsMap::instance()->get_int("S3ClientTestWrapper.ErrorCode", static_cast<int>(Aws::S3::S3Errors::NETWORK_CONNECTION));
+    auto retryable = ConfigsMap::instance()->get_int("S3ClientTestWrapper.ErrorRetryable", 0) == 1;
 
     auto failure_error_ = Aws::S3::S3Error(Aws::Client::AWSError<Aws::S3::S3Errors>(
         static_cast<Aws::S3::S3Errors>(error_code),
@@ -66,7 +66,7 @@ std::optional<Aws::S3::S3Error> S3ClientWrapper::has_failure_trigger(const std::
     return failure_error_;
 }
 
-S3Result<std::monostate> S3ClientWrapper::head_object(
+S3Result<std::monostate> S3ClientTestWrapper::head_object(
         const std::string& s3_object_name,
         const std::string &bucket_name) const {
     auto maybe_error = has_failure_trigger(bucket_name);
@@ -78,7 +78,7 @@ S3Result<std::monostate> S3ClientWrapper::head_object(
     return actual_client_->head_object(s3_object_name, bucket_name);
 }
 
-S3Result<Segment> S3ClientWrapper::get_object(
+S3Result<Segment> S3ClientTestWrapper::get_object(
         const std::string &s3_object_name,
         const std::string &bucket_name) const {
     auto maybe_error = has_failure_trigger(bucket_name);
@@ -89,7 +89,7 @@ S3Result<Segment> S3ClientWrapper::get_object(
     return actual_client_->get_object(s3_object_name, bucket_name);
 }
 
-folly::Future<S3Result<Segment>> S3ClientWrapper::get_object_async(
+folly::Future<S3Result<Segment>> S3ClientTestWrapper::get_object_async(
     const std::string &s3_object_name,
     const std::string &bucket_name) const {
     auto maybe_error = has_failure_trigger(bucket_name);
@@ -100,7 +100,7 @@ folly::Future<S3Result<Segment>> S3ClientWrapper::get_object_async(
     return actual_client_->get_object_async(s3_object_name, bucket_name);
 }
 
-S3Result<std::monostate> S3ClientWrapper::put_object(
+S3Result<std::monostate> S3ClientTestWrapper::put_object(
         const std::string &s3_object_name,
         Segment &&segment,
         const std::string &bucket_name,
@@ -113,7 +113,7 @@ S3Result<std::monostate> S3ClientWrapper::put_object(
     return actual_client_->put_object(s3_object_name, std::move(segment), bucket_name, header);
 }
 
-S3Result<DeleteOutput> S3ClientWrapper::delete_objects(
+S3Result<DeleteOutput> S3ClientTestWrapper::delete_objects(
         const std::vector<std::string>& s3_object_names,
         const std::string& bucket_name) {
     auto maybe_error = has_failure_trigger(bucket_name);
@@ -128,7 +128,7 @@ S3Result<DeleteOutput> S3ClientWrapper::delete_objects(
 // Using a fixed page size since it's only being used for simple tests.
 // If we ever need to configure it we should move it to the s3 proto config instead.
 constexpr auto page_size = 10;
-S3Result<ListObjectsOutput> S3ClientWrapper::list_objects(
+S3Result<ListObjectsOutput> S3ClientTestWrapper::list_objects(
         const std::string& name_prefix,
         const std::string& bucket_name,
         const std::optional<std::string>& continuation_token) const {

cpp/arcticdb/storage/s3/s3_client_wrapper.hpp

@@ -26,15 +26,15 @@
 namespace arcticdb::storage::s3 {
 
 // A wrapper around the actual S3 client which can simulate failures based on the configuration.
-// The S3ClientWrapper delegates to the real client by default, but can intercept operations
+// The S3ClientTestWrapper delegates to the real client by default, but can intercept operations
 // to simulate failures or track operations for testing purposes.
-class S3ClientWrapper : public S3ClientInterface {
+class S3ClientTestWrapper : public S3ClientInterface {
 public:
-    explicit S3ClientWrapper(std::unique_ptr<S3ClientInterface> actual_client) : 
+    explicit S3ClientTestWrapper(std::unique_ptr<S3ClientInterface> actual_client) : 
         actual_client_(std::move(actual_client)) {
     }
 
-    ~S3ClientWrapper() override = default; 
+    ~S3ClientTestWrapper() override = default; 
 
     [[nodiscard]] S3Result<std::monostate> head_object(
         const std::string& s3_object_name,

cpp/arcticdb/storage/s3/s3_storage.cpp

@@ -139,7 +139,7 @@ void S3Storage::create_s3_client(const S3Settings &conf, const Aws::Auth::AWSCre
 
     if (conf.use_internal_client_wrapper_for_testing()){
         ARCTICDB_RUNTIME_DEBUG(log::storage(), "Using internal client wrapper for testing");
-        s3_client_ = std::make_unique<S3ClientWrapper>(std::move(s3_client_));
+        s3_client_ = std::make_unique<S3ClientTestWrapper>(std::move(s3_client_));
     }
 }
 

cpp/arcticdb/storage/test/test_s3_storage.cpp

@@ -250,11 +250,11 @@ class WrapperS3StorageFixture : public testing::Test {
     S3Storage store;
 
     void SetUp() override {
-        ConfigsMap::instance()->set_int("S3ClientWrapper.EnableFailures", 1);
+        ConfigsMap::instance()->set_int("S3ClientTestWrapper.EnableFailures", 1);
     }
 
     void TearDown() override {
-        ConfigsMap::instance()->unset_int("S3ClientWrapper.EnableFailures");
+        ConfigsMap::instance()->unset_int("S3ClientTestWrapper.EnableFailures");
     }
 };
 arcticdb::storage::nfs_backed::NfsBackedStorage::Config get_test_nfs_config() {

python/arcticdb/storage_fixtures/s3.py

@@ -18,6 +18,7 @@
 import time
 import random
 from datetime import datetime
+import string
 
 import requests
 from typing import Optional, Any, Type
@@ -524,6 +525,9 @@ def __init__(
         self.use_raw_prefix = use_raw_prefix
         self.use_mock_storage_for_testing = use_mock_storage_for_testing
         self.use_internal_client_wrapper_for_testing = use_internal_client_wrapper_for_testing
+        # This is needed because we might have multiple factories in the same test
+        # and we need to make sure the bucket names are unique
+        self.unique_id = "".join(random.choices(string.ascii_letters + string.digits, k=5))
 
     @staticmethod
     def run_server(port, key_file, cert_file):
@@ -676,7 +680,7 @@ def _policy(*statements):
         self._enforcing_permissions = enforcing
 
     def create_fixture(self) -> S3Bucket:
-        bucket = f"test_bucket_{self._bucket_id}"
+        bucket = f"test_bucket_{self.unique_id}_{self._bucket_id}"
         self._s3_admin.create_bucket(Bucket=bucket)
         self._bucket_id += 1
         if self.bucket_versioning:

python/tests/integration/arcticdb/test_s3.py

@@ -169,21 +169,21 @@ def test_wrapped_s3_storage(lib_name, wrapped_s3_storage_bucket):
     lib.write("s", data=create_df())
     test_bucket_name = wrapped_s3_storage_bucket.bucket
 
-    with config_context("S3ClientWrapper.EnableFailures", 1):
+    with config_context("S3ClientTestWrapper.EnableFailures", 1):
         with pytest.raises(NoDataFoundException, match="Unexpected network error: S3Error#99"):
             lib.read("s")
 
-        with config_context_string("S3ClientWrapper.FailureBucket", test_bucket_name):
+        with config_context_string("S3ClientTestWrapper.FailureBucket", test_bucket_name):
             with pytest.raises(StorageException, match="Unexpected network error: S3Error#99"):
                 lib.write("s", data=create_df())
 
-        with config_context_string("S3ClientWrapper.FailureBucket", f"{test_bucket_name},non_existent_bucket"):
+        with config_context_string("S3ClientTestWrapper.FailureBucket", f"{test_bucket_name},non_existent_bucket"):
             with pytest.raises(StorageException, match="Unexpected network error: S3Error#99"):
                 lib.write("s", data=create_df())
 
         # There should be no failures
         # given that we should not be simulating any failures for the test bucket
-        with config_context_string("S3ClientWrapper.FailureBucket", "non_existent_bucket"):
+        with config_context_string("S3ClientTestWrapper.FailureBucket", "non_existent_bucket"):
             lib.read("s")
             lib.write("s", data=create_df())