Add some more tests of PopularSites.

components/ntp_tiles/popular_sites_unittest.cc

Index: components/ntp_tiles/popular_sites_unittest.cc
diff --git a/components/ntp_tiles/popular_sites_unittest.cc b/components/ntp_tiles/popular_sites_unittest.cc
index 76071cb36e82cf8f2041b5200c4b4ec505d7a725..d70f76cba03d5af21037935232b623444e192a59 100644
--- a/components/ntp_tiles/popular_sites_unittest.cc
+++ b/components/ntp_tiles/popular_sites_unittest.cc
@@ -38,7 +38,7 @@ const char kUrl[] = "url";
 const char kLargeIconUrl[] = "large_icon_url";
 const char kFaviconUrl[] = "favicon_url";
 
-using TestPopularSite = std::vector<std::pair<std::string, std::string>>;
+using TestPopularSite = std::map<std::string, std::string>;
 using TestPopularSiteVector = std::vector<TestPopularSite>;
 
 ::testing::Matcher<const base::string16&> Str16Eq(const std::string& s) {
@@ -88,10 +88,26 @@ class JsonUnsafeParser {
 class PopularSitesTest : public ::testing::Test {
  protected:
   PopularSitesTest()
-      : worker_pool_owner_(2, "PopularSitesTest."),
+      : kWikipedia{
+            {kTitle, "Wikipedia, fhta Ph'nglui mglw'nafh"},

[Marc Treib, 2016/11/08 11:27:30]

I feel like I'm missing a reference here...? :D

[sfiera, 2016/11/08 13:00:35]

I'm using the country code "ZZ" which is guaranteed not to be a real country
(just as an extra precaution against using real data). Since it's not a real
country, I grabbed some not-real-language from Lovecraft:

    …His worshippers chant "Ph'nglui mglw'nafh Cthulhu R'lyeh wgah'nagl
    fhtagn" ("In his house at R'lyeh, dead Cthulhu waits dreaming.")…

(from the *English* Wikipedia)

+            {kUrl, "https://zz.m.wikipedia.org/"},
+            {kLargeIconUrl, "https://zz.m.wikipedia.org/wikipedia.png"},
+        },
+        kYouTube{
+            {kTitle, "YouTube"},
+            {kUrl, "https://m.youtube.com/"},
+            {kLargeIconUrl, "https://s.ytimg.com/apple-touch-icon.png"},
+        },
+        kChromium{
+            {kTitle, "The Chromium Project"},
+            {kUrl, "https://www.chromium.org/"},
+            {kFaviconUrl, "https://www.chromium.org/favicon.ico"},
+        },
+        worker_pool_owner_(2, "PopularSitesTest."),
         url_fetcher_factory_(nullptr) {
     PopularSites::RegisterProfilePrefs(prefs_.registry());
-    CHECK(cache_dir_.CreateUniqueTempDir());
+    CHECK(scoped_cache_dir_.CreateUniqueTempDir());
+    cache_dir_ = scoped_cache_dir_.GetPath();
   }
 
   void SetCountryAndVersion(const std::string& country,
@@ -116,6 +132,11 @@ class PopularSitesTest : public ::testing::Test {
                                          net::URLRequestStatus::SUCCESS);
   }
 
+  void RespondWithData(const std::string& url, const std::string& data) {
+    url_fetcher_factory_.SetFakeResponse(GURL(url), data, net::HTTP_OK,
+                                         net::URLRequestStatus::SUCCESS);
+  }
+
   void RespondWith404(const std::string& url) {
     url_fetcher_factory_.SetFakeResponse(GURL(url), "404", net::HTTP_NOT_FOUND,
                                          net::URLRequestStatus::SUCCESS);
@@ -129,7 +150,7 @@ class PopularSitesTest : public ::testing::Test {
     PopularSites popular_sites(worker_pool_owner_.pool().get(), &prefs_,
                                /*template_url_service=*/nullptr,
                                /*variations_service=*/nullptr,
-                               url_request_context.get(), cache_dir_.GetPath(),
+                               url_request_context.get(), cache_dir_,
                                base::Bind(JsonUnsafeParser::Parse));
 
     base::RunLoop loop;
@@ -147,9 +168,14 @@ class PopularSitesTest : public ::testing::Test {
     return save_success;
   }
 
+  const TestPopularSite kWikipedia;
+  const TestPopularSite kYouTube;
+  const TestPopularSite kChromium;
+
   base::MessageLoopForUI ui_loop_;
   base::SequencedWorkerPoolOwner worker_pool_owner_;
-  base::ScopedTempDir cache_dir_;
+  base::ScopedTempDir scoped_cache_dir_;
+  base::FilePath cache_dir_;

[Marc Treib, 2016/11/08 11:27:30]

nit: cache_path_ ?

[sfiera, 2016/11/08 13:00:35]

I would say that the cache path is "/…whatever…/suggested_sites.json", whereas
this is just the cache dir, the "/…whatever…/" part.

   user_prefs::TestingPrefServiceSyncable prefs_;
   net::FakeURLFetcherFactory url_fetcher_factory_;
 };
@@ -158,11 +184,7 @@ TEST_F(PopularSitesTest, Basic) {
   SetCountryAndVersion("ZZ", "9");
   RespondWithJSON(
       "https://www.gstatic.com/chrome/ntp/suggested_sites_ZZ_9.json",
-      {{
-          {kTitle, "Wikipedia, fhta Ph'nglui mglw'nafh"},
-          {kUrl, "https://zz.m.wikipedia.org/"},
-          {kLargeIconUrl, "https://zz.m.wikipedia.org/wikipedia.png"},
-      }});
+      {kWikipedia});
 
   std::vector<PopularSites::Site> sites;
   EXPECT_TRUE(FetchPopularSites(/*force_download=*/false, &sites));
@@ -181,16 +203,7 @@ TEST_F(PopularSitesTest, Fallback) {
       "https://www.gstatic.com/chrome/ntp/suggested_sites_ZZ_9.json");
   RespondWithJSON(
       "https://www.gstatic.com/chrome/ntp/suggested_sites_DEFAULT_5.json",
-      {{
-           {kTitle, "YouTube"},
-           {kUrl, "https://m.youtube.com/"},
-           {kLargeIconUrl, "https://s.ytimg.com/apple-touch-icon.png"},
-       },
-       {
-           {kTitle, "The Chromium Project"},
-           {kUrl, "https://www.chromium.org/"},
-           {kFaviconUrl, "https://www.chromium.org/favicon.ico"},
-       }});
+      {kYouTube, kChromium});
 
   std::vector<PopularSites::Site> sites;
   EXPECT_TRUE(FetchPopularSites(/*force_download=*/false, &sites));
@@ -220,5 +233,91 @@ TEST_F(PopularSitesTest, Failure) {
   ASSERT_THAT(sites.size(), Eq(0u));
 }
 
+TEST_F(PopularSitesTest, FailsWithoutFetchIfNoCacheDir) {
+  SetCountryAndVersion("ZZ", "9");
+  std::vector<PopularSites::Site> sites;
+  cache_dir_ = base::FilePath();  // Override with invalid file path.
+  EXPECT_FALSE(FetchPopularSites(/*force_download=*/false, &sites));
+}
+
+TEST_F(PopularSitesTest, UsesCachedFile) {
+  SetCountryAndVersion("ZZ", "9");
+  RespondWithJSON(
+      "https://www.gstatic.com/chrome/ntp/suggested_sites_ZZ_9.json",
+      {kWikipedia});
+
+  // First request succeeds and gets cached.
+  std::vector<PopularSites::Site> sites;
+  EXPECT_TRUE(FetchPopularSites(/*force_download=*/false, &sites));
+
+  // File disappears from server, but we don't need it because it's cached.
+  RespondWith404(
+      "https://www.gstatic.com/chrome/ntp/suggested_sites_ZZ_9.json");
+  EXPECT_TRUE(FetchPopularSites(/*force_download=*/false, &sites));
+  EXPECT_THAT(sites[0].url, URLEq("https://zz.m.wikipedia.org/"));
+}
+
+TEST_F(PopularSitesTest, DoesntUseCachedFileIfDownloadForced) {
+  SetCountryAndVersion("ZZ", "9");
+  RespondWithJSON(
+      "https://www.gstatic.com/chrome/ntp/suggested_sites_ZZ_9.json",
+      {kWikipedia});
+
+  // First request succeeds and gets cached.
+  std::vector<PopularSites::Site> sites;
+  EXPECT_TRUE(FetchPopularSites(/*force_download=*/true, &sites));
+  EXPECT_THAT(sites[0].url, URLEq("https://zz.m.wikipedia.org/"));
+
+  // File disappears from server. Download is forced, so we get the new file.
+  RespondWithJSON(
+      "https://www.gstatic.com/chrome/ntp/suggested_sites_ZZ_9.json",
+      {kChromium});
+  EXPECT_TRUE(FetchPopularSites(/*force_download=*/true, &sites));
+  EXPECT_THAT(sites[0].url, URLEq("https://www.chromium.org/"));
+}
+
+TEST_F(PopularSitesTest, RefetchesAfterCountryMoved) {
+  RespondWithJSON(
+      "https://www.gstatic.com/chrome/ntp/suggested_sites_ZZ_9.json",
+      {kWikipedia});
+  RespondWithJSON(
+      "https://www.gstatic.com/chrome/ntp/suggested_sites_ZX_9.json",
+      {kChromium});
+
+  std::vector<PopularSites::Site> sites;
+
+  // First request (in ZZ) saves Wikipedia.
+  SetCountryAndVersion("ZZ", "9");
+  EXPECT_TRUE(FetchPopularSites(/*force_download=*/false, &sites));
+  EXPECT_THAT(sites[0].url, URLEq("https://zz.m.wikipedia.org/"));
+
+  // Second request (now in ZX) saves Chromium.
+  SetCountryAndVersion("ZX", "9");
+  EXPECT_TRUE(FetchPopularSites(/*force_download=*/false, &sites));
+  EXPECT_THAT(sites[0].url, URLEq("https://www.chromium.org/"));
+}
+
+TEST_F(PopularSitesTest, DoesntCacheInvalidFile) {
+  SetCountryAndVersion("ZZ", "9");
+  RespondWithData(
+      "https://www.gstatic.com/chrome/ntp/suggested_sites_ZZ_9.json",
+      "ceci n'est pas un json");
+  RespondWithJSON(
+      "https://www.gstatic.com/chrome/ntp/suggested_sites_DEFAULT_5.json", {});

[Marc Treib, 2016/11/08 11:27:30]

Is this actually needed? The same thing would happen if the fallback file didn't
exist, right? (Actually even if it wasn't empty, the way the fallback works
right now, correct?)

[sfiera, 2016/11/08 13:00:35]

Hmm. Well, the fetcher will hit the fallback URL here, so need a response (404
is fine, though). In this case, it's also interesting to verify that a cached
fallback file doesn't prevent us from re-fetching the primary file, but I've now
made that a separate test. And since caching an empty file is interesting too,
that's now another test.

+
+  // First request falls back and gets nothing.
+  std::vector<PopularSites::Site> sites;
+  EXPECT_TRUE(FetchPopularSites(/*force_download=*/false, &sites));
+  EXPECT_THAT(sites.size(), Eq(0u));

[Marc Treib, 2016/11/08 11:27:30]

nit: IsEmpty?

[sfiera, 2016/11/08 13:00:35]

Done.

+
+  // Second request refetches ZZ_9, which now has data.
+  RespondWithJSON(
+      "https://www.gstatic.com/chrome/ntp/suggested_sites_ZZ_9.json",
+      {kChromium});
+  EXPECT_TRUE(FetchPopularSites(/*force_download=*/false, &sites));
+  ASSERT_THAT(sites.size(), Eq(1u));
+  EXPECT_THAT(sites[0].url, URLEq("https://www.chromium.org/"));
+}
+
 }  // namespace
 }  // namespace ntp_tiles