fix(ut): fix flaky unit test test_rename_path_while_writing (apache#1630

) apache#1631 Add retry logic for test `HDFSClientTest.test_rename_path_while_writing`, fix an used-after-free ASAN issue and remove tar files after downloading from artifacts to reduce space consumption.
acelyc111 · Oct 11, 2023 · 4cb6828 · 4cb6828
1 parent b551e26
commit 4cb6828
Show file tree

Hide file tree

Showing 3 changed files with 49 additions and 36 deletions.
diff --git a/.github/workflows/lint_and_test_cpp.yaml b/.github/workflows/lint_and_test_cpp.yaml
@@ -247,6 +247,7 @@ jobs:
       - name: Tar files
         run: |
           tar -zxvf release__builder.tar
+          rm -f release__builder.tar
       - name: Unit Testing
         run: |
           export LD_LIBRARY_PATH=`pwd`/thirdparty/output/lib:/usr/lib/jvm/java-8-openjdk-amd64/jre/lib/amd64/server
@@ -386,6 +387,7 @@ jobs:
       - name: Tar files
         run: |
           tar -zxvf release_address_builder.tar
+          rm -f release_address_builder.tar
       - name: Unit Testing
         run: |
           export LD_LIBRARY_PATH=`pwd`/thirdparty/output/lib:/usr/lib/jvm/java-8-openjdk-amd64/jre/lib/amd64/server
@@ -527,6 +529,7 @@ jobs:
 #      - name: Tar files
 #        run: |
 #          tar -zxvf release_undefined_builder.tar
+#          rm -f release_undefined_builder.tar
 #      - name: Unit Testing
 #        run: |
 #          export LD_LIBRARY_PATH=`pwd`/thirdparty/output/lib:/usr/lib/jvm/java-8-openjdk-amd64/jre/lib/amd64/server
@@ -639,6 +642,7 @@ jobs:
       - name: Tar files
         run: |
           tar -zxvf release_jemalloc_builder.tar
+          rm -f release_jemalloc_builder.tar
       - name: Unit Testing
         run: |
           export LD_LIBRARY_PATH=`pwd`/thirdparty/output/lib:/usr/lib/jvm/java-8-openjdk-amd64/jre/lib/amd64/server

diff --git a/src/block_service/test/hdfs_service_test.cpp b/src/block_service/test/hdfs_service_test.cpp
@@ -69,11 +69,21 @@ DEFINE_TASK_CODE(LPC_TEST_HDFS, TASK_PRIORITY_HIGH, dsn::THREAD_POOL_DEFAULT)
 class HDFSClientTest : public pegasus::encrypt_data_test_base
 {
 protected:
+    HDFSClientTest() : pegasus::encrypt_data_test_base()
+    {
+        for (int i = 0; i < FLAGS_num_test_file_lines; ++i) {
+            _local_test_data += "test";
+        }
+    }
+
     void generate_test_file(const std::string &filename);
     void write_test_files_async(const std::string &local_test_path,
                                 int total_files,
                                 int *success_count,
                                 task_tracker *tracker);
+
+private:
+    std::string _local_test_data;
 };
 
 void HDFSClientTest::generate_test_file(const std::string &filename)
