[Background fetching] Background fetching when opening an NTP.

components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc

Index: components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc
diff --git a/components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc b/components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc
index 0015c8356ad8bb8724768dd2f3cf2b0b9b2df446..b5ed0cd3149ca58ae7b51dc87aa3e9bc4bf9c268 100644
--- a/components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc
+++ b/components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc
@@ -16,6 +16,7 @@
 #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"
@@ -38,6 +39,7 @@ using testing::IsEmpty;
 using testing::Mock;
 using testing::MockFunction;
 using testing::Not;
+using testing::Return;
 using testing::SaveArg;
 using testing::SaveArgPointee;
 using testing::SizeIs;
@@ -60,6 +62,11 @@ class MockPersistentScheduler : public PersistentScheduler {
   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:
@@ -158,9 +165,152 @@ class 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,
@@ -267,7 +417,7 @@ TEST_F(SchedulingRemoteSuggestionsProviderTest,
   // 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.
@@ -285,7 +435,25 @@ TEST_F(SchedulingRemoteSuggestionsProviderTest,
   // 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.