[Background fetching] Background fetching when opening an NTP.

components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc

 // Copyright 2016 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/scheduling_remote_suggestions_provider.h"
 
 #include <memory>
 #include <set>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/clock.h"
 #include "base/time/time.h"
 #include "components/ntp_snippets/features.h"
 #include "components/ntp_snippets/ntp_snippets_constants.h"
 #include "components/ntp_snippets/pref_names.h"
 #include "components/ntp_snippets/remote/persistent_scheduler.h"
 #include "components/ntp_snippets/remote/remote_suggestions_provider.h"
 #include "components/ntp_snippets/remote/test_utils.h"
 #include "components/ntp_snippets/status.h"
 #include "components/ntp_snippets/user_classifier.h"
 #include "components/prefs/testing_pref_service.h"
 #include "components/variations/variations_params_manager.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::ElementsAre;
 using testing::Eq;
 using testing::InSequence;
 using testing::Invoke;
 using testing::IsEmpty;
 using testing::Mock;
 using testing::MockFunction;
 using testing::Not;
+using testing::Return;
 using testing::SaveArg;
 using testing::SaveArgPointee;
 using testing::SizeIs;
 using testing::StartsWith;
 using testing::StrictMock;
 using testing::WithArgs;
 using testing::_;
 
 namespace ntp_snippets {
 
 class NTPSnippetsFetcher;
 
 namespace {
 
 class MockPersistentScheduler : public PersistentScheduler {
  public:
   MOCK_METHOD2(Schedule,
                bool(base::TimeDelta period_wifi,
                     base::TimeDelta period_fallback));
   MOCK_METHOD0(Unschedule, bool());
 };
 
+class MockClock : public base::Clock {

[tschumann, 2017/01/04 10:43:52]

for clocks, you usually don't want behavioral testing (like mocks). Use
base/test/simple_test_clock.h instead.

[jkrcal, 2017/01/04 14:19:04]

Done.

+ public:
+  MOCK_METHOD0(Now, base::Time());
+};
+
 // TODO(jkrcal): Move into its own library to reuse in other unit-tests?
 class MockRemoteSuggestionsProvider : public RemoteSuggestionsProvider {
  public:
   MockRemoteSuggestionsProvider(Observer* observer)
       : RemoteSuggestionsProvider(observer) {}
 
   // Move-only params are not supported by GMock. We want to mock out
   // RefetchInTheBackground() which takes a unique_ptr<>. Instead, we add a new
   // mock function which takes a copy of the callback and override the
   // RemoteSuggestionsProvider's method to forward the call into the new mock
@@ skipping 78 matching lines @@
   }
 
  private:
   test::RemoteSuggestionsTestUtils utils_;
   UserClassifier user_classifier_;
 
   DISALLOW_COPY_AND_ASSIGN(SchedulingRemoteSuggestionsProviderTest);
 };
 
 TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldIgnoreSignalsWhenNotEnabled) {
+  scheduling_provider_->OnPersistentSchedulerWakeUp();
+  scheduling_provider_->OnNTPOpened();
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
        ShouldFetchOnPersistentSchedulerWakeUp) {
+  // First enable the scheduler.
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+  ChangeStatusOfUnderlyingProvider(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+
   EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_));
   scheduling_provider_->OnPersistentSchedulerWakeUp();
 }
 
 TEST_F(SchedulingRemoteSuggestionsProviderTest,

[tschumann, 2017/01/04 10:43:52]

there's one similar test missing:
ShouldNotTriggerBackgroundFetchIfAlreadyInProgess

[jkrcal, 2017/01/04 14:19:03]

Ah, sure! Done.

EDIT: This added test showed there was a bug in the code. You are a unit-test
magician! ;)

[tschumann, 2017/01/04 15:15:24]

hehehe... I just know that I cannot spot bugs well -- so I don't believe things
are working until I see a test :-)