@@ -97,29 +107,24 @@ void HDFSClientTest::write_test_files_async(const std::string &local_test_path,
                                             task_tracker *tracker)
 {
     dsn::utils::filesystem::create_directory(local_test_path);
-    std::string local_test_data;
-    for (int i = 0; i < FLAGS_num_test_file_lines; ++i) {
-        local_test_data += "test";
-    }
     for (int i = 0; i < total_files; ++i) {
-        tasking::enqueue(
-            LPC_TEST_HDFS, tracker, [&local_test_path, &local_test_data, i, success_count]() {
-                // mock the writing process in hdfs_file_object::download().
-                std::string test_file_name = local_test_path + "/test_file_" + std::to_string(i);
-                auto s = rocksdb::WriteStringToFile(rocksdb::Env::Default(),
-                                                    rocksdb::Slice(local_test_data),
-                                                    test_file_name,
-                                                    /* should_sync */ true);
-                if (s.ok()) {
-                    ++(*success_count);
-                } else {
-                    CHECK(s.IsIOError(), "{}", s.ToString());
-                    auto pos1 = s.ToString().find(
-                        "IO error: No such file or directory: While open a file for appending: ");
-                    auto pos2 = s.ToString().find("IO error: While appending to file: ");
-                    CHECK(pos1 == 0 || pos2 == 0, "{}", s.ToString());
-                }
-            });
+        tasking::enqueue(LPC_TEST_HDFS, tracker, [this, &local_test_path, i, success_count]() {
+            // mock the writing process in hdfs_file_object::download().
+            std::string test_file_name = local_test_path + "/test_file_" + std::to_string(i);
+            auto s = rocksdb::WriteStringToFile(rocksdb::Env::Default(),
+                                                rocksdb::Slice(_local_test_data),
+                                                test_file_name,
+                                                /* should_sync */ true);
+            if (s.ok()) {
+                ++(*success_count);
+            } else {
+                CHECK(s.IsIOError(), "{}", s.ToString());
+                auto pos1 = s.ToString().find(
+                    "IO error: No such file or directory: While open a file for appending: ");
+                auto pos2 = s.ToString().find("IO error: While appending to file: ");
+                CHECK(pos1 == 0 || pos2 == 0, "{}", s.ToString());
+            }
+        });
     }
 }
 
@@ -443,17 +448,21 @@ TEST_P(HDFSClientTest, test_rename_path_while_writing)
     std::string kLocalTestPath = "test_dir";
     const int kTotalFiles = 100;
 
-    task_tracker tracker;
-    int success_count = 0;
-    write_test_files_async(kLocalTestPath, kTotalFiles, &success_count, &tracker);
-    usleep(100);
-
-    std::string kLocalRenamedTestPath = "rename_dir." + std::to_string(dsn_now_ms());
-    // Rename succeed but some files write failed.
-    ASSERT_TRUE(dsn::utils::filesystem::rename_path(kLocalTestPath, kLocalRenamedTestPath));
-    tracker.cancel_outstanding_tasks();
-    // Generally, we can assume partial files are written failed.
-    // It maybe flaky, please retry if it failed.
-    ASSERT_GT(success_count, 0) << success_count;
-    ASSERT_LT(success_count, kTotalFiles) << success_count;
+    // The test is flaky, retry if it failed in 300 seconds.
+    ASSERT_IN_TIME_WITH_FIXED_INTERVAL(
+        [&] {
+            task_tracker tracker;
+            int success_count = 0;
+            write_test_files_async(kLocalTestPath, kTotalFiles, &success_count, &tracker);
+            usleep(100);
+
+            std::string kLocalRenamedTestPath = "rename_dir." + std::to_string(dsn_now_ms());
+            // Rename succeed but some files write failed.
+            ASSERT_TRUE(dsn::utils::filesystem::rename_path(kLocalTestPath, kLocalRenamedTestPath));
+            tracker.cancel_outstanding_tasks();
+            // Generally, we can assume partial files are written failed.
+            ASSERT_GT(success_count, 0) << success_count;
+            ASSERT_LT(success_count, kTotalFiles) << success_count;
+        },
+        300);
 }
diff --git a/src/test_util/test_util.h b/src/test_util/test_util.h
@@ -59,7 +59,7 @@ void create_local_test_file(const std::string &full_name, dsn::replication::file
 
 #define ASSERT_IN_TIME_WITH_FIXED_INTERVAL(expr, sec)                                              \
     do {                                                                                           \
-        AssertEventually(expr, sec, WaitBackoff::NONE);                                            \
+        AssertEventually(expr, sec, ::pegasus::WaitBackoff::NONE);                                 \
         NO_PENDING_FATALS();                                                                       \
     } while (0)