Overhaul ntp_snippets_service_unittest.cc.

components/ntp_snippets/ntp_snippets_service_unittest.cc

Index: components/ntp_snippets/ntp_snippets_service_unittest.cc
diff --git a/components/ntp_snippets/ntp_snippets_service_unittest.cc b/components/ntp_snippets/ntp_snippets_service_unittest.cc
index 840b94483e972ef45aaaf38df540d0657a9245b1..7adc3368a98f74a089ba06451946a6f3ad8e9c95 100644
--- a/components/ntp_snippets/ntp_snippets_service_unittest.cc
+++ b/components/ntp_snippets/ntp_snippets_service_unittest.cc
@@ -26,8 +26,8 @@
 #include "components/image_fetcher/image_fetcher.h"
 #include "components/image_fetcher/image_fetcher_delegate.h"
 #include "components/ntp_snippets/category_factory.h"
-#include "components/ntp_snippets/mock_content_suggestions_provider_observer.h"
 #include "components/ntp_snippets/ntp_snippet.h"
+#include "components/ntp_snippets/ntp_snippets_constants.h"
 #include "components/ntp_snippets/ntp_snippets_database.h"
 #include "components/ntp_snippets/ntp_snippets_fetcher.h"
 #include "components/ntp_snippets/ntp_snippets_scheduler.h"
@@ -36,6 +36,7 @@
 #include "components/prefs/testing_pref_service.h"
 #include "components/signin/core/browser/fake_profile_oauth2_token_service.h"
 #include "components/signin/core/browser/fake_signin_manager.h"
+#include "components/variations/variations_associated_data.h"
 #include "google_apis/google_api_keys.h"
 #include "net/url_request/test_url_fetcher_factory.h"
 #include "net/url_request/url_request_test_util.h"
@@ -48,12 +49,18 @@ using image_fetcher::ImageFetcher;
 using image_fetcher::ImageFetcherDelegate;
 using testing::ElementsAre;
 using testing::Eq;
+using testing::InSequence;
 using testing::Invoke;
 using testing::IsEmpty;
 using testing::Mock;
+using testing::MockFunction;
+using testing::NiceMock;
 using testing::Return;
+using testing::SaveArg;
 using testing::SizeIs;
 using testing::StartsWith;