+       ShouldFetchOnPersistentSchedulerWakeUpRepeated) {
+  RemoteSuggestionsProvider::FetchStatusCallback signal_fetch_done;
+  {
+    InSequence s;
+    EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+    EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_))
+        .WillOnce(SaveArg<0>(&signal_fetch_done));
+    EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+    EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_));
+  }
+  // First enable the scheduler -- calling Schedule() for the first time.
+  ChangeStatusOfUnderlyingProvider(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+  // Make the first persistent fetch successful -- calling Schedule() again.
+  scheduling_provider_->OnPersistentSchedulerWakeUp();
+  signal_fetch_done.Run(Status::Success());
+  // Make the second fetch.
+  scheduling_provider_->OnPersistentSchedulerWakeUp();
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldFetchOnNTPOpenedForTheFirstTime) {
+  // First enable the scheduler.
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+  ChangeStatusOfUnderlyingProvider(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+
+  EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_));
+  scheduling_provider_->OnNTPOpened();
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldNotFetchOnNTPOpenedAfterSuccessfulSoftFetch) {
+  // First enable the scheduler; the second Schedule is called after the
+  // successful fetch.
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _)).Times(2);
+  ChangeStatusOfUnderlyingProvider(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+
+  // Make the first soft fetch successful.
+  RemoteSuggestionsProvider::FetchStatusCallback signal_fetch_done;
+  EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_))
+      .WillOnce(SaveArg<0>(&signal_fetch_done));
+  scheduling_provider_->OnNTPOpened();
+  signal_fetch_done.Run(Status::Success());
+  // The second call is ignored if it happens right after the first one.
+  scheduling_provider_->OnNTPOpened();
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldNotFetchOnNTPOpenedAfterSuccessfulPersistentFetch) {
+  // First enable the scheduler; the second Schedule is called after the
+  // successful fetch.
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _)).Times(2);
+  ChangeStatusOfUnderlyingProvider(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+
+  // Make the first soft fetch successful.

[tschumann, 2017/01/04 10:43:52]

that's not a soft fetch, right?

[jkrcal, 2017/01/04 14:19:03]

Done.

+  RemoteSuggestionsProvider::FetchStatusCallback signal_fetch_done;
+  EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_))
+      .WillOnce(SaveArg<0>(&signal_fetch_done));
+  scheduling_provider_->OnPersistentSchedulerWakeUp();
+  signal_fetch_done.Run(Status::Success());
+  // The second call is ignored if it happens right after the first one.
+  scheduling_provider_->OnNTPOpened();
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldNotFetchOnNTPOpenedAfterFailedSoftFetch) {
+  // First enable the scheduler.
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+  ChangeStatusOfUnderlyingProvider(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+
+  // Make the first soft fetch failed.
+  RemoteSuggestionsProvider::FetchStatusCallback signal_fetch_done;
+  EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_))
+      .WillOnce(SaveArg<0>(&signal_fetch_done));
+  scheduling_provider_->OnNTPOpened();
+  signal_fetch_done.Run(Status(StatusCode::PERMANENT_ERROR, ""));
+
+  // The second call is ignored if it happens right after the first one.
+  scheduling_provider_->OnNTPOpened();
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldNotFetchOnNTPOpenedAfterFailedPersistentFetch) {
+  // First enable the scheduler.
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+  ChangeStatusOfUnderlyingProvider(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+
+  // Make the first soft fetch failed.

[tschumann, 2017/01/04 10:43:52]

first hard fetch?

[jkrcal, 2017/01/04 14:19:04]

Done.

+  RemoteSuggestionsProvider::FetchStatusCallback signal_fetch_done;
+  EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_))
+      .WillOnce(SaveArg<0>(&signal_fetch_done));
+  scheduling_provider_->OnPersistentSchedulerWakeUp();
+  signal_fetch_done.Run(Status(StatusCode::PERMANENT_ERROR, ""));
+
+  // The second call is ignored if it happens right after the first one.
+  scheduling_provider_->OnNTPOpened();
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldFetchAgainOnNTPOpenedLaterAgain) {
+  // First enable the scheduler.
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+  ChangeStatusOfUnderlyingProvider(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+
+  auto ptr = base::MakeUnique<MockClock>();
+  MockClock* mock_clock = ptr.get();
+  scheduling_provider_->SetClockForTesting(std::move(ptr));
+  base::Time first_time = base::Time::Now();
+
+  {
+    InSequence s;
+    // The first call to NTPOpened at |first_time| results in a fetch.
+    EXPECT_CALL(*mock_clock, Now()).WillOnce(Return(first_time));
+    EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_));
+
+    // The second call to NTPOpened at |first_time| + 2hrs again results in a
+    // fetch.
+    EXPECT_CALL(*mock_clock, Now())
+        .WillOnce(Return(first_time + base::TimeDelta::FromHours(2)));
+    EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_));
+  }
+  scheduling_provider_->OnNTPOpened();
+  scheduling_provider_->OnNTPOpened();
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
        ShouldRescheduleOnRescheduleFetching) {
   EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
   scheduling_provider_->RescheduleFetching();
 }
 
 TEST_F(SchedulingRemoteSuggestionsProviderTest, ShouldScheduleOnActivation) {
   EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
   ChangeStatusOfUnderlyingProvider(
       RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
 }
@@ skipping 83 matching lines @@
 TEST_F(SchedulingRemoteSuggestionsProviderTest,
        ReschedulesWhenWifiParamChanges) {
   EXPECT_CALL(persistent_scheduler_, Schedule(_, _)).Times(2);
   ChangeStatusOfUnderlyingProvider(
       RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
 
   // UserClassifier defaults to UserClass::ACTIVE_NTP_USER if PrefService is
   // null. Change the wifi interval for this class.
   variations::testing::VariationParamsManager params_manager(
       ntp_snippets::kStudyName,
-      {{"fetching_interval_hours-wifi-active_ntp_user", "2"}},
+      {{"fetching_interval_hours-wifi-active_ntp_user", "1.5"}},
       {kArticleSuggestionsFeature.name});
 
   // Schedule() should get called for the second time after params have changed.
   ChangeStatusOfUnderlyingProvider(
       RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
 }
 
 TEST_F(SchedulingRemoteSuggestionsProviderTest,
        ReschedulesWhenFallbackParamChanges) {
   EXPECT_CALL(persistent_scheduler_, Schedule(_, _)).Times(2);
   ChangeStatusOfUnderlyingProvider(
       RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
 
   // UserClassifier defaults to UserClass::ACTIVE_NTP_USER if PrefService is
   // null. Change the wifi interval for this class.
   variations::testing::VariationParamsManager params_manager(
       ntp_snippets::kStudyName,
-      {{"fetching_interval_hours-fallback-active_ntp_user", "2"}},
+      {{"fetching_interval_hours-fallback-active_ntp_user", "1.5"}},
       {kArticleSuggestionsFeature.name});
 
   // Schedule() should get called for the second time after params have changed.
+  ChangeStatusOfUnderlyingProvider(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ReschedulesWhenOnUsageEventParamChanges) {
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _)).Times(2);
+  ChangeStatusOfUnderlyingProvider(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+
+  // UserClassifier defaults to UserClass::ACTIVE_NTP_USER if PrefService is
+  // null. Change the wifi interval for this class.
+  variations::testing::VariationParamsManager params_manager(
+      ntp_snippets::kStudyName,
+      {{"soft_fetching_interval_hours-active-active_ntp_user", "1.5"}},
+      {kArticleSuggestionsFeature.name});
+
+  // Schedule() should get called for the second time after params have changed.
   ChangeStatusOfUnderlyingProvider(
       RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
 }
 
 }  // namespace ntp_snippets