Combine all suggestions factories into ContentSuggestionsServiceFactory

components/ntp_snippets/ntp_snippets_service_unittest.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/ntp_snippets/ntp_snippets_service.h"
 
 #include <memory>
 #include <vector>
 
 #include "base/command_line.h"
@@ skipping 23 matching lines @@
 #include "components/signin/core/browser/fake_profile_oauth2_token_service.h"
 #include "components/signin/core/browser/fake_signin_manager.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"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::ElementsAre;
 using testing::Eq;
+using testing::Invoke;
+using testing::IsEmpty;
+using testing::Mock;
 using testing::Return;
-using testing::IsEmpty;
 using testing::SizeIs;
 using testing::StartsWith;
 using testing::_;
 
 namespace ntp_snippets {
 
 namespace {
 
 MATCHER_P(IdEq, value, "") {
   return arg->id() == value;
@@ skipping 161 matching lines @@
 class MockScheduler : public NTPSnippetsScheduler {
  public:
   MOCK_METHOD4(Schedule,
                bool(base::TimeDelta period_wifi_charging,
                     base::TimeDelta period_wifi,
                     base::TimeDelta period_fallback,
                     base::Time reschedule_time));
   MOCK_METHOD0(Unschedule, bool());
 };
 
-class MockServiceObserver : public NTPSnippetsServiceObserver {
+class MockProviderObserver : public ContentSuggestionsProvider::Observer {
  public:
-  MOCK_METHOD0(NTPSnippetsServiceLoaded, void());
-  MOCK_METHOD0(NTPSnippetsServiceShutdown, void());
-  MOCK_METHOD1(NTPSnippetsServiceDisabledReasonChanged,
-               void(DisabledReason disabled_reason));
+  void OnNewSuggestions(ContentSuggestionsProvider* provider,
+                        Category category,
+                        std::vector<ContentSuggestion> suggestions) {}

[Marc Treib, 2016/08/03 11:53:23]

Shouldn't this be marked override? Or maybe just defined as a MOCK_METHOD too?

[Philipp Keck, 2016/08/03 12:57:23]

Done.

+  MOCK_METHOD3(OnCategoryStatusChanged,
+               void(ContentSuggestionsProvider* provider,
+                    Category category,
+                    CategoryStatus new_status));
 };
 
-class WaitForDBLoad : public NTPSnippetsServiceObserver {
+class WaitForDBLoad {
  public:
-  WaitForDBLoad(NTPSnippetsService* service) : service_(service) {
-    service_->AddObserver(this);
-    if (!service_->ready())
+  WaitForDBLoad(MockProviderObserver* observer, NTPSnippetsService* service)
+      : observer_(observer) {
+    ON_CALL(*observer_, OnCategoryStatusChanged(_, _, _))
+        .WillByDefault(Invoke(this, &WaitForDBLoad::OnCategoryStatusChanged));

[Marc Treib, 2016/08/03 11:53:23]

WillOnce?

[Philipp Keck, 2016/08/03 12:28:25]

Not available for ON_CALL, unfortunately. That's also why I have to call
MOCK::[VerifyAnd]Clear below.

[Marc Treib, 2016/08/03 12:37:56]

EXPECT_CALL then?

[Philipp Keck, 2016/08/03 12:57:23]

Ok, using EXPECT_CALL now. It's not really what it means here, but it works.

+    if (!service->ready())
       run_loop_.Run();
   }
 
-  ~WaitForDBLoad() override {
-    service_->RemoveObserver(this);
-  }
+  ~WaitForDBLoad() { Mock::VerifyAndClear(observer_); }
 
  private:
-  void NTPSnippetsServiceLoaded() override {
-    EXPECT_TRUE(service_->ready());
+  void OnCategoryStatusChanged(ContentSuggestionsProvider* provider,
+                               Category category,
+                               CategoryStatus new_status) {
+    EXPECT_EQ(new_status, CategoryStatus::AVAILABLE_LOADING);
     run_loop_.Quit();
   }
 
-  void NTPSnippetsServiceShutdown() override {}
-  void NTPSnippetsServiceDisabledReasonChanged(
-      DisabledReason disabled_reason) override {}
-
-  NTPSnippetsService* service_;
+  MockProviderObserver* observer_;
   base::RunLoop run_loop_;
 
   DISALLOW_COPY_AND_ASSIGN(WaitForDBLoad);
 };
 
 }  // namespace
 
 class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase {
  public:
   NTPSnippetsServiceTest()
       : 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());
 
     // Since no SuggestionsService is injected in tests, we need to force the
     // service to fetch from all hosts.
     base::CommandLine::ForCurrentProcess()->AppendSwitch(
         switches::kDontRestrict);
     EXPECT_TRUE(database_dir_.CreateUniqueTempDir());
   }
 
   ~NTPSnippetsServiceTest() override {
-    if (service_)
-      service_->Shutdown();
-
     // 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(/*enabled=*/true);
+    CreateSnippetsService();
   }
 
-  void CreateSnippetsService(bool enabled) {
-    if (service_)
-      service_->Shutdown();
+  void RecreateSnippetsService() {
+    Mock::VerifyAndClear(&mock_scheduler());
+    service_.reset();
+    base::RunLoop().RunUntilIdle();
+    EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)).Times(1);
+    CreateSnippetsService();
+  }
 
+  void CreateSnippetsService() {
     scoped_refptr<base::SingleThreadTaskRunner> task_runner(
         base::ThreadTaskRunnerHandle::Get());
     scoped_refptr<net::TestURLRequestContextGetter> request_context_getter =
         new net::TestURLRequestContextGetter(task_runner.get());
 
     // Delete the current service, so that the database is destroyed before we
     // create the new one, otherwise opening the new database will fail.
     service_.reset();

[Marc Treib, 2016/08/03 11:53:23]

Is this still required now that there is an explicit Recreate? Should we DCHECK
that service_ is null?

[Philipp Keck, 2016/08/03 12:28:25]

Right. Done.

 
     ResetSigninManager();
     std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher =
         base::MakeUnique<NTPSnippetsFetcher>(
             fake_signin_manager(), fake_token_service_.get(),
             std::move(request_context_getter), pref_service(),
             base::Bind(&ParseJson),
             /*is_stable_channel=*/true);
 
     fake_signin_manager()->SignIn("foo@bar.com");
     snippets_fetcher->SetPersonalizationForTesting(
         NTPSnippetsFetcher::Personalization::kNonPersonal);
 
     // Add an initial fetch response, as the service tries to fetch when there
     // is nothing in the DB.
     SetUpFetchResponse(GetTestJson({GetSnippet()}));
 
     service_.reset(new NTPSnippetsService(
-        enabled, pref_service(), nullptr, &category_factory_, "fr", &scheduler_,
-        std::move(snippets_fetcher), /*image_fetcher=*/nullptr,
+        &observer_, &category_factory_, pref_service(), nullptr, "fr",
+        &scheduler_, std::move(snippets_fetcher), /*image_fetcher=*/nullptr,
         /*image_fetcher=*/nullptr, base::MakeUnique<NTPSnippetsDatabase>(
                                        database_dir_.path(), task_runner),
         base::MakeUnique<NTPSnippetsStatusService>(fake_signin_manager(),
                                                    pref_service())));
 
-    if (enabled)
-      WaitForDBLoad(service_.get());
+    WaitForDBLoad(&observer_, service_.get());
   }
 
   std::string MakeUniqueID(const std::string& within_category_id) {
     return service()->MakeUniqueID(
         category_factory_.FromKnownCategory(KnownCategories::ARTICLES),
         within_category_id);
   }
 
  protected:
   const GURL& test_url() { return test_url_; }
   NTPSnippetsService* service() { return service_.get(); }
-  MockScheduler& mock_scheduler() {  return scheduler_; }
+  MockProviderObserver& observer() { return observer_; }
+  MockScheduler& mock_scheduler() { return scheduler_; }
 
   // Provide the json to be returned by the fake fetcher.
   void SetUpFetchResponse(const std::string& json) {
     fake_url_fetcher_factory_.SetFakeResponse(test_url_, json, net::HTTP_OK,
                                               net::URLRequestStatus::SUCCESS);
   }
 
   void LoadFromJSONString(const std::string& json) {
     SetUpFetchResponse(json);
     service()->FetchSnippets(true);
     base::RunLoop().RunUntilIdle();
   }
 
  private:
   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_;
+  MockProviderObserver observer_;
   CategoryFactory category_factory_;
   // Last so that the dependencies are deleted after the service.
   std::unique_ptr<NTPSnippetsService> service_;
 
   base::ScopedTempDir database_dir_;
 
   DISALLOW_COPY_AND_ASSIGN(NTPSnippetsServiceTest);
 };
 
-class NTPSnippetsServiceDisabledTest : public NTPSnippetsServiceTest {
- public:
-  void SetUp() override {
-    test::NTPSnippetsTestBase::SetUp();
-    EXPECT_CALL(mock_scheduler(), Unschedule()).Times(1);
-    CreateSnippetsService(/*enabled=*/false);
-  }
-};
-
-TEST_F(NTPSnippetsServiceTest, ScheduleIfEnabled) {
+TEST_F(NTPSnippetsServiceTest, ScheduleOnStart) {
   // SetUp() checks that Schedule is called.
 
   // When we have no snippets are all, loading the service initiates a fetch.
   base::RunLoop().RunUntilIdle();
   ASSERT_EQ("OK", service()->snippets_fetcher()->last_status());
 }
 
-TEST_F(NTPSnippetsServiceDisabledTest, Unschedule) {
-  // SetUp() checks that Unschedule is called.
-
-  // Make sure we don't fetch anything.
-  base::RunLoop().RunUntilIdle();
-  EXPECT_THAT(service()->snippets_fetcher()->last_status(), IsEmpty());
-}
-
 TEST_F(NTPSnippetsServiceTest, Full) {
   std::string json_str(GetTestJson({GetSnippet()}));
 
   LoadFromJSONString(json_str);
   ASSERT_THAT(service()->snippets(), SizeIs(1));
   const NTPSnippet& snippet = *service()->snippets().front();
 
   EXPECT_EQ(snippet.id(), kSnippetUrl);
   EXPECT_EQ(snippet.title(), kSnippetTitle);
   EXPECT_EQ(snippet.snippet(), kSnippetText);
@@ skipping 99 matching lines @@
 
   // Dismiss the snippet.
   service()->DismissSuggestion(MakeUniqueID(kSnippetUrl));
   EXPECT_THAT(service()->snippets(), IsEmpty());
 
   // Make sure that fetching the same snippet again does not re-add it.
   LoadFromJSONString(json_str);
   EXPECT_THAT(service()->snippets(), IsEmpty());
 
   // The snippet should stay dismissed even after re-creating the service.
-  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)).Times(1);
-  CreateSnippetsService(/*enabled=*/true);
+  RecreateSnippetsService();
   LoadFromJSONString(json_str);
   EXPECT_THAT(service()->snippets(), IsEmpty());
 
   // The snippet can be added again after clearing dismissed snippets.
   service()->ClearDismissedSuggestionsForDebugging();
   EXPECT_THAT(service()->snippets(), IsEmpty());
   LoadFromJSONString(json_str);
   EXPECT_THAT(service()->snippets(), SizeIs(1));
 }
 
@@ skipping 293 matching lines @@
                           base::Bucket(/*min=*/1, /*count=*/2)));
   // Dismissed snippets shouldn't influence NumArticlesFetched.
   EXPECT_THAT(tester.GetAllSamples("NewTabPage.Snippets.NumArticlesFetched"),
               ElementsAre(base::Bucket(/*min=*/0, /*count=*/1),
                           base::Bucket(/*min=*/1, /*count=*/3)));
   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.
