From 4cb68284cf4931b0ade34c31ce18156ad618d158 Mon Sep 17 00:00:00 2001 From: Yingchun Lai Date: Wed, 11 Oct 2023 15:41:17 +0800 Subject: [PATCH] fix(ut): fix flaky unit test test_rename_path_while_writing (#1630) https://github.com/apache/incubator-pegasus/issues/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. --- .github/workflows/lint_and_test_cpp.yaml | 4 + src/block_service/test/hdfs_service_test.cpp | 79 +++++++++++--------- src/test_util/test_util.h | 2 +- 3 files changed, 49 insertions(+), 36 deletions(-) diff --git a/.github/workflows/lint_and_test_cpp.yaml b/.github/workflows/lint_and_test_cpp.yaml index 6c125727c2..f3dc28d389 100644 --- 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 index d7b33a8daf..0eaec29e88 100644 --- 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 index 762d154367..debeb7fa90 100644 --- 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)