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.
realm · Oct 4, 2023 · 181edea · 181edea
1 parent 8948e83
commit 181edea
Show file tree

Hide file tree

Showing 8 changed files with 64 additions and 102 deletions.
diff --git 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
@@ -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> 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
@@ -100,13 +100,14 @@ class Logger {
         return static_cast<int>(level) >= static_cast<int>(get_level_threshold());
     }
 
-    virtual inline ~Logger() noexcept = default;
+    virtual ~Logger() noexcept = default;
 
     static void set_default_logger(std::shared_ptr<util::Logger>) noexcept;
     static std::shared_ptr<util::Logger>& 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> level_from_string(std::string_view);
 
 protected:
     // Used by subclasses that link to a base logger
@@ -376,43 +377,9 @@ template <class C, class T>
 std::basic_istream<C, T>& operator>>(std::basic_istream<C, T>& in, Logger::Level& level)
 {
     std::basic_string<C, T> 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
@@ -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
@@ -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<char>(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<SynchronousTestTransport>;
     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
@@ -22,7 +22,6 @@
 #include <realm/object-store/shared_realm.hpp>
 #include <realm/util/tagged_bool.hpp>
 
-#include <realm/util/logger.hpp>
 #include <realm/util/optional.hpp>
 
 #include <thread>
@@ -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<class StartImmediatelyTag>;
@@ -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<realm::util::Logger> 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<realm::app::GenericNetworkTransport> transport;
         bool start_sync_client = true;

diff --git a/test/test_all.cpp b/test/test_all.cpp
@@ -470,14 +470,13 @@ bool run_tests(const std::shared_ptr<realm::util::Logger>& 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<char>::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
@@ -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<NullLogger>());
+    }
+    else {
+        Logger::set_default_logger(std::make_shared<util::StderrLogger>(log_level));
+    }
+
     return true;
 }
 
@@ -317,17 +330,7 @@ std::string TestDirNameGenerator::next()
 
 std::shared_ptr<DB> 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<util::StderrLogger>(core_log_level);
     options.encryption_key = crypt_key;
     return DB::create(make_in_realm_history(), path, options);
 }