ntp_tiles: Add tests covering tile-fetching logic
The previous tests only covered a tiny part of the component, which is
the merging logic, but not the actual combinations of events that can
happen during the fetching of tiles from the backends (top sites,
suggestions service, popular sites).
Mocks are used extensively due to the lack of fakes, and because the
interfaces are really big. It's somewhat painful to read, but
alternatives are not better.
The tests surface one bug which is not addressed here, left more a
follow-up patch and represented with a TODO.
Popular Sites fetching is still not covered by these tests, and
another TODO is added for this purpose.
BUG=619584
Review-Url: https://codereview.chromium.org/2579813002
Cr-Commit-Position: refs/heads/master@{#439335}
diff --git a/components/ntp_tiles/most_visited_sites.cc b/components/ntp_tiles/most_visited_sites.cc
index 35bfd44..376c4e45 100644
--- a/components/ntp_tiles/most_visited_sites.cc
+++ b/components/ntp_tiles/most_visited_sites.cc
@@ -187,6 +187,8 @@
void MostVisitedSites::OnMostVisitedURLsAvailable(
const history::MostVisitedURLList& visited_list) {
+ // TODO(mastiz): Verify if suggestion service results have been fetched in
+ // the meantime, and if that's the case ignore this event.
NTPTilesVector tiles;
size_t num_tiles =
std::min(visited_list.size(), static_cast<size_t>(num_sites_));
diff --git a/components/ntp_tiles/most_visited_sites_unittest.cc b/components/ntp_tiles/most_visited_sites_unittest.cc
index 029968df..bc7f877 100644
--- a/components/ntp_tiles/most_visited_sites_unittest.cc
+++ b/components/ntp_tiles/most_visited_sites_unittest.cc
@@ -10,9 +10,17 @@
#include <ostream>
#include <vector>
+#include "base/callback_list.h"
+#include "base/command_line.h"
#include "base/macros.h"
#include "base/memory/ptr_util.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
#include "base/strings/utf_string_conversions.h"
+#include "components/history/core/browser/top_sites.h"
+#include "components/ntp_tiles/icon_cacher.h"
+#include "components/ntp_tiles/switches.h"
+#include "components/sync_preferences/testing_pref_service_syncable.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
@@ -26,7 +34,23 @@
namespace {
+using history::MostVisitedURL;
+using history::MostVisitedURLList;
+using history::TopSites;
+using suggestions::ChromeSuggestion;
+using suggestions::SuggestionsProfile;
+using suggestions::SuggestionsService;
+using testing::AtLeast;
+using testing::ByMove;
using testing::ElementsAre;
+using testing::InSequence;
+using testing::Invoke;
+using testing::Mock;
+using testing::Return;
+using testing::ReturnRef;
+using testing::SizeIs;
+using testing::StrictMock;
+using testing::_;
MATCHER_P3(MatchesTile,
title,
@@ -50,6 +74,392 @@
return tile;
}
+ChromeSuggestion MakeSuggestion(const std::string& title,
+ const std::string& url) {
+ ChromeSuggestion suggestion;
+ suggestion.set_title(title);
+ suggestion.set_url(url);
+ return suggestion;
+}
+
+SuggestionsProfile MakeProfile(
+ const std::vector<ChromeSuggestion>& suggestions) {
+ SuggestionsProfile profile;
+ for (const ChromeSuggestion& suggestion : suggestions) {
+ *profile.add_suggestions() = suggestion;
+ }
+ return profile;
+}
+
+MostVisitedURL MakeMostVisitedURL(const std::string& title,
+ const std::string& url) {
+ MostVisitedURL result;
+ result.title = base::ASCIIToUTF16(title);
+ result.url = GURL(url);
+ return result;
+}
+
+class MockTopSites : public TopSites {
+ public:
+ MOCK_METHOD0(ShutdownOnUIThread, void());
+ MOCK_METHOD3(SetPageThumbnail,
+ bool(const GURL& url,
+ const gfx::Image& thumbnail,
+ const ThumbnailScore& score));
+ MOCK_METHOD3(SetPageThumbnailToJPEGBytes,
+ bool(const GURL& url,
+ const base::RefCountedMemory* memory,
+ const ThumbnailScore& score));
+ MOCK_METHOD2(GetMostVisitedURLs,
+ void(const GetMostVisitedURLsCallback& callback,
+ bool include_forced_urls));
+ MOCK_METHOD3(GetPageThumbnail,
+ bool(const GURL& url,
+ bool prefix_match,
+ scoped_refptr<base::RefCountedMemory>* bytes));
+ MOCK_METHOD2(GetPageThumbnailScore,
+ bool(const GURL& url, ThumbnailScore* score));
+ MOCK_METHOD2(GetTemporaryPageThumbnailScore,
+ bool(const GURL& url, ThumbnailScore* score));
+ MOCK_METHOD0(SyncWithHistory, void());
+ MOCK_CONST_METHOD0(HasBlacklistedItems, bool());
+ MOCK_METHOD1(AddBlacklistedURL, void(const GURL& url));
+ MOCK_METHOD1(RemoveBlacklistedURL, void(const GURL& url));
+ MOCK_METHOD1(IsBlacklisted, bool(const GURL& url));
+ MOCK_METHOD0(ClearBlacklistedURLs, void());
+ MOCK_METHOD0(StartQueryForMostVisited, base::CancelableTaskTracker::TaskId());
+ MOCK_METHOD1(IsKnownURL, bool(const GURL& url));
+ MOCK_CONST_METHOD1(GetCanonicalURLString,
+ const std::string&(const GURL& url));
+ MOCK_METHOD0(IsNonForcedFull, bool());
+ MOCK_METHOD0(IsForcedFull, bool());
+ MOCK_CONST_METHOD0(loaded, bool());
+ MOCK_METHOD0(GetPrepopulatedPages, history::PrepopulatedPageList());
+ MOCK_METHOD2(AddForcedURL, bool(const GURL& url, const base::Time& time));
+ MOCK_METHOD1(OnNavigationCommitted, void(const GURL& url));
+
+ protected:
+ ~MockTopSites() override = default;
+};
+
+class MockSuggestionsService : public SuggestionsService {
+ public:
+ MOCK_METHOD0(FetchSuggestionsData, bool());
+ MOCK_CONST_METHOD0(GetSuggestionsDataFromCache,
+ base::Optional<SuggestionsProfile>());
+ MOCK_METHOD1(AddCallback,
+ std::unique_ptr<ResponseCallbackList::Subscription>(
+ const ResponseCallback& callback));
+ MOCK_METHOD2(GetPageThumbnail,
+ void(const GURL& url, const BitmapCallback& callback));
+ MOCK_METHOD3(GetPageThumbnailWithURL,
+ void(const GURL& url,
+ const GURL& thumbnail_url,
+ const BitmapCallback& callback));
+ MOCK_METHOD1(BlacklistURL, bool(const GURL& candidate_url));
+ MOCK_METHOD1(UndoBlacklistURL, bool(const GURL& url));
+ MOCK_METHOD0(ClearBlacklist, void());
+};
+
+class MockPopularSites : public PopularSites {
+ public:
+ MOCK_METHOD2(StartFetch,
+ void(bool force_download, const FinishedCallback& callback));
+ MOCK_CONST_METHOD0(sites, const SitesVector&());
+ MOCK_CONST_METHOD0(GetLastURLFetched, GURL());
+ MOCK_CONST_METHOD0(local_path, const base::FilePath&());
+ MOCK_METHOD0(GetURLToFetch, GURL());
+ MOCK_METHOD0(GetCountryToFetch, std::string());
+ MOCK_METHOD0(GetVersionToFetch, std::string());
+};
+
+class MockMostVisitedSitesObserver : public MostVisitedSites::Observer {
+ public:
+ MOCK_METHOD1(OnMostVisitedURLsAvailable, void(const NTPTilesVector& tiles));
+ MOCK_METHOD1(OnIconMadeAvailable, void(const GURL& site_url));
+};
+
+// CallbackList-like container without Subscription, mimic-ing the
+// implementation in TopSites (which doesn't use base::CallbackList).
+class TopSitesCallbackList {
+ public:
+ // Second argument declared to match the signature of GetMostVisitedURLs().
+ void Add(const TopSites::GetMostVisitedURLsCallback& callback,
+ testing::Unused) {
+ callbacks_.push_back(callback);
+ }
+
+ void ClearAndNotify(const MostVisitedURLList& list) {
+ std::vector<TopSites::GetMostVisitedURLsCallback> callbacks;
+ callbacks.swap(callbacks_);
+ for (const auto& callback : callbacks) {
+ callback.Run(list);
+ }
+ }
+
+ bool empty() const { return callbacks_.empty(); }
+
+ private:
+ std::vector<TopSites::GetMostVisitedURLsCallback> callbacks_;
+};
+
+class MostVisitedSitesTest : public ::testing::Test {
+ protected:
+ MostVisitedSitesTest()
+ : mock_top_sites_(new StrictMock<MockTopSites>()),
+ mock_popular_sites_(new StrictMock<MockPopularSites>()),
+ most_visited_sites_(&pref_service_,
+ mock_top_sites_,
+ &mock_suggestions_service_,
+ base::WrapUnique(mock_popular_sites_),
+ /*icon_cacher=*/nullptr,
+ /*supervisor=*/nullptr) {
+ MostVisitedSites::RegisterProfilePrefs(pref_service_.registry());
+ // TODO(mastiz): Add test coverage including Popular Sites.
+ base::CommandLine::ForCurrentProcess()->AppendSwitch(
+ switches::kDisableNTPPopularSites);
+ // PopularSites::sites() might be called even if the feature is disabled.
+ // An empty vector is returned because there was no actual fetch.
+ EXPECT_CALL(*mock_popular_sites_, sites())
+ .Times(AtLeast(0))
+ .WillRepeatedly(ReturnRef(empty_popular_sites_vector_));
+ }
+
+ bool VerifyAndClearExpectations() {
+ base::RunLoop().RunUntilIdle();
+ // Note that we don't verify or clear mock_popular_sites_, since there's
+ // no interaction expected except sites() possibly being called, which is
+ // verified during teardown.
+ return Mock::VerifyAndClearExpectations(mock_top_sites_.get()) &&
+ Mock::VerifyAndClearExpectations(&mock_suggestions_service_) &&
+ Mock::VerifyAndClearExpectations(&mock_observer_);
+ }
+
+ base::CallbackList<SuggestionsService::ResponseCallback::RunType>
+ suggestions_service_callbacks_;
+ TopSitesCallbackList top_sites_callbacks_;
+
+ base::MessageLoop message_loop_;
+ sync_preferences::TestingPrefServiceSyncable pref_service_;
+ scoped_refptr<StrictMock<MockTopSites>> mock_top_sites_;
+ StrictMock<MockSuggestionsService> mock_suggestions_service_;
+ StrictMock<MockPopularSites>* const mock_popular_sites_;
+ StrictMock<MockMostVisitedSitesObserver> mock_observer_;
+ MostVisitedSites most_visited_sites_;
+ const PopularSites::SitesVector empty_popular_sites_vector_;
+};
+
+TEST_F(MostVisitedSitesTest, ShouldStartNoCallInConstructor) {
+ // No call to mocks expected by the mere fact of instantiating
+ // MostVisitedSites.
+ base::RunLoop().RunUntilIdle();
+}
+
+class MostVisitedSitesWithCacheHitTest : public MostVisitedSitesTest {
+ public:
+ // Constructor sets the common expectations for the case where suggestions
+ // service has cached results when the observer is registered.
+ MostVisitedSitesWithCacheHitTest() {
+ InSequence seq;
+ EXPECT_CALL(*mock_top_sites_, SyncWithHistory());
+ EXPECT_CALL(mock_suggestions_service_, AddCallback(_))
+ .WillOnce(Invoke(&suggestions_service_callbacks_,
+ &SuggestionsService::ResponseCallbackList::Add));
+ EXPECT_CALL(mock_suggestions_service_, GetSuggestionsDataFromCache())
+ .WillOnce(Return(MakeProfile({
+ MakeSuggestion("Site 1", "http://site1/"),
+ MakeSuggestion("Site 2", "http://site2/"),
+ MakeSuggestion("Site 3", "http://site3/"),
+ MakeSuggestion("Site 4", "http://site4/"),
+ })));
+ EXPECT_CALL(mock_observer_,
+ OnMostVisitedURLsAvailable(ElementsAre(
+ MatchesTile("Site 1", "http://site1/",
+ NTPTileSource::SUGGESTIONS_SERVICE),
+ MatchesTile("Site 2", "http://site2/",
+ NTPTileSource::SUGGESTIONS_SERVICE),
+ MatchesTile("Site 3", "http://site3/",
+ NTPTileSource::SUGGESTIONS_SERVICE))));
+ EXPECT_CALL(mock_suggestions_service_, FetchSuggestionsData())
+ .WillOnce(Return(true));
+ most_visited_sites_.SetMostVisitedURLsObserver(&mock_observer_,
+ /*num_sites=*/3);
+ VerifyAndClearExpectations();
+
+ EXPECT_FALSE(suggestions_service_callbacks_.empty());
+ EXPECT_TRUE(top_sites_callbacks_.empty());
+ }
+};
+
+TEST_F(MostVisitedSitesWithCacheHitTest, ShouldFavorSuggestionsServiceCache) {
+ // Constructor sets basic expectations for a suggestions service cache hit.
+}
+
+TEST_F(MostVisitedSitesWithCacheHitTest,
+ ShouldPropagateUpdateBySuggestionsService) {
+ EXPECT_CALL(
+ mock_observer_,
+ OnMostVisitedURLsAvailable(ElementsAre(MatchesTile(
+ "Site 5", "http://site5/", NTPTileSource::SUGGESTIONS_SERVICE))));
+ suggestions_service_callbacks_.Notify(
+ MakeProfile({MakeSuggestion("Site 5", "http://site5/")}));
+ base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(MostVisitedSitesWithCacheHitTest,
+ ShouldSwitchToTopSitesIfEmptyUpdateBySuggestionsService) {
+ EXPECT_CALL(*mock_top_sites_, GetMostVisitedURLs(_, false))
+ .WillOnce(Invoke(&top_sites_callbacks_, &TopSitesCallbackList::Add));
+ suggestions_service_callbacks_.Notify(SuggestionsProfile());
+ VerifyAndClearExpectations();
+
+ EXPECT_CALL(mock_observer_,
+ OnMostVisitedURLsAvailable(ElementsAre(MatchesTile(
+ "Site 5", "http://site5/", NTPTileSource::TOP_SITES))));
+ top_sites_callbacks_.ClearAndNotify(
+ {MakeMostVisitedURL("Site 5", "http://site5/")});
+ base::RunLoop().RunUntilIdle();
+}
+
+class MostVisitedSitesWithEmptyCacheTest : public MostVisitedSitesTest {
+ public:
+ // Constructor sets the common expectations for the case where suggestions
+ // service doesn't have cached results when the observer is registered.
+ MostVisitedSitesWithEmptyCacheTest() {
+ InSequence seq;
+ EXPECT_CALL(*mock_top_sites_, SyncWithHistory());
+ EXPECT_CALL(mock_suggestions_service_, AddCallback(_))
+ .WillOnce(Invoke(&suggestions_service_callbacks_,
+ &SuggestionsService::ResponseCallbackList::Add));
+ EXPECT_CALL(mock_suggestions_service_, GetSuggestionsDataFromCache())
+ .WillOnce(Return(SuggestionsProfile())); // Empty cache.
+ EXPECT_CALL(*mock_top_sites_, GetMostVisitedURLs(_, false))
+ .WillOnce(Invoke(&top_sites_callbacks_, &TopSitesCallbackList::Add));
+ EXPECT_CALL(mock_suggestions_service_, FetchSuggestionsData())
+ .WillOnce(Return(true));
+ most_visited_sites_.SetMostVisitedURLsObserver(&mock_observer_,
+ /*num_sites=*/3);
+ VerifyAndClearExpectations();
+
+ EXPECT_FALSE(suggestions_service_callbacks_.empty());
+ EXPECT_FALSE(top_sites_callbacks_.empty());
+ }
+};
+
+TEST_F(MostVisitedSitesWithEmptyCacheTest,
+ ShouldQueryTopSitesAndSuggestionsService) {
+ // Constructor sets basic expectations for a suggestions service cache miss.
+}
+
+// TODO(mastiz): This describes the current behavior but it actually is a bug.
+TEST_F(MostVisitedSitesWithEmptyCacheTest,
+ DoesNotIgnoreTopSitesIfSuggestionsServiceFaster) {
+ // Reply from suggestions service triggers and update to our observer.
+ EXPECT_CALL(
+ mock_observer_,
+ OnMostVisitedURLsAvailable(ElementsAre(MatchesTile(
+ "Site 1", "http://site1/", NTPTileSource::SUGGESTIONS_SERVICE))));
+ suggestions_service_callbacks_.Notify(
+ MakeProfile({MakeSuggestion("Site 1", "http://site1/")}));
+ VerifyAndClearExpectations();
+
+ // Reply from top sites is currently not ignored (i.e. is actually reported to
+ // observer).
+ EXPECT_CALL(mock_observer_,
+ OnMostVisitedURLsAvailable(ElementsAre(MatchesTile(
+ "Site 2", "http://site2/", NTPTileSource::TOP_SITES))));
+ top_sites_callbacks_.ClearAndNotify(
+ {MakeMostVisitedURL("Site 2", "http://site2/")});
+ base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(MostVisitedSitesWithEmptyCacheTest,
+ ShouldExposeTopSitesIfSuggestionsServiceFasterButEmpty) {
+ // Empty reply from suggestions service causes no update to our observer.
+ // However, the current implementation issues a redundant query to TopSites.
+ // TODO(mastiz): Avoid this redundant call.
+ EXPECT_CALL(*mock_top_sites_, GetMostVisitedURLs(_, false))
+ .WillOnce(Invoke(&top_sites_callbacks_, &TopSitesCallbackList::Add));
+ suggestions_service_callbacks_.Notify(SuggestionsProfile());
+ VerifyAndClearExpectations();
+
+ // Reply from top sites is propagated to observer.
+ // TODO(mastiz): Avoid a second redundant call to the observer.
+ EXPECT_CALL(mock_observer_,
+ OnMostVisitedURLsAvailable(ElementsAre(MatchesTile(
+ "Site 1", "http://site1/", NTPTileSource::TOP_SITES))))
+ .Times(2);
+ top_sites_callbacks_.ClearAndNotify(
+ {MakeMostVisitedURL("Site 1", "http://site1/")});
+ base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(MostVisitedSitesWithEmptyCacheTest,
+ ShouldFavorSuggestionsServiceAlthoughSlower) {
+ // Reply from top sites is propagated to observer.
+ EXPECT_CALL(mock_observer_,
+ OnMostVisitedURLsAvailable(ElementsAre(MatchesTile(
+ "Site 1", "http://site1/", NTPTileSource::TOP_SITES))));
+ top_sites_callbacks_.ClearAndNotify(
+ {MakeMostVisitedURL("Site 1", "http://site1/")});
+ VerifyAndClearExpectations();
+
+ // Reply from suggestions service overrides top sites.
+ InSequence seq;
+ EXPECT_CALL(
+ mock_observer_,
+ OnMostVisitedURLsAvailable(ElementsAre(MatchesTile(
+ "Site 2", "http://site2/", NTPTileSource::SUGGESTIONS_SERVICE))));
+ suggestions_service_callbacks_.Notify(
+ MakeProfile({MakeSuggestion("Site 2", "http://site2/")}));
+ base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(MostVisitedSitesWithEmptyCacheTest,
+ ShouldIgnoreSuggestionsServiceIfSlowerAndEmpty) {
+ // Reply from top sites is propagated to observer.
+ EXPECT_CALL(mock_observer_,
+ OnMostVisitedURLsAvailable(ElementsAre(MatchesTile(
+ "Site 1", "http://site1/", NTPTileSource::TOP_SITES))));
+ top_sites_callbacks_.ClearAndNotify(
+ {MakeMostVisitedURL("Site 1", "http://site1/")});
+ VerifyAndClearExpectations();
+
+ // Reply from suggestions service is empty and thus ignored. However, the
+ // current implementation issues a redundant query to TopSites.
+ // TODO(mastiz): Avoid this redundant call.
+ EXPECT_CALL(*mock_top_sites_, GetMostVisitedURLs(_, false))
+ .WillOnce(Invoke(&top_sites_callbacks_, &TopSitesCallbackList::Add));
+ suggestions_service_callbacks_.Notify(SuggestionsProfile());
+ base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(MostVisitedSitesWithEmptyCacheTest, ShouldPropagateUpdateByTopSites) {
+ // Reply from top sites is propagated to observer.
+ EXPECT_CALL(mock_observer_,
+ OnMostVisitedURLsAvailable(ElementsAre(MatchesTile(
+ "Site 1", "http://site1/", NTPTileSource::TOP_SITES))));
+ top_sites_callbacks_.ClearAndNotify(
+ {MakeMostVisitedURL("Site 1", "http://site1/")});
+ VerifyAndClearExpectations();
+
+ // Reply from suggestions service is empty and thus ignored. However, the
+ // current implementation issues a redundant query to TopSites.
+ // TODO(mastiz): Avoid this redundant call.
+ EXPECT_CALL(*mock_top_sites_, GetMostVisitedURLs(_, false))
+ .WillOnce(Invoke(&top_sites_callbacks_, &TopSitesCallbackList::Add));
+ suggestions_service_callbacks_.Notify(SuggestionsProfile());
+ VerifyAndClearExpectations();
+
+ // Update from top sites is propagated to observer.
+ EXPECT_CALL(mock_observer_,
+ OnMostVisitedURLsAvailable(ElementsAre(MatchesTile(
+ "Site 2", "http://site2/", NTPTileSource::TOP_SITES))));
+ top_sites_callbacks_.ClearAndNotify(
+ {MakeMostVisitedURL("Site 2", "http://site2/")});
+ base::RunLoop().RunUntilIdle();
+}
+
// This a test for MostVisitedSites::MergeTiles(...) method, and thus has the
// same scope as the method itself. This tests merging popular sites with
// personal tiles.
@@ -57,9 +467,7 @@
// - Removing blacklisted tiles.
// - Correct host extraction from the URL.
// - Ensuring personal tiles are not duplicated in popular tiles.
-//
-// TODO(mastiz): Add tests for the functionality listed above.
-TEST(MostVisitedSitesTest, ShouldMergeTilesWithPersonalOnly) {
+TEST(MostVisitedSitesMergeTest, ShouldMergeTilesWithPersonalOnly) {
std::vector<NTPTile> personal_tiles{
MakeTile("Site 1", "https://www.site1.com/", NTPTileSource::TOP_SITES),
MakeTile("Site 2", "https://www.site2.com/", NTPTileSource::TOP_SITES),
@@ -81,7 +489,7 @@
NTPTileSource::TOP_SITES)));
}
-TEST(MostVisitedSitesTest, ShouldMergeTilesWithPopularOnly) {
+TEST(MostVisitedSitesMergeTest, ShouldMergeTilesWithPopularOnly) {
std::vector<NTPTile> popular_tiles{
MakeTile("Site 1", "https://www.site1.com/", NTPTileSource::POPULAR),
MakeTile("Site 2", "https://www.site2.com/", NTPTileSource::POPULAR),
@@ -104,7 +512,7 @@
NTPTileSource::POPULAR)));
}
-TEST(MostVisitedSitesTest, ShouldMergeTilesFavoringPersonalOverPopular) {
+TEST(MostVisitedSitesMergeTest, ShouldMergeTilesFavoringPersonalOverPopular) {
std::vector<NTPTile> popular_tiles{
MakeTile("Site 1", "https://www.site1.com/", NTPTileSource::POPULAR),
MakeTile("Site 2", "https://www.site2.com/", NTPTileSource::POPULAR),