+using testing::StrictMock;
+using testing::WithArgs;
 using testing::_;
 
 namespace ntp_snippets {
@@ -65,8 +72,12 @@ MATCHER_P(IdEq, value, "") {
 }
 
 const base::Time::Exploded kDefaultCreationTime = {2015, 11, 4, 25, 13, 46, 45};
-const char kTestContentSnippetsServerFormat[] =
-    "https://chromereader-pa.googleapis.com/v1/fetch?key=%s";
+const char kTestContentSuggestionsServerEndpoint[] =
+    "https://localunittest-chromecontentsuggestions-pa.googleapis.com/v1/"
+    "suggestions/fetch";
+const char kTestContentSuggestionsServerFormat[] =
+    "https://localunittest-chromecontentsuggestions-pa.googleapis.com/v1/"
+    "suggestions/fetch?key=%s";
 
 const char kSnippetUrl[] = "http://localhost/foobar";
 const char kSnippetTitle[] = "Title";
@@ -74,7 +85,6 @@ const char kSnippetText[] = "Snippet";
 const char kSnippetSalientImage[] = "http://localhost/salient_image";
 const char kSnippetPublisherName[] = "Foo News";
 const char kSnippetAmpUrl[] = "http://localhost/amp";
-const float kSnippetScore = 5.0;
 
 const char kSnippetUrl2[] = "http://foo.com/bar";
 
@@ -89,96 +99,85 @@ base::Time GetDefaultExpirationTime() {
 }
 
 std::string GetTestJson(const std::vector<std::string>& snippets) {
-  return base::StringPrintf("{\"recos\":[%s]}",
-                            base::JoinString(snippets, ", ").c_str());
+  return base::StringPrintf(
+      "{\n"
+      "  \"categories\": [{\n"
+      "    \"id\": 1,\n"
+      "    \"localizedTitle\": \"Articles for You\",\n"
+      "    \"suggestions\": [%s]\n"
+      "  }]\n"
+      "}\n",
+      base::JoinString(snippets, ", ").c_str());
 }
 
-std::string GetSnippetWithUrlAndTimesAndSources(
-    const std::string& url,
-    const std::string& content_creation_time_str,
-    const std::string& expiry_time_str,
-    const std::vector<std::string>& source_urls,
-    const std::vector<std::string>& publishers,
-    const std::vector<std::string>& amp_urls) {
-  char json_str_format[] =
-      "{ \"contentInfo\": {"
-      "\"url\" : \"%s\","
-      "\"title\" : \"%s\","
-      "\"snippet\" : \"%s\","
-      "\"thumbnailUrl\" : \"%s\","
-      "\"creationTimestampSec\" : \"%s\","
-      "\"expiryTimestampSec\" : \"%s\","
-      "\"sourceCorpusInfo\" : [%s]"
-      "}, "
-      "\"score\" : %f}";
-
-  char source_corpus_info_format[] =
-      "{\"corpusId\": \"%s\","
-      "\"publisherData\": {"
-      "\"sourceName\": \"%s\""
-      "},"
-      "\"ampUrl\": \"%s\"}";
+std::string FormatTime(const base::Time& t) {
+  base::Time::Exploded x;
+  t.UTCExplode(&x);
+  return base::StringPrintf("%04d-%02d-%02dT%02d:%02d:%02dZ", x.year, x.month,
+                            x.day_of_month, x.hour, x.minute, x.second);
+}
 
-  std::string source_corpus_info_list_str;
-  for (size_t i = 0; i < source_urls.size(); ++i) {
-    std::string source_corpus_info_str =
-        base::StringPrintf(source_corpus_info_format,
-                           source_urls[i].empty() ? "" : source_urls[i].c_str(),
-                           publishers[i].empty() ? "" : publishers[i].c_str(),
-                           amp_urls[i].empty() ? "" : amp_urls[i].c_str());
-    source_corpus_info_list_str.append(source_corpus_info_str);
-    source_corpus_info_list_str.append(",");
-  }
-  // Remove the last comma
-  source_corpus_info_list_str.erase(source_corpus_info_list_str.size()-1, 1);
-  return base::StringPrintf(json_str_format, url.c_str(), kSnippetTitle,
-                            kSnippetText, kSnippetSalientImage,
-                            content_creation_time_str.c_str(),
-                            expiry_time_str.c_str(),
-                            source_corpus_info_list_str.c_str(), kSnippetScore);
+std::string GetSnippetWithUrlAndTimesAndSource(
+    const std::vector<std::string>& ids,
+    const std::string& url,
+    const base::Time& creation_time,
+    const base::Time& expiry_time,
+    const std::string& publisher,
+    const std::string& amp_url) {
+  const std::string ids_string = base::JoinString(ids, "\",\n      \"");
+  return base::StringPrintf(
+      "{\n"
+      "      \"ids\": [\n"
+      "        \"%s\"\n"
+      "      ],\n"
+      "      \"title\": \"%s\",\n"
+      "      \"snippet\": \"%s\",\n"
+      "      \"fullPageUrl\": \"%s\",\n"
+      "      \"creationTime\": \"%s\",\n"
+      "      \"expirationTime\": \"%s\",\n"
+      "      \"attribution\": \"%s\",\n"
+      "      \"imageUrl\": \"%s\",\n"
+      "      \"ampUrl\": \"%s\"\n"
+      "    }",
+      ids_string.c_str(), kSnippetTitle, kSnippetText, url.c_str(),
+      FormatTime(creation_time).c_str(), FormatTime(expiry_time).c_str(),
+      publisher.c_str(), kSnippetSalientImage, amp_url.c_str());
 }
 
-std::string GetSnippetWithSources(const std::vector<std::string>& source_urls,
-                                  const std::vector<std::string>& publishers,
-                                  const std::vector<std::string>& amp_urls) {
-  return GetSnippetWithUrlAndTimesAndSources(
-      kSnippetUrl, NTPSnippet::TimeToJsonString(GetDefaultCreationTime()),
-      NTPSnippet::TimeToJsonString(GetDefaultExpirationTime()), source_urls,
-      publishers, amp_urls);
+std::string GetSnippetWithSources(const std::string& source_url,
+                                  const std::string& publisher,
+                                  const std::string& amp_url) {
+  return GetSnippetWithUrlAndTimesAndSource(
+      {kSnippetUrl}, source_url, GetDefaultCreationTime(),
+      GetDefaultExpirationTime(), publisher, amp_url);
 }
 
-std::string GetSnippetWithUrlAndTimes(
-    const std::string& url,
-    const std::string& content_creation_time_str,
-    const std::string& expiry_time_str) {
-  return GetSnippetWithUrlAndTimesAndSources(
-      url, content_creation_time_str, expiry_time_str,
-      {std::string(url)}, {std::string(kSnippetPublisherName)},
-      {std::string(kSnippetAmpUrl)});
+std::string GetSnippetWithUrlAndTimes(const std::string& url,
+                                      const base::Time& content_creation_time,
+                                      const base::Time& expiry_time) {
+  return GetSnippetWithUrlAndTimesAndSource({url}, url, content_creation_time,
+                                            expiry_time, kSnippetPublisherName,
+                                            kSnippetAmpUrl);
 }
 
-std::string GetSnippetWithTimes(const std::string& content_creation_time_str,
-                                const std::string& expiry_time_str) {
-  return GetSnippetWithUrlAndTimes(kSnippetUrl, content_creation_time_str,
-                                   expiry_time_str);
+std::string GetSnippetWithTimes(const base::Time& content_creation_time,
+                                const base::Time& expiry_time) {
+  return GetSnippetWithUrlAndTimes(kSnippetUrl, content_creation_time,
+                                   expiry_time);
 }
 
 std::string GetSnippetWithUrl(const std::string& url) {
-  return GetSnippetWithUrlAndTimes(
-      url, NTPSnippet::TimeToJsonString(GetDefaultCreationTime()),
-      NTPSnippet::TimeToJsonString(GetDefaultExpirationTime()));
+  return GetSnippetWithUrlAndTimes(url, GetDefaultCreationTime(),
+                                   GetDefaultExpirationTime());
 }
 
 std::string GetSnippet() {
-  return GetSnippetWithUrlAndTimes(
-      kSnippetUrl, NTPSnippet::TimeToJsonString(GetDefaultCreationTime()),
-      NTPSnippet::TimeToJsonString(GetDefaultExpirationTime()));
+  return GetSnippetWithUrlAndTimes(kSnippetUrl, GetDefaultCreationTime(),
+                                   GetDefaultExpirationTime());
 }
 
 std::string GetExpiredSnippet() {
-  return GetSnippetWithTimes(
-      NTPSnippet::TimeToJsonString(GetDefaultCreationTime()),
-      NTPSnippet::TimeToJsonString(base::Time::Now()));
+  return GetSnippetWithTimes(GetDefaultCreationTime(), base::Time::Now());
 }
 
 std::string GetInvalidSnippet() {
@@ -191,7 +190,7 @@ std::string GetIncompleteSnippet() {
   std::string json_str = GetSnippet();
   // Rename the "url" entry. The result is syntactically valid json that will
   // fail to parse as snippets.
-  size_t pos = json_str.find("\"url\"");
+  size_t pos = json_str.find("\"fullPageUrl\"");
   if (pos == std::string::npos) {
     NOTREACHED();
     return std::string();
@@ -242,33 +241,6 @@ class MockScheduler : public NTPSnippetsScheduler {
   MOCK_METHOD0(Unschedule, bool());
 };
 
-class WaitForDBLoad {
- public:
-  WaitForDBLoad(MockContentSuggestionsProviderObserver* observer,
-                NTPSnippetsService* service)
-      : observer_(observer) {
-    EXPECT_CALL(*observer_, OnCategoryStatusChanged(_, _, _))
-        .WillOnce(Invoke(this, &WaitForDBLoad::OnCategoryStatusChanged));
-    if (!service->ready())
-      run_loop_.Run();
-  }
-
-  ~WaitForDBLoad() { Mock::VerifyAndClearExpectations(observer_); }
-
- private:
-  void OnCategoryStatusChanged(ContentSuggestionsProvider* provider,
-                               Category category,
-                               CategoryStatus new_status) {
-    EXPECT_EQ(new_status, CategoryStatus::AVAILABLE_LOADING);
-    run_loop_.Quit();
-  }
-
-  MockContentSuggestionsProviderObserver* observer_;
-  base::RunLoop run_loop_;
-
-  DISALLOW_COPY_AND_ASSIGN(WaitForDBLoad);
-};
-
 class MockImageFetcher : public ImageFetcher {
  public:
   MOCK_METHOD1(SetImageFetcherDelegate, void(ImageFetcherDelegate*));
@@ -280,29 +252,75 @@ class MockImageFetcher : public ImageFetcher {
            base::Callback<void(const std::string&, const gfx::Image&)>));
 };
 
-class MockDismissedSuggestionsCallback
-    : public ContentSuggestionsProvider::DismissedSuggestionsCallback {
+class FakeContentSuggestionsProviderObserver
+    : public ContentSuggestionsProvider::Observer {
  public:
-  MOCK_METHOD2(MockRun,
-               void(Category category,
-                    std::vector<ContentSuggestion>* dismissed_suggestions));
-  void Run(Category category,
-           std::vector<ContentSuggestion> dismissed_suggestions) {
-    MockRun(category, &dismissed_suggestions);
+  FakeContentSuggestionsProviderObserver()
+      : loaded_(base::WaitableEvent::ResetPolicy::MANUAL,
+                base::WaitableEvent::InitialState::NOT_SIGNALED) {}
+
+  void OnNewSuggestions(ContentSuggestionsProvider* provider,
+                        Category category,
+                        std::vector<ContentSuggestion> suggestions) override {
+    suggestions_[category] = std::move(suggestions);
   }
+
+  void OnCategoryStatusChanged(ContentSuggestionsProvider* provider,
+                               Category category,
+                               CategoryStatus new_status) override {
+    loaded_.Signal();
+    statuses_[category] = new_status;
+  }
+
+  void OnSuggestionInvalidated(ContentSuggestionsProvider* provider,
+                               Category category,
+                               const std::string& suggestion_id) override {}
+
+  CategoryStatus StatusForCategory(Category category) const {
+    auto it = statuses_.find(category);
+    if (it == statuses_.end()) {
+      return CategoryStatus::NOT_PROVIDED;
+    }
+    return it->second;
+  }
+
+  const std::vector<ContentSuggestion>& SuggestionsForCategory(
+      Category category) {
+    return suggestions_[category];
+  }
+
+  void WaitForLoad() { loaded_.Wait(); }
+
+  void Reset() {
+    loaded_.Reset();
+    statuses_.clear();
+  }
+
+ private:
+  base::WaitableEvent loaded_;
+  std::map<Category, CategoryStatus, Category::CompareByID> statuses_;
+  std::map<Category, std::vector<ContentSuggestion>, Category::CompareByID>
+      suggestions_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeContentSuggestionsProviderObserver);
 };
 
 }  // namespace
 
-class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase {
+class NTPSnippetsServiceTest : public ::testing::Test {
  public:
   NTPSnippetsServiceTest()
-      : fake_url_fetcher_factory_(
+      : params_manager_(ntp_snippets::kStudyName,
+                        {{"content_suggestions_backend",
+                          kTestContentSuggestionsServerEndpoint}}),
+        fake_url_fetcher_factory_(
             /*default_factory=*/&failing_url_fetcher_factory_),
-        test_url_(base::StringPrintf(kTestContentSnippetsServerFormat,
-                                     google_apis::GetAPIKey().c_str())) {
-    NTPSnippetsService::RegisterProfilePrefs(pref_service()->registry());
-    RequestThrottler::RegisterProfilePrefs(pref_service()->registry());
+        test_url_(base::StringPrintf(kTestContentSuggestionsServerFormat,
+                                     google_apis::GetAPIKey().c_str())),
+
+        observer_(base::MakeUnique<FakeContentSuggestionsProviderObserver>()) {
+    NTPSnippetsService::RegisterProfilePrefs(utils_.pref_service()->registry());
+    RequestThrottler::RegisterProfilePrefs(utils_.pref_service()->registry());
 
     // Since no SuggestionsService is injected in tests, we need to force the
     // service to fetch from all hosts.
@@ -315,67 +333,57 @@ class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase {
     // We need to run the message loop after deleting the database, because
     // ProtoDatabaseImpl deletes the actual LevelDB asynchronously on the task
     // runner. Without this, we'd get reports of memory leaks.
-    Mock::VerifyAndClear(&mock_scheduler());
-    service_.reset();
     base::RunLoop().RunUntilIdle();
   }
 
-  void SetUp() override {
-    test::NTPSnippetsTestBase::SetUp();
-    EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)).Times(1);
-    CreateSnippetsService();
-  }
-
-  void RecreateSnippetsService() {
-    Mock::VerifyAndClear(&mock_scheduler());
-    service_.reset();
-    base::RunLoop().RunUntilIdle();
-    EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)).Times(1);
-    CreateSnippetsService();
-  }
-
-  void CreateSnippetsService() {
-    DCHECK(!service_);
-
+  std::unique_ptr<NTPSnippetsService> MakeSnippetsService() {
     scoped_refptr<base::SingleThreadTaskRunner> task_runner(
         base::ThreadTaskRunnerHandle::Get());
     scoped_refptr<net::TestURLRequestContextGetter> request_context_getter =
         new net::TestURLRequestContextGetter(task_runner.get());
 
-    ResetSigninManager();
+    utils_.ResetSigninManager();
     std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher =
         base::MakeUnique<NTPSnippetsFetcher>(
-            fake_signin_manager(), fake_token_service_.get(),
-            std::move(request_context_getter), pref_service(),
+            utils_.fake_signin_manager(), fake_token_service_.get(),
+            std::move(request_context_getter), utils_.pref_service(),
             &category_factory_, base::Bind(&ParseJson),
             /*is_stable_channel=*/true);
 
-    fake_signin_manager()->SignIn("foo@bar.com");
+    utils_.fake_signin_manager()->SignIn("foo@bar.com");
     snippets_fetcher->SetPersonalizationForTesting(
         NTPSnippetsFetcher::Personalization::kNonPersonal);
 
-    auto image_fetcher =
-        base::MakeUnique<testing::NiceMock<MockImageFetcher>>();
+    auto image_fetcher = base::MakeUnique<NiceMock<MockImageFetcher>>();
     image_fetcher_ = image_fetcher.get();
 
     // Add an initial fetch response, as the service tries to fetch when there
     // is nothing in the DB.
-    SetUpFetchResponse(GetTestJson({GetSnippet()}));
+    SetUpFetchResponse(GetTestJson({}));
 
-    service_.reset(new NTPSnippetsService(
-        &observer_, &category_factory_, pref_service(), nullptr, nullptr, "fr",
-        &scheduler_, std::move(snippets_fetcher),
+    auto service = base::MakeUnique<NTPSnippetsService>(
+        observer_.get(), &category_factory_, utils_.pref_service(), nullptr,
+        nullptr, "fr", &scheduler_, std::move(snippets_fetcher),
         std::move(image_fetcher), /*image_decoder=*/nullptr,
         base::MakeUnique<NTPSnippetsDatabase>(database_dir_.path(),
                                               task_runner),
-        base::MakeUnique<NTPSnippetsStatusService>(fake_signin_manager(),
-                                                   pref_service())));
+        base::MakeUnique<NTPSnippetsStatusService>(utils_.fake_signin_manager(),
+                                                   utils_.pref_service()));
 
-    WaitForDBLoad(&observer_, service_.get());
+    base::RunLoop().RunUntilIdle();
+    observer_->WaitForLoad();
+    return service;
   }
 
-  std::string MakeUniqueID(const std::string& within_category_id) {
-    return service()->MakeUniqueID(articles_category(), within_category_id);
+  void ResetSnippetsService(std::unique_ptr<NTPSnippetsService>* service) {
+    service->reset();
+    observer_ = base::MakeUnique<FakeContentSuggestionsProviderObserver>();
+    *service = MakeSnippetsService();
+  }
+
+  std::string MakeUniqueID(const NTPSnippetsService& service,
+                           const std::string& within_category_id) {
+    return service.MakeUniqueID(articles_category(), within_category_id);
   }
 
   Category articles_category() {
@@ -384,12 +392,9 @@ class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase {
 
  protected:
   const GURL& test_url() { return test_url_; }
-  NTPSnippetsService* service() { return service_.get(); }
-  MockContentSuggestionsProviderObserver& observer() { return observer_; }
+  FakeContentSuggestionsProviderObserver& observer() { return *observer_; }
   MockScheduler& mock_scheduler() { return scheduler_; }
-  testing::NiceMock<MockImageFetcher>* image_fetcher() {
-    return image_fetcher_;
-  }
+  NiceMock<MockImageFetcher>* image_fetcher() { return image_fetcher_; }
 
   // Provide the json to be returned by the fake fetcher.
   void SetUpFetchResponse(const std::string& json) {
@@ -397,25 +402,26 @@ class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase {
                                               net::URLRequestStatus::SUCCESS);
   }
 
-  void LoadFromJSONString(const std::string& json) {
+  void LoadFromJSONString(NTPSnippetsService* service,
+                          const std::string& json) {
     SetUpFetchResponse(json);
-    service()->FetchSnippets(true);
+    service->FetchSnippets(true);
     base::RunLoop().RunUntilIdle();
   }
 
  private:
+  variations::testing::VariationParamsManager params_manager_;
+  test::NTPSnippetsTestUtils utils_;
   base::MessageLoop message_loop_;
   FailingFakeURLFetcherFactory failing_url_fetcher_factory_;
   // Instantiation of factory automatically sets itself as URLFetcher's factory.
   net::FakeURLFetcherFactory fake_url_fetcher_factory_;
   const GURL test_url_;
   std::unique_ptr<OAuth2TokenService> fake_token_service_;
-  MockScheduler scheduler_;
-  MockContentSuggestionsProviderObserver observer_;
+  StrictMock<MockScheduler> scheduler_;
+  std::unique_ptr<FakeContentSuggestionsProviderObserver> observer_;
   CategoryFactory category_factory_;
-  testing::NiceMock<MockImageFetcher>* image_fetcher_;
-  // Last so that the dependencies are deleted after the service.
-  std::unique_ptr<NTPSnippetsService> service_;
+  NiceMock<MockImageFetcher>* image_fetcher_;
 
   base::ScopedTempDir database_dir_;
 
@@ -423,52 +429,69 @@ class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase {
 };
 
 TEST_F(NTPSnippetsServiceTest, ScheduleOnStart) {
-  // SetUp() checks that Schedule is called.
+  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _));
+  auto service = MakeSnippetsService();
 
   // When we have no snippets are all, loading the service initiates a fetch.
   base::RunLoop().RunUntilIdle();
-  ASSERT_EQ("OK", service()->snippets_fetcher()->last_status());
+  ASSERT_EQ("OK", service->snippets_fetcher()->last_status());
 }
 
 TEST_F(NTPSnippetsServiceTest, Full) {
   std::string json_str(GetTestJson({GetSnippet()}));
 
-  LoadFromJSONString(json_str);
-  ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1));
-  const NTPSnippet& snippet = *service()->GetSnippetsForTesting().front();
+  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _));
+  auto service = MakeSnippetsService();
 
-  EXPECT_EQ(snippet.id(), kSnippetUrl);
-  EXPECT_EQ(snippet.title(), kSnippetTitle);
-  EXPECT_EQ(snippet.snippet(), kSnippetText);
-  EXPECT_EQ(snippet.salient_image_url(), GURL(kSnippetSalientImage));
-  EXPECT_EQ(GetDefaultCreationTime(), snippet.publish_date());
-  EXPECT_EQ(snippet.best_source().publisher_name, kSnippetPublisherName);
-  EXPECT_EQ(snippet.best_source().amp_url, GURL(kSnippetAmpUrl));
+  LoadFromJSONString(service.get(), json_str);
+  ASSERT_THAT(observer().SuggestionsForCategory(articles_category()),
+              SizeIs(1));
+  ASSERT_THAT(service->GetSnippetsForTesting(), SizeIs(1));
+  const ContentSuggestion& suggestion =
+      observer().SuggestionsForCategory(articles_category()).front();
+
+  EXPECT_EQ(MakeUniqueID(*service, kSnippetUrl), suggestion.id());
+  EXPECT_EQ(kSnippetTitle, base::UTF16ToUTF8(suggestion.title()));
+  EXPECT_EQ(kSnippetText, base::UTF16ToUTF8(suggestion.snippet_text()));
+  // EXPECT_EQ(GURL(kSnippetSalientImage), suggestion.salient_image_url());
+  EXPECT_EQ(GetDefaultCreationTime(), suggestion.publish_date());
+  EXPECT_EQ(kSnippetPublisherName,
+            base::UTF16ToUTF8(suggestion.publisher_name()));
+  EXPECT_EQ(GURL(kSnippetAmpUrl), suggestion.amp_url());
 }
 
 TEST_F(NTPSnippetsServiceTest, Clear) {
+  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _));
+  auto service = MakeSnippetsService();
+
   std::string json_str(GetTestJson({GetSnippet()}));
 
-  LoadFromJSONString(json_str);
-  EXPECT_THAT(service()->GetSnippetsForTesting(), SizeIs(1));
+  LoadFromJSONString(service.get(), json_str);
+  EXPECT_THAT(service->GetSnippetsForTesting(), SizeIs(1));
 
-  service()->ClearCachedSuggestions(articles_category());
-  EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty());
+  service->ClearCachedSuggestions(articles_category());
+  EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty());
 }
 
 TEST_F(NTPSnippetsServiceTest, InsertAtFront) {
+  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _));
+  auto service = MakeSnippetsService();
+
   std::string first("http://first");
-  LoadFromJSONString(GetTestJson({GetSnippetWithUrl(first)}));
-  EXPECT_THAT(service()->GetSnippetsForTesting(), ElementsAre(IdEq(first)));
+  LoadFromJSONString(service.get(), GetTestJson({GetSnippetWithUrl(first)}));
+  EXPECT_THAT(service->GetSnippetsForTesting(), ElementsAre(IdEq(first)));
 
   std::string second("http://second");
-  LoadFromJSONString(GetTestJson({GetSnippetWithUrl(second)}));
+  LoadFromJSONString(service.get(), GetTestJson({GetSnippetWithUrl(second)}));
   // The snippet loaded last should be at the first position in the list now.
-  EXPECT_THAT(service()->GetSnippetsForTesting(),
+  EXPECT_THAT(service->GetSnippetsForTesting(),
               ElementsAre(IdEq(second), IdEq(first)));
 }
 
 TEST_F(NTPSnippetsServiceTest, LimitNumSnippets) {
+  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _));
+  auto service = MakeSnippetsService();
+
   int max_snippet_count = NTPSnippetsService::GetMaxSnippetCountForTesting();
   int snippets_per_load = max_snippet_count / 2 + 1;
   char url_format[] = "http://localhost/%i";
@@ -481,154 +504,166 @@ TEST_F(NTPSnippetsServiceTest, LimitNumSnippets) {
         base::StringPrintf(url_format, snippets_per_load + i)));
   }
 
