[Remote suggestions] Refactor the scheduler

components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc

Index: components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc
diff --git a/components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc b/components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc
index f2039ae9af1cf82ef1600e2bb897a9bcedb0149d..e8776b3321db0d567a1f35110fc2eba391340809 100644
--- a/components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc
+++ b/components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc
@@ -382,6 +382,8 @@ class MockScheduler : public RemoteSuggestionsScheduler {
   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());
@@ -458,7 +460,7 @@ class RemoteSuggestionsProviderImplTest : public ::testing::Test {
     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()));
@@ -526,6 +528,7 @@ class RemoteSuggestionsProviderImplTest : public ::testing::Test {
   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) {
@@ -554,6 +557,8 @@ class RemoteSuggestionsProviderImplTest : public ::testing::Test {
                               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();
   }
@@ -572,6 +577,7 @@ class RemoteSuggestionsProviderImplTest : public ::testing::Test {
   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_;
@@ -1724,59 +1730,33 @@ TEST_F(RemoteSuggestionsProviderImplTest,
   // 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);
 }
@@ -1784,12 +1764,9 @@ TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerWhenDisabled) {
 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)>());
 }
@@ -1797,13 +1774,9 @@ TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerWhenHistoryCleared) {
 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);
+  // The scheduler should be notified of clearing the history.
+  EXPECT_CALL(*scheduler(), OnSuggestionsCleared());
   service->OnStatusChanged(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN,
                            RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT);
 }
@@ -1811,13 +1784,9 @@ TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerWhenSignedIn) {
 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);
+  // The scheduler should be notified of clearing the history.
+  EXPECT_CALL(*scheduler(), OnSuggestionsCleared());
   service->OnStatusChanged(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT,
                            RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN);
 }