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 6c551c44b47b2b26dae18c2a2fddb40023ff2cf2..e8dc37b5b5835f8cc83dadafc9f0f5e40c878286 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,9 +49,11 @@ 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::Return;
 using testing::SizeIs;
 using testing::StartsWith;
@@ -65,8 +68,9 @@ 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 kTestContentSuggestionsServerFormat[] =
+    "https://chromecontentsuggestions-pa.googleapis.com/v1/suggestions/"

[tschumann, 2016/08/30 06:57:21]

reading this test, you might think that we're actually fetching data from
service (which we hopefully don't as we are using a fake fetcher).
What requirements to the to-be-fake-fetched URLs do we need to meet?
Would be nice if we could have a string that makes it very clear that this is
not a real URL to be fetched, like "fake-url-to-suggestions-api"

[sfiera, 2016/08/30 13:16:44]

Done.

+    "fetch?key=%s";
 
 const char kSnippetUrl[] = "http://localhost/foobar";
 const char kSnippetTitle[] = "Title";
@@ -74,7 +78,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 +92,91 @@ 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 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 GetSnippetWithUrlAndTimesAndSources(

[Marc Treib, 2016/08/30 09:29:29]

"Sources" isn't accurate anymore, there's only one.

[sfiera, 2016/08/30 13:16:43]

Done.

+    const std::vector<std::string>& ids,
     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 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(",");
+    const base::Time& creation_time,
+    const base::Time& expiry_time,
+    const std::string& publisher,
+    const std::string& amp_url) {
+  std::string ids_string;

[tschumann, 2016/08/30 06:57:21]

ids_string = base::JoinStrings(ids, "\",\n      \"");

[sfiera, 2016/08/30 13:16:44]

Done.

+  for (const std::string& id : ids) {
+    if (!ids_string.empty())
+      ids_string += "\",\n      \"";
+    ids_string += id;
   }
-  // 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);
+
+  return base::StringPrintf(
+      "{\n"

[Marc Treib, 2016/08/30 09:29:29]

nit: indentation?

[sfiera, 2016/08/30 13:16:44]

Do you mean in the JSON? It's indented so that it will look right when it's
slotted into GetTestJson() above.

+      "      \"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) {
+                                  const std::string& publisher,
+                                  const std::string& amp_url) {
   return GetSnippetWithUrlAndTimesAndSources(
-      kSnippetUrl, NTPSnippet::TimeToJsonString(GetDefaultCreationTime()),
-      NTPSnippet::TimeToJsonString(GetDefaultExpirationTime()), source_urls,
-      publishers, amp_urls);
+      {kSnippetUrl}, source_urls[0], GetDefaultCreationTime(),

[Marc Treib, 2016/08/30 09:29:29]

This seems wrong - why pass in a vector of source_urls but only use one?

[sfiera, 2016/08/30 13:16:44]

Not sure. I think this was used for the tests that were moved to NTPSnippetTest,
but the whole multiple-source thing doesn't exist here anymore now that we use
the new-style API.

It's only ever called with one source anyway, so I've changed that.

+      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 GetSnippetWithUrlAndTimesAndSources({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 +189,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) {

[tschumann, 2016/08/30 06:57:21]

couldn't we just CHECK() then?

[tschumann, 2016/08/30 14:01:39]

In this particular case, CHECK() would be cleaner though as your program would
crash 2 lines later anyways, no? (plus you don't have to deal with returning and
empty string or the like).

     NOTREACHED();
     return std::string();
@@ -242,33 +240,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,6 +251,61 @@ class MockImageFetcher : public ImageFetcher {
            base::Callback<void(const std::string&, const gfx::Image&)>));
 };
 
+class FakeContentSuggestionsProviderObserver

[tschumann, 2016/08/30 06:57:21]

nice!

+    : public ContentSuggestionsProvider::Observer {
+ public:
+  FakeContentSuggestionsProviderObserver()
+      : loaded_(base::WaitableEvent::ResetPolicy::MANUAL,
+                base::WaitableEvent::InitialState::NOT_SIGNALED) {}
+
+  void OnNewSuggestions(ContentSuggestionsProvider* provider,
+                        Category category,
+                        std::vector<ContentSuggestion> suggestions) override {
+    suggestions_[category].insert(suggestions_[category].begin(),

[Marc Treib, 2016/08/30 09:29:29]

Shouldn't you clear suggestions_[category_] first? That's what you would expect
of a ProviderObserver - OnNewSuggestions replaces any that were there before.

[sfiera, 2016/08/30 13:16:43]

Oh, apparently. Done.

+                                  std::make_move_iterator(suggestions.begin()),
+                                  std::make_move_iterator(suggestions.end()));
+  }
+
+  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);
+};
+
 class MockDismissedSuggestionsCallback
     : public ContentSuggestionsProvider::DismissedSuggestionsCallback {
  public:
@@ -294,15 +320,20 @@ class MockDismissedSuggestionsCallback
 
 }  // namespace
 
-class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase {
+class NTPSnippetsServiceTest : public ::testing::Test {

[tschumann, 2016/08/30 06:57:21]

yay!

  public:
   NTPSnippetsServiceTest()
-      : fake_url_fetcher_factory_(
+      : params_manager_(
+            ntp_snippets::kStudyName,
+            {{"content_suggestions_backend", kContentSuggestionsServer}}),
+        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())),

[Marc Treib, 2016/08/30 09:29:29]

I don't think the actual API key matters here? We can probably just hardcode
some arbitrary value into our URL constant?

[sfiera, 2016/08/30 13:16:43]

We're using a real fetcher, which will use a real API key.

Actually, we probably *should* inject the API key into the fetcher, instead of
is_stable_channel, but I think that change is separate from this one.

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

Aah, I remember now. Carry on then :)

[tschumann, 2016/08/30 14:01:39]

but please tell me we're not making an actual fetch from some system requiring
the key, right?

+
+        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,42 +346,24 @@ 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();

[Marc Treib, 2016/08/30 09:29:29]

So if you call MakeSnippetsService twice in a row, the first service will be in
a bad state, right? Can we somehow ensure that that doesn't happen?

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

Not addressed yet.

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

Checked !observer.Loaded() above (which is reset first when resetting the
service).

     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);
 
@@ -360,22 +373,31 @@ class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase {
 
     // 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,8 +406,7 @@ 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_;
@@ -397,13 +418,16 @@ 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.
@@ -411,11 +435,9 @@ class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase {
   const GURL test_url_;
   std::unique_ptr<OAuth2TokenService> fake_token_service_;
   MockScheduler scheduler_;

[tschumann, 2016/08/30 06:57:21]

consider making this a ::testing::NiceMock<MockScheduler>
The nice thing is that if you don't define an expectation for a function call,
it won't trigger a warning. So test cases which don't care about whether or not
the scheduler get's called don't need to care about the EXPECT_CALL()
statements. But if you add them, then they get verified like with normal (or
StrictMock<>s).

[sfiera, 2016/08/30 13:16:44]

I see your NiceMock<> and raise you a StrictMock<>.

[tschumann, 2016/08/30 14:01:39]

but please make sure the test doesn't get too brittle with that. It's easy to
end-up with noise and copy-past expectations all over the place.

-  MockContentSuggestionsProviderObserver observer_;
+  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_;
 
   base::ScopedTempDir database_dir_;
 
@@ -423,52 +445,68 @@ 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));
+  LoadFromJSONString(service.get(), json_str);
+  ASSERT_THAT(observer().SuggestionsForCategory(articles_category()),
+              SizeIs(1));
+  ASSERT_THAT(service->GetSnippetsForTesting(), SizeIs(1));
+  const ContentSuggestion& snippet =

[Marc Treib, 2016/08/30 09:29:29]

Can we rename this to |suggestion| please?

[sfiera, 2016/08/30 13:16:43]

Done.

+      observer().SuggestionsForCategory(articles_category()).front();
+
+  EXPECT_EQ(MakeUniqueID(*service, kSnippetUrl), snippet.id());
+  EXPECT_EQ(kSnippetTitle, base::UTF16ToUTF8(snippet.title()));
+  EXPECT_EQ(kSnippetText, base::UTF16ToUTF8(snippet.snippet_text()));
+  // EXPECT_EQ(GURL(kSnippetSalientImage), snippet.salient_image_url());

[tschumann, 2016/08/30 06:57:21]

why is this commented out?

[sfiera, 2016/08/30 13:16:43]

Right now ContentSuggestion::salient_image_url_ has no accessor or mutator (?).

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

Because it shouldn't be there :)  Mind removing it?

[tschumann, 2016/08/30 14:01:39]

+1 for removing it. Commented out code tends to rot. It's easy to write once
it's there.

[Marc Treib, 2016/08/30 14:04:39]

I mostly meant removing that member, which is completely unused. The
commented-out test code should also go of course :)

   EXPECT_EQ(GetDefaultCreationTime(), snippet.publish_date());