-  LoadFromJSONString(GetTestJson(snippets1));
-  ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(snippets1.size()));
+  LoadFromJSONString(service.get(), GetTestJson(snippets1));
+  ASSERT_THAT(service->GetSnippetsForTesting(), SizeIs(snippets1.size()));
 
-  LoadFromJSONString(GetTestJson(snippets2));
-  EXPECT_THAT(service()->GetSnippetsForTesting(), SizeIs(max_snippet_count));
+  LoadFromJSONString(service.get(), GetTestJson(snippets2));
+  EXPECT_THAT(service->GetSnippetsForTesting(), SizeIs(max_snippet_count));
 }
 
 TEST_F(NTPSnippetsServiceTest, LoadInvalidJson) {
-  LoadFromJSONString(GetTestJson({GetInvalidSnippet()}));
-  EXPECT_THAT(service()->snippets_fetcher()->last_status(),
+  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _));
+  auto service = MakeSnippetsService();
+
+  LoadFromJSONString(service.get(), GetTestJson({GetInvalidSnippet()}));
+  EXPECT_THAT(service->snippets_fetcher()->last_status(),
               StartsWith("Received invalid JSON"));
-  EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty());
+  EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty());
 }
 
 TEST_F(NTPSnippetsServiceTest, LoadInvalidJsonWithExistingSnippets) {
-  LoadFromJSONString(GetTestJson({GetSnippet()}));
-  ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1));
-  ASSERT_EQ("OK", service()->snippets_fetcher()->last_status());
+  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _));
+  auto service = MakeSnippetsService();
 
