From c9256b09096a1c4bb589a0e8bb30d0a4d2ad5105 Mon Sep 17 00:00:00 2001 From: Yuri Gorshenin Date: Tue, 16 Jun 2015 12:58:49 +0300 Subject: [PATCH] Review fixes. --- storage/map_files_downloader.hpp | 7 +- storage/storage.cpp | 1 - storage/storage.hpp | 13 +- .../fake_map_files_downloader.cpp | 30 +-- .../fake_map_files_downloader.hpp | 10 +- storage/storage_tests/storage_tests.cpp | 220 +++++++++++++----- storage/storage_tests/storage_tests.pro | 4 +- .../{message_loop.cpp => task_runner.cpp} | 11 +- .../{message_loop.hpp => task_runner.hpp} | 10 +- 9 files changed, 211 insertions(+), 95 deletions(-) rename storage/storage_tests/{message_loop.cpp => task_runner.cpp} (60%) rename storage/storage_tests/{message_loop.hpp => task_runner.hpp} (89%) diff --git a/storage/map_files_downloader.hpp b/storage/map_files_downloader.hpp index 5c888b1d78..ce0cf2fc1f 100644 --- a/storage/map_files_downloader.hpp +++ b/storage/map_files_downloader.hpp @@ -2,8 +2,8 @@ #include "std/function.hpp" #include "std/string.hpp" -#include "std/vector.hpp" #include "std/utility.hpp" +#include "std/vector.hpp" namespace storage { @@ -12,6 +12,7 @@ namespace storage class MapFilesDownloader { public: + // Denotes bytes downloaded and total number of bytes. using TProgress = pair; using TFileDownloadedCallback = function; @@ -20,12 +21,12 @@ public: virtual ~MapFilesDownloader() = default; - /// Asynchroniously receives a list of all servers that can be asked + /// Asynchronously receives a list of all servers that can be asked /// for a map file and invokes callback on the original thread. virtual void GetServersList(string const & mapFileName, TServersListCallback const & callback) = 0; - /// Asynchroniously downloads a map file, periodically invokes + /// Asynchronously downloads a map file, periodically invokes /// onProgress callback and finally invokes onDownloaded /// callback. Both callbacks will be invoked on the original thread. virtual void DownloadMapFile(vector const & urls, string const & path, int64_t size, diff --git a/storage/storage.cpp b/storage/storage.cpp index b6557bad02..3d432e69c4 100644 --- a/storage/storage.cpp +++ b/storage/storage.cpp @@ -174,7 +174,6 @@ namespace storage return m_pFile->GetFileWithExt(TMapOptions::EMap); } - Storage::Storage() : m_downloader(new HttpMapFilesDownloader()), m_currentSlotId(0) { LoadCountriesFile(false); diff --git a/storage/storage.hpp b/storage/storage.hpp index 2ccdd53d6d..efdc07f4c6 100644 --- a/storage/storage.hpp +++ b/storage/storage.hpp @@ -98,13 +98,20 @@ namespace storage void ReportProgress(TIndex const & index, pair const & p); - /// @name - //@{ + /// Called on the main thread by MapFilesDownloader when list of + /// suitable servers is received. void OnServerListDownloaded(vector const & urls); + + /// Called on the main thread by MapFilesDownloader when + /// downloading of a map file succeeds/fails. void OnMapDownloadFinished(bool success, MapFilesDownloader::TProgress const & progress); + + /// Periodically called on the main thread by MapFilesDownloader + /// during the downloading process. void OnMapDownloadProgress(MapFilesDownloader::TProgress const & progress); + + /// Initiates downloading of the next file from the queue. void DownloadNextFile(QueuedCountry const & cnt); - //@} public: Storage(); diff --git a/storage/storage_tests/fake_map_files_downloader.cpp b/storage/storage_tests/fake_map_files_downloader.cpp index bfd116f6a4..0e72edde1b 100644 --- a/storage/storage_tests/fake_map_files_downloader.cpp +++ b/storage/storage_tests/fake_map_files_downloader.cpp @@ -1,11 +1,14 @@ #include "storage/storage_tests/fake_map_files_downloader.hpp" -#include "storage/storage_tests/message_loop.hpp" +#include "storage/storage_tests/task_runner.hpp" #include "coding/file_writer.hpp" + #include "base/assert.hpp" #include "base/scope_guard.hpp" + #include "std/algorithm.hpp" +#include "std/bind.hpp" namespace storage { @@ -14,24 +17,21 @@ namespace int64_t const kBlockSize = 1024 * 1024; } // namespace -FakeMapFilesDownloader::FakeMapFilesDownloader(MessageLoop & loop) - : m_progress(make_pair(0, 0)), m_idle(true), m_loop(loop) +FakeMapFilesDownloader::FakeMapFilesDownloader(TaskRunner & taskRunner) + : m_progress(make_pair(0, 0)), m_idle(true), m_taskRunner(taskRunner) { m_servers.push_back("http://test-url/"); } -FakeMapFilesDownloader::~FakeMapFilesDownloader() -{ - CHECK(m_checker.CalledOnOriginalThread(), ()); -} +FakeMapFilesDownloader::~FakeMapFilesDownloader() { CHECK(m_checker.CalledOnOriginalThread(), ()); } void FakeMapFilesDownloader::GetServersList(string const & mapFileName, TServersListCallback const & callback) { CHECK(m_checker.CalledOnOriginalThread(), ()); m_idle = false; - MY_SCOPE_GUARD(resetIdle, [this]() { m_idle = true; }); - m_loop.PostTask(bind(callback, m_servers)); + MY_SCOPE_GUARD(resetIdle, bind(&FakeMapFilesDownloader::Reset, this)); + m_taskRunner.PostTask(bind(callback, m_servers)); } void FakeMapFilesDownloader::DownloadMapFile(vector const & urls, string const & path, @@ -44,7 +44,7 @@ void FakeMapFilesDownloader::DownloadMapFile(vector const & urls, string m_progress.first = 0; m_progress.second = size; m_idle = false; - MY_SCOPE_GUARD(resetIdle, [this]() { m_idle = true; }); + MY_SCOPE_GUARD(resetIdle, bind(&FakeMapFilesDownloader::Reset, this)); { FileWriter writer(path); @@ -54,10 +54,10 @@ void FakeMapFilesDownloader::DownloadMapFile(vector const & urls, string writer.Write(kZeroes.data(), blockSize); size -= blockSize; m_progress.first += blockSize; - m_loop.PostTask(bind(onProgress, m_progress)); + m_taskRunner.PostTask(bind(onProgress, m_progress)); } } - m_loop.PostTask(bind(onDownloaded, true /* success */, m_progress)); + m_taskRunner.PostTask(bind(onDownloaded, true /* success */, m_progress)); } MapFilesDownloader::TProgress FakeMapFilesDownloader::GetDownloadingProgress() @@ -65,12 +65,14 @@ MapFilesDownloader::TProgress FakeMapFilesDownloader::GetDownloadingProgress() return m_progress; } -bool FakeMapFilesDownloader::IsIdle() { +bool FakeMapFilesDownloader::IsIdle() +{ CHECK(m_checker.CalledOnOriginalThread(), ()); return m_idle; } -void FakeMapFilesDownloader::Reset() { +void FakeMapFilesDownloader::Reset() +{ CHECK(m_checker.CalledOnOriginalThread(), ()); m_idle = true; } diff --git a/storage/storage_tests/fake_map_files_downloader.hpp b/storage/storage_tests/fake_map_files_downloader.hpp index fb9ec9f19f..ae544db595 100644 --- a/storage/storage_tests/fake_map_files_downloader.hpp +++ b/storage/storage_tests/fake_map_files_downloader.hpp @@ -5,19 +5,19 @@ namespace storage { -class MessageLoop; +class TaskRunner; // This class can be used in tests to mimic a real downloader. It // always returns a single URL for map files downloading and when // asked for a file, creates a file with zero-bytes content on a disk. -// Because all callbacks must be invoked asynchroniously, it needs a +// Because all callbacks must be invoked asynchronously, it needs a // single-thread message loop runner to run callbacks. // // *NOTE*, this class is not thread-safe. class FakeMapFilesDownloader : public MapFilesDownloader { public: - FakeMapFilesDownloader(MessageLoop & loop); + FakeMapFilesDownloader(TaskRunner & taskRunner); virtual ~FakeMapFilesDownloader(); // MapFilesDownloader overrides: @@ -29,12 +29,12 @@ public: bool IsIdle() override; void Reset() override; - private: +private: vector m_servers; TProgress m_progress; bool m_idle; - MessageLoop & m_loop; + TaskRunner & m_taskRunner; ThreadChecker m_checker; }; } // namespace storage diff --git a/storage/storage_tests/storage_tests.cpp b/storage/storage_tests/storage_tests.cpp index e66be7889a..5ce8f52b68 100644 --- a/storage/storage_tests/storage_tests.cpp +++ b/storage/storage_tests/storage_tests.cpp @@ -3,7 +3,7 @@ #include "storage/storage.hpp" #include "storage/storage_defines.hpp" #include "storage/storage_tests/fake_map_files_downloader.hpp" -#include "storage/storage_tests/message_loop.hpp" +#include "storage/storage_tests/task_runner.hpp" #include "platform/platform.hpp" @@ -22,33 +22,45 @@ using namespace storage; namespace { -// This class checks steps Storage::DownloadMap() performs to download a map. class CountryDownloaderChecker { public: - CountryDownloaderChecker(Storage & storage, string const & countryFileName) + CountryDownloaderChecker(Storage & storage, string const & countryFileName, TMapOptions files) : m_storage(storage), m_index(m_storage.FindIndexByFile(countryFileName)), + m_files(files), m_lastStatus(TStatus::ENotDownloaded), m_bytesDownloaded(0), - m_totalBytesToDownload(-1) + m_totalBytesToDownload(0), + m_slot(0) { - CHECK(m_index.IsValid(), ()); - CHECK_EQUAL(m_lastStatus, m_storage.CountryStatusEx(m_index), ()); m_slot = m_storage.Subscribe( bind(&CountryDownloaderChecker::OnCountryStatusChanged, this, _1), bind(&CountryDownloaderChecker::OnCountryDownloadingProgress, this, _1, _2)); - - m_storage.DownloadCountry(m_index, TMapOptions::EMap); + CHECK(m_index.IsValid(), ()); } - ~CountryDownloaderChecker() + void StartDownload() { + CHECK_EQUAL(m_lastStatus, m_storage.CountryStatusEx(m_index), ()); + m_storage.DownloadCountry(m_index, m_files); + } + + virtual ~CountryDownloaderChecker() + { + m_storage.Unsubscribe(m_slot); + CHECK_EQUAL(TStatus::EOnDisk, m_lastStatus, ()); CHECK_EQUAL(m_bytesDownloaded, m_totalBytesToDownload, ()); - m_storage.Unsubscribe(m_slot); + + LocalAndRemoteSizeT localAndRemoteSize = m_storage.CountrySizeInBytes(m_index, m_files); + CHECK_EQUAL(m_bytesDownloaded, localAndRemoteSize.first, ()); + CHECK_EQUAL(m_totalBytesToDownload, localAndRemoteSize.second, ()); } +protected: + virtual void CheckStatusTransition(TStatus oldStatus, TStatus newStatus) const = 0; + private: void OnCountryStatusChanged(TIndex const & index) { @@ -56,25 +68,12 @@ private: return; TStatus status = m_storage.CountryStatusEx(m_index); - LOG(LINFO, ("OnCountryStatusChanged", status)); - switch (status) + CheckStatusTransition(m_lastStatus, status); + if (status == TStatus::EDownloading) { - case TStatus::EDownloading: - CHECK(m_lastStatus == TStatus::EDownloading || m_lastStatus == TStatus::ENotDownloaded, - ("It's only possible to move from {Downloading|NotDownloaded} to Downloading," - "old status:", - m_lastStatus, ", new status:", status)); - break; - case TStatus::EOnDisk: - CHECK(m_lastStatus == TStatus::EDownloading || m_lastStatus == TStatus::ENotDownloaded, - ("It's only possible to move from {Downloading|NotDownloaded} to OnDisk,", - "old status:", m_lastStatus, ", new status:", status)); - CHECK_EQUAL(m_totalBytesToDownload, m_bytesDownloaded, ()); - break; - default: - CHECK(false, ("Unknown state change: from", m_lastStatus, "to", status)); + LocalAndRemoteSizeT localAndRemoteSize = m_storage.CountrySizeInBytes(m_index, m_files); + m_totalBytesToDownload = localAndRemoteSize.second; } - m_lastStatus = status; } @@ -82,30 +81,119 @@ private: { if (index != m_index) return; - LOG(LINFO, ("OnCountryDownloadingProgress:", progress)); + CHECK_GREATER(progress.first, m_bytesDownloaded, ()); m_bytesDownloaded = progress.first; - m_totalBytesToDownload = progress.second; CHECK_LESS_OR_EQUAL(m_bytesDownloaded, m_totalBytesToDownload, ()); + + LocalAndRemoteSizeT localAndRemoteSize = m_storage.CountrySizeInBytes(m_index, m_files); + CHECK_EQUAL(m_totalBytesToDownload, localAndRemoteSize.second, ()); } Storage & m_storage; TIndex const m_index; - TStatus m_lastStatus; - int m_slot; + TMapOptions const m_files; + TStatus m_lastStatus; int64_t m_bytesDownloaded; int64_t m_totalBytesToDownload; + int m_slot; }; +// Checks following state transitions: +// NotDownloaded -> Downloading -> OnDisk. +class AbsentCountryDownloaderChecker : public CountryDownloaderChecker +{ +public: + AbsentCountryDownloaderChecker(Storage & storage, string const & countryFileName, + TMapOptions files) + : CountryDownloaderChecker(storage, countryFileName, files) + { + } + + ~AbsentCountryDownloaderChecker() override = default; + +protected: + void CheckStatusTransition(TStatus oldStatus, TStatus newStatus) const override + { + switch (newStatus) + { + case TStatus::EDownloading: + CHECK_EQUAL(oldStatus, TStatus::ENotDownloaded, + ("It's only possible to move from NotDownloaded to Downloading.")); + break; + case TStatus::EOnDisk: + CHECK_EQUAL(oldStatus, TStatus::EDownloading, + ("It's only possible to move from Downloading to OnDisk.")); + break; + default: + CHECK(false, ("Unknown state change: from", oldStatus, "to", newStatus)); + } + } +}; + +// Checks following state transitions: +// NotDownloaded -> InQueue -> Downloading -> OnDisk. +class QueuedCountryDownloaderChecker : public CountryDownloaderChecker +{ +public: + QueuedCountryDownloaderChecker(Storage & storage, string const & countryFileName, + TMapOptions files) + : CountryDownloaderChecker(storage, countryFileName, files) + { + } + + ~QueuedCountryDownloaderChecker() override = default; + +protected: + void CheckStatusTransition(TStatus oldStatus, TStatus newStatus) const override + { + switch (newStatus) + { + case TStatus::EInQueue: + CHECK_EQUAL(oldStatus, TStatus::ENotDownloaded, + ("It's only possible to move from NotDownloaded to InQueue.")); + break; + case TStatus::EDownloading: + CHECK_EQUAL(oldStatus, TStatus::EInQueue, + ("It's only possible to move from InQueue to Downloading.")); + break; + case TStatus::EOnDisk: + CHECK_EQUAL(oldStatus, TStatus::EDownloading, + ("It's only possible to move from Downloading to OnDisk.")); + break; + default: + CHECK(false, ("Unknown state change: from", oldStatus, "to", newStatus)); + } + } +}; + +// Removes country's files that can be created during and after downloading. +void CleanupCountryFiles(string const & countryFileName) +{ + Platform & platform = GetPlatform(); + + string const localMapFile = + my::JoinFoldersToPath(platform.WritableDir(), countryFileName + DATA_FILE_EXTENSION); + my::DeleteFileX(localMapFile); + my::DeleteFileX(localMapFile + READY_FILE_EXTENSION); + + string const localRoutingFile = localMapFile + ROUTING_FILE_EXTENSION; + my::DeleteFileX(localRoutingFile); + my::DeleteFileX(localRoutingFile + READY_FILE_EXTENSION); +} + void OnCountryDownloaded(string const & mapFileName, TMapOptions files) { - LOG(LINFO, ("OnCountryDownloaded:", mapFileName, static_cast(files))); Platform & platform = GetPlatform(); - CHECK(my::RenameFileX( - my::JoinFoldersToPath(platform.WritableDir(), mapFileName + READY_FILE_EXTENSION), - my::JoinFoldersToPath(platform.WritableDir(), mapFileName)), - ()); + + string const localMapFile = my::JoinFoldersToPath(platform.WritableDir(), mapFileName); + string const localRoutingFile = localMapFile + ROUTING_FILE_EXTENSION; + + if (files & TMapOptions::EMap) + CHECK(my::RenameFileX(localMapFile + READY_FILE_EXTENSION, localMapFile), ()); + if (files & TMapOptions::ECarRouting) + CHECK(my::RenameFileX(localRoutingFile + READY_FILE_EXTENSION, localRoutingFile), ()); } } // namespace @@ -126,31 +214,53 @@ UNIT_TEST(StorageTest_Smoke) TEST_NOT_EQUAL(i1, i2, ()); } -UNIT_TEST(StorageTest_Download) +UNIT_TEST(StorageTest_SingleCountryDownloading) { - Platform & platform = GetPlatform(); - - string const countryFileName = "Azerbaijan"; - string const localFileName = - my::JoinFoldersToPath(platform.WritableDir(), countryFileName + DATA_FILE_EXTENSION); - string const downloadedFileName = my::JoinFoldersToPath( - platform.WritableDir(), countryFileName + DATA_FILE_EXTENSION + READY_FILE_EXTENSION); - - TEST(!Platform::IsFileExistsByFullPath(localFileName), - ("Please, remove", localFileName, "before running the test.")); - TEST(!Platform::IsFileExistsByFullPath(downloadedFileName), - ("Please, remove", downloadedFileName, "before running the test.")); - MY_SCOPE_GUARD(removeLocalFile, bind(&FileWriter::DeleteFileX, localFileName)); - MY_SCOPE_GUARD(removeDownloadedFile, bind(&FileWriter::DeleteFileX, downloadedFileName)); + string const azerbaijanFileName = "Azerbaijan"; + CleanupCountryFiles(azerbaijanFileName); Storage storage; storage.Init(&OnCountryDownloaded); - MessageLoop loop; - storage.SetDownloaderForTesting(make_unique(loop)); + TaskRunner taskRunner; + storage.SetDownloaderForTesting(make_unique(taskRunner)); { - CountryDownloaderChecker checker(storage, countryFileName); - loop.Run(); + MY_SCOPE_GUARD(cleanupCountryFiles, bind(&CleanupCountryFiles, azerbaijanFileName)); + AbsentCountryDownloaderChecker checker(storage, azerbaijanFileName, TMapOptions::EMap); + checker.StartDownload(); + taskRunner.Run(); + } + + { + MY_SCOPE_GUARD(cleanupCountryFiles, bind(&CleanupCountryFiles, azerbaijanFileName)); + AbsentCountryDownloaderChecker checker(storage, azerbaijanFileName, + TMapOptions::EMapWithCarRouting); + checker.StartDownload(); + taskRunner.Run(); } } + +UNIT_TEST(StorageTest_TwoCountriesDownloading) +{ + string const uruguayFileName = "Uruguay"; + string const venezuelaFileName = "Venezuela"; + CleanupCountryFiles(uruguayFileName); + MY_SCOPE_GUARD(cleanupUruguayFiles, bind(&CleanupCountryFiles, uruguayFileName)); + + CleanupCountryFiles(venezuelaFileName); + MY_SCOPE_GUARD(cleanupVenezuelaFiles, bind(&CleanupCountryFiles, venezuelaFileName)); + + Storage storage; + storage.Init(&OnCountryDownloaded); + + TaskRunner taskRunner; + storage.SetDownloaderForTesting(make_unique(taskRunner)); + + AbsentCountryDownloaderChecker uruguayChecker(storage, uruguayFileName, TMapOptions::EMap); + QueuedCountryDownloaderChecker venezuelaChecker(storage, venezuelaFileName, + TMapOptions::EMapWithCarRouting); + uruguayChecker.StartDownload(); + venezuelaChecker.StartDownload(); + taskRunner.Run(); +} diff --git a/storage/storage_tests/storage_tests.pro b/storage/storage_tests/storage_tests.pro index c0394a310b..86879502d5 100644 --- a/storage/storage_tests/storage_tests.pro +++ b/storage/storage_tests/storage_tests.pro @@ -19,13 +19,13 @@ QT *= core HEADERS += \ fake_map_files_downloader.hpp \ - message_loop.hpp \ + task_runner.hpp \ SOURCES += \ ../../testing/testingmain.cpp \ country_info_test.cpp \ fake_map_files_downloader.cpp \ - message_loop.cpp \ simple_tree_test.cpp \ storage_tests.cpp \ + task_runner.cpp \ diff --git a/storage/storage_tests/message_loop.cpp b/storage/storage_tests/task_runner.cpp similarity index 60% rename from storage/storage_tests/message_loop.cpp rename to storage/storage_tests/task_runner.cpp index da91b22341..a78572c3d7 100644 --- a/storage/storage_tests/message_loop.cpp +++ b/storage/storage_tests/task_runner.cpp @@ -1,15 +1,12 @@ -#include "storage/storage_tests/message_loop.hpp" +#include "storage/storage_tests/task_runner.hpp" #include "base/assert.hpp" namespace storage { -MessageLoop::~MessageLoop() -{ - CHECK(m_checker.CalledOnOriginalThread(), ()); -} +TaskRunner::~TaskRunner() { CHECK(m_checker.CalledOnOriginalThread(), ()); } -void MessageLoop::Run() +void TaskRunner::Run() { CHECK(m_checker.CalledOnOriginalThread(), ()); while (!m_tasks.empty()) @@ -20,7 +17,7 @@ void MessageLoop::Run() } } -void MessageLoop::PostTask(TTask const & task) +void TaskRunner::PostTask(TTask const & task) { CHECK(m_checker.CalledOnOriginalThread(), ()); m_tasks.push(task); diff --git a/storage/storage_tests/message_loop.hpp b/storage/storage_tests/task_runner.hpp similarity index 89% rename from storage/storage_tests/message_loop.hpp rename to storage/storage_tests/task_runner.hpp index b8e9eab1b4..4acd7a533c 100644 --- a/storage/storage_tests/message_loop.hpp +++ b/storage/storage_tests/task_runner.hpp @@ -14,19 +14,19 @@ namespace storage // message loop where B is run. // // *NOTE*, this class is not thread-safe. -class MessageLoop +class TaskRunner { - public: +public: using TTask = function; - ~MessageLoop(); + ~TaskRunner(); void Run(); void PostTask(TTask const & task); - private: +private: queue m_tasks; ThreadChecker m_checker; }; -} // namespace +} // namespace storage