From ad9c2cceda9cd893c0f754e49f7fca6e417ee95f Mon Sep 17 00:00:00 2001 From: furszy Date: Sun, 29 Sep 2024 14:21:49 -0300 Subject: [PATCH 1/2] test, bench: specialize working directory name Since G_TEST_GET_FULL_NAME is not initialized in the benchmark framework, benchmarks using the unit test setup run in the same directory without any clear distinction between them. This poses an extra complication for locating any specific benchmark directory during a failure. In master, unit tests and benchmarks run in the following path: //test_common bitcoin// After this commit, unit tests and benchmarks are contained within its own directory: //test_common bitcoin/// This makes it easier to find any benchmark run when a failure occurs. --- src/bench/bench.cpp | 12 +++++++++++- src/test/util/setup_common.cpp | 11 ++++------- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/bench/bench.cpp b/src/bench/bench.cpp index a2dbb118885..d6913a05f2a 100644 --- a/src/bench/bench.cpp +++ b/src/bench/bench.cpp @@ -29,7 +29,16 @@ const std::function G_TEST_LOG_FUN{}; const std::function()> G_TEST_COMMAND_LINE_ARGUMENTS{}; -const std::function G_TEST_GET_FULL_NAME{}; +/** + * Retrieve the name of the currently in-use benchmark. + * This is applicable only to benchmarks that utilize the unit test + * framework context setup (e.g. ones using 'MakeNoLogFileContext()'). + * It places the datadir of each benchmark run within their respective benchmark name. + */ +static std::string g_running_benchmark_name; +const std::function G_TEST_GET_FULL_NAME = []() { + return g_running_benchmark_name; +}; namespace { @@ -117,6 +126,7 @@ void BenchRunner::RunAll(const Args& args) bench.output(nullptr); } bench.name(name); + g_running_benchmark_name = name; if (args.min_time > 0ms) { // convert to nanos before dividing to reduce rounding errors std::chrono::nanoseconds min_time_ns = args.min_time; diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 2de282dbf6d..a22d34c075f 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -75,8 +75,6 @@ using node::VerifyLoadedChainstate; const std::function G_TRANSLATION_FUN = nullptr; constexpr inline auto TEST_DIR_PATH_ELEMENT{"test_common bitcoin"}; // Includes a space to catch possible path escape issues. -/** Random context to get unique temp data dirs. Separate from m_rng, which can be seeded from a const env var */ -static FastRandomContext g_rng_temp_path; struct NetworkSetup { @@ -139,10 +137,10 @@ BasicTestingSetup::BasicTestingSetup(const ChainType chainType, TestOpts opts) // data directories use a random name that doesn't overlap with other tests. SeedRandomForTest(SeedRand::FIXED_SEED); + const std::string test_name{G_TEST_GET_FULL_NAME ? G_TEST_GET_FULL_NAME() : ""}; if (!m_node.args->IsArgSet("-testdatadir")) { - // By default, the data directory has a random name - const auto rand_str{g_rng_temp_path.rand256().ToString()}; - m_path_root = fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / rand_str; + const auto now{TicksSinceEpoch(SystemClock::now())}; + m_path_root = fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / test_name / util::ToString(now); TryCreateDirectories(m_path_root); } else { // Custom data directory @@ -151,8 +149,7 @@ BasicTestingSetup::BasicTestingSetup(const ChainType chainType, TestOpts opts) if (root_dir.empty()) ExitFailure("-testdatadir argument is empty, please specify a path"); root_dir = fs::absolute(root_dir); - const std::string test_path{G_TEST_GET_FULL_NAME ? G_TEST_GET_FULL_NAME() : ""}; - m_path_lock = root_dir / TEST_DIR_PATH_ELEMENT / fs::PathFromString(test_path); + m_path_lock = root_dir / TEST_DIR_PATH_ELEMENT / fs::PathFromString(test_name); m_path_root = m_path_lock / "datadir"; // Try to obtain the lock; if unsuccessful don't disturb the existing test. From fa66e0887ca1a1445d8b18ba1fadb12b2d911048 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 28 Dec 2023 23:51:13 -0300 Subject: [PATCH 2/2] bench: add support for custom data directory Expands the benchmark framework with the existing '-testdatadir' arg, enabling the ability to change the benchmark data directory. This is useful for running benchmarks on different storage devices, and not just under the OS /tmp/ directory. --- src/bench/bench.cpp | 18 +++++++++++++++++- src/bench/bench.h | 1 + src/bench/bench_bitcoin.cpp | 15 +++++++++++++++ src/test/util/setup_common.cpp | 5 ++--- src/test/util/setup_common.h | 3 +++ 5 files changed, 38 insertions(+), 4 deletions(-) diff --git a/src/bench/bench.cpp b/src/bench/bench.cpp index d6913a05f2a..26daff50705 100644 --- a/src/bench/bench.cpp +++ b/src/bench/bench.cpp @@ -27,7 +27,15 @@ using util::Join; const std::function G_TEST_LOG_FUN{}; -const std::function()> G_TEST_COMMAND_LINE_ARGUMENTS{}; +/** + * Retrieves the available test setup command line arguments that may be used + * in the benchmark. They will be used only if the benchmark utilizes a + * 'BasicTestingSetup' or any child of it. + */ +static std::function()> g_bench_command_line_args{}; +const std::function()> G_TEST_COMMAND_LINE_ARGUMENTS = []() { + return g_bench_command_line_args(); +}; /** * Retrieve the name of the currently in-use benchmark. @@ -103,6 +111,14 @@ void BenchRunner::RunAll(const Args& args) std::cout << "Running with -sanity-check option, output is being suppressed as benchmark results will be useless." << std::endl; } + // Load inner test setup args + g_bench_command_line_args = [&args]() { + std::vector ret; + ret.reserve(args.setup_args.size()); + for (const auto& arg : args.setup_args) ret.emplace_back(arg.c_str()); + return ret; + }; + std::vector benchmarkResults; for (const auto& [name, bench_func] : benchmarks()) { const auto& [func, priority_level] = bench_func; diff --git a/src/bench/bench.h b/src/bench/bench.h index f0705f4fed8..2df203ce236 100644 --- a/src/bench/bench.h +++ b/src/bench/bench.h @@ -61,6 +61,7 @@ struct Args { fs::path output_json; std::string regex_filter; uint8_t priority; + std::vector setup_args; }; class BenchRunner diff --git a/src/bench/bench_bitcoin.cpp b/src/bench/bench_bitcoin.cpp index 555dca7d598..88afe68a1a9 100644 --- a/src/bench/bench_bitcoin.cpp +++ b/src/bench/bench_bitcoin.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include @@ -27,6 +28,7 @@ static const std::string DEFAULT_PRIORITY{"all"}; static void SetupBenchArgs(ArgsManager& argsman) { SetupHelpOptions(argsman); + SetupCommonTestArgs(argsman); argsman.AddArg("-asymptote=", "Test asymptotic growth of the runtime of an algorithm, if supported by the benchmark", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-filter=", strprintf("Regular expression filter to select benchmark by name (default: %s)", DEFAULT_BENCH_FILTER), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); @@ -60,6 +62,18 @@ static uint8_t parsePriorityLevel(const std::string& str) { return levels; } +static std::vector parseTestSetupArgs(const ArgsManager& argsman) +{ + // Parses unit test framework arguments supported by the benchmark framework. + std::vector args; + static std::vector AVAILABLE_ARGS = {"-testdatadir"}; + for (const std::string& arg_name : AVAILABLE_ARGS) { + auto op_arg = argsman.GetArg(arg_name); + if (op_arg) args.emplace_back(strprintf("%s=%s", arg_name, *op_arg)); + } + return args; +} + int main(int argc, char** argv) { ArgsManager argsman; @@ -131,6 +145,7 @@ int main(int argc, char** argv) args.regex_filter = argsman.GetArg("-filter", DEFAULT_BENCH_FILTER); args.sanity_check = argsman.GetBoolArg("-sanity-check", false); args.priority = parsePriorityLevel(argsman.GetArg("-priority-level", DEFAULT_PRIORITY)); + args.setup_args = parseTestSetupArgs(argsman); benchmark::BenchRunner::RunAll(args); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index a22d34c075f..46a6daa88cf 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -85,8 +85,7 @@ struct NetworkSetup }; static NetworkSetup g_networksetup_instance; -/** Register test-only arguments */ -static void SetupUnitTestArgs(ArgsManager& argsman) +void SetupCommonTestArgs(ArgsManager& argsman) { argsman.AddArg("-testdatadir", strprintf("Custom data directory (default: %s)", fs::PathToString(fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / "")), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); @@ -125,7 +124,7 @@ BasicTestingSetup::BasicTestingSetup(const ChainType chainType, TestOpts opts) gArgs.ClearPathCache(); { SetupServerArgs(*m_node.args); - SetupUnitTestArgs(*m_node.args); + SetupCommonTestArgs(*m_node.args); std::string error; if (!m_node.args->ParseParameters(arguments.size(), arguments.data(), error)) { m_node.args->ClearArgs(); diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h index f9cf5d91577..b0f7bdade20 100644 --- a/src/test/util/setup_common.h +++ b/src/test/util/setup_common.h @@ -45,6 +45,9 @@ extern const std::function G_TEST_GET_FULL_NAME; static constexpr CAmount CENT{1000000}; +/** Register common test args. Shared across binaries that rely on the test framework. */ +void SetupCommonTestArgs(ArgsManager& argsman); + struct TestOpts { std::vector extra_args{}; bool coins_db_in_memory{true};