-  LoadFromJSONString(GetTestJson({GetInvalidSnippet()}));
-  EXPECT_THAT(service()->snippets_fetcher()->last_status(),
+  LoadFromJSONString(service.get(), GetTestJson({GetSnippet()}));
+  ASSERT_THAT(service->GetSnippetsForTesting(), SizeIs(1));
+  ASSERT_EQ("OK", service->snippets_fetcher()->last_status());
+
+  LoadFromJSONString(service.get(), GetTestJson({GetInvalidSnippet()}));
+  EXPECT_THAT(service->snippets_fetcher()->last_status(),
               StartsWith("Received invalid JSON"));
   // This should not have changed the existing snippets.
-  EXPECT_THAT(service()->GetSnippetsForTesting(), SizeIs(1));
+  EXPECT_THAT(service->GetSnippetsForTesting(), SizeIs(1));
 }
 
 TEST_F(NTPSnippetsServiceTest, LoadIncompleteJson) {
-  LoadFromJSONString(GetTestJson({GetIncompleteSnippet()}));
+  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _));
+  auto service = MakeSnippetsService();
+
+  LoadFromJSONString(service.get(), GetTestJson({GetIncompleteSnippet()}));
   EXPECT_EQ("Invalid / empty list.",
-            service()->snippets_fetcher()->last_status());
-  EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty());
+            service->snippets_fetcher()->last_status());
+  EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty());
 }
 
 TEST_F(NTPSnippetsServiceTest, LoadIncompleteJsonWithExistingSnippets) {
-  LoadFromJSONString(GetTestJson({GetSnippet()}));
-  ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1));
+  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _));
+  auto service = MakeSnippetsService();
 
-  LoadFromJSONString(GetTestJson({GetIncompleteSnippet()}));
+  LoadFromJSONString(service.get(), GetTestJson({GetSnippet()}));
+  ASSERT_THAT(service->GetSnippetsForTesting(), SizeIs(1));
+
+  LoadFromJSONString(service.get(), GetTestJson({GetIncompleteSnippet()}));
   EXPECT_EQ("Invalid / empty list.",
-            service()->snippets_fetcher()->last_status());
+            service->snippets_fetcher()->last_status());
   // This should not have changed the existing snippets.