-  EXPECT_EQ(snippet.best_source().publisher_name, kSnippetPublisherName);
-  EXPECT_EQ(snippet.best_source().amp_url, GURL(kSnippetAmpUrl));
+  EXPECT_EQ(kSnippetPublisherName, base::UTF16ToUTF8(snippet.publisher_name()));
+  EXPECT_EQ(GURL(kSnippetAmpUrl), snippet.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 +519,175 @@ 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());
-  std::string json_str(
-      GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)}));
+  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)).Times(2);
+  auto service = MakeSnippetsService();
 
-  LoadFromJSONString(json_str);
+  std::string json_str(GetTestJson(
+      {GetSnippetWithSources({"http://site.com"}, "Source 1", "")}));
 
-  ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1));
+  LoadFromJSONString(service.get(), json_str);
+
+  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(&MockDismissedSuggestionsCallback::Run,
-                 base::Unretained(&callback), articles_category()));
-  Mock::VerifyAndClearExpectations(&callback);
+  {
+    MockDismissedSuggestionsCallback callback;
+    EXPECT_CALL(callback, MockRun(_, _))

[tschumann, 2016/08/30 06:57:21]

not now, but this mock usage also has a pretty bad smell.
The problem is that we are not setting up a proper expectation of the call
itself but verify the parameters as part of the action. Parameters should be
verified with parameter matchers.

So either, we follow this pattern, but then I'd rather have a free function that
simply captures the state (as done with CaptureDismissedSuggestions in
https://codereview.chromium.org/2291593002/).

Or we provide proper matchers to verify the parameter. In this case (and because
of the trickiness with mocking move-only parameters), I'd be in favor of just
binding a normal function that captures the state to be verified after the call.

But like I said, this can totally happen in another CL.

[sfiera, 2016/08/30 13:16:43]

This is actually doable just with a lambda, which reads better in all ways.

[tschumann, 2016/08/30 14:01:39]

Actually, it would be even nice as a standalone function like
ExpectDismissedSuggestions() to avoid duplication and easier to read code
(lambdas at this length start cluttering the code and as you can't give them a
name, tend to have hard-to-guess semantics).
Still okay though.

+        .WillOnce(Invoke([this, &service](
+            Category category,
+            std::vector<ContentSuggestion>* dismissed_suggestions) {
+          EXPECT_EQ(1u, dismissed_suggestions->size());
+          for (auto& suggestion : *dismissed_suggestions) {
+            EXPECT_EQ(MakeUniqueID(*service, kSnippetUrl), suggestion.id());
+          }
+        }));
 
-  // 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(
-      articles_category(),
-      base::Bind(&MockDismissedSuggestionsCallback::Run,
-                 base::Unretained(&callback), articles_category()));
+    service->GetDismissedSuggestionsForDebugging(
+        articles_category(),
+        base::Bind(&MockDismissedSuggestionsCallback::Run,
+                   base::Unretained(&callback), articles_category()));
+  }
+
+  {
+    MockDismissedSuggestionsCallback callback;
+    EXPECT_CALL(callback, MockRun(_, _))
+        .WillOnce(Invoke(
+            [this](Category category,
+                   std::vector<ContentSuggestion>* dismissed_suggestions) {
+              EXPECT_EQ(0u, dismissed_suggestions->size());
+            }));
+
+    // There should be no dismissed snippet after clearing the list.
+    service->ClearDismissedSuggestionsForDebugging(articles_category());
+    service->GetDismissedSuggestionsForDebugging(
+        articles_category(),
+        base::Bind(&MockDismissedSuggestionsCallback::Run,
+                   base::Unretained(&callback), articles_category()));
+  }
 }
 
 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);
+  ASSERT_THAT(service->GetSnippetsForTesting(), SizeIs(0));

[Marc Treib, 2016/08/30 09:29:29]

Why has the behavior changed here?
Also, this should be EXPECT rather than ASSERT.

[sfiera, 2016/08/30 13:16:43]

NTPSnippet::CreateFromContentSuggestionsDictionary() is stricter about this.
Before, we'd succeed at creating the snippet, but it would be !is_complete().

 }
 
 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,207 +696,33 @@ 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({"aaaa"}, "Source 1", "http://source1.amp.com")}));

[Marc Treib, 2016/08/30 09:29:29]

nit: s/aaaa/not a url/ ?

[sfiera, 2016/08/30 13:16:44]

Done (for locale fr)

+
+  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(2);
+  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)));
@@ -845,13 +730,13 @@ TEST_F(NTPSnippetsServiceTest, LogNumArticlesHistogram) {
   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)));
@@ -859,7 +744,7 @@ TEST_F(NTPSnippetsServiceTest, LogNumArticlesHistogram) {
               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)));
@@ -871,8 +756,8 @@ TEST_F(NTPSnippetsServiceTest, LogNumArticlesHistogram) {
       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,17 +768,22 @@ 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

[Marc Treib, 2016/08/30 09:29:29]

Yay outdated comments - "loading from prefs" isn't correct anymore; we're
loading from the DB.

-  // articles.
-  RecreateSnippetsService();
+  // articles. However, recreating the service does trigger one fetch.

[Marc Treib, 2016/08/30 09:29:29]

Why? If we get suggestions from the DB, we shouldn't fetch again.

[sfiera, 2016/08/30 13:16:44]

We don't get suggestions from the DB. There's one snippet there, but it's
dismissed.

I've fixed this up to test what you thought it did :)

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

Yay! Thanks!

   tester.ExpectTotalCount("NewTabPage.Snippets.NumArticlesFetched", 4);
+  ResetSnippetsService(&service);
+  EXPECT_EQ(observer().StatusForCategory(articles_category()),
+            CategoryStatus::AVAILABLE);
+  tester.ExpectTotalCount("NewTabPage.Snippets.NumArticlesFetched", 5);
 }
 
 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",
@@ -905,58 +795,72 @@ TEST_F(NTPSnippetsServiceTest, DismissShouldRespectAllKnownUrls) {
       "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({GetSnippetWithUrlAndTimesAndSources(
+                         source_urls, source_urls[0], creation, expiry,
+                         publishers[0], amp_urls[1])}));

[Marc Treib, 2016/08/30 09:29:29]

Should this be amp_urls[0]?

[sfiera, 2016/08/30 13:16:44]

I suppose so. Done.

+  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({GetSnippetWithUrlAndTimesAndSources(
+                         source_urls, source_urls[1], creation, expiry,
+                         publishers[1], amp_urls[1])}));
+  ASSERT_THAT(service->GetSnippetsForTesting(), IsEmpty());
 }
 
 TEST_F(NTPSnippetsServiceTest, StatusChanges) {
+  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)).Times(2);
+  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 std::string&, const gfx::Image&)>
       image_fetched;
-  EXPECT_CALL(image_fetched, Call(MakeUniqueID(kSnippetUrl), _))
-      .WillOnce(testing::SaveArg<1>(&image));
+  {
+    InSequence s;
+    EXPECT_CALL(*image_fetcher(), StartOrQueueNetworkRequest(_, _, _))
+        .WillOnce(testing::WithArgs<0, 2>(Invoke(ServeOneByOneImage)));

[Marc Treib, 2016/08/30 09:29:29]

optional: You could add "using testing::..." statements, there's already a bunch
of them.

[sfiera, 2016/08/30 13:16:43]

Done (for several testing::)

+    EXPECT_CALL(image_fetched, Call(MakeUniqueID(*service, kSnippetUrl), _))
+        .WillOnce(testing::SaveArg<1>(&image));
+  }
 
-  service()->FetchSuggestionImage(
-      MakeUniqueID(kSnippetUrl),
+  service->FetchSuggestionImage(
+      MakeUniqueID(*service, kSnippetUrl),
       base::Bind(&testing::MockFunction<void(const std::string&,
-                                              const gfx::Image&)>::Call,
+                                             const gfx::Image&)>::Call,
                  base::Unretained(&image_fetched)));
   base::RunLoop().RunUntilIdle();
   // Check that the image by ServeOneByOneImage is really served.
@@ -964,18 +868,20 @@ TEST_F(NTPSnippetsServiceTest, ImageReturnedWithTheSameId) {
 }
 
 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 std::string&, const gfx::Image&)>
       image_fetched;
-  EXPECT_CALL(image_fetched,
-              Call(MakeUniqueID(kSnippetUrl2), _))
+  EXPECT_CALL(image_fetched, Call(MakeUniqueID(*service, kSnippetUrl2), _))
       .WillOnce(testing::SaveArg<1>(&image));
 
-  service()->FetchSuggestionImage(
-      MakeUniqueID(kSnippetUrl2),
+  service->FetchSuggestionImage(
+      MakeUniqueID(*service, kSnippetUrl2),
       base::Bind(&testing::MockFunction<void(const std::string&,
-                                              const gfx::Image&)>::Call,
+                                             const gfx::Image&)>::Call,
                  base::Unretained(&image_fetched)));
 
   base::RunLoop().RunUntilIdle();