kou commented on code in PR #39009:
URL: https://github.com/apache/arrow/pull/39009#discussion_r1415098474
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -213,7 +213,7 @@ class AzureFileSystemTest : public ::testing::Test {
suite_skipped_ = true;
GTEST_SKIP() << options.status().message();
}
- container_name_ = RandomChars(32);
+ container_name_ = "z" + RandomChars(31);
Review Comment:
Could you open an issue for the idea and add a comment with the issue number
to defer the cleanup?
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -479,6 +522,133 @@ TEST_F(AzureHierarchicalNamespaceFileSystemTest,
GetFileInfoObject) {
RunGetFileInfoObjectTest();
}
+TEST_F(AzuriteFileSystemTest, GetFileInfoSelector) {
+ SetUpSmallFileSystemTree();
+
+ FileSelector select;
+ std::vector<FileInfo> infos;
+
+ // Root dir
+ select.base_dir = "";
+ ASSERT_OK_AND_ASSIGN(infos, fs_->GetFileInfo(select));
+ ASSERT_EQ(infos.size(), 3);
+ ASSERT_EQ(infos, SortedInfos(infos));
+ AssertFileInfo(infos[0], "container", FileType::Directory);
+ AssertFileInfo(infos[1], "empty-container", FileType::Directory);
+ AssertFileInfo(infos[2], container_name_, FileType::Directory);
+
+ // Empty container
+ select.base_dir = "empty-container";
+ ASSERT_OK_AND_ASSIGN(infos, fs_->GetFileInfo(select));
+ ASSERT_EQ(infos.size(), 0);
+ // Nonexistent container
+ select.base_dir = "nonexistent-container";
+ ASSERT_RAISES(IOError, fs_->GetFileInfo(select));
+ select.allow_not_found = true;
+ ASSERT_OK_AND_ASSIGN(infos, fs_->GetFileInfo(select));
+ ASSERT_EQ(infos.size(), 0);
+ select.allow_not_found = false;
+ // Non-empty container
+ select.base_dir = "container";
+ ASSERT_OK_AND_ASSIGN(infos, fs_->GetFileInfo(select));
+ ASSERT_EQ(infos, SortedInfos(infos));
+ ASSERT_EQ(infos.size(), 4);
+ AssertFileInfo(infos[0], "container/emptydir", FileType::Directory);
+ AssertFileInfo(infos[1], "container/otherdir", FileType::Directory);
+ AssertFileInfo(infos[2], "container/somedir", FileType::Directory);
+ AssertFileInfo(infos[3], "container/somefile", FileType::File, 9);
Review Comment:
> Single-use constants are very common in tests.
I think that it's useful only when all related values (`const char*
some_data = "some data"` in this case) located nearby. (e.g. in the same test.)
If all related values located nearby, single-use constants in test is easy
to understand as you said (no idirection).
If we change `somefile` content, this test starts failing. The person
debugging these unit tests can't understand why the actual value isn't `9` (and
why the expected value is `9`) immediately.
> Do you have name suggestions for the constants?
How about this like existing `kLoremIpsum`?
```diff
diff --git a/cpp/src/arrow/filesystem/azurefs_test.cc
b/cpp/src/arrow/filesystem/azurefs_test.cc
index 22cebb179..669b5d81f 100644
--- a/cpp/src/arrow/filesystem/azurefs_test.cc
+++ b/cpp/src/arrow/filesystem/azurefs_test.cc
@@ -79,6 +79,8 @@ fugiat nulla pariatur. Excepteur sint occaecat cupidatat
non proident, sunt in
culpa qui officia deserunt mollit anim id est laborum.
)""";
+auto const* kSomeFileData = "some data";
+
class AzuriteEnv : public ::testing::Environment {
public:
AzuriteEnv() {
@@ -342,9 +344,8 @@ class AzureFileSystemTest : public ::testing::Test {
blob_client.UploadFrom(reinterpret_cast<const uint8_t*>(sub_data),
strlen(sub_data));
blob_client = container_client.GetBlockBlobClient("somefile");
- const char* some_data = "some data";
- blob_client.UploadFrom(reinterpret_cast<const uint8_t*>(some_data),
- strlen(some_data));
+ blob_client.UploadFrom(reinterpret_cast<const uint8_t*>(kSomeFileData),
+ strlen(kSomeFileData));
blob_client =
container_client.GetBlockBlobClient("otherdir/1/2/3/otherfile");
const char* other_data = "other data";
@@ -364,7 +365,7 @@ class AzureFileSystemTest : public ::testing::Test {
AssertFileInfo(infos[7], "container/somedir", FileType::Directory);
AssertFileInfo(infos[8], "container/somedir/subdir",
FileType::Directory);
AssertFileInfo(infos[9], "container/somedir/subdir/subfile",
FileType::File, 8);
- AssertFileInfo(infos[10], "container/somefile", FileType::File, 9);
+ AssertFileInfo(infos[10], "container/somefile", FileType::File,
strlen(kSomeFileData));
AssertFileInfo(infos[11], "empty-container", FileType::Directory);
AssertFileInfo(infos[12], PreexistingContainerName(),
FileType::Directory);
AssertFileInfo(infos[13], PreexistingObjectPath(), FileType::File);
@@ -595,7 +596,7 @@ TEST_F(AzuriteFileSystemTest, GetFileInfoSelector) {
AssertFileInfo(infos[0], "container/emptydir", FileType::Directory);
AssertFileInfo(infos[1], "container/otherdir", FileType::Directory);
AssertFileInfo(infos[2], "container/somedir", FileType::Directory);
- AssertFileInfo(infos[3], "container/somefile", FileType::File, 9);
+ AssertFileInfo(infos[3], "container/somefile", FileType::File,
strlen(kSomeFileData));
// Empty "directory"
select.base_dir = "container/emptydir";
@@ -663,7 +664,7 @@ TEST_F(AzuriteFileSystemTest,
GetFileInfoSelectorRecursive) {
AssertFileInfo(infos[6], "container/somedir", FileType::Directory);
AssertFileInfo(infos[7], "container/somedir/subdir", FileType::Directory);
AssertFileInfo(infos[8], "container/somedir/subdir/subfile",
FileType::File, 8);
- AssertFileInfo(infos[9], "container/somefile", FileType::File, 9);
+ AssertFileInfo(infos[9], "container/somefile", FileType::File,
strlen(kSomeFileData));
// Empty "directory"
select.base_dir = "container/emptydir";
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