-  EXPECT_THAT(service()->GetSnippetsForTesting(), SizeIs(1));
+  EXPECT_THAT(service->GetSnippetsForTesting(), SizeIs(1));
 }
 
 TEST_F(NTPSnippetsServiceTest, Dismiss) {
-  std::vector<std::string> source_urls, publishers, amp_urls;
-  source_urls.push_back(std::string("http://site.com"));
-  publishers.push_back(std::string("Source 1"));
-  amp_urls.push_back(std::string());
+  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)).Times(2);
+  auto service = MakeSnippetsService();
+
   std::string json_str(
-      GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)}));
+      GetTestJson({GetSnippetWithSources("http://site.com", "Source 1", "")}));
 
-  LoadFromJSONString(json_str);
+  LoadFromJSONString(service.get(), json_str);
 
-  ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1));
+  ASSERT_THAT(service->GetSnippetsForTesting(), SizeIs(1));
 
   // Dismissing a non-existent snippet shouldn't do anything.
-  service()->DismissSuggestion(MakeUniqueID("http://othersite.com"));
-  EXPECT_THAT(service()->GetSnippetsForTesting(), SizeIs(1));
+  service->DismissSuggestion(MakeUniqueID(*service, "http://othersite.com"));
+  EXPECT_THAT(service->GetSnippetsForTesting(), SizeIs(1));
 
   // Dismiss the snippet.
-  service()->DismissSuggestion(MakeUniqueID(kSnippetUrl));
-  EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty());
+  service->DismissSuggestion(MakeUniqueID(*service, kSnippetUrl));
+  EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty());
 
   // Make sure that fetching the same snippet again does not re-add it.
-  LoadFromJSONString(json_str);
-  EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty());
+  LoadFromJSONString(service.get(), json_str);
+  EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty());
 
   // The snippet should stay dismissed even after re-creating the service.
-  RecreateSnippetsService();
-  LoadFromJSONString(json_str);
-  EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty());
+  ResetSnippetsService(&service);
+  LoadFromJSONString(service.get(), json_str);
+  EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty());
 
   // The snippet can be added again after clearing dismissed snippets.
-  service()->ClearDismissedSuggestionsForDebugging(articles_category());
-  EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty());
-  LoadFromJSONString(json_str);
-  EXPECT_THAT(service()->GetSnippetsForTesting(), SizeIs(1));
+  service->ClearDismissedSuggestionsForDebugging(articles_category());
+  EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty());
+  LoadFromJSONString(service.get(), json_str);
+  EXPECT_THAT(service->GetSnippetsForTesting(), SizeIs(1));
 }
 
 TEST_F(NTPSnippetsServiceTest, GetDismissed) {
-  LoadFromJSONString(GetTestJson({GetSnippet()}));
+  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _));
+  auto service = MakeSnippetsService();
 
-  service()->DismissSuggestion(MakeUniqueID(kSnippetUrl));
+  LoadFromJSONString(service.get(), GetTestJson({GetSnippet()}));
 
-  MockDismissedSuggestionsCallback callback;
+  service->DismissSuggestion(MakeUniqueID(*service, kSnippetUrl));
 
-  EXPECT_CALL(callback, MockRun(_, _))
-      .WillOnce(
-          Invoke([this](Category category,
-                        std::vector<ContentSuggestion>* dismissed_suggestions) {
-            EXPECT_EQ(1u, dismissed_suggestions->size());
-            for (auto& suggestion : *dismissed_suggestions) {
-              EXPECT_EQ(MakeUniqueID(kSnippetUrl), suggestion.id());
+  service->GetDismissedSuggestionsForDebugging(
+      articles_category(),
+      base::Bind(
+          [](NTPSnippetsService* service, NTPSnippetsServiceTest* test,
+             std::vector<ContentSuggestion> dismissed_suggestions) {
+            EXPECT_EQ(1u, dismissed_suggestions.size());
+            for (auto& suggestion : dismissed_suggestions) {
+              EXPECT_EQ(test->MakeUniqueID(*service, kSnippetUrl),
+                        suggestion.id());
             }
-          }));
-  service()->GetDismissedSuggestionsForDebugging(
-      articles_category(),
-      base::Bind(&MockDismissedSuggestionsCallback::Run,
-                 base::Unretained(&callback), articles_category()));
-  Mock::VerifyAndClearExpectations(&callback);
+          },
+          service.get(), this));
+  base::RunLoop().RunUntilIdle();
 
   // There should be no dismissed snippet after clearing the list.
-  EXPECT_CALL(callback, MockRun(_, _))
-      .WillOnce(
-          Invoke([this](Category category,
-                        std::vector<ContentSuggestion>* dismissed_suggestions) {
-            EXPECT_EQ(0u, dismissed_suggestions->size());
-          }));
-  service()->ClearDismissedSuggestionsForDebugging(articles_category());
-  service()->GetDismissedSuggestionsForDebugging(
+  service->ClearDismissedSuggestionsForDebugging(articles_category());
+  service->GetDismissedSuggestionsForDebugging(
       articles_category(),
-      base::Bind(&MockDismissedSuggestionsCallback::Run,
-                 base::Unretained(&callback), articles_category()));
+      base::Bind(
+          [](NTPSnippetsService* service, NTPSnippetsServiceTest* test,
+             std::vector<ContentSuggestion> dismissed_suggestions) {
+            EXPECT_EQ(0u, dismissed_suggestions.size());
+          },
+          service.get(), this));
+  base::RunLoop().RunUntilIdle();
 }
 
 TEST_F(NTPSnippetsServiceTest, CreationTimestampParseFail) {
-  std::string json_str(GetTestJson({GetSnippetWithTimes(
-      "aaa1448459205",
-      NTPSnippet::TimeToJsonString(GetDefaultExpirationTime()))}));
+  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _));
+  auto service = MakeSnippetsService();
 
-  LoadFromJSONString(json_str);
-  ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1));
-  const NTPSnippet& snippet = *service()->GetSnippetsForTesting().front();
-  EXPECT_EQ(snippet.id(), kSnippetUrl);
-  EXPECT_EQ(snippet.title(), kSnippetTitle);
-  EXPECT_EQ(snippet.snippet(), kSnippetText);
-  EXPECT_EQ(base::Time::UnixEpoch(), snippet.publish_date());
+  std::string json =
+      GetSnippetWithTimes(GetDefaultCreationTime(), GetDefaultExpirationTime());
+  base::ReplaceFirstSubstringAfterOffset(
+      &json, 0, FormatTime(GetDefaultCreationTime()), "aaa1448459205");
+  std::string json_str(GetTestJson({json}));
+
+  LoadFromJSONString(service.get(), json_str);
+  EXPECT_THAT(service->GetSnippetsForTesting(), SizeIs(0));

[Marc Treib, 2016/08/30 13:43:39]

nit: IsEmpty() rather than SizeIs(0)

[sfiera, 2016/08/30 16:33:09]

Done.

 }
 
 TEST_F(NTPSnippetsServiceTest, RemoveExpiredContent) {
+  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _));
+  auto service = MakeSnippetsService();
+
   std::string json_str(GetTestJson({GetExpiredSnippet()}));
 
-  LoadFromJSONString(json_str);
-  EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty());
+  LoadFromJSONString(service.get(), json_str);
+  EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty());
 }
 
 TEST_F(NTPSnippetsServiceTest, TestSingleSource) {
-  std::vector<std::string> source_urls, publishers, amp_urls;
-  source_urls.push_back(std::string("http://source1.com"));
-  publishers.push_back(std::string("Source 1"));
-  amp_urls.push_back(std::string("http://source1.amp.com"));
-  std::string json_str(
-      GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)}));
+  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _));
+  auto service = MakeSnippetsService();
 
-  LoadFromJSONString(json_str);
-  ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1));
-  const NTPSnippet& snippet = *service()->GetSnippetsForTesting().front();
+  std::string json_str(GetTestJson({GetSnippetWithSources(
+      "http://source1.com", "Source 1", "http://source1.amp.com")}));
+
+  LoadFromJSONString(service.get(), json_str);
+  ASSERT_THAT(service->GetSnippetsForTesting(), SizeIs(1));
+  const NTPSnippet& snippet = *service->GetSnippetsForTesting().front();
   EXPECT_EQ(snippet.sources().size(), 1u);
   EXPECT_EQ(snippet.id(), kSnippetUrl);
   EXPECT_EQ(snippet.best_source().url, GURL("http://source1.com"));
