[Remote suggestions] Refactor the scheduler

components/ntp_snippets/remote/remote_suggestions_provider_impl_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/remote/remote_suggestions_provider_impl.h"
 
 #include <memory>
 #include <utility>
 #include <vector>
 
@@ skipping 364 matching lines @@
  private:
   gfx::Image decoded_image_;
 };
 
 class MockScheduler : public RemoteSuggestionsScheduler {
  public:
   MOCK_METHOD0(OnProviderActivated, void());
   MOCK_METHOD0(OnProviderDeactivated, void());
   MOCK_METHOD0(OnSuggestionsCleared, void());
   MOCK_METHOD0(OnHistoryCleared, void());
+  MOCK_METHOD0(AcquireQuotaForInteractiveFetch, bool());
+  MOCK_METHOD1(OnInteractiveFetchFinished, void(Status fetch_status));
   MOCK_METHOD0(OnBrowserForegrounded, void());
   MOCK_METHOD0(OnBrowserColdStart, void());
   MOCK_METHOD0(OnNTPOpened, void());
   MOCK_METHOD0(OnPersistentSchedulerWakeUp, void());
   MOCK_METHOD0(RescheduleFetching, void());
 };
 
 }  // namespace
 
 class RemoteSuggestionsProviderImplTest : public ::testing::Test {
@@ skipping 56 matching lines @@
     EXPECT_CALL(*image_fetcher, SetImageFetcherDelegate(_));
     ON_CALL(*image_fetcher, GetImageDecoder())
         .WillByDefault(Return(&image_decoder_));
     EXPECT_FALSE(observer_);
     observer_ = base::MakeUnique<FakeContentSuggestionsProviderObserver>();
     auto database = base::MakeUnique<RemoteSuggestionsDatabase>(
         database_dir_.GetPath(), task_runner);
     database_ = database.get();
     return base::MakeUnique<RemoteSuggestionsProviderImpl>(
         observer_.get(), utils_.pref_service(), "fr", category_ranker_.get(),
-        std::move(suggestions_fetcher), std::move(image_fetcher),
+        &scheduler_, std::move(suggestions_fetcher), std::move(image_fetcher),
         std::move(database),
         base::MakeUnique<RemoteSuggestionsStatusService>(
             utils_.fake_signin_manager(), utils_.pref_service()));
   }
 
   void WaitForSuggestionsProviderInitialization(
       RemoteSuggestionsProviderImpl* service,
       bool set_empty_response) {
     EXPECT_EQ(RemoteSuggestionsProviderImpl::State::NOT_INITED,
               service->state_);
@@ skipping 47 matching lines @@
   FakeContentSuggestionsProviderObserver& observer() { return *observer_; }
   RemoteSuggestionsFetcher* suggestions_fetcher() {
     return suggestions_fetcher_;
   }
   // TODO(tschumann): Make this a strict-mock. We want to avoid unneccesary
   // network requests.
   NiceMock<MockImageFetcher>* image_fetcher() { return image_fetcher_; }
   FakeImageDecoder* image_decoder() { return &image_decoder_; }
   PrefService* pref_service() { return utils_.pref_service(); }
   RemoteSuggestionsDatabase* database() { return database_; }
+  MockScheduler* 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);
   }
 
   // Have the fake fetcher fail due to a HTTP error like a 404.
   void SetUpHttpError() {
     fake_url_fetcher_factory_.SetFakeResponse(test_url_, /*json=*/std::string(),
                                               net::HTTP_NOT_FOUND,
                                               net::URLRequestStatus::SUCCESS);
   }
 
   void LoadFromJSONString(RemoteSuggestionsProviderImpl* service,
                           const std::string& json) {
     SetUpFetchResponse(json);
     service->FetchSuggestions(/*interactive_request=*/true,
                               /*callback=*/nullptr);
     base::RunLoop().RunUntilIdle();
   }
 
   void LoadMoreFromJSONString(RemoteSuggestionsProviderImpl* service,
                               const Category& category,
                               const std::string& json,
                               const std::set<std::string>& known_ids,
                               FetchDoneCallback callback) {
     SetUpFetchResponse(json);
+    ON_CALL(scheduler_, AcquireQuotaForInteractiveFetch())
+        .WillByDefault(Return(true));

[Marc Treib, 2017/03/27 11:31:00]

Hm. Could this be an EXPECT_CALL with RetiresOnSaturation? Seems like that would
mess less with the tests' own expectations.

[jkrcal, 2017/03/27 14:09:31]

Done.

     service->Fetch(category, known_ids, callback);
     base::RunLoop().RunUntilIdle();
   }
 
  private:
   variations::testing::VariationParamsManager params_manager_;
   test::RemoteSuggestionsTestUtils 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<CategoryRanker> category_ranker_;
   UserClassifier user_classifier_;
   std::unique_ptr<FakeContentSuggestionsProviderObserver> observer_;
   RemoteSuggestionsFetcher* suggestions_fetcher_;
   NiceMock<MockImageFetcher>* image_fetcher_;
   FakeImageDecoder image_decoder_;
+  NiceMock<MockScheduler> scheduler_;

[Marc Treib, 2017/03/27 11:31:00]

Hm hm. Okay for now I think, but could we add a TODO to make this a StrictMock?
Or do you have good reasons for keeping it Nice?

[jkrcal, 2017/03/27 14:09:31]

The only reason is not to plague every unit-test with
"EXPECT_CALL(*scheduler(), OnProviderActivated());"

I could add it into WaitForSuggestionsProviderInitialization(). Do you think it
is cleaner than a NiceMock?

[Marc Treib, 2017/03/27 14:27:20]

Hm. Maybe? :)
I'm a bit worried that with a NiceMock, we're not properly testing the
integration with the scheduler. But putting scheduler expectations into every
test below clearly isn't the solution. Ideally, we'd have a separate suite of
tests for the scheduler integration, but with the current state of the provider,
that's probably not possible.

One idea: Make this a unique_ptr<MockScheduler>, and instantiate it as a
NiceMock by default. Then add some scheduler-specific tests where it's
instantiated as a StrictMock instead. Would that work?

[jkrcal, 2017/03/29 10:55:57]

Done.

 
   base::ScopedTempDir database_dir_;
   RemoteSuggestionsDatabase* database_;
 
   DISALLOW_COPY_AND_ASSIGN(RemoteSuggestionsProviderImplTest);
 };
 
 TEST_F(RemoteSuggestionsProviderImplTest, Full) {
   std::string json_str(GetTestJson({GetSuggestion()}));
 
@@ skipping 1132 matching lines @@
   service->RefetchInTheBackground(/*callback=*/nullptr);
   base::RunLoop().RunUntilIdle();
   // TODO(jkrcal): Move together with the pref storage into the scheduler.
   EXPECT_EQ(
       simple_test_clock_ptr->Now().ToInternalValue(),
       pref_service()->GetInt64(prefs::kLastSuccessfulBackgroundFetchTime));
   // TODO(markusheintz): Add a test that simulates a browser restart once the
   // scheduler refactoring is done (crbug.com/672434).
 }
 
-TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerIfInited) {
-  // Initiate the service so that it is already READY.
-  auto service = MakeSuggestionsProvider();
-  StrictMock<MockScheduler> scheduler;
-  // The scheduler should be notified of activation of the provider.
-  EXPECT_CALL(scheduler, OnProviderActivated());
-  service->SetRemoteSuggestionsScheduler(&scheduler);
-}
-
-TEST_F(RemoteSuggestionsProviderImplTest, DoesNotCallSchedulerIfNotInited) {
-  auto service = MakeSuggestionsProviderWithoutInitialization();
-  StrictMock<MockScheduler> scheduler;
-  // The provider is not initialized yet, no callback should be called on
-  // registering.
-  service->SetRemoteSuggestionsScheduler(&scheduler);
-}
-
 TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerWhenReady) {
   auto service = MakeSuggestionsProviderWithoutInitialization();
-  StrictMock<MockScheduler> scheduler;
-  // The provider is not initialized yet, no callback should be called yet.
-  service->SetRemoteSuggestionsScheduler(&scheduler);
 
   // Should be called when becoming ready.
-  EXPECT_CALL(scheduler, OnProviderActivated());
+  EXPECT_CALL(*scheduler(), OnProviderActivated());
   WaitForSuggestionsProviderInitialization(service.get(),
                                            /*set_empty_response=*/true);
 }
 
 TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerOnError) {
   auto service = MakeSuggestionsProviderWithoutInitialization();
-  StrictMock<MockScheduler> scheduler;
-  // The provider is not initialized yet, no callback should be called yet.
-  service->SetRemoteSuggestionsScheduler(&scheduler);
 
   // Should be called on error.
-  EXPECT_CALL(scheduler, OnProviderDeactivated());
+  EXPECT_CALL(*scheduler(), OnProviderDeactivated());
   service->EnterState(RemoteSuggestionsProviderImpl::State::ERROR_OCCURRED);
 }
 
 TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerWhenDisabled) {
   auto service = MakeSuggestionsProviderWithoutInitialization();
-  StrictMock<MockScheduler> scheduler;
-  // The provider is not initialized yet, no callback should be called yet.
-  service->SetRemoteSuggestionsScheduler(&scheduler);
 
   // Should be called when becoming disabled. First deactivate and only after
   // that clear the suggestions so that they are not fetched again.
   {
     InSequence s;
-    EXPECT_CALL(scheduler, OnProviderDeactivated());
+    EXPECT_CALL(*scheduler(), OnProviderDeactivated());
     ASSERT_THAT(service->ready(), Eq(false));
-    EXPECT_CALL(scheduler, OnSuggestionsCleared());
+    EXPECT_CALL(*scheduler(), OnSuggestionsCleared());
   }
   service->EnterState(RemoteSuggestionsProviderImpl::State::DISABLED);
 }
 
 TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerWhenHistoryCleared) {
   // Initiate the service so that it is already READY.
   auto service = MakeSuggestionsProvider();
-  StrictMock<MockScheduler> scheduler;
-  // The scheduler should be notified of activation of the provider.
-  EXPECT_CALL(scheduler, OnProviderActivated());
+
   // The scheduler should be notified of clearing the history.
-  EXPECT_CALL(scheduler, OnHistoryCleared());
-  service->SetRemoteSuggestionsScheduler(&scheduler);
+  EXPECT_CALL(*scheduler(), OnHistoryCleared());
   service->ClearHistory(GetDefaultCreationTime(), GetDefaultExpirationTime(),
                         base::Callback<bool(const GURL& url)>());
 }
 
 TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerWhenSignedIn) {
   // Initiate the service so that it is already READY.
   auto service = MakeSuggestionsProvider();
-  StrictMock<MockScheduler> scheduler;
-  // The scheduler should be notified of activation of the provider.
-  EXPECT_CALL(scheduler, OnProviderActivated());
+
   // The scheduler should be notified of clearing the history.
-  EXPECT_CALL(scheduler, OnSuggestionsCleared());
-
-  service->SetRemoteSuggestionsScheduler(&scheduler);
+  EXPECT_CALL(*scheduler(), OnSuggestionsCleared());
   service->OnStatusChanged(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN,
                            RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT);
 }
 
 TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerWhenSignedOut) {
   // Initiate the service so that it is already READY.
   auto service = MakeSuggestionsProvider();
-  StrictMock<MockScheduler> scheduler;
-  // The scheduler should be notified of activation of the provider.
-  EXPECT_CALL(scheduler, OnProviderActivated());
+
   // The scheduler should be notified of clearing the history.
-  EXPECT_CALL(scheduler, OnSuggestionsCleared());
-
-  service->SetRemoteSuggestionsScheduler(&scheduler);
+  EXPECT_CALL(*scheduler(), OnSuggestionsCleared());
   service->OnStatusChanged(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT,
                            RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN);
 }
 
 }  // namespace ntp_snippets