[NTP Snippets] Refactor background scheduling for remote suggestions

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/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::SaveArg;
+using testing::SaveArgPointee;
+using testing::SizeIs;
+using testing::StartsWith;
+using testing::StrictMock;
+using testing::WithArgs;
+using testing::_;
+
+namespace ntp_snippets {
+
+class NTPSnippetsFetcher;
+
+namespace {
+
+const char* kDifferentInterval = "2";

[tschumann, 2016/12/21 09:41:02]

as it is, this as very little context. I'd probably define this constant inside
the 2 tests which actually use them so there's better context.

[jkrcal, 2016/12/21 11:54:39]

Agreed, I think this is actually clear enough with the literals, directly.

+
+class MockPersistentScheduler : public PersistentScheduler {
+ public:
+  MOCK_METHOD2(Schedule,
+               bool(base::TimeDelta period_wifi,
+                    base::TimeDelta period_fallback));
+  MOCK_METHOD0(Unschedule, bool());
+};
+
+class MockRemoteSuggestionsProvider : public RemoteSuggestionsProvider {

[tschumann, 2016/12/21 09:41:01]

this guy should probably go into it's on library so that other tests can use it
as well. Not necessarily in this CL but add a TODO() to follow up.

[jkrcal, 2016/12/21 11:54:40]

I am not sure which other tests need it in the close future. Added a TODO.

+ public:
+  MockRemoteSuggestionsProvider(Observer* observer)
+      : RemoteSuggestionsProvider(observer),
+        provider_status_callback_(nullptr),
+        fetch_status_callback_(nullptr) {}
+
+  void SetProviderStatusCallback(

[tschumann, 2016/12/21 09:41:02]

a comment might be nice, something like:

// SchedulingRemoteSuggestionsProvider calls this to stay in the loop of status
changes.

also shouldn't that be marked 'override'?

While I'm fine with this for now, this should actually be a mock method (using
the same pattern i explained in RefetchInTheBackground()).
So that test code can listen to events, capture the callback and run it by
itself.

If that's too much boilerplate in the single test, you can add that EXPECT call
in the test's constructor and capture the callback inside the test class.

The reasoning is that the mock is not responsible for any action, including
capturing that callback.

[jkrcal, 2016/12/21 11:54:40]

Added the comment.

+      std::unique_ptr<RemoteSuggestionsProvider::ProviderStatusCallback>
+          callback) {
+    provider_status_callback_ = std::move(callback);
+  }
+
+  void ChangeStatus(RemoteSuggestionsProvider::ProviderStatus new_status) {
+    provider_status_callback_->Run(new_status);
+  }
+
+  // Move-only params are not supported by GMock.
+  void RefetchInTheBackground(
+      std::unique_ptr<RemoteSuggestionsProvider::FetchStatusCallback>
+          callback) {

[tschumann, 2016/12/21 09:41:02]

that method is overriding RemoteSuggestionsProvider's method right? 
Please add the 'override' keyword.

Also, please extend the comment, e.g.:
// 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 plain pointer and override the
RemoteSuggestionsProvider's method to forward the call into the new mock
function.

[jkrcal, 2016/12/21 11:54:40]

Done.

+    RefetchInTheBackground(callback.get());
+    fetch_status_callback_ = std::move(callback);

[tschumann, 2016/12/21 09:41:01]

this is fishy as you're adding fake semantics.
The TEST code should run the callback itself.
Why not change the new MockMethod to take the parameter by value and we simply
copy it?

void RefetchInTheBackground(
    std::unique_ptr<RemoteSuggestionsProvider::FetchStatusCallback>
        callback) {
  RefetchInTheBackground(*callback);
}

Test code like this

  // Trigger a fetch.
  EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_));

looks really fishy as it seems you're dropping callbacks.

The code should more look like this:

  RemoteSuggestionsProvider::FetchStatusCallback signal_fetch_done;
  EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_)).
    .WillOnce(SaveArgPointee<0>(&signal_fetch_done));
  // do stuff ...
  signal_fetch_done.Run(Status::Success());

[jkrcal, 2016/12/21 11:54:40]

I am puzzled by your solution. Would not the unique pointer and the callback
itself get destroyed after the function
  
RefetchInTheBackground(std::unique_ptr<RemoteSuggestionsProvider::FetchStatusCallback>
callback)
is left?

+  }
+  MOCK_METHOD1(RefetchInTheBackground,
+               void(RemoteSuggestionsProvider::FetchStatusCallback*));
+  void CompleteBackgroundFetch(Status status_code) {
+    fetch_status_callback_->Run(status_code);
+    fetch_status_callback_.reset();
+  }
+
+  MOCK_CONST_METHOD0(snippets_fetcher_for_testing_and_debugging,
+                     const NTPSnippetsFetcher*());
+
+  MOCK_METHOD1(GetCategoryStatus, CategoryStatus(Category));
+  MOCK_METHOD1(GetCategoryInfo, CategoryInfo(Category));
+  MOCK_METHOD3(ClearHistory,
+               void(base::Time begin,
+                    base::Time end,
+                    const base::Callback<bool(const GURL& url)>& filter));
+  MOCK_METHOD3(Fetch,
+               void(const Category&,
+                    const std::set<std::string>&,
+                    const FetchDoneCallback&));
+  MOCK_METHOD1(ClearCachedSuggestions, void(Category));
+  MOCK_METHOD1(ClearDismissedSuggestionsForDebugging, void(Category));
+  MOCK_METHOD1(DismissSuggestion, void(const ContentSuggestion::ID&));
+  MOCK_METHOD2(FetchSuggestionImage,
+               void(const ContentSuggestion::ID&, const ImageFetchedCallback&));
+  MOCK_METHOD2(GetDismissedSuggestionsForDebugging,
+               void(Category, const DismissedSuggestionsCallback&));
+  MOCK_METHOD0(OnSignInStateChanged, void());
+
+  std::unique_ptr<RemoteSuggestionsProvider::ProviderStatusCallback>
+      provider_status_callback_;
+  std::unique_ptr<RemoteSuggestionsProvider::FetchStatusCallback>
+      fetch_status_callback_;
+};
+
+}  // namespace
+
+class SchedulingRemoteSuggestionsProviderTest
+    : public ::testing::Test {
+ public:
+  SchedulingRemoteSuggestionsProviderTest()
+      : underlying_provider_(nullptr),
+        scheduling_provider_(nullptr),
+        user_classifier_(/*pref_service=*/nullptr) {
+    SchedulingRemoteSuggestionsProvider::RegisterProfilePrefs(
+        utils_.pref_service()->registry());
+
+    auto underlying_provider =
+        base::MakeUnique<StrictMock<MockRemoteSuggestionsProvider>>(
+            /*observer=*/nullptr);
+    underlying_provider_ = underlying_provider.get();
+
+    scheduling_provider_ =
+        base::MakeUnique<SchedulingRemoteSuggestionsProvider>(
+            /*observer=*/nullptr, std::move(underlying_provider),
+            &persistent_scheduler_, &user_classifier_, utils_.pref_service());
+  }
+
+ protected:
+  StrictMock<MockPersistentScheduler> persistent_scheduler_;
+  StrictMock<MockRemoteSuggestionsProvider>* underlying_provider_;
+  std::unique_ptr<SchedulingRemoteSuggestionsProvider> scheduling_provider_;
+
+ private:
+  test::RemoteSuggestionsTestUtils utils_;
+  UserClassifier user_classifier_;
+
+  DISALLOW_COPY_AND_ASSIGN(SchedulingRemoteSuggestionsProviderTest);
+};
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldFetchOnPersistentSchedulerWakeUp) {
+  EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_));
+  scheduling_provider_->OnPersistentSchedulerWakeUp();
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldRescheduleOnRescheduleFetching) {
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+  scheduling_provider_->RescheduleFetching();
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest, ShouldScheduleOnActivation) {
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+  underlying_provider_->ChangeStatus(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldUnscheduleOnLaterInactivation) {
+  {
+    InSequence s;
+    EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+    EXPECT_CALL(persistent_scheduler_, Unschedule());
+  }
+  underlying_provider_->ChangeStatus(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+  underlying_provider_->ChangeStatus(
+      RemoteSuggestionsProvider::ProviderStatus::INACTIVE);
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldScheduleOnLaterActivation) {
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+  // There is no schedule yet, so inactivation does not trigger unschedule.
+  underlying_provider_->ChangeStatus(
+      RemoteSuggestionsProvider::ProviderStatus::INACTIVE);
+  underlying_provider_->ChangeStatus(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldRescheduleAfterSuccessfulFetch) {
+  // First reschedule on becoming active.
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _)).Times(2);
+  underlying_provider_->ChangeStatus(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+
+  // Trigger a fetch.
+  EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_));
+  scheduling_provider_->OnPersistentSchedulerWakeUp();
+  // Second reschedule after a successful fetch.
+  underlying_provider_->CompleteBackgroundFetch(Status::Success());
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldNotRescheduleAfterFailedFetch) {
+  // Only reschedule on becoming active.
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+  underlying_provider_->ChangeStatus(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+
+  // Trigger a fetch.
+  EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_));
+  scheduling_provider_->OnPersistentSchedulerWakeUp();
+  // No furter reschedule after a failure.
+  underlying_provider_->CompleteBackgroundFetch(
+      Status(StatusCode::PERMANENT_ERROR, ""));
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest, ShouldScheduleOnlyOnce) {
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+  underlying_provider_->ChangeStatus(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+  // No further call to Schedule on a second status callback.
+  underlying_provider_->ChangeStatus(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest, ShouldUnscheduleOnlyOnce) {
+  {
+    InSequence s;
+    EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+    EXPECT_CALL(persistent_scheduler_, Unschedule());
+  }
+  // First schedule so that later we really unschedule.
+  underlying_provider_->ChangeStatus(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+  underlying_provider_->ChangeStatus(
+      RemoteSuggestionsProvider::ProviderStatus::INACTIVE);
+  // No further call to Unschedule on second status callback.
+  underlying_provider_->ChangeStatus(
+      RemoteSuggestionsProvider::ProviderStatus::INACTIVE);
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ReschedulesWhenWifiParamChanges) {
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _)).Times(2);
+  underlying_provider_->ChangeStatus(
+      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", kDifferentInterval}},
+      {kArticleSuggestionsFeature.name});
+
+  // Schedule() should get called for the second time after params have changed.
+  underlying_provider_->ChangeStatus(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ReschedulesWhenFallbackParamChanges) {
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _)).Times(2);
+  underlying_provider_->ChangeStatus(
+      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",
+        kDifferentInterval}},
+      {kArticleSuggestionsFeature.name});
+
+  // Schedule() should get called for the second time after params have changed.
+  underlying_provider_->ChangeStatus(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+}
+
+}  // namespace ntp_snippets