@@ -637,229 +672,59 @@ TEST_F(NTPSnippetsServiceTest, TestSingleSource) {
 }
 
 TEST_F(NTPSnippetsServiceTest, TestSingleSourceWithMalformedUrl) {
-  std::vector<std::string> source_urls, publishers, amp_urls;
-  source_urls.push_back(std::string("aaaa"));
-  publishers.push_back(std::string("Source 1"));
-  amp_urls.push_back(std::string("http://source1.amp.com"));
-  std::string json_str(
-      GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)}));
+  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _));
+  auto service = MakeSnippetsService();
 
-  LoadFromJSONString(json_str);
-  EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty());
+  std::string json_str(GetTestJson({GetSnippetWithSources(
+      "ceci n'est pas un url", "Source 1", "http://source1.amp.com")}));
+
+  LoadFromJSONString(service.get(), json_str);
+  EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty());
 }
 
 TEST_F(NTPSnippetsServiceTest, TestSingleSourceWithMissingData) {
-  std::vector<std::string> source_urls, publishers, amp_urls;
-  source_urls.push_back(std::string("http://source1.com"));
-  publishers.push_back(std::string());
-  amp_urls.push_back(std::string());
-  std::string json_str(
-      GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)}));
-
-  LoadFromJSONString(json_str);
-  EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty());
-}
-
-TEST_F(NTPSnippetsServiceTest, TestMultipleSources) {
-  std::vector<std::string> source_urls, publishers, amp_urls;
-  source_urls.push_back(std::string("http://source1.com"));
-  source_urls.push_back(std::string("http://source2.com"));
-  publishers.push_back(std::string("Source 1"));
-  publishers.push_back(std::string("Source 2"));
-  amp_urls.push_back(std::string("http://source1.amp.com"));
-  amp_urls.push_back(std::string("http://source2.amp.com"));
-  std::string json_str(
-      GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)}));
-
-  LoadFromJSONString(json_str);
-  ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1));
-  const NTPSnippet& snippet = *service()->GetSnippetsForTesting().front();
-  // Expect the first source to be chosen
-  EXPECT_EQ(snippet.sources().size(), 2u);
-  EXPECT_EQ(snippet.id(), kSnippetUrl);
-  EXPECT_EQ(snippet.best_source().url, GURL("http://source1.com"));
-  EXPECT_EQ(snippet.best_source().publisher_name, std::string("Source 1"));
-  EXPECT_EQ(snippet.best_source().amp_url, GURL("http://source1.amp.com"));
-}
-
-TEST_F(NTPSnippetsServiceTest, TestMultipleIncompleteSources) {
-  // Set Source 2 to have no AMP url, and Source 1 to have no publisher name
-  // Source 2 should win since we favor publisher name over amp url
-  std::vector<std::string> source_urls, publishers, amp_urls;
-  source_urls.push_back(std::string("http://source1.com"));
-  source_urls.push_back(std::string("http://source2.com"));
-  publishers.push_back(std::string());
-  publishers.push_back(std::string("Source 2"));
-  amp_urls.push_back(std::string("http://source1.amp.com"));
-  amp_urls.push_back(std::string());
-  std::string json_str(
-      GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)}));
-
-  LoadFromJSONString(json_str);
-  ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1));
-  {
-    const NTPSnippet& snippet = *service()->GetSnippetsForTesting().front();
-    EXPECT_EQ(snippet.sources().size(), 2u);
-    EXPECT_EQ(snippet.id(), kSnippetUrl);
-    EXPECT_EQ(snippet.best_source().url, GURL("http://source2.com"));
-    EXPECT_EQ(snippet.best_source().publisher_name, std::string("Source 2"));
-    EXPECT_EQ(snippet.best_source().amp_url, GURL());
-  }
+  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _));
+  auto service = MakeSnippetsService();
 
-  service()->ClearCachedSuggestions(articles_category());
-  // Set Source 1 to have no AMP url, and Source 2 to have no publisher name
-  // Source 1 should win in this case since we prefer publisher name to AMP url
-  source_urls.clear();
-  source_urls.push_back(std::string("http://source1.com"));
-  source_urls.push_back(std::string("http://source2.com"));
-  publishers.clear();
-  publishers.push_back(std::string("Source 1"));
-  publishers.push_back(std::string());
-  amp_urls.clear();
-  amp_urls.push_back(std::string());
-  amp_urls.push_back(std::string("http://source2.amp.com"));
-  json_str =
-      GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)});
-
-  LoadFromJSONString(json_str);
-  ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1));
-  {
-    const NTPSnippet& snippet = *service()->GetSnippetsForTesting().front();
-    EXPECT_EQ(snippet.sources().size(), 2u);
-    EXPECT_EQ(snippet.id(), kSnippetUrl);
-    EXPECT_EQ(snippet.best_source().url, GURL("http://source1.com"));
-    EXPECT_EQ(snippet.best_source().publisher_name, std::string("Source 1"));
-    EXPECT_EQ(snippet.best_source().amp_url, GURL());
-  }
-
-  service()->ClearCachedSuggestions(articles_category());
-  // Set source 1 to have no AMP url and no source, and source 2 to only have
-  // amp url. There should be no snippets since we only add sources we consider
-  // complete
-  source_urls.clear();
-  source_urls.push_back(std::string("http://source1.com"));
-  source_urls.push_back(std::string("http://source2.com"));
-  publishers.clear();
-  publishers.push_back(std::string());
-  publishers.push_back(std::string());
-  amp_urls.clear();
-  amp_urls.push_back(std::string());
-  amp_urls.push_back(std::string("http://source2.amp.com"));
-  json_str =
-      GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)});
-
-  LoadFromJSONString(json_str);
-  EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty());
-}
-
-TEST_F(NTPSnippetsServiceTest, TestMultipleCompleteSources) {
-  // Test 2 complete sources, we should choose the first complete source
-  std::vector<std::string> source_urls, publishers, amp_urls;
-  source_urls.push_back(std::string("http://source1.com"));
-  source_urls.push_back(std::string("http://source2.com"));
-  source_urls.push_back(std::string("http://source3.com"));
-  publishers.push_back(std::string("Source 1"));
-  publishers.push_back(std::string());
-  publishers.push_back(std::string("Source 3"));
-  amp_urls.push_back(std::string("http://source1.amp.com"));
-  amp_urls.push_back(std::string("http://source2.amp.com"));
-  amp_urls.push_back(std::string("http://source3.amp.com"));
   std::string json_str(
-      GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)}));
-
-  LoadFromJSONString(json_str);
-  ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1));
-  {
-    const NTPSnippet& snippet = *service()->GetSnippetsForTesting().front();
-    EXPECT_EQ(snippet.sources().size(), 3u);
-    EXPECT_EQ(snippet.id(), kSnippetUrl);
-    EXPECT_EQ(snippet.best_source().url, GURL("http://source1.com"));
-    EXPECT_EQ(snippet.best_source().publisher_name, std::string("Source 1"));
-    EXPECT_EQ(snippet.best_source().amp_url, GURL("http://source1.amp.com"));
-  }
-
-  // Test 2 complete sources, we should choose the first complete source
-  service()->ClearCachedSuggestions(articles_category());
-  source_urls.clear();
-  source_urls.push_back(std::string("http://source1.com"));
-  source_urls.push_back(std::string("http://source2.com"));
-  source_urls.push_back(std::string("http://source3.com"));
-  publishers.clear();
-  publishers.push_back(std::string());
-  publishers.push_back(std::string("Source 2"));
-  publishers.push_back(std::string("Source 3"));
-  amp_urls.clear();
-  amp_urls.push_back(std::string("http://source1.amp.com"));
-  amp_urls.push_back(std::string("http://source2.amp.com"));
-  amp_urls.push_back(std::string("http://source3.amp.com"));
-  json_str =
-      GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)});
-
-  LoadFromJSONString(json_str);
-  ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1));
-  {
-    const NTPSnippet& snippet = *service()->GetSnippetsForTesting().front();
-    EXPECT_EQ(snippet.sources().size(), 3u);
-    EXPECT_EQ(snippet.id(), kSnippetUrl);
-    EXPECT_EQ(snippet.best_source().url, GURL("http://source2.com"));
-    EXPECT_EQ(snippet.best_source().publisher_name, std::string("Source 2"));
-    EXPECT_EQ(snippet.best_source().amp_url, GURL("http://source2.amp.com"));
-  }
-
-  // Test 3 complete sources, we should choose the first complete source
-  service()->ClearCachedSuggestions(articles_category());
-  source_urls.clear();
-  source_urls.push_back(std::string("http://source1.com"));
-  source_urls.push_back(std::string("http://source2.com"));
-  source_urls.push_back(std::string("http://source3.com"));
-  publishers.clear();
-  publishers.push_back(std::string("Source 1"));
-  publishers.push_back(std::string("Source 2"));
-  publishers.push_back(std::string("Source 3"));
-  amp_urls.clear();
-  amp_urls.push_back(std::string());
-  amp_urls.push_back(std::string("http://source2.amp.com"));
-  amp_urls.push_back(std::string("http://source3.amp.com"));
-  json_str =
-      GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)});
+      GetTestJson({GetSnippetWithSources("http://source1.com", "", "")}));
 
