From c3bff0036ca4fe9faeb01cb5d8dcd0209051599d Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Tue, 2 Apr 2024 06:15:53 +0900 Subject: [PATCH] GH-39069: [C++][FS][Azure] Use the generic filesystem tests (#40567) ### Rationale for this change We should provide common spec for all filesystem API. ### What changes are included in this PR? Enable the generic filesystem tests. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: #39069 Authored-by: Sutou Kouhei Signed-off-by: Sutou Kouhei --- cpp/src/arrow/filesystem/azurefs.cc | 117 +++++++-- cpp/src/arrow/filesystem/azurefs_test.cc | 319 +++++++++++++++-------- cpp/src/arrow/filesystem/test_util.cc | 30 ++- cpp/src/arrow/filesystem/test_util.h | 4 + 4 files changed, 333 insertions(+), 137 deletions(-) diff --git a/cpp/src/arrow/filesystem/azurefs.cc b/cpp/src/arrow/filesystem/azurefs.cc index 260478b068ed1..84733a824e7ba 100644 --- a/cpp/src/arrow/filesystem/azurefs.cc +++ b/cpp/src/arrow/filesystem/azurefs.cc @@ -1591,7 +1591,9 @@ class AzureFileSystem::Impl { if (info.type() == FileType::NotFound) { return PathNotFound(location); } - DCHECK_EQ(info.type(), FileType::Directory); + if (info.type() != FileType::Directory) { + return NotADir(location); + } return Status::OK(); } @@ -1818,8 +1820,67 @@ class AzureFileSystem::Impl { const AzureLocation& location, bool recursive) { DCHECK(!location.container.empty()); DCHECK(!location.path.empty()); - // Non-recursive CreateDir calls require the parent directory to exist. - if (!recursive) { + if (recursive) { + // Recursive CreateDir calls require that all path segments be + // either a directory or not found. + + // Check each path segment is a directory or not + // found. Nonexistent segments are collected to + // nonexistent_locations. We'll create directories for + // nonexistent segments later. + std::vector nonexistent_locations; + for (auto prefix = location; !prefix.path.empty(); prefix = prefix.parent()) { + ARROW_ASSIGN_OR_RAISE(auto info, GetFileInfo(container_client, prefix)); + if (info.type() == FileType::File) { + return NotADir(prefix); + } + if (info.type() == FileType::NotFound) { + nonexistent_locations.push_back(prefix); + } + } + // Ensure container exists + ARROW_ASSIGN_OR_RAISE(auto container, + AzureLocation::FromString(location.container)); + ARROW_ASSIGN_OR_RAISE(auto container_info, + GetContainerPropsAsFileInfo(container, container_client)); + if (container_info.type() == FileType::NotFound) { + try { + container_client.CreateIfNotExists(); + } catch (const Storage::StorageException& exception) { + return ExceptionToStatus(exception, "Failed to create directory '", + location.all, "': ", container_client.GetUrl()); + } + } + // Create nonexistent directories from shorter to longer: + // + // Example: + // + // * location: /container/a/b/c/d/ + // * Nonexistent path segments: + // * /container/a/ + // * /container/a/c/ + // * /container/a/c/d/ + // * target_locations: + // 1. /container/a/c/d/ + // 2. /container/a/c/ + // 3. /container/a/ + // + // Create order: + // 1. /container/a/ + // 2. /container/a/c/ + // 3. /container/a/c/d/ + for (size_t i = nonexistent_locations.size(); i > 0; --i) { + const auto& nonexistent_location = nonexistent_locations[i - 1]; + try { + create_if_not_exists(container_client, nonexistent_location); + } catch (const Storage::StorageException& exception) { + return ExceptionToStatus(exception, "Failed to create directory '", + location.all, "': ", container_client.GetUrl()); + } + } + return Status::OK(); + } else { + // Non-recursive CreateDir calls require the parent directory to exist. auto parent = location.parent(); if (!parent.path.empty()) { RETURN_NOT_OK(CheckDirExists(container_client, parent)); @@ -1827,28 +1888,17 @@ class AzureFileSystem::Impl { // If the parent location is just the container, we don't need to check if it // exists because the operation we perform below will fail if the container // doesn't exist and we can handle that error according to the recursive flag. - } - try { - create_if_not_exists(container_client, location); - return Status::OK(); - } catch (const Storage::StorageException& exception) { - if (IsContainerNotFound(exception)) { - try { - if (recursive) { - container_client.CreateIfNotExists(); - create_if_not_exists(container_client, location); - return Status::OK(); - } else { - auto parent = location.parent(); - return PathNotFound(parent); - } - } catch (const Storage::StorageException& second_exception) { - return ExceptionToStatus(second_exception, "Failed to create directory '", - location.all, "': ", container_client.GetUrl()); + try { + create_if_not_exists(container_client, location); + return Status::OK(); + } catch (const Storage::StorageException& exception) { + if (IsContainerNotFound(exception)) { + auto parent = location.parent(); + return PathNotFound(parent); } + return ExceptionToStatus(exception, "Failed to create directory '", location.all, + "': ", container_client.GetUrl()); } - return ExceptionToStatus(exception, "Failed to create directory '", location.all, - "': ", container_client.GetUrl()); } } @@ -2016,8 +2066,15 @@ class AzureFileSystem::Impl { bool found_dir_marker_blob = false; try { auto list_response = container_client.ListBlobs(options); - if (require_dir_to_exist && list_response.Blobs.empty()) { - return PathNotFound(location); + if (list_response.Blobs.empty()) { + if (require_dir_to_exist) { + return PathNotFound(location); + } else { + ARROW_ASSIGN_OR_RAISE(auto info, GetFileInfo(container_client, location)); + if (info.type() == FileType::File) { + return NotADir(location); + } + } } for (; list_response.HasPage(); list_response.MoveToNextPage()) { if (list_response.Blobs.empty()) { @@ -2732,6 +2789,16 @@ class AzureFileSystem::Impl { } auto dest_blob_client = GetBlobClient(dest.container, dest.path); auto src_url = GetBlobClient(src.container, src.path).GetUrl(); + if (!dest.path.empty()) { + auto dest_parent = dest.parent(); + if (!dest_parent.path.empty()) { + auto dest_container_client = GetBlobContainerClient(dest_parent.container); + ARROW_ASSIGN_OR_RAISE(auto info, GetFileInfo(dest_container_client, dest_parent)); + if (info.type() == FileType::File) { + return NotADir(dest_parent); + } + } + } try { dest_blob_client.CopyFromUri(src_url); } catch (const Storage::StorageException& exception) { diff --git a/cpp/src/arrow/filesystem/azurefs_test.cc b/cpp/src/arrow/filesystem/azurefs_test.cc index 7ea5eb446bc12..24031e313f798 100644 --- a/cpp/src/arrow/filesystem/azurefs_test.cc +++ b/cpp/src/arrow/filesystem/azurefs_test.cc @@ -98,6 +98,7 @@ class BaseAzureEnv : public ::testing::Environment { virtual AzureBackend backend() const = 0; + virtual bool HasSubmitBatchBug() const { return false; } virtual bool WithHierarchicalNamespace() const { return false; } virtual Result GetDebugLogSize() { return 0; } @@ -207,6 +208,18 @@ class AzuriteEnv : public AzureEnvImpl { return self; } + /// Azurite has a bug that causes BlobContainerClient::SubmitBatch to fail on macOS. + /// SubmitBatch is used by: + /// - AzureFileSystem::DeleteDir + /// - AzureFileSystem::DeleteDirContents + bool HasSubmitBatchBug() const override { +#ifdef __APPLE__ + return true; +#else + return false; +#endif + } + Result GetDebugLogSize() override { ARROW_ASSIGN_OR_RAISE(auto exists, arrow::internal::FileExists(debug_log_path_)); if (!exists) { @@ -274,6 +287,186 @@ class AzureHierarchicalNSEnv : public AzureEnvImpl { bool WithHierarchicalNamespace() const final { return true; } }; +namespace { +Result MakeOptions(BaseAzureEnv* env) { + AzureOptions options; + options.account_name = env->account_name(); + switch (env->backend()) { + case AzureBackend::kAzurite: + options.blob_storage_authority = "127.0.0.1:10000"; + options.dfs_storage_authority = "127.0.0.1:10000"; + options.blob_storage_scheme = "http"; + options.dfs_storage_scheme = "http"; + break; + case AzureBackend::kAzure: + // Use the default values + break; + } + ARROW_EXPECT_OK(options.ConfigureAccountKeyCredential(env->account_key())); + return options; +} +} // namespace + +struct PreexistingData { + public: + using RNG = random::pcg32_fast; + + public: + const std::string container_name; + static constexpr char const* kObjectName = "test-object-name"; + + static constexpr char const* kLoremIpsum = R"""( +Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor +incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis +nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. +Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu +fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in +culpa qui officia deserunt mollit anim id est laborum. +)"""; + + public: + explicit PreexistingData(RNG& rng) : container_name{RandomContainerName(rng)} {} + + // Creates a path by concatenating the container name and the stem. + std::string ContainerPath(std::string_view stem) const { return Path(stem); } + + // Short alias to ContainerPath() + std::string Path(std::string_view stem) const { + return ConcatAbstractPath(container_name, stem); + } + + std::string ObjectPath() const { return ContainerPath(kObjectName); } + std::string NotFoundObjectPath() const { return ContainerPath("not-found"); } + + std::string RandomDirectoryPath(RNG& rng) const { + return ContainerPath(RandomChars(32, rng)); + } + + // Utilities + static std::string RandomContainerName(RNG& rng) { return RandomChars(32, rng); } + + static std::string RandomChars(int count, RNG& rng) { + auto const fillers = std::string("abcdefghijlkmnopqrstuvwxyz0123456789"); + std::uniform_int_distribution d(0, static_cast(fillers.size()) - 1); + std::string s; + std::generate_n(std::back_inserter(s), count, [&] { return fillers[d(rng)]; }); + return s; + } + + static int RandomIndex(int end, RNG& rng) { + return std::uniform_int_distribution(0, end - 1)(rng); + } + + static std::string RandomLine(int lineno, int width, RNG& rng) { + auto line = std::to_string(lineno) + ": "; + line += RandomChars(width - static_cast(line.size()) - 1, rng); + line += '\n'; + return line; + } +}; + +class TestGeneric : public ::testing::Test, public GenericFileSystemTest { + public: + void TearDown() override { + if (azure_fs_) { + ASSERT_OK(azure_fs_->DeleteDir(container_name_)); + } + } + + protected: + void SetUpInternal(BaseAzureEnv* env) { + env_ = env; + random::pcg32_fast rng((std::random_device()())); + container_name_ = PreexistingData::RandomContainerName(rng); + ASSERT_OK_AND_ASSIGN(auto options, MakeOptions(env_)); + ASSERT_OK_AND_ASSIGN(azure_fs_, AzureFileSystem::Make(options)); + ASSERT_OK(azure_fs_->CreateDir(container_name_, true)); + fs_ = std::make_shared(container_name_, azure_fs_); + } + + std::shared_ptr GetEmptyFileSystem() override { return fs_; } + + bool have_implicit_directories() const override { return true; } + bool allow_write_file_over_dir() const override { return true; } + bool allow_read_dir_as_file() const override { return true; } + bool allow_move_dir() const override { return false; } + bool allow_move_file() const override { return true; } + bool allow_append_to_file() const override { return true; } + bool have_directory_mtimes() const override { return false; } + bool have_flaky_directory_tree_deletion() const override { return false; } + bool have_file_metadata() const override { return true; } + // calloc() used in libxml2's xmlNewGlobalState() is detected as a + // memory leak like the following. But it's a false positive. It's + // used in ListBlobsByHierarchy() for GetFileInfo() and it's freed + // in the call. This is detected as a memory leak only with + // generator API (GetFileInfoGenerator()) and not detected with + // non-generator API (GetFileInfo()). So this is a false positive. + // + // ==2875409==ERROR: LeakSanitizer: detected memory leaks + // + // Direct leak of 968 byte(s) in 1 object(s) allocated from: + // #0 0x55d02c967bdc in calloc (build/debug/arrow-azurefs-test+0x17bbdc) (BuildId: + // 520690d1b20e860cc1feef665dce8196e64f955e) #1 0x7fa914b1cd1e in xmlNewGlobalState + // builddir/main/../../threads.c:580:10 #2 0x7fa914b1cd1e in xmlGetGlobalState + // builddir/main/../../threads.c:666:31 + bool have_false_positive_memory_leak_with_generator() const override { return true; } + + BaseAzureEnv* env_; + std::shared_ptr azure_fs_; + std::shared_ptr fs_; + + private: + std::string container_name_; +}; + +class TestAzuriteGeneric : public TestGeneric { + public: + void SetUp() override { + ASSERT_OK_AND_ASSIGN(auto env, AzuriteEnv::GetInstance()); + SetUpInternal(env); + } + + protected: + // Azurite doesn't support moving files over containers. + bool allow_move_file() const override { return false; } + // DeleteDir() doesn't work with Azurite on macOS + bool have_flaky_directory_tree_deletion() const override { + return env_->HasSubmitBatchBug(); + } +}; + +class TestAzureFlatNSGeneric : public TestGeneric { + public: + void SetUp() override { + auto env_result = AzureFlatNSEnv::GetInstance(); + if (env_result.status().IsCancelled()) { + GTEST_SKIP() << env_result.status().message(); + } + ASSERT_OK_AND_ASSIGN(auto env, env_result); + SetUpInternal(env); + } + + protected: + // Flat namespace account doesn't support moving files over containers. + bool allow_move_file() const override { return false; } +}; + +class TestAzureHierarchicalNSGeneric : public TestGeneric { + public: + void SetUp() override { + auto env_result = AzureHierarchicalNSEnv::GetInstance(); + if (env_result.status().IsCancelled()) { + GTEST_SKIP() << env_result.status().message(); + } + ASSERT_OK_AND_ASSIGN(auto env, env_result); + SetUpInternal(env); + } +}; + +GENERIC_FS_TEST_FUNCTIONS(TestAzuriteGeneric); +GENERIC_FS_TEST_FUNCTIONS(TestAzureFlatNSGeneric); +GENERIC_FS_TEST_FUNCTIONS(TestAzureHierarchicalNSGeneric); + TEST(AzureFileSystem, InitializingFilesystemWithoutAccountNameFails) { AzureOptions options; ASSERT_RAISES(Invalid, options.ConfigureAccountKeyCredential("account_key")); @@ -532,64 +725,6 @@ TEST_F(TestAzureOptions, FromUriInvalidQueryParameter) { TestFromUriInvalidQueryParameter(); } -struct PreexistingData { - public: - using RNG = random::pcg32_fast; - - public: - const std::string container_name; - static constexpr char const* kObjectName = "test-object-name"; - - static constexpr char const* kLoremIpsum = R"""( -Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor -incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis -nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. -Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu -fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in -culpa qui officia deserunt mollit anim id est laborum. -)"""; - - public: - explicit PreexistingData(RNG& rng) : container_name{RandomContainerName(rng)} {} - - // Creates a path by concatenating the container name and the stem. - std::string ContainerPath(std::string_view stem) const { return Path(stem); } - - // Short alias to ContainerPath() - std::string Path(std::string_view stem) const { - return ConcatAbstractPath(container_name, stem); - } - - std::string ObjectPath() const { return ContainerPath(kObjectName); } - std::string NotFoundObjectPath() const { return ContainerPath("not-found"); } - - std::string RandomDirectoryPath(RNG& rng) const { - return ContainerPath(RandomChars(32, rng)); - } - - // Utilities - static std::string RandomContainerName(RNG& rng) { return RandomChars(32, rng); } - - static std::string RandomChars(int count, RNG& rng) { - auto const fillers = std::string("abcdefghijlkmnopqrstuvwxyz0123456789"); - std::uniform_int_distribution d(0, static_cast(fillers.size()) - 1); - std::string s; - std::generate_n(std::back_inserter(s), count, [&] { return fillers[d(rng)]; }); - return s; - } - - static int RandomIndex(int end, RNG& rng) { - return std::uniform_int_distribution(0, end - 1)(rng); - } - - static std::string RandomLine(int lineno, int width, RNG& rng) { - auto line = std::to_string(lineno) + ": "; - line += RandomChars(width - static_cast(line.size()) - 1, rng); - line += '\n'; - return line; - } -}; - class TestAzureFileSystem : public ::testing::Test { protected: // Set in constructor @@ -621,24 +756,6 @@ class TestAzureFileSystem : public ::testing::Test { return fs(CachedHNSSupport(*env)); } - static Result MakeOptions(BaseAzureEnv* env) { - AzureOptions options; - options.account_name = env->account_name(); - switch (env->backend()) { - case AzureBackend::kAzurite: - options.blob_storage_authority = "127.0.0.1:10000"; - options.dfs_storage_authority = "127.0.0.1:10000"; - options.blob_storage_scheme = "http"; - options.dfs_storage_scheme = "http"; - break; - case AzureBackend::kAzure: - // Use the default values - break; - } - ARROW_EXPECT_OK(options.ConfigureAccountKeyCredential(env->account_key())); - return options; - } - void SetUp() override { auto make_options = [this]() -> Result { ARROW_ASSIGN_OR_RAISE(auto env, GetAzureEnv()); @@ -824,19 +941,6 @@ class TestAzureFileSystem : public ::testing::Test { "This test is affected by an Azurite issue: " "https://github.com/Azure/Azurite/pull/2302"; - /// Azurite has a bug that causes BlobContainerClient::SubmitBatch to fail on macOS. - /// SubmitBatch is used by: - /// - AzureFileSystem::DeleteDir - /// - AzureFileSystem::DeleteDirContents - bool HasSubmitBatchBug() const { -#ifdef __APPLE__ - EXPECT_OK_AND_ASSIGN(auto env, GetAzureEnv()); - return env->backend() == AzureBackend::kAzurite; -#else - return false; -#endif - } - static bool WithErrno(const Status& status, int expected_errno) { auto* detail = status.detail().get(); return detail && @@ -1059,9 +1163,7 @@ class TestAzureFileSystem : public ::testing::Test { auto path2 = data.Path("directory2"); ASSERT_OK(fs()->OpenOutputStream(path2)); - // CreateDir returns OK even if there is already a file or directory at this - // location. Whether or not this is the desired behaviour is debatable. - ASSERT_OK(fs()->CreateDir(path2)); + ASSERT_RAISES(IOError, fs()->CreateDir(path2)); AssertFileInfo(fs(), path2, FileType::File); } @@ -1070,7 +1172,8 @@ class TestAzureFileSystem : public ::testing::Test { } void TestDeleteDirSuccessEmpty() { - if (HasSubmitBatchBug()) { + ASSERT_OK_AND_ASSIGN(auto env, GetAzureEnv()); + if (env->HasSubmitBatchBug()) { GTEST_SKIP() << kSubmitBatchBugMessage; } auto data = SetUpPreexistingData(); @@ -1090,7 +1193,8 @@ class TestAzureFileSystem : public ::testing::Test { } void TestDeleteDirSuccessHaveBlob() { - if (HasSubmitBatchBug()) { + ASSERT_OK_AND_ASSIGN(auto env, GetAzureEnv()); + if (env->HasSubmitBatchBug()) { GTEST_SKIP() << kSubmitBatchBugMessage; } auto data = SetUpPreexistingData(); @@ -1105,7 +1209,8 @@ class TestAzureFileSystem : public ::testing::Test { } void TestNonEmptyDirWithTrailingSlash() { - if (HasSubmitBatchBug()) { + ASSERT_OK_AND_ASSIGN(auto env, GetAzureEnv()); + if (env->HasSubmitBatchBug()) { GTEST_SKIP() << kSubmitBatchBugMessage; } auto data = SetUpPreexistingData(); @@ -1120,7 +1225,8 @@ class TestAzureFileSystem : public ::testing::Test { } void TestDeleteDirSuccessHaveDirectory() { - if (HasSubmitBatchBug()) { + ASSERT_OK_AND_ASSIGN(auto env, GetAzureEnv()); + if (env->HasSubmitBatchBug()) { GTEST_SKIP() << kSubmitBatchBugMessage; } auto data = SetUpPreexistingData(); @@ -1135,7 +1241,8 @@ class TestAzureFileSystem : public ::testing::Test { } void TestDeleteDirContentsSuccessExist() { - if (HasSubmitBatchBug()) { + ASSERT_OK_AND_ASSIGN(auto env, GetAzureEnv()); + if (env->HasSubmitBatchBug()) { GTEST_SKIP() << kSubmitBatchBugMessage; } auto preexisting_data = SetUpPreexistingData(); @@ -1149,7 +1256,8 @@ class TestAzureFileSystem : public ::testing::Test { } void TestDeleteDirContentsSuccessExistWithTrailingSlash() { - if (HasSubmitBatchBug()) { + ASSERT_OK_AND_ASSIGN(auto env, GetAzureEnv()); + if (env->HasSubmitBatchBug()) { GTEST_SKIP() << kSubmitBatchBugMessage; } auto preexisting_data = SetUpPreexistingData(); @@ -1163,7 +1271,8 @@ class TestAzureFileSystem : public ::testing::Test { } void TestDeleteDirContentsSuccessNonexistent() { - if (HasSubmitBatchBug()) { + ASSERT_OK_AND_ASSIGN(auto env, GetAzureEnv()); + if (env->HasSubmitBatchBug()) { GTEST_SKIP() << kSubmitBatchBugMessage; } auto data = SetUpPreexistingData(); @@ -2174,7 +2283,8 @@ TEST_F(TestAzuriteFileSystem, DeleteDirSuccessContainer) { } TEST_F(TestAzuriteFileSystem, DeleteDirSuccessNonexistent) { - if (HasSubmitBatchBug()) { + ASSERT_OK_AND_ASSIGN(auto env, GetAzureEnv()); + if (env->HasSubmitBatchBug()) { GTEST_SKIP() << kSubmitBatchBugMessage; } auto data = SetUpPreexistingData(); @@ -2185,7 +2295,8 @@ TEST_F(TestAzuriteFileSystem, DeleteDirSuccessNonexistent) { } TEST_F(TestAzuriteFileSystem, DeleteDirSuccessHaveBlobs) { - if (HasSubmitBatchBug()) { + ASSERT_OK_AND_ASSIGN(auto env, GetAzureEnv()); + if (env->HasSubmitBatchBug()) { GTEST_SKIP() << kSubmitBatchBugMessage; } auto data = SetUpPreexistingData(); @@ -2213,7 +2324,8 @@ TEST_F(TestAzuriteFileSystem, DeleteDirUri) { } TEST_F(TestAzuriteFileSystem, DeleteDirContentsSuccessContainer) { - if (HasSubmitBatchBug()) { + ASSERT_OK_AND_ASSIGN(auto env, GetAzureEnv()); + if (env->HasSubmitBatchBug()) { GTEST_SKIP() << kSubmitBatchBugMessage; } auto data = SetUpPreexistingData(); @@ -2228,7 +2340,8 @@ TEST_F(TestAzuriteFileSystem, DeleteDirContentsSuccessContainer) { } TEST_F(TestAzuriteFileSystem, DeleteDirContentsSuccessDirectory) { - if (HasSubmitBatchBug()) { + ASSERT_OK_AND_ASSIGN(auto env, GetAzureEnv()); + if (env->HasSubmitBatchBug()) { GTEST_SKIP() << kSubmitBatchBugMessage; } auto data = SetUpPreexistingData(); diff --git a/cpp/src/arrow/filesystem/test_util.cc b/cpp/src/arrow/filesystem/test_util.cc index 040917dcd218a..19226ce01ae2f 100644 --- a/cpp/src/arrow/filesystem/test_util.cc +++ b/cpp/src/arrow/filesystem/test_util.cc @@ -252,8 +252,7 @@ void GenericFileSystemTest::TestCreateDir(FileSystem* fs) { } void GenericFileSystemTest::TestDeleteDir(FileSystem* fs) { - if (have_flaky_directory_tree_deletion()) - GTEST_SKIP() << "Flaky directory deletion on Windows"; + if (have_flaky_directory_tree_deletion()) GTEST_SKIP() << "Flaky directory deletion"; ASSERT_OK(fs->CreateDir("AB/CD/EF")); ASSERT_OK(fs->CreateDir("AB/GH/IJ")); @@ -281,8 +280,7 @@ void GenericFileSystemTest::TestDeleteDir(FileSystem* fs) { } void GenericFileSystemTest::TestDeleteDirContents(FileSystem* fs) { - if (have_flaky_directory_tree_deletion()) - GTEST_SKIP() << "Flaky directory deletion on Windows"; + if (have_flaky_directory_tree_deletion()) GTEST_SKIP() << "Flaky directory deletion"; ASSERT_OK(fs->CreateDir("AB/CD/EF")); ASSERT_OK(fs->CreateDir("AB/GH/IJ")); @@ -313,6 +311,8 @@ void GenericFileSystemTest::TestDeleteDirContents(FileSystem* fs) { } void GenericFileSystemTest::TestDeleteRootDirContents(FileSystem* fs) { + if (have_flaky_directory_tree_deletion()) GTEST_SKIP() << "Flaky directory deletion"; + ASSERT_OK(fs->CreateDir("AB/CD")); CreateFile(fs, "AB/abc", ""); @@ -323,9 +323,7 @@ void GenericFileSystemTest::TestDeleteRootDirContents(FileSystem* fs) { AssertAllDirs(fs, {"AB", "AB/CD"}); AssertAllFiles(fs, {"AB/abc"}); } else { - if (!have_flaky_directory_tree_deletion()) { - AssertAllDirs(fs, {}); - } + AssertAllDirs(fs, {}); AssertAllFiles(fs, {}); } } @@ -385,6 +383,10 @@ void GenericFileSystemTest::TestDeleteFiles(FileSystem* fs) { } void GenericFileSystemTest::TestMoveFile(FileSystem* fs) { + if (!allow_move_file()) { + GTEST_SKIP() << "Filesystem doesn't allow moving files"; + } + ASSERT_OK(fs->CreateDir("AB/CD")); ASSERT_OK(fs->CreateDir("EF")); CreateFile(fs, "abc", "data"); @@ -750,6 +752,12 @@ void GenericFileSystemTest::TestGetFileInfoSelector(FileSystem* fs) { } void GenericFileSystemTest::TestGetFileInfoGenerator(FileSystem* fs) { +#ifdef ADDRESS_SANITIZER + if (have_false_positive_memory_leak_with_generator()) { + GTEST_SKIP() << "Filesystem have false positive memory leak with generator"; + } +#endif + ASSERT_OK(fs->CreateDir("AB/CD")); CreateFile(fs, "abc", "data"); CreateFile(fs, "AB/def", "some data"); @@ -1177,8 +1185,12 @@ void GenericFileSystemTest::TestSpecialChars(FileSystem* fs) { AssertFileContents(fs, "Special and%different.txt", "data"); ASSERT_OK(fs->DeleteFile("Special and%different.txt")); - ASSERT_OK(fs->DeleteDir("Blank Char")); - AssertAllDirs(fs, {}); + if (have_flaky_directory_tree_deletion()) { + ASSERT_OK(fs->DeleteFile("Blank Char/Special%Char.txt")); + } else { + ASSERT_OK(fs->DeleteDir("Blank Char")); + AssertAllDirs(fs, {}); + } AssertAllFiles(fs, {}); } diff --git a/cpp/src/arrow/filesystem/test_util.h b/cpp/src/arrow/filesystem/test_util.h index 62b488e159a24..e70c787aa85c4 100644 --- a/cpp/src/arrow/filesystem/test_util.h +++ b/cpp/src/arrow/filesystem/test_util.h @@ -168,6 +168,8 @@ class ARROW_TESTING_EXPORT GenericFileSystemTest { virtual bool allow_write_file_over_dir() const { return false; } // - Whether the filesystem allows reading a directory virtual bool allow_read_dir_as_file() const { return false; } + // - Whether the filesystem allows moving a file + virtual bool allow_move_file() const { return true; } // - Whether the filesystem allows moving a directory virtual bool allow_move_dir() const { return true; } // - Whether the filesystem allows moving a directory "over" a non-empty destination @@ -182,6 +184,8 @@ class ARROW_TESTING_EXPORT GenericFileSystemTest { virtual bool have_flaky_directory_tree_deletion() const { return false; } // - Whether the filesystem stores some metadata alongside files virtual bool have_file_metadata() const { return false; } + // - Whether the filesystem has a false positive memory leak with generator + virtual bool have_false_positive_memory_leak_with_generator() const { return false; } void TestEmpty(FileSystem* fs); void TestNormalizePath(FileSystem* fs);