Repository: bitcoin/bitcoin
File: src/test/logging_tests.cpp (C++)

Code before:
        LogDebug(BCLog::LogFlags::HTTP, "%s\n", message);
        return;
    case Location::FOURTH:
        LogPrintLevel_(BCLog::LogFlags::ALL, BCLog::Level::Info, /*should_ratelimit=*/false, "%s\n", message);
        return;
    } // no default case, so the compiler can warn about missing cases
    assert(false);
}

void LogFromLocationAndExpect(Location location, const std::string& message, const std::string& expect) {
    ASSERT_DEBUG_LOG(expect);
    LogFromLocation(location, message);
}

} // namespace

BOOST_FIXTURE_TEST_CASE(logging_filesize_rate_limit, LogSetup)
{
    bool prev_log_timestamps{LogInstance().m_log_timestamps};
    LogInstance().m_log_timestamps = false;
    bool prev_log_sourcelocations{LogInstance().m_log_sourcelocations};
    LogInstance().m_log_sourcelocations = false;
    bool prev_log_threadnames{LogInstance().m_log_threadnames};
    LogInstance().m_log_threadnames = false;

    int64_t line_length{1023};
    int64_t num_lines{1024};

    // Add 1 to line_length because of newline.
    int64_t bytes_quota{(line_length + 1) * num_lines};

    std::chrono::seconds time_window{20};

    CScheduler scheduler{};
    scheduler.m_service_thread = std::thread([&] { scheduler.serviceQueue(); });
    BCLog::LogRateLimiter::SchedulerFunction sched_func{[&scheduler](std::function<void()> func, std::chrono::milliseconds window) {
        scheduler.scheduleEvery(std::move(func), window);
    }};
    std::unique_ptr<BCLog::LogRateLimiter> limiter{std::make_unique<BCLog::LogRateLimiter>(sched_func, bytes_quota, time_window)};
    LogInstance().SetRateLimiting(std::move(limiter));

    std::string log_message(line_length, 'a');

    std::string utf8_path{LogInstance().m_file_path.utf8string()};
    const char* log_path{utf8_path.c_str()};

    // Use GetFileSize because fs::file_size may require a flush to be accurate.
    std::streamsize log_file_size{static_cast<std::streamsize>(GetFileSize(log_path))};

    for (int64_t i{0}; i < num_lines; ++i) {
        LogFromLocation(Location::FIRST, log_message);

Review comment (suggestion):
nit: I find setting `line_length{1024}` and documenting the subtraction more straightforward:

<details>
<summary>git diff on d0b3a80b7f</summary>

```diff
diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp
index 81c0bacba0..245940f284 100644
--- a/src/test/logging_tests.cpp
+++ b/src/test/logging_tests.cpp
@@ -414,11 +414,10 @@ BOOST_FIXTURE_TEST_CASE(logging_filesize_rate_limit, LogSetup)
     bool prev_log_threadnames{LogInstance().m_log_threadnames};
     LogInstance().m_log_threadnames = false;
 
-    int64_t line_length{1023};
+    int64_t line_length{1024};
     int64_t num_lines{1024};
 
-    // Add 1 to line_length because of newline.
-    int64_t bytes_quota{(line_length + 1) * num_lines};
+    int64_t bytes_quota{line_length * num_lines};
 
     std::chrono::seconds time_window{20};
 
@@ -430,7 +429,7 @@ BOOST_FIXTURE_TEST_CASE(logging_filesize_rate_limit, LogSetup)
     std::unique_ptr<BCLog::LogRateLimiter> limiter{std::make_unique<BCLog::LogRateLimiter>(sched_func, bytes_quota, time_window)};
     LogInstance().SetRateLimiting(std::move(limiter));
 
-    std::string log_message(line_length, 'a');
+    std::string log_message(line_length - 1, 'a'); // subtract one for newline
 
     std::string utf8_path{LogInstance().m_file_path.utf8string()};
     const char* log_path{utf8_path.c_str()};

```
</details>

Also, constants could be marked as such. Diff including the previous suggestion:

<details>
<summary>git diff on d0b3a80b7f</summary>

```diff
diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp
index 81c0bacba0..62c92287dd 100644
--- a/src/test/logging_tests.cpp
+++ b/src/test/logging_tests.cpp
@@ -407,20 +407,17 @@ void LogFromLocationAndExpect(Location location, const std::string& message, con
 
 BOOST_FIXTURE_TEST_CASE(logging_filesize_rate_limit, LogSetup)
 {
-    bool prev_log_timestamps{LogInstance().m_log_timestamps};
+    const bool prev_log_timestamps{LogInstance().m_log_timestamps};
     LogInstance().m_log_timestamps = false;
-    bool prev_log_sourcelocations{LogInstance().m_log_sourcelocations};
+    const bool prev_log_sourcelocations{LogInstance().m_log_sourcelocations};
     LogInstance().m_log_sourcelocations = false;
-    bool prev_log_threadnames{LogInstance().m_log_threadnames};
+    const bool prev_log_threadnames{LogInstance().m_log_threadnames};
     LogInstance().m_log_threadnames = false;
 
-    int64_t line_length{1023};
-    int64_t num_lines{1024};
-
-    // Add 1 to line_length because of newline.
-    int64_t bytes_quota{(line_length + 1) * num_lines};
-
-    std::chrono::seconds time_window{20};
+    constexpr int64_t line_length{1024};
+    constexpr int64_t num_lines{1024};
+    constexpr int64_t bytes_quota{line_length * num_lines};
+    constexpr std::chrono::seconds time_window{20};
 
     CScheduler scheduler{};
     scheduler.m_service_thread = std::thread([&] { scheduler.serviceQueue(); });
@@ -430,9 +427,8 @@ BOOST_FIXTURE_TEST_CASE(logging_filesize_rate_limit, LogSetup)
     std::unique_ptr<BCLog::LogRateLimiter> limiter{std::make_unique<BCLog::LogRateLimiter>(sched_func, bytes_quota, time_window)};
     LogInstance().SetRateLimiting(std::move(limiter));
 
-    std::string log_message(line_length, 'a');
-
-    std::string utf8_path{LogInstance().m_file_path.utf8string()};
+    const std::string log_message(line_length - 1, 'a'); // subtract one for newline
+    const std::string utf8_path{LogInstance().m_file_path.utf8string()};
     const char* log_path{utf8_path.c_str()};
 
     // Use GetFileSize because fs::file_size may require a flush to be accurate.

```
</details>


Code after:
};

void LogFromLocation(Location location, const std::string& message) {
    switch (location) {
    case Location::INFO_1:
        LogInfo("%s\n", message);
        return;
    case Location::INFO_2:
        LogInfo("%s\n", message);
        return;
    case Location::DEBUG_LOG:
        LogDebug(BCLog::LogFlags::HTTP, "%s\n", message);
        return;
    case Location::INFO_NOLIMIT:
        LogPrintLevel_(BCLog::LogFlags::ALL, BCLog::Level::Info, /*should_ratelimit=*/false, "%s\n", message);
        return;
    } // no default case, so the compiler can warn about missing cases
    assert(false);
}

/**
 * For a given `location` and `message`, ensure that the on-disk debug log behaviour resembles what
 * we'd expect it to be for `status` and `suppressions_active`.
 */
void TestLogFromLocation(Location location, const std::string& message,
                         BCLog::LogRateLimiter::Status status, bool suppressions_active,
                         std::source_location source = std::source_location::current())
{
    using Status = BCLog::LogRateLimiter::Status;
    if (!suppressions_active) assert(status == Status::UNSUPPRESSED); // developer error

    std::ofstream ofs(LogInstance().m_file_path, std::ios::out | std::ios::trunc); // clear debug log
    LogFromLocation(location, message);
    auto log_lines{ReadDebugLogLines()};

    BOOST_TEST_CONTEXT("TestLogFromLocation failed from " << source.file_name() << ":" << source.line())
    {
        if (status == Status::STILL_SUPPRESSED) {
            BOOST_CHECK_EQUAL(log_lines.size(), 0);
            return;
        }

        if (status == Status::NEWLY_SUPPRESSED) {
            BOOST_REQUIRE_EQUAL(log_lines.size(), 2);
            BOOST_CHECK(log_lines[0].starts_with("[*] [warning] Excessive logging detected"));
            log_lines.erase(log_lines.begin());
        }
        BOOST_REQUIRE_EQUAL(log_lines.size(), 1);
        auto& payload{log_lines.back()};
        BOOST_CHECK_EQUAL(suppressions_active, payload.starts_with("[*]"));
        BOOST_CHECK(payload.ends_with(message));