From 079cb01c10001aaaa3107413fa463c937005b48e Mon Sep 17 00:00:00 2001 From: Lev Kropp Date: Thu, 21 Nov 2024 18:35:38 -0500 Subject: [PATCH] Preserve original error messages in URLDownloader and update tests --- src/network/CMakeLists.txt | 1 + src/network/url_downloader.cpp | 42 +++++++++++++---------- tests/test_url_downloader.cpp | 61 +++++++++++++++++++++++++++------- 3 files changed, 74 insertions(+), 30 deletions(-) diff --git a/src/network/CMakeLists.txt b/src/network/CMakeLists.txt index a967bd2b5f..afafe9f79c 100644 --- a/src/network/CMakeLists.txt +++ b/src/network/CMakeLists.txt @@ -27,5 +27,6 @@ add_library(ip_address STATIC target_link_libraries(network fmt logger + utils Qt6::Core Qt6::Network) diff --git a/src/network/url_downloader.cpp b/src/network/url_downloader.cpp index fcf3fa44e4..d0f976c994 100644 --- a/src/network/url_downloader.cpp +++ b/src/network/url_downloader.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -108,35 +109,27 @@ download(QNetworkAccessManager* manager, const Time& timeout, QUrl const& url, P // Log the original error message at debug level mpl::log(mpl::Level::debug, category, - fmt::format("Qt error {}: {}", static_cast(error_code), error_string)); + fmt::format("Qt error {}: {}", mp::utils::qenum_to_string(error_code), error_string)); - const auto msg = (error_code == QNetworkReply::TimeoutError) ? "Network timeout" : error_string; - - if (error_code == QNetworkReply::TimeoutError) - { - // Log a debug message to verify our assumption about download_timeout - if (download_timeout.isActive()) - { - mpl::log(mpl::Level::debug, category, "Timeout error detected but download_timeout is still active"); - } - } + mpl::log(mpl::Level::debug, category, fmt::format("download_timeout is {}active", + download_timeout.isActive() ? "" : "in")); if (reply->error() == QNetworkReply::ProxyAuthenticationRequiredError || abort_download) { on_error(); - throw mp::AbortedDownloadException{msg}; + throw mp::AbortedDownloadException{error_string}; } if (cache_load_control == QNetworkRequest::CacheLoadControl::AlwaysCache) { on_error(); // Log at error level since we are giving up - mpl::log(mpl::Level::error, category, fmt::format("Failed to get {}: {}", url.toString(), msg)); - throw mp::DownloadException{url.toString().toStdString(), msg}; + mpl::log(mpl::Level::error, category, fmt::format("Failed to get {}: {}", url.toString(), error_string)); + throw mp::DownloadException{url.toString().toStdString(), error_string}; } // Log at warning level when we are going to retry mpl::log(mpl::Level::warning, category, - fmt::format("Failed to get {}: {} - trying cache.", url.toString(), msg)); + fmt::format("Failed to get {}: {} - trying cache.", url.toString(), error_string)); return ::download(manager, timeout, url, on_progress, on_download, on_error, abort_download, QNetworkRequest::CacheLoadControl::AlwaysCache); } @@ -163,15 +156,28 @@ auto get_header(QNetworkAccessManager* manager, const QUrl& url, const QNetworkR if (reply->error() != QNetworkReply::NoError) { - const auto msg = download_timeout.isActive() ? reply->errorString().toStdString() : "Network timeout"; + const auto error_code = reply->error(); + const auto error_string = reply->errorString().toStdString(); + + // Log the original error message at debug level + mpl::log(mpl::Level::debug, + category, + fmt::format("Qt error {}: {}", mp::utils::qenum_to_string(error_code), error_string)); - mpl::log(mpl::Level::warning, category, fmt::format("Cannot retrieve headers for {}: {}", url.toString(), msg)); + mpl::log(mpl::Level::debug, category, fmt::format("download_timeout is {}active", + download_timeout.isActive() ? "" : "in")); - throw mp::DownloadException{url.toString().toStdString(), reply->errorString().toStdString()}; + // Log at error level when we give up on getting headers + mpl::log(mpl::Level::error, + category, + fmt::format("Cannot retrieve headers for {}: {}", url.toString(), error_string)); + + throw mp::DownloadException{url.toString().toStdString(), error_string}; } return reply->header(header); } + } // namespace mp::NetworkManagerFactory::NetworkManagerFactory(const Singleton::PrivatePass& pass) noexcept diff --git a/tests/test_url_downloader.cpp b/tests/test_url_downloader.cpp index e153cf4138..e890236932 100644 --- a/tests/test_url_downloader.cpp +++ b/tests/test_url_downloader.cpp @@ -113,11 +113,17 @@ TEST_F(URLDownloader, simpleDownloadNetworkTimeoutTriesCache) return mock_reply_cache; }); - logger_scope.mock_logger->screen_logs(mpl::Level::trace); + logger_scope.mock_logger->screen_logs(mpl::Level::error); + + // Add expectations for the new debug log and the warning log + EXPECT_CALL(*logger_scope.mock_logger, + log(mpl::Level::debug, _, Property(&multipass::logging::CString::c_str, StartsWith("Qt error")))); + + logger_scope.mock_logger->expect_log( + mpl::Level::warning, + fmt::format("Failed to get {}: Operation canceled - trying cache.", fake_url.toString())); logger_scope.mock_logger->expect_log(mpl::Level::trace, fmt::format("Found {} in cache: true", fake_url.toString())); - logger_scope.mock_logger->expect_log( - mpl::Level::warning, fmt::format("Error getting {}: Network timeout - trying cache.", fake_url.toString())); mp::URLDownloader downloader(cache_dir.path(), 10ms); @@ -248,11 +254,15 @@ TEST_F(URLDownloader, fileDownloadErrorTriesCache) auto progress_monitor = [](auto...) { return true; }; - logger_scope.mock_logger->screen_logs(mpl::Level::trace); + logger_scope.mock_logger->screen_logs(mpl::Level::error); logger_scope.mock_logger->expect_log(mpl::Level::trace, fmt::format("Found {} in cache: true", fake_url.toString())); logger_scope.mock_logger->expect_log( - mpl::Level::warning, fmt::format("Error getting {}: Network timeout - trying cache.", fake_url.toString())); + mpl::Level::warning, + fmt::format("Failed to get {}: Operation canceled - trying cache.", fake_url.toString())); + + EXPECT_CALL(*logger_scope.mock_logger, + log(mpl::Level::debug, _, Property(&multipass::logging::CString::c_str, StartsWith("Qt error")))); mp::URLDownloader downloader(cache_dir.path(), 10ms); @@ -417,9 +427,27 @@ TEST_F(URLDownloader, fileDownloadTimeoutDoesNotWriteFile) auto progress_monitor = [](auto...) { return true; }; - logger_scope.mock_logger->screen_logs(mpl::Level::warning); - logger_scope.mock_logger->expect_log( - mpl::Level::warning, fmt::format("Error getting {}: Network timeout - trying cache.", fake_url.toString())); + logger_scope.mock_logger->screen_logs(mpl::Level::error); + + // Expect warning log for the first failed attempt + EXPECT_CALL(*logger_scope.mock_logger, + log(mpl::Level::warning, + _, + Property(&multipass::logging::CString::c_str, + HasSubstr(fmt::format("Failed to get {}: Operation canceled - trying cache.", + fake_url.toString()))))); + + // Expect error log for the second failed attempt + EXPECT_CALL(*logger_scope.mock_logger, + log(mpl::Level::error, + _, + Property(&multipass::logging::CString::c_str, + HasSubstr(fmt::format("Failed to get {}: Operation canceled", fake_url.toString()))))); + + // Expect two debug logs for each failure + EXPECT_CALL(*logger_scope.mock_logger, + log(mpl::Level::debug, _, Property(&multipass::logging::CString::c_str, StartsWith("Qt error")))) + .Times(2); mp::URLDownloader downloader(cache_dir.path(), 10ms); @@ -499,10 +527,14 @@ TEST_F(URLDownloader, lastModifiedHeaderTimeoutThrows) EXPECT_CALL(*mock_network_access_manager, createRequest(_, _, _)).WillOnce(Return(mock_reply)); - logger_scope.mock_logger->screen_logs(mpl::Level::warning); + logger_scope.mock_logger->screen_logs(mpl::Level::error); logger_scope.mock_logger->expect_log( - mpl::Level::warning, fmt::format("Cannot retrieve headers for {}: Network timeout", fake_url.toString())); + mpl::Level::error, + fmt::format("Cannot retrieve headers for {}: Operation canceled", fake_url.toString())); + // Expectation for debug log + EXPECT_CALL(*logger_scope.mock_logger, + log(mpl::Level::debug, _, Property(&multipass::logging::CString::c_str, StartsWith("Qt error")))); mp::URLDownloader downloader(cache_dir.path(), 10ms); EXPECT_THROW(downloader.last_modified(fake_url), mp::DownloadException); @@ -521,9 +553,14 @@ TEST_F(URLDownloader, lastModifiedHeaderErrorThrows) return mock_reply; }); - logger_scope.mock_logger->screen_logs(mpl::Level::warning); + logger_scope.mock_logger->screen_logs(mpl::Level::error); logger_scope.mock_logger->expect_log( - mpl::Level::warning, fmt::format("Cannot retrieve headers for {}: {}", fake_url.toString(), error_msg)); + mpl::Level::error, + fmt::format("Cannot retrieve headers for {}: {}", fake_url.toString(), error_msg)); + + // Expectation for debug log + EXPECT_CALL(*logger_scope.mock_logger, + log(mpl::Level::debug, _, Property(&multipass::logging::CString::c_str, StartsWith("Qt error")))); mp::URLDownloader downloader(cache_dir.path(), 1s);