-  LoadFromJSONString(json_str);
-  ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1));
-  {
-    const NTPSnippet& snippet = *service()->GetSnippetsForTesting().front();
-    EXPECT_EQ(snippet.sources().size(), 3u);
-    EXPECT_EQ(snippet.id(), kSnippetUrl);
-    EXPECT_EQ(snippet.best_source().url, GURL("http://source2.com"));
-    EXPECT_EQ(snippet.best_source().publisher_name, std::string("Source 2"));
-    EXPECT_EQ(snippet.best_source().amp_url, GURL("http://source2.amp.com"));
-  }
+  LoadFromJSONString(service.get(), json_str);
+  EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty());
 }
 
 TEST_F(NTPSnippetsServiceTest, LogNumArticlesHistogram) {
+  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)).Times(3);

[Marc Treib, 2016/08/30 13:43:39]

?

I think this is evidence in favor of Tim's argument for a NiceMock: I don't
think this test particularly cares if/how often Schedule gets called.

[sfiera, 2016/08/30 16:33:09]

Done.

+  auto service = MakeSnippetsService();
+
   base::HistogramTester tester;
-  LoadFromJSONString(GetTestJson({GetInvalidSnippet()}));
+  LoadFromJSONString(service.get(), GetTestJson({GetInvalidSnippet()}));
 
   EXPECT_THAT(tester.GetAllSamples("NewTabPage.Snippets.NumArticles"),
               ElementsAre(base::Bucket(/*min=*/0, /*count=*/1)));
+
   // Invalid JSON shouldn't contribute to NumArticlesFetched.
   EXPECT_THAT(tester.GetAllSamples("NewTabPage.Snippets.NumArticlesFetched"),
               IsEmpty());
+
   // Valid JSON with empty list.
-  LoadFromJSONString(GetTestJson(std::vector<std::string>()));
+  LoadFromJSONString(service.get(), GetTestJson(std::vector<std::string>()));
   EXPECT_THAT(tester.GetAllSamples("NewTabPage.Snippets.NumArticles"),
               ElementsAre(base::Bucket(/*min=*/0, /*count=*/2)));
   EXPECT_THAT(tester.GetAllSamples("NewTabPage.Snippets.NumArticlesFetched"),
               ElementsAre(base::Bucket(/*min=*/0, /*count=*/1)));
+
   // Snippet list should be populated with size 1.
-  LoadFromJSONString(GetTestJson({GetSnippet()}));
+  LoadFromJSONString(service.get(), GetTestJson({GetSnippet()}));
   EXPECT_THAT(tester.GetAllSamples("NewTabPage.Snippets.NumArticles"),
               ElementsAre(base::Bucket(/*min=*/0, /*count=*/2),
                           base::Bucket(/*min=*/1, /*count=*/1)));
   EXPECT_THAT(tester.GetAllSamples("NewTabPage.Snippets.NumArticlesFetched"),
               ElementsAre(base::Bucket(/*min=*/0, /*count=*/1),
                           base::Bucket(/*min=*/1, /*count=*/1)));
+
   // Duplicate snippet shouldn't increase the list size.
-  LoadFromJSONString(GetTestJson({GetSnippet()}));
+  LoadFromJSONString(service.get(), GetTestJson({GetSnippet()}));
   EXPECT_THAT(tester.GetAllSamples("NewTabPage.Snippets.NumArticles"),
               ElementsAre(base::Bucket(/*min=*/0, /*count=*/2),
                           base::Bucket(/*min=*/1, /*count=*/2)));
@@ -869,10 +734,11 @@ TEST_F(NTPSnippetsServiceTest, LogNumArticlesHistogram) {
   EXPECT_THAT(
       tester.GetAllSamples("NewTabPage.Snippets.NumArticlesZeroDueToDiscarded"),
       IsEmpty());
+
   // Dismissing a snippet should decrease the list size. This will only be
   // logged after the next fetch.
-  service()->DismissSuggestion(MakeUniqueID(kSnippetUrl));
-  LoadFromJSONString(GetTestJson({GetSnippet()}));
+  service->DismissSuggestion(MakeUniqueID(*service, kSnippetUrl));
+  LoadFromJSONString(service.get(), GetTestJson({GetSnippet()}));
   EXPECT_THAT(tester.GetAllSamples("NewTabPage.Snippets.NumArticles"),
               ElementsAre(base::Bucket(/*min=*/0, /*count=*/3),
                           base::Bucket(/*min=*/1, /*count=*/2)));
@@ -883,93 +749,134 @@ TEST_F(NTPSnippetsServiceTest, LogNumArticlesHistogram) {
   EXPECT_THAT(
       tester.GetAllSamples("NewTabPage.Snippets.NumArticlesZeroDueToDiscarded"),
       ElementsAre(base::Bucket(/*min=*/1, /*count=*/1)));
-  // Recreating the service and loading from prefs shouldn't count as fetched
-  // articles.
-  RecreateSnippetsService();
+
+  // There is only a single, dismissed snippet in the database, so recreating
+  // the service will require us to re-fetch.
   tester.ExpectTotalCount("NewTabPage.Snippets.NumArticlesFetched", 4);
+  ResetSnippetsService(&service);
+  EXPECT_EQ(observer().StatusForCategory(articles_category()),
+            CategoryStatus::AVAILABLE);
+  tester.ExpectTotalCount("NewTabPage.Snippets.NumArticlesFetched", 5);
+  EXPECT_THAT(
+      tester.GetAllSamples("NewTabPage.Snippets.NumArticlesZeroDueToDiscarded"),
+      ElementsAre(base::Bucket(/*min=*/1, /*count=*/2)));
+
+  // But if there's a non-dismissed snippet in the database, recreating it
+  // shouldn't trigger a fetch.
+  LoadFromJSONString(
+      service.get(),
+      GetTestJson({GetSnippetWithUrl("http://not-dismissed.com")}));
+  tester.ExpectTotalCount("NewTabPage.Snippets.NumArticlesFetched", 6);
+  ResetSnippetsService(&service);
+  tester.ExpectTotalCount("NewTabPage.Snippets.NumArticlesFetched", 6);
 }
 
 TEST_F(NTPSnippetsServiceTest, DismissShouldRespectAllKnownUrls) {
-  const std::string creation =
-      NTPSnippet::TimeToJsonString(GetDefaultCreationTime());
-  const std::string expiry =
-      NTPSnippet::TimeToJsonString(GetDefaultExpirationTime());
+  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _));
+  auto service = MakeSnippetsService();
+
+  const base::Time creation = GetDefaultCreationTime();
+  const base::Time expiry = GetDefaultExpirationTime();
   const std::vector<std::string> source_urls = {
       "http://mashable.com/2016/05/11/stolen",
-      "http://www.aol.com/article/2016/05/stolen-doggie",
-      "http://mashable.com/2016/05/11/stolen?utm_cid=1"};
-  const std::vector<std::string> publishers = {"Mashable", "AOL", "Mashable"};
+      "http://www.aol.com/article/2016/05/stolen-doggie"};
+  const std::vector<std::string> publishers = {"Mashable", "AOL"};
   const std::vector<std::string> amp_urls = {
       "http://mashable-amphtml.googleusercontent.com/1",
-      "http://t2.gstatic.com/images?q=tbn:3",
       "http://t2.gstatic.com/images?q=tbn:3"};
 
   // Add the snippet from the mashable domain.
