ntp_tiles: Add tests covering tile-fetching logic

components/ntp_tiles/most_visited_sites_unittest.cc

Index: components/ntp_tiles/most_visited_sites_unittest.cc
diff --git a/components/ntp_tiles/most_visited_sites_unittest.cc b/components/ntp_tiles/most_visited_sites_unittest.cc
index 029968dfa138b8e924036cd006e8badde08cd2a1..71255a6caca851d492ec995b347152c24b6e75be 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/prefs/testing_pref_service.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -26,7 +34,23 @@ void PrintTo(const NTPTile& tile, std::ostream* os) {
 
 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,320 @@ NTPTile MakeTile(const std::string& title,
   return tile;
 }
 
+ChromeSuggestion MakeSuggestion(const std::string& title,
+                                const std::string& url) {
+  ChromeSuggestion suggestion;
+  suggestion.set_title(title);
+  suggestion.set_url(url);
+  return suggestion;
+}
+
+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 Notify(const MostVisitedURLList& list) {
+    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_),

[Marc Treib, 2016/12/19 09:11:50]

Hm, IMO this makes ownership kinda hard to understand. I usually prefer the
pattern:
auto ptr = MakeUnique<..>();
Class* raw_ptr = ptr.get();
DoTheThing(std::move(ptr));

WDYT?
Could that work here?

[mastiz, 2017/01/09 10:40:59]

The main reason for this was to avoid the need for a unique_ptr with
MostVisitedSites. This will change soon anyway, so I'll do as you suggest in a
future patch.

+                            /*icon_cacher=*/{},
+                            /*supervisor=*/{}) {

[Marc Treib, 2016/12/19 09:11:50]

nullptr?

[mastiz, 2017/01/09 10:40:59]

Obsolete (done).

+    MostVisitedSites::RegisterProfilePrefs(pref_service_.registry());

[Marc Treib, 2016/12/19 09:11:50]

I think it'd be best to instantiate MostVisitedSites *after* we have registered
its prefs.

[mastiz, 2017/01/09 10:40:59]

Will address in a future patch, probably in the form of SetUpTestCase().

+    // 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_));

[Marc Treib, 2016/12/19 09:11:50]

Is this a case for ON_CALL(...).WillByDefault?

[mastiz, 2017/01/09 10:40:58]

I think not: setting a default could (in the future) interfere with other
expectations with PopularSites::sites(). IIUC WillByDefault is used to avoid
boilerplate when there's multiple expectations. See
http://stackoverflow.com/questions/13933475/gmock-setting-default-actions-on-...

+  }
+
+  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_;
+  TestingPrefServiceSimple 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();
+}
+
+TEST_F(MostVisitedSitesTest, ShouldFavorSuggestionsServiceCache) {
+  InSequence seq;
+  SuggestionsProfile cached_profile;
+  *cached_profile.add_suggestions() = MakeSuggestion("Site 1", "http://site1/");
+  *cached_profile.add_suggestions() = MakeSuggestion("Site 2", "http://site2/");
+  *cached_profile.add_suggestions() = MakeSuggestion("Site 3", "http://site3/");
+  *cached_profile.add_suggestions() = MakeSuggestion("Site 4", "http://site4/");
+  EXPECT_CALL(*mock_top_sites_, SyncWithHistory());
+  EXPECT_CALL(mock_suggestions_service_, AddCallback(_));

[Marc Treib, 2016/12/19 09:11:50]

Here the InSequence might be a bit too strong: It doesn't really matter which of
the two above is called first. Or am I missing something?

[mastiz, 2017/01/09 10:40:59]

Obsolete.

+  EXPECT_CALL(mock_suggestions_service_, GetSuggestionsDataFromCache())
+      .WillOnce(Return(cached_profile));
+  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);
+  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());
+  }
+};
+
+// 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.
+  {
+    InSequence seq;
+    SuggestionsProfile profile;
+    *profile.add_suggestions() = MakeSuggestion("Site 1", "http://site1/");
+    EXPECT_CALL(
+        mock_observer_,
+        OnMostVisitedURLsAvailable(ElementsAre(MatchesTile(
+            "Site 1", "http://site1/", NTPTileSource::SUGGESTIONS_SERVICE))));
+    suggestions_service_callbacks_.Notify(profile);
+    VerifyAndClearExpectations();
+  }
+
+  // Reply from top sites is currently not ignored (i.e. not reported to
+  // observer).
+  EXPECT_CALL(mock_observer_,
+              OnMostVisitedURLsAvailable(ElementsAre(MatchesTile(
+                  "Site 2", "http://site2/", NTPTileSource::TOP_SITES))));
+  MostVisitedURLList top_sites_result;
+  top_sites_result.push_back(MakeMostVisitedURL("Site 2", "http://site2/"));
+  top_sites_callbacks_.Notify(top_sites_result);
+  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.
+  MostVisitedURLList top_sites_result;
+  top_sites_result.push_back(MakeMostVisitedURL("Site 1", "http://site1/"));
+  EXPECT_CALL(mock_observer_,
+              OnMostVisitedURLsAvailable(ElementsAre(MatchesTile(
+                  "Site 1", "http://site1/", NTPTileSource::TOP_SITES))))
+      .Times(2);
+  top_sites_callbacks_.Notify(top_sites_result);
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(MostVisitedSitesWithEmptyCacheTest,
+       ShouldFavorSuggestionsServiceAlthoughSlower) {
+  // Reply from top sites is propagated to observer.
+  MostVisitedURLList top_sites_result;
+  top_sites_result.push_back(MakeMostVisitedURL("Site 1", "http://site1/"));
+  EXPECT_CALL(mock_observer_,
+              OnMostVisitedURLsAvailable(ElementsAre(MatchesTile(
+                  "Site 1", "http://site1/", NTPTileSource::TOP_SITES))));
+  top_sites_callbacks_.Notify(top_sites_result);
+  VerifyAndClearExpectations();
+
+  // Reply from suggestions service overrides top sites.
+  InSequence seq;
+  SuggestionsProfile profile;
+  *profile.add_suggestions() = MakeSuggestion("Site 2", "http://site2/");
+  EXPECT_CALL(
+      mock_observer_,
+      OnMostVisitedURLsAvailable(ElementsAre(MatchesTile(
+          "Site 2", "http://site2/", NTPTileSource::SUGGESTIONS_SERVICE))));
+  suggestions_service_callbacks_.Notify(profile);
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(MostVisitedSitesWithEmptyCacheTest,
+       ShouldIgnoreSuggestionsServiceIfSlowerAndEmpty) {
+  // Reply from top sites is propagated to observer.
+  {
+    MostVisitedURLList top_sites_result;
+    top_sites_result.push_back(MakeMostVisitedURL("Site 1", "http://site1/"));
+    EXPECT_CALL(mock_observer_,
+                OnMostVisitedURLsAvailable(ElementsAre(MatchesTile(
+                    "Site 1", "http://site1/", NTPTileSource::TOP_SITES))));
+    top_sites_callbacks_.Notify(top_sites_result);
+    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();
+}
+
 // 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 +395,7 @@ NTPTile MakeTile(const std::string& title,
 // - 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 +417,7 @@ TEST(MostVisitedSitesTest, ShouldMergeTilesWithPersonalOnly) {
                                       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 +440,7 @@ TEST(MostVisitedSitesTest, ShouldMergeTilesWithPopularOnly) {
                               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),