ntp_tiles: Refactor existing MostVisitedSites tests

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 6fbc9dcd0872690aeb5e5ffa28b2cb70b036f900..0baab4ad41055523f52f6da66b19bb49b0752dd1 100644
--- a/components/ntp_tiles/most_visited_sites_unittest.cc
+++ b/components/ntp_tiles/most_visited_sites_unittest.cc
@@ -7,34 +7,52 @@
 #include <stddef.h>
 
 #include <memory>
+#include <ostream>
 #include <vector>
 
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace ntp_tiles {
 
+// Defined for googletest. Must be defined in the same namespace.

[Marc Treib, 2016/12/13 16:44:15]

I think it's also possible to define an "operator<<" instead, which to me feels
a bit less magic. WDYT?

[mastiz, 2016/12/14 08:54:27]

I *think*, for test-oriented printing, which is the case because we don't even
print all fields, PrintTo is preferred. It also seems quite widely used in
chromium, although operator<< is harder to search for.

[Marc Treib, 2016/12/14 09:27:34]

I guess it's just me being unfamiliar with the testing framework, but I wonder
why PrintTo even exists, if there is also a standard way of achieving the same
thing.
Anyway, it's not a big deal.

+void PrintTo(const NTPTile& tile, std::ostream* os) {
+  *os << "{\"" << tile.title << "\", \"" << tile.url << "\", "
+      << static_cast<int>(tile.source) << "}";
+}
+
 namespace {
 
-struct TitleURL {
-  TitleURL(const std::string& title, const std::string& url)
-      : title(base::UTF8ToUTF16(title)), url(url) {}
-  TitleURL(const base::string16& title, const std::string& url)
-      : title(title), url(url) {}
+using testing::AllOf;
+using testing::ElementsAre;
 
-  base::string16 title;
-  std::string url;
+MATCHER_P(HasSource,
+          source,
+          std::string("has source ") +
+              testing::PrintToString(static_cast<int>(source))) {
+  return arg.source == source;
+}
 
-  bool operator==(const TitleURL& other) const {
-    return title == other.title && url == other.url;
-  }
-};
+MATCHER_P(HasTitle, title, "") {
+  return arg.title == base::ASCIIToUTF16(title);
+}
 
-static const size_t kNumSites = 4;
+MATCHER_P(HasUrl, url, "") {
+  return arg.url == GURL(url);
+}
 
-}  // namespace
+NTPTile MakeTile(const std::string& title,
+                 const std::string& url,
+                 NTPTileSource source) {
+  NTPTile tile;
+  tile.title = base::ASCIIToUTF16(title);
+  tile.url = GURL(url);
+  tile.source = source;
+  return tile;
+}
 
 // This a test for MostVisitedSites::MergeTiles(...) method, and thus has the
 // same scope as the method itself. This tests merging popular sites with
@@ -43,96 +61,76 @@ static const size_t kNumSites = 4;
 // - Removing blacklisted tiles.
 // - Correct host extraction from the URL.
 // - Ensuring personal tiles are not duplicated in popular tiles.
-class MostVisitedSitesTest : public testing::Test {
- protected:
-  void Check(const std::vector<TitleURL>& popular_sites,
-             const std::vector<TitleURL>& whitelist_entry_points,
-             const std::vector<TitleURL>& personal_sites,
-             const std::vector<bool>& expected_sites_is_personal,
-             const std::vector<TitleURL>& expected_sites) {
-    NTPTilesVector personal_tiles;
-    for (const TitleURL& site : personal_sites)
-      personal_tiles.push_back(MakeTileFrom(site, true, false));
-    NTPTilesVector whitelist_tiles;
-    for (const TitleURL& site : whitelist_entry_points)
-      whitelist_tiles.push_back(MakeTileFrom(site, false, true));
-    NTPTilesVector popular_tiles;
-    for (const TitleURL& site : popular_sites)
-      popular_tiles.push_back(MakeTileFrom(site, false, false));
-    NTPTilesVector result_tiles = MostVisitedSites::MergeTiles(
-        std::move(personal_tiles), std::move(whitelist_tiles),
-        std::move(popular_tiles));
-    std::vector<TitleURL> result_sites;
-    std::vector<bool> result_is_personal;
-    result_sites.reserve(result_tiles.size());
-    result_is_personal.reserve(result_tiles.size());
-    for (const auto& tile : result_tiles) {
-      result_sites.push_back(TitleURL(tile.title, tile.url.spec()));
-      result_is_personal.push_back(tile.source != NTPTileSource::POPULAR);
-    }
-    EXPECT_EQ(expected_sites_is_personal, result_is_personal);
-    EXPECT_EQ(expected_sites, result_sites);
-  }
-  static NTPTile MakeTileFrom(const TitleURL& title_url,
-                              bool is_personal,
-                              bool whitelist) {
-    NTPTile tile;
-    tile.title = title_url.title;
-    tile.url = GURL(title_url.url);
-    tile.source = whitelist ? NTPTileSource::WHITELIST
-                            : (is_personal ? NTPTileSource::TOP_SITES
-                                           : NTPTileSource::POPULAR);
-    return tile;
-  }
-};
-
-TEST_F(MostVisitedSitesTest, PersonalSites) {
-  std::vector<TitleURL> personal_sites{
-      TitleURL("Site 1", "https://www.site1.com/"),
-      TitleURL("Site 2", "https://www.site2.com/"),
-      TitleURL("Site 3", "https://www.site3.com/"),
-      TitleURL("Site 4", "https://www.site4.com/"),
+//
+// TODO(mastiz): Add tests for the functionality listed above.
+TEST(MostVisitedSitesTest, 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),
+      MakeTile("Site 3", "https://www.site3.com/", NTPTileSource::TOP_SITES),
+      MakeTile("Site 4", "https://www.site4.com/", NTPTileSource::TOP_SITES),
   };
   // Without any popular tiles, the result after merge should be the personal
   // tiles.
-  std::vector<bool> expected_sites_source(kNumSites, true /*personal source*/);
-  Check(std::vector<TitleURL>(), std::vector<TitleURL>(), personal_sites,
-        expected_sites_source, personal_sites);
+  EXPECT_THAT(
+      MostVisitedSites::MergeTiles(std::move(personal_tiles),
+                                   /*whitelist_tiles=*/{},
+                                   /*popular_tiles=*/{}),
+      ElementsAre(AllOf(HasSource(NTPTileSource::TOP_SITES), HasTitle("Site 1"),
+                        HasUrl("https://www.site1.com/")),

[Marc Treib, 2016/12/13 16:44:15]

Instead of checking three separate properties, could we define an operator== for
NTPTile?

[mastiz, 2016/12/14 08:54:27]

Done.

+                  AllOf(HasSource(NTPTileSource::TOP_SITES), HasTitle("Site 2"),
+                        HasUrl("https://www.site2.com/")),
+                  AllOf(HasSource(NTPTileSource::TOP_SITES), HasTitle("Site 3"),
+                        HasUrl("https://www.site3.com/")),
+                  AllOf(HasSource(NTPTileSource::TOP_SITES), HasTitle("Site 4"),
+                        HasUrl("https://www.site4.com/"))));
 }
 
-TEST_F(MostVisitedSitesTest, PopularSites) {
-  std::vector<TitleURL> popular_sites{
-      TitleURL("Site 1", "https://www.site1.com/"),
-      TitleURL("Site 2", "https://www.site2.com/"),
-      TitleURL("Site 3", "https://www.site3.com/"),
-      TitleURL("Site 4", "https://www.site4.com/"),
+TEST(MostVisitedSitesTest, ShouldMergeTilesWithPopularOnly) {
+  std::vector<NTPTile> popular_tiles{
+      MakeTile("Site 1", "https://www.site1.com/", NTPTileSource::POPULAR),
+      MakeTile("Site 2", "https://www.site2.com/", NTPTileSource::POPULAR),
+      MakeTile("Site 3", "https://www.site3.com/", NTPTileSource::POPULAR),
+      MakeTile("Site 4", "https://www.site4.com/", NTPTileSource::POPULAR),
   };
   // Without any personal tiles, the result after merge should be the popular
   // tiles.
-  std::vector<bool> expected_sites_source(kNumSites, false /*popular source*/);
-  Check(popular_sites, std::vector<TitleURL>(), std::vector<TitleURL>(),
-        expected_sites_source, popular_sites);
+  EXPECT_THAT(
+      MostVisitedSites::MergeTiles(/*personal_tiles=*/{},
+                                   /*whitelist_tiles=*/{},
+                                   /*popular_tiles=*/std::move(popular_tiles)),
+      ElementsAre(AllOf(HasSource(NTPTileSource::POPULAR), HasTitle("Site 1"),
+                        HasUrl("https://www.site1.com/")),
+                  AllOf(HasSource(NTPTileSource::POPULAR), HasTitle("Site 2"),
+                        HasUrl("https://www.site2.com/")),
+                  AllOf(HasSource(NTPTileSource::POPULAR), HasTitle("Site 3"),
+                        HasUrl("https://www.site3.com/")),
+                  AllOf(HasSource(NTPTileSource::POPULAR), HasTitle("Site 4"),
+                        HasUrl("https://www.site4.com/"))));
 }
 
-TEST_F(MostVisitedSitesTest, PersonalPrecedePopularSites) {
-  std::vector<TitleURL> popular_sites{
-      TitleURL("Site 1", "https://www.site1.com/"),
-      TitleURL("Site 2", "https://www.site2.com/"),
+TEST(MostVisitedSitesTest, ShouldMergeTilesFavoringPersonalOverPopular) {
+  std::vector<NTPTile> popular_tiles{
+      MakeTile("Site 1", "https://www.site1.com/", NTPTileSource::POPULAR),
+      MakeTile("Site 2", "https://www.site2.com/", NTPTileSource::POPULAR),
   };
-  std::vector<TitleURL> personal_sites{
-      TitleURL("Site 3", "https://www.site3.com/"),
-      TitleURL("Site 4", "https://www.site4.com/"),
+  std::vector<NTPTile> personal_tiles{
+      MakeTile("Site 3", "https://www.site3.com/", NTPTileSource::TOP_SITES),
+      MakeTile("Site 4", "https://www.site4.com/", NTPTileSource::TOP_SITES),
   };
-  // Personal tiles should precede popular tiles.
-  std::vector<TitleURL> expected_sites{
-      TitleURL("Site 3", "https://www.site3.com/"),
-      TitleURL("Site 4", "https://www.site4.com/"),
-      TitleURL("Site 1", "https://www.site1.com/"),
-      TitleURL("Site 2", "https://www.site2.com/"),
-  };
-  std::vector<bool> expected_sites_source{true, true, false, false};
-  Check(popular_sites, std::vector<TitleURL>(), personal_sites,
-        expected_sites_source, expected_sites);
+  EXPECT_THAT(
+      MostVisitedSites::MergeTiles(std::move(personal_tiles),
+                                   /*whitelist_tiles=*/{},
+                                   /*popular_tiles=*/std::move(popular_tiles)),
+      ElementsAre(AllOf(HasSource(NTPTileSource::TOP_SITES), HasTitle("Site 3"),
+                        HasUrl("https://www.site3.com/")),
+                  AllOf(HasSource(NTPTileSource::TOP_SITES), HasTitle("Site 4"),
+                        HasUrl("https://www.site4.com/")),
+                  AllOf(HasSource(NTPTileSource::POPULAR), HasTitle("Site 1"),
+                        HasUrl("https://www.site1.com/")),
+                  AllOf(HasSource(NTPTileSource::POPULAR), HasTitle("Site 2"),
+                        HasUrl("https://www.site2.com/"))));
 }
 
+}  // namespace
 }  // namespace ntp_tiles