-  LoadFromJSONString(GetTestJson({GetSnippetWithUrlAndTimesAndSources(
-      source_urls[0], creation, expiry, source_urls, publishers, amp_urls)}));
-  ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1));
+  LoadFromJSONString(service.get(),
+                     GetTestJson({GetSnippetWithUrlAndTimesAndSource(
+                         source_urls, source_urls[0], creation, expiry,
+                         publishers[0], amp_urls[0])}));
+  ASSERT_THAT(service->GetSnippetsForTesting(), SizeIs(1));
   // Dismiss the snippet via the mashable source corpus ID.
-  service()->DismissSuggestion(MakeUniqueID(source_urls[0]));
-  EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty());
+  service->DismissSuggestion(MakeUniqueID(*service, source_urls[0]));
+  EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty());
 
   // The same article from the AOL domain should now be detected as dismissed.
-  LoadFromJSONString(GetTestJson({GetSnippetWithUrlAndTimesAndSources(
-      source_urls[1], creation, expiry, source_urls, publishers, amp_urls)}));
-  ASSERT_THAT(service()->GetSnippetsForTesting(), IsEmpty());
+  LoadFromJSONString(service.get(),
+                     GetTestJson({GetSnippetWithUrlAndTimesAndSource(
+                         source_urls, source_urls[1], creation, expiry,
+                         publishers[1], amp_urls[1])}));
+  ASSERT_THAT(service->GetSnippetsForTesting(), IsEmpty());

[Marc Treib, 2016/08/30 13:43:39]

This should be EXPECT_THAT

[sfiera, 2016/08/30 16:33:09]

Done.

 }
 
 TEST_F(NTPSnippetsServiceTest, StatusChanges) {
+  {
+    InSequence s;
+    EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _));
+    EXPECT_CALL(mock_scheduler(), Unschedule());
+    EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _));
+  }
+  auto service = MakeSnippetsService();
+
   // Simulate user signed out
   SetUpFetchResponse(GetTestJson({GetSnippet()}));
-  EXPECT_CALL(observer(),
-              OnCategoryStatusChanged(_, _, CategoryStatus::SIGNED_OUT));
-  service()->OnDisabledReasonChanged(DisabledReason::SIGNED_OUT);
+  service->OnDisabledReasonChanged(DisabledReason::SIGNED_OUT);
+
   base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(NTPSnippetsService::State::DISABLED, service()->state_);
-  EXPECT_THAT(service()->GetSnippetsForTesting(),
+  EXPECT_THAT(observer().StatusForCategory(articles_category()),
+              Eq(CategoryStatus::SIGNED_OUT));
+  EXPECT_THAT(NTPSnippetsService::State::DISABLED, Eq(service->state_));
+  EXPECT_THAT(service->GetSnippetsForTesting(),
               IsEmpty());  // No fetch should be made.
 
   // Simulate user sign in. The service should be ready again and load snippets.
   SetUpFetchResponse(GetTestJson({GetSnippet()}));
-  EXPECT_CALL(observer(),
-              OnCategoryStatusChanged(_, _, CategoryStatus::AVAILABLE_LOADING));
-  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)).Times(1);
-  service()->OnDisabledReasonChanged(DisabledReason::NONE);
-  EXPECT_CALL(observer(),
-              OnCategoryStatusChanged(_, _, CategoryStatus::AVAILABLE));
+  service->OnDisabledReasonChanged(DisabledReason::NONE);
+  EXPECT_THAT(observer().StatusForCategory(articles_category()),
+              Eq(CategoryStatus::AVAILABLE_LOADING));
+
   base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(NTPSnippetsService::State::READY, service()->state_);
-  EXPECT_FALSE(service()->GetSnippetsForTesting().empty());
+  EXPECT_THAT(observer().StatusForCategory(articles_category()),
+              Eq(CategoryStatus::AVAILABLE));
+  EXPECT_THAT(NTPSnippetsService::State::READY, Eq(service->state_));
+  EXPECT_FALSE(service->GetSnippetsForTesting().empty());
 }
 
 TEST_F(NTPSnippetsServiceTest, ImageReturnedWithTheSameId) {
-  LoadFromJSONString(GetTestJson({GetSnippet()}));
+  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _));
+  auto service = MakeSnippetsService();
+
+  LoadFromJSONString(service.get(), GetTestJson({GetSnippet()}));
 
   gfx::Image image;
-  EXPECT_CALL(*image_fetcher(), StartOrQueueNetworkRequest(_, _, _))
-      .WillOnce(testing::WithArgs<0, 2>(Invoke(ServeOneByOneImage)));
-  testing::MockFunction<void(const gfx::Image&)> image_fetched;
-  EXPECT_CALL(image_fetched, Call(_)).WillOnce(testing::SaveArg<0>(&image));
+  MockFunction<void(gfx::Image&)> image_fetched;
+  {
+    InSequence s;
+    EXPECT_CALL(*image_fetcher(), StartOrQueueNetworkRequest(_, _, _))
+        .WillOnce(WithArgs<0, 2>(Invoke(ServeOneByOneImage)));
+    EXPECT_CALL(image_fetched, Call(_))
+        .WillOnce(SaveArg<0>(&image));
+  }
 
-  service()->FetchSuggestionImage(
-      MakeUniqueID(kSnippetUrl),
-      base::Bind(&testing::MockFunction<void(const gfx::Image&)>::Call,
-                 base::Unretained(&image_fetched)));
+  service->FetchSuggestionImage(
+      MakeUniqueID(*service, kSnippetUrl),
+      base::Bind(
+          &MockFunction<void(const gfx::Image&)>::Call,
+          base::Unretained(&image_fetched)));
   base::RunLoop().RunUntilIdle();
   // Check that the image by ServeOneByOneImage is really served.
   EXPECT_EQ(1, image.Width());
 }
 
 TEST_F(NTPSnippetsServiceTest, EmptyImageReturnedForNonExistentId) {
+  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _));
+  auto service = MakeSnippetsService();
+
   // Create a non-empty image so that we can test the image gets updated.
   gfx::Image image = gfx::test::CreateImage(1, 1);
-  testing::MockFunction<void(const gfx::Image&)> image_fetched;
-  EXPECT_CALL(image_fetched, Call(_)).WillOnce(testing::SaveArg<0>(&image));
+  MockFunction<void(const gfx::Image&)> image_fetched;
+  EXPECT_CALL(image_fetched, Call(_))
+      .WillOnce(SaveArg<0>(&image));
 
-  service()->FetchSuggestionImage(
-      MakeUniqueID(kSnippetUrl2),
-      base::Bind(&testing::MockFunction<void(const gfx::Image&)>::Call,
-                 base::Unretained(&image_fetched)));
+  service->FetchSuggestionImage(
+      MakeUniqueID(*service, kSnippetUrl2),
+      base::Bind(
+          &MockFunction<void(const gfx::Image&)>::Call,
+          base::Unretained(&image_fetched)));
 
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(image.IsEmpty());