-  EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)).Times(1);
-  CreateSnippetsService(/*enabled=*/true);
+  RecreateSnippetsService();
   tester.ExpectTotalCount("NewTabPage.Snippets.NumArticlesFetched", 4);
 }
 
 TEST_F(NTPSnippetsServiceTest, DismissShouldRespectAllKnownUrls) {
   const std::string creation =
       NTPSnippet::TimeToJsonString(GetDefaultCreationTime());
   const std::string expiry =
       NTPSnippet::TimeToJsonString(GetDefaultExpirationTime());
   const std::vector<std::string> source_urls = {
       "http://mashable.com/2016/05/11/stolen",
@@ skipping 13 matching lines @@
   service()->DismissSuggestion(MakeUniqueID(source_urls[0]));
   EXPECT_THAT(service()->snippets(), 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()->snippets(), IsEmpty());
 }
 
 TEST_F(NTPSnippetsServiceTest, StatusChanges) {
-  MockServiceObserver mock_observer;
-  service()->AddObserver(&mock_observer);
-
   // Simulate user signed out
   SetUpFetchResponse(GetTestJson({GetSnippet()}));
-  EXPECT_CALL(mock_observer, NTPSnippetsServiceDisabledReasonChanged(
-                                 DisabledReason::SIGNED_OUT));
+  EXPECT_CALL(observer(),
+              OnCategoryStatusChanged(_, _, CategoryStatus::SIGNED_OUT));
   service()->OnDisabledReasonChanged(DisabledReason::SIGNED_OUT);
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(NTPSnippetsService::State::DISABLED, service()->state_);
   EXPECT_THAT(service()->snippets(), IsEmpty());  // No fetch should be made.
 
   // Simulate user sign in. The service should be ready again and load snippets.
   SetUpFetchResponse(GetTestJson({GetSnippet()}));
-  EXPECT_CALL(mock_observer,
-              NTPSnippetsServiceDisabledReasonChanged(DisabledReason::NONE));
+  EXPECT_CALL(observer(),
+              OnCategoryStatusChanged(_, _, CategoryStatus::AVAILABLE_LOADING));
   EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)).Times(1);
   service()->OnDisabledReasonChanged(DisabledReason::NONE);
+  EXPECT_CALL(observer(),
+              OnCategoryStatusChanged(_, _, CategoryStatus::AVAILABLE));
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(NTPSnippetsService::State::READY, service()->state_);
   EXPECT_FALSE(service()->snippets().empty());
-
-  service()->RemoveObserver(&mock_observer);
 }
 
 }  // namespace ntp_snippets