From 181edeaff03fdadc5edb4ef6fa2bedd6eaf3ed81 Mon Sep 17 00:00:00 2001 From: Thomas Goyne Date: Tue, 3 Oct 2023 17:14:53 -0700 Subject: [PATCH] Use UNITTEST_LOG_LEVEL in objectstore tests For historical reasons core and sync tests use the UNITTEST_LOG_LEVEL environment variable to determine the test log level, while object store tests used a build time setting. This brings them into alignment on using the env variable, and applies it via setting the default log level on startup in a single place. --- evergreen/config.yml | 8 ++++-- src/realm/util/logger.cpp | 34 +++++++++++++++++++++- src/realm/util/logger.hpp | 43 ++++------------------------ test/object-store/CMakeLists.txt | 15 ---------- test/object-store/util/test_file.cpp | 13 +-------- test/object-store/util/test_file.hpp | 15 ---------- test/test_all.cpp | 15 +++++----- test/util/test_path.cpp | 23 ++++++++------- 8 files changed, 64 insertions(+), 102 deletions(-) diff --git a/evergreen/config.yml b/evergreen/config.yml index fd5fd21e90d..654415508ef 100644 --- a/evergreen/config.yml +++ b/evergreen/config.yml @@ -131,9 +131,6 @@ functions: set_cmake_var realm_vars CMAKE_TOOLCHAIN_FILE PATH "${cmake_toolchain_file}" fi - set_cmake_var realm_vars REALM_TEST_LOGGING BOOL On - set_cmake_var realm_vars REALM_TEST_LOGGING_LEVEL STRING "debug" - GENERATOR="${cmake_generator}" if [ -z "${cmake_generator|}" ]; then GENERATOR="Ninja Multi-Config" @@ -257,6 +254,9 @@ functions: if [[ -n "${run_with_encryption}" ]]; then export UNITTEST_ENCRYPT_ALL=1 fi + if [[ -n "${test_log_level}" ]]; then + export UNITTEST_LOG_LEVEL="${test_log_level}" + fi export TSAN_OPTIONS="suppressions=$(pwd)/test/tsan.suppress" cd build @@ -877,6 +877,7 @@ tasks: vars: test_label: objstore-local test_executable_name: "realm-object-store-tests" + test_log_level: debug verbose_test_output: true - func: "check branch state" @@ -896,6 +897,7 @@ tasks: vars: test_label: objstore-baas test_executable_name: "realm-object-store-tests" + test_log_level: debug verbose_test_output: true - func: "check branch state" diff --git a/src/realm/util/logger.cpp b/src/realm/util/logger.cpp index a826f1d23e5..10ef355fbaf 100644 --- a/src/realm/util/logger.cpp +++ b/src/realm/util/logger.cpp @@ -84,7 +84,7 @@ const char* Logger::get_level_prefix(Level level) noexcept return ""; } -const std::string_view Logger::level_to_string(Level level) noexcept +std::string_view Logger::level_to_string(Level level) noexcept { switch (level) { case Logger::Level::all: @@ -110,6 +110,38 @@ const std::string_view Logger::level_to_string(Level level) noexcept return ""; } +std::optional Logger::level_from_string(std::string_view str) +{ + if (str == "all") { + return Logger::Level::all; + } + if (str == "trace") { + return Logger::Level::trace; + } + if (str == "debug") { + return Logger::Level::debug; + } + if (str == "detail") { + return Logger::Level::detail; + } + if (str == "info") { + return Logger::Level::info; + } + if (str == "warn") { + return Logger::Level::warn; + } + if (str == "error") { + return Logger::Level::error; + } + if (str == "fatal") { + return Logger::Level::fatal; + } + if (str == "off") { + return Logger::Level::off; + } + return std::nullopt; +} + void StderrLogger::do_log(Level level, const std::string& message) { // std::cerr is unbuffered, so no need to flush diff --git a/src/realm/util/logger.hpp b/src/realm/util/logger.hpp index eb1d10d54e2..29bee1ec796 100644 --- a/src/realm/util/logger.hpp +++ b/src/realm/util/logger.hpp @@ -100,13 +100,14 @@ class Logger { return static_cast(level) >= static_cast(get_level_threshold()); } - virtual inline ~Logger() noexcept = default; + virtual ~Logger() noexcept = default; static void set_default_logger(std::shared_ptr) noexcept; static std::shared_ptr& get_default_logger() noexcept; static void set_default_level_threshold(Level level) noexcept; static Level get_default_level_threshold() noexcept; - static const std::string_view level_to_string(Level level) noexcept; + static std::string_view level_to_string(Level level) noexcept; + static std::optional level_from_string(std::string_view); protected: // Used by subclasses that link to a base logger @@ -376,43 +377,9 @@ template std::basic_istream& operator>>(std::basic_istream& in, Logger::Level& level) { std::basic_string str; - auto check = [&](const char* name) { - size_t n = strlen(name); - if (n != str.size()) - return false; - for (size_t i = 0; i < n; ++i) { - if (in.widen(name[i]) != str[i]) - return false; - } - return true; - }; if (in >> str) { - if (check("all")) { - level = Logger::Level::all; - } - else if (check("trace")) { - level = Logger::Level::trace; - } - else if (check("debug")) { - level = Logger::Level::debug; - } - else if (check("detail")) { - level = Logger::Level::detail; - } - else if (check("info")) { - level = Logger::Level::info; - } - else if (check("warn")) { - level = Logger::Level::warn; - } - else if (check("error")) { - level = Logger::Level::error; - } - else if (check("fatal")) { - level = Logger::Level::fatal; - } - else if (check("off")) { - level = Logger::Level::off; + if (auto parsed = Logger::level_from_string(str)) { + level = *parsed; } else { in.setstate(std::ios_base::failbit); diff --git a/test/object-store/CMakeLists.txt b/test/object-store/CMakeLists.txt index 241c5eb58da..48308ebcb06 100644 --- a/test/object-store/CMakeLists.txt +++ b/test/object-store/CMakeLists.txt @@ -137,21 +137,6 @@ if(REALM_ENABLE_SYNC) endif() endif() -if(REALM_TEST_LOGGING) - target_compile_definitions(ObjectStoreTests PRIVATE - TEST_ENABLE_LOGGING=1 - ) - - if(REALM_TEST_LOGGING_LEVEL) - message(STATUS "Test logging level: ${REALM_TEST_LOGGING_LEVEL}") - target_compile_definitions(ObjectStoreTests PRIVATE - TEST_LOGGING_LEVEL=${REALM_TEST_LOGGING_LEVEL} - ) - else() - message(STATUS "Test logging enabled") - endif() -endif() - target_include_directories(ObjectStoreTests PRIVATE ${CATCH_INCLUDE_DIR} ${JSON_INCLUDE_DIR} diff --git a/test/object-store/util/test_file.cpp b/test/object-store/util/test_file.cpp index 7c2a2f00db3..35feb94a420 100644 --- a/test/object-store/util/test_file.cpp +++ b/test/object-store/util/test_file.cpp @@ -70,7 +70,6 @@ TestFile::TestFile() disable_sync_to_disk(); m_temp_dir = util::make_temp_dir(); path = (fs::path(m_temp_dir) / "realm.XXXXXX").string(); - util::Logger::set_default_level_threshold(realm::util::Logger::Level::TEST_LOGGING_LEVEL); if (const char* crypt_key = test_util::crypt_key()) { encryption_key = std::vector(crypt_key, crypt_key + 64); } @@ -197,16 +196,8 @@ SyncServer::SyncServer(const SyncServer::Config& config) , m_server(m_local_root_dir, util::none, ([&] { using namespace std::literals::chrono_literals; -#if TEST_ENABLE_LOGGING - m_logger = util::Logger::get_default_logger(); - -#else - // Logging is disabled, use a NullLogger to prevent printing anything - m_logger.reset(new util::NullLogger()); -#endif - sync::Server::Config c; - c.logger = m_logger; + c.logger = util::Logger::get_default_logger(); c.token_expiration_clock = this; c.listen_address = "127.0.0.1"; c.disable_sync_to_disk = true; @@ -339,7 +330,6 @@ TestAppSession::TestAppSession(AppSession session, if (!m_transport) m_transport = instance_of; auto app_config = get_config(m_transport, *m_app_session); - util::Logger::set_default_level_threshold(realm::util::Logger::Level::TEST_LOGGING_LEVEL); set_app_config_defaults(app_config, m_transport); util::try_make_dir(m_base_file_path); @@ -429,7 +419,6 @@ TestSyncManager::TestSyncManager(const Config& config, const SyncServer::Config& { app::App::Config app_config = config.app_config; set_app_config_defaults(app_config, transport); - util::Logger::set_default_level_threshold(config.log_level); SyncClientConfig sc_config; m_base_file_path = config.base_path.empty() ? util::make_temp_dir() + random_string(10) : config.base_path; diff --git a/test/object-store/util/test_file.hpp b/test/object-store/util/test_file.hpp index 905cb95299a..c21a06a2212 100644 --- a/test/object-store/util/test_file.hpp +++ b/test/object-store/util/test_file.hpp @@ -22,7 +22,6 @@ #include #include -#include #include #include @@ -97,18 +96,6 @@ struct InMemoryTestFile : realm::Realm::Config { void advance_and_notify(realm::Realm& realm); void on_change_but_no_notify(realm::Realm& realm); -#ifndef TEST_ENABLE_LOGGING -#define TEST_ENABLE_LOGGING 0 // change to 1 to enable trace-level logging -#endif - -#ifndef TEST_LOGGING_LEVEL -#if TEST_ENABLE_LOGGING -#define TEST_LOGGING_LEVEL all -#else -#define TEST_LOGGING_LEVEL off -#endif // TEST_ENABLE_LOGGING -#endif // TEST_LOGGING_LEVEL - #if REALM_ENABLE_SYNC using StartImmediately = realm::util::TaggedBool; @@ -147,7 +134,6 @@ class SyncServer : private realm::sync::Clock { friend class TestSyncManager; SyncServer(const Config& config); std::string m_local_root_dir; - std::shared_ptr m_logger; realm::sync::Server m_server; std::thread m_thread; std::string m_url; @@ -224,7 +210,6 @@ class TestSyncManager { std::string base_path; realm::SyncManager::MetadataMode metadata_mode = realm::SyncManager::MetadataMode::NoEncryption; bool should_teardown_test_directory = true; - realm::util::Logger::Level log_level = realm::util::Logger::Level::TEST_LOGGING_LEVEL; bool override_sync_route = true; std::shared_ptr transport; bool start_sync_client = true; diff --git a/test/test_all.cpp b/test/test_all.cpp index 6363776a887..205bb6e073d 100644 --- a/test/test_all.cpp +++ b/test/test_all.cpp @@ -470,14 +470,13 @@ bool run_tests(const std::shared_ptr& logger = nullptr) // Set intra test log level threshold { const char* str = getenv("UNITTEST_LOG_LEVEL"); - if (str && strlen(str) != 0) { - std::istringstream in(str); - in.imbue(std::locale::classic()); - in.flags(in.flags() & ~std::ios_base::skipws); // Do not accept white space - in >> config.intra_test_log_level; - bool bad = !in || in.get() != std::char_traits::eof(); - if (bad) - throw std::runtime_error("Bad intra test log level"); + if (str && *str) { + if (auto level = realm::util::Logger::level_from_string(str)) { + config.intra_test_log_level = *level; + } + else { + throw std::runtime_error(util::format("Invalid log level '%1'", str)); + } } } diff --git a/test/util/test_path.cpp b/test/util/test_path.cpp index bca0afd988d..dd32ab64a28 100644 --- a/test/util/test_path.cpp +++ b/test/util/test_path.cpp @@ -159,6 +159,19 @@ bool initialize_test_path(int argc, const char* argv[]) g_path_prefix = argv[1]; } + Logger::Level log_level = Logger::Level::off; + if (const char* str = getenv("UNITTEST_LOG_LEVEL"); str && *str) { + if (auto custom_level = Logger::level_from_string(str)) { + log_level = *custom_level; + } + } + if (log_level == Logger::Level::off) { + Logger::set_default_logger(std::make_shared()); + } + else { + Logger::set_default_logger(std::make_shared(log_level)); + } + return true; } @@ -317,17 +330,7 @@ std::string TestDirNameGenerator::next() std::shared_ptr get_test_db(const std::string& path, const char* crypt_key) { - const char* str = getenv("UNITTEST_LOG_LEVEL"); - realm::util::Logger::Level core_log_level = realm::util::Logger::Level::off; - if (str && strlen(str) != 0) { - std::istringstream in(str); - in.imbue(std::locale::classic()); - in.flags(in.flags() & ~std::ios_base::skipws); // Do not accept white space - in >> core_log_level; - } - DBOptions options; - options.logger = std::make_shared(core_log_level); options.encryption_key = crypt_key; return DB::create(make_in_realm_history(), path, options); }