[NTP Snippets] Refactor background scheduling for remote suggestions

components/ntp_snippets/remote/remote_suggestions_provider_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.h"
 
 #include <memory>
 #include <utility>
 #include <vector>
 
@@ skipping 14 matching lines @@
 #include "base/time/time.h"
 #include "components/image_fetcher/image_decoder.h"
 #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/category_info.h"
 #include "components/ntp_snippets/ntp_snippets_constants.h"
 #include "components/ntp_snippets/pref_names.h"
 #include "components/ntp_snippets/remote/ntp_snippet.h"
 #include "components/ntp_snippets/remote/ntp_snippets_fetcher.h"
-#include "components/ntp_snippets/remote/ntp_snippets_scheduler.h"
 #include "components/ntp_snippets/remote/remote_suggestions_database.h"
+#include "components/ntp_snippets/remote/remote_suggestions_hard_scheduler.h"
 #include "components/ntp_snippets/remote/test_utils.h"
 #include "components/ntp_snippets/user_classifier.h"
 #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_params_manager.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"
@@ skipping 253 matching lines @@
       int id,
       const GURL& url,
       net::URLFetcher::RequestType request_type,
       net::URLFetcherDelegate* d) override {
     return base::MakeUnique<net::FakeURLFetcher>(
         url, d, /*response_data=*/std::string(), net::HTTP_NOT_FOUND,
         net::URLRequestStatus::FAILED);
   }
 };
 
-class MockScheduler : public NTPSnippetsScheduler {
+class MockHardScheduler : public RemoteSuggestionsHardScheduler {
  public:
   MOCK_METHOD2(Schedule,
                bool(base::TimeDelta period_wifi,
                     base::TimeDelta period_fallback));
   MOCK_METHOD0(Unschedule, bool());
 };
 
 class MockImageFetcher : public ImageFetcher {
  public:
   MOCK_METHOD1(SetImageFetcherDelegate, void(ImageFetcherDelegate*));
@@ skipping 124 matching lines @@
     auto image_fetcher = base::MakeUnique<NiceMock<MockImageFetcher>>();
 
     image_fetcher_ = image_fetcher.get();
     EXPECT_CALL(*image_fetcher, SetImageFetcherDelegate(_));
     auto image_decoder = base::MakeUnique<FakeImageDecoder>();
     image_decoder_ = image_decoder.get();
     EXPECT_FALSE(observer_);
     observer_ = base::MakeUnique<FakeContentSuggestionsProviderObserver>();
     return base::MakeUnique<RemoteSuggestionsProvider>(
         observer_.get(), &category_factory_, utils_.pref_service(), "fr",
-        &user_classifier_, &scheduler_, std::move(snippets_fetcher),
+        &user_classifier_, &hard_scheduler_, std::move(snippets_fetcher),
         std::move(image_fetcher), std::move(image_decoder),
         base::MakeUnique<RemoteSuggestionsDatabase>(database_dir_.GetPath(),
                                                     task_runner),
         base::MakeUnique<RemoteSuggestionsStatusService>(
             utils_.fake_signin_manager(), utils_.pref_service()));
   }
 
   void WaitForSnippetsServiceInitialization(RemoteSuggestionsProvider* service,
                                             bool set_empty_response) {
     EXPECT_EQ(RemoteSuggestionsProvider::State::NOT_INITED, service->state_);
@@ skipping 30 matching lines @@
 
   Category other_category() { return category_factory_.FromRemoteCategory(2); }
 
   Category unknown_category() {
     return category_factory_.FromRemoteCategory(kUnknownRemoteCategoryId);
   }
 
  protected:
   const GURL& test_url() { return test_url_; }
   FakeContentSuggestionsProviderObserver& observer() { return *observer_; }
-  MockScheduler& mock_scheduler() { return scheduler_; }
+  MockHardScheduler& mock_scheduler() { return hard_scheduler_; }
   // 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(); }
 
   // 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);
@@ skipping 19 matching lines @@
  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<OAuth2TokenService> fake_token_service_;
   UserClassifier user_classifier_;
-  NiceMock<MockScheduler> scheduler_;
+  NiceMock<MockHardScheduler> hard_scheduler_;
   std::unique_ptr<FakeContentSuggestionsProviderObserver> observer_;
   CategoryFactory category_factory_;
   NiceMock<MockImageFetcher>* image_fetcher_;
   FakeImageDecoder* image_decoder_;
 
   base::ScopedTempDir database_dir_;
 
   DISALLOW_COPY_AND_ASSIGN(RemoteSuggestionsProviderTest);
 };
 
-TEST_F(RemoteSuggestionsProviderTest, ScheduleOnStart) {

[Marc Treib, 2016/12/09 12:25:26]

I think some of the removed tests kinda still apply - what's the new behavior of
RemoteSuggestionsProvider vs the scheduler?
I think injecting a mock scheduler (the full thing, not just the "hard" one)
might be good, then we could test that the provider calls the right things at
the right time.

[jkrcal, 2016/12/19 09:40:23]

Not relevant any more. I've added some more tests for notifying status changes.

-  // We should get two |Schedule| calls: The first when initialization
-  // completes, the second one after the automatic (since the service doesn't
-  // have any data yet) fetch finishes.
-  EXPECT_CALL(mock_scheduler(), Schedule(_, _)).Times(2);
-  EXPECT_CALL(mock_scheduler(), Unschedule()).Times(0);
-  auto service = MakeSnippetsService();
-
-  // When we have no snippets are all, loading the service initiates a fetch.
-  EXPECT_EQ("OK", service->snippets_fetcher()->last_status());
-}
-
-TEST_F(RemoteSuggestionsProviderTest, DontRescheduleOnStart) {
-  EXPECT_CALL(mock_scheduler(), Schedule(_, _)).Times(2);
-  EXPECT_CALL(mock_scheduler(), Unschedule()).Times(0);
-  SetUpFetchResponse(GetTestJson({GetSnippet()}));
-  auto service = MakeSnippetsService(/*set_empty_response=*/false);
-
-  // When recreating the service, we should not get any |Schedule| calls:
-  // The tasks are already scheduled with the correct intervals, so nothing on
-  // initialization, and the service has data from the DB, so no automatic fetch
-  // should happen.
-  Mock::VerifyAndClearExpectations(&mock_scheduler());
-  EXPECT_CALL(mock_scheduler(), Schedule(_, _)).Times(0);
-  EXPECT_CALL(mock_scheduler(), Unschedule()).Times(0);
-  ResetSnippetsService(&service);
-}
-
-TEST_F(RemoteSuggestionsProviderTest, RescheduleAfterSuccessfulFetch) {
-  // We should get two |Schedule| calls: The first when initialization
-  // completes, the second one after the automatic (since the service doesn't
-  // have any data yet) fetch finishes.
-  EXPECT_CALL(mock_scheduler(), Schedule(_, _)).Times(2);
-  auto service = MakeSnippetsService();
-
-  // A successful fetch should trigger another |Schedule|.
-  EXPECT_CALL(mock_scheduler(), Schedule(_, _));
-  LoadFromJSONString(service.get(), GetTestJson({GetSnippet()}));
-}
-
 TEST_F(RemoteSuggestionsProviderTest, DontRescheduleAfterFailedFetch) {
   // We should get two |Schedule| calls: The first when initialization
   // completes, the second one after the automatic (since the service doesn't
   // have any data yet) fetch finishes.
   EXPECT_CALL(mock_scheduler(), Schedule(_, _)).Times(2);
   auto service = MakeSnippetsService();
 
   // A failed fetch should NOT trigger another |Schedule|.
   EXPECT_CALL(mock_scheduler(), Schedule(_, _)).Times(0);
   LoadFromJSONString(service.get(), GetTestJson({GetInvalidSnippet()}));
 }
 
-TEST_F(RemoteSuggestionsProviderTest, IgnoreRescheduleBeforeInit) {
-  // We should get two |Schedule| calls: The first when initialization
-  // completes, the second one after the automatic (since the service doesn't
-  // have any data yet) fetch finishes.
-  EXPECT_CALL(mock_scheduler(), Schedule(_, _)).Times(2);
-  // The |RescheduleFetching| call shouldn't do anything (in particular not
-  // result in an |Unschedule|), since the service isn't initialized yet.
-  EXPECT_CALL(mock_scheduler(), Unschedule()).Times(0);
-  auto service = MakeSnippetsServiceWithoutInitialization();
-  service->RescheduleFetching(false);
-  WaitForSnippetsServiceInitialization(service.get(),
-                                       /*set_empty_response=*/true);
-}
-
-TEST_F(RemoteSuggestionsProviderTest, HandleForcedRescheduleBeforeInit) {
-  {
-    InSequence s;
-    // The |RescheduleFetching| call with force=true should result in an
-    // |Unschedule|, since the service isn't initialized yet.
-    EXPECT_CALL(mock_scheduler(), Unschedule()).Times(1);
-    // We should get two |Schedule| calls: The first when initialization
-    // completes, the second one after the automatic (since the service doesn't
-    // have any data yet) fetch finishes.
-    EXPECT_CALL(mock_scheduler(), Schedule(_, _)).Times(2);
-  }
-  auto service = MakeSnippetsServiceWithoutInitialization();
-  service->RescheduleFetching(true);
-  WaitForSnippetsServiceInitialization(service.get(),
-                                       /*set_empty_response=*/true);
-}
-
 TEST_F(RemoteSuggestionsProviderTest, RescheduleOnStateChange) {
   {
     InSequence s;
     // Initial startup.
     EXPECT_CALL(mock_scheduler(), Schedule(_, _)).Times(2);
     // Service gets disabled.
     EXPECT_CALL(mock_scheduler(), Unschedule());
     // Service gets enabled again.
     EXPECT_CALL(mock_scheduler(), Schedule(_, _)).Times(2);
   }
   auto service = MakeSnippetsService();
   ASSERT_TRUE(service->ready());
 
   service->OnStatusChanged(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN,
                            RemoteSuggestionsStatus::EXPLICITLY_DISABLED);
   ASSERT_FALSE(service->ready());
   base::RunLoop().RunUntilIdle();
 
   service->OnStatusChanged(RemoteSuggestionsStatus::EXPLICITLY_DISABLED,
                            RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT);
   ASSERT_TRUE(service->ready());
   base::RunLoop().RunUntilIdle();
 }
 
-TEST_F(RemoteSuggestionsProviderTest, DontUnscheduleOnShutdown) {
-  EXPECT_CALL(mock_scheduler(), Schedule(_, _)).Times(2);
-  EXPECT_CALL(mock_scheduler(), Unschedule()).Times(0);
-
-  auto service = MakeSnippetsService();
-
-  service.reset();
-  base::RunLoop().RunUntilIdle();
-}
 
 TEST_F(RemoteSuggestionsProviderTest, Full) {
   std::string json_str(GetTestJson({GetSnippet()}));
 
   auto service = MakeSnippetsService();
 
   LoadFromJSONString(service.get(), json_str);
 
   ASSERT_THAT(observer().SuggestionsForCategory(articles_category()),
               SizeIs(1));
@@ skipping 928 matching lines @@
 
   WaitForSnippetsServiceInitialization(service.get(),
                                        /*set_empty_response=*/true);
   EXPECT_EQ(
       simple_test_tick_clock_ptr->NowTicks().ToInternalValue(),
       pref_service()->GetInt64(prefs::kLastSuccessfulBackgroundFetchTime));
 
   // Advance the time and check whether the time was updated correctly after the
   // background fetch.
   simple_test_tick_clock_ptr->Advance(TimeDelta::FromHours(1));
-  service->FetchSnippetsInTheBackground();
+
+  // TODO(jkrcal): Move together with the pref storage into the scheduler.
+  static_cast<RemoteSuggestionsScheduler::Updater*>(service.get())
+      ->UpdateRemoteSuggestionsBySchedule();
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(
       simple_test_tick_clock_ptr->NowTicks().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).
 }
 
 }  // namespace ntp_snippets