Request Picker task

components/offline_pages/background/pick_request_task_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/offline_pages/background/request_picker.h"
+#include "components/offline_pages/background/pick_request_task.h"
+
+#include <memory>
+#include <set>
 
 #include "base/bind.h"
 #include "base/test/test_simple_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
-#include "base/time/time.h"
 #include "components/offline_pages/background/device_conditions.h"
-#include "components/offline_pages/background/offliner_factory.h"
 #include "components/offline_pages/background/offliner_policy.h"
+#include "components/offline_pages/background/request_coordinator_event_logger.h"
 #include "components/offline_pages/background/request_notifier.h"
-#include "components/offline_pages/background/request_queue.h"
 #include "components/offline_pages/background/request_queue_in_memory_store.h"
+#include "components/offline_pages/background/request_queue_store.h"
 #include "components/offline_pages/background/save_page_request.h"
-#include "components/offline_pages/offline_page_item.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace offline_pages {
 
 namespace {
 // Data for request 1.
 const int64_t kRequestId1 = 17;
 const GURL kUrl1("https://google.com");
 const ClientId kClientId1("bookmark", "1234");
 // Data for request 2.
@@ skipping 12 matching lines @@
 const int kBackgroundProcessingTimeBudgetSeconds = 170;
 
 // Default request
 const SavePageRequest kEmptyRequest(0UL,
                                     GURL(""),
                                     ClientId("", ""),
                                     base::Time(),
                                     true);
 }  // namespace
 
+// Helper class needed by the PickRequestTask
 class RequestNotifierStub : public RequestNotifier {
  public:
   RequestNotifierStub()
       : last_expired_request_(kEmptyRequest), total_expired_requests_(0) {}
 
   void NotifyAdded(const SavePageRequest& request) override {}
   void NotifyChanged(const SavePageRequest& request) override {}
 
   void NotifyCompleted(const SavePageRequest& request,
                        BackgroundSavePageResult status) override {
     last_expired_request_ = request;
     last_request_expiration_status_ = status;
     total_expired_requests_++;
   }
 
   const SavePageRequest& last_expired_request() {
     return last_expired_request_;
   }
 
   RequestCoordinator::BackgroundSavePageResult
   last_request_expiration_status() {
     return last_request_expiration_status_;
   }
 
   int32_t total_expired_requests() { return total_expired_requests_; }
 
  private:
   BackgroundSavePageResult last_request_expiration_status_;
   SavePageRequest last_expired_request_;

[fgorski, 2016/11/09 17:58:07]

is this field used?

[Pete Williamson, 2016/11/09 23:53:56]

Yes, it is used in the ChooseNonExpiredRequest test to verify that the request
we expected to get expired actually got expired.  It is used indirectly via the
last_expired_request() accessor function.

[fgorski, 2016/11/10 19:15:50]

Yeah, I meant to remove this comment, once I unfolded the code.

   int32_t total_expired_requests_;
 };
 
-class RequestPickerTest : public testing::Test {
+class PickRequestTaskTest : public testing::Test {

[fgorski, 2016/11/09 17:58:07]

Can you amend the tests to cover TaskComplete() calls?

SetTaskCompletionCallbackForTesting is the method to use.

Alternatively (or in addition to that) tests for this functionality in on the
queue would help you discover missing taskcomplete() calls.

[Pete Williamson, 2016/11/09 23:53:56]

Added tests in this class.  I'm not sure how to add a test to the task queue
tests to make sure that TackComplete() is always called, if that is what you
meant.  We could add a complete called tracker to the task_class, and check it
manually from the tests, is that what you meant?

  public:
-  RequestPickerTest();
+  PickRequestTaskTest();
 
-  ~RequestPickerTest() override;
+  ~PickRequestTaskTest() override;
 
   void SetUp() override;
 
   void PumpLoop();
 
-  void AddRequestDone(RequestQueue::AddRequestResult result,
-                      const SavePageRequest& request);
+  void AddRequestDone(ItemActionStatus status);
 
   void RequestPicked(const SavePageRequest& request);
 
   void RequestNotPicked(const bool non_user_requested_tasks_remaining);
 
-  void QueueRequestsAndChooseOne(const SavePageRequest& request1,
-                                 const SavePageRequest& request2);
+  void QueueRequests(const SavePageRequest& request1,
+                     const SavePageRequest& request2);
+
+  // Reset the factory and the task using the current policy.
+  void MakeFactoryAndTask();
 
   RequestNotifierStub* GetNotifier() { return notifier_.get(); }
 
+  PickRequestTask* task() { return task_.get(); }
+
  protected:
-  // The request queue is simple enough we will use a real queue with a memory
-  // store instead of a stub.
-  std::unique_ptr<RequestQueue> queue_;
-  std::unique_ptr<RequestPicker> picker_;
+  std::unique_ptr<RequestQueueStore> store_;
   std::unique_ptr<RequestNotifierStub> notifier_;
   std::unique_ptr<SavePageRequest> last_picked_;
   std::unique_ptr<OfflinerPolicy> policy_;
   RequestCoordinatorEventLogger event_logger_;
+  std::set<int64_t> disabled_requests_;
+  std::unique_ptr<PickRequestTaskFactory> factory_;
+  std::unique_ptr<PickRequestTask> task_;
   bool request_queue_not_picked_called_;
 
  private:
   scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
   base::ThreadTaskRunnerHandle task_runner_handle_;
 };
 
-RequestPickerTest::RequestPickerTest()
+PickRequestTaskTest::PickRequestTaskTest()
     : task_runner_(new base::TestSimpleTaskRunner),
       task_runner_handle_(task_runner_) {}
 
-RequestPickerTest::~RequestPickerTest() {}
+PickRequestTaskTest::~PickRequestTaskTest() {}
 
-void RequestPickerTest::SetUp() {
-  std::unique_ptr<RequestQueueInMemoryStore> store(
-      new RequestQueueInMemoryStore());
-  queue_.reset(new RequestQueue(std::move(store)));
+void PickRequestTaskTest::SetUp() {
+  DeviceConditions conditions;
+  store_.reset(new RequestQueueInMemoryStore());
   policy_.reset(new OfflinerPolicy());
   notifier_.reset(new RequestNotifierStub());
-  picker_.reset(new RequestPicker(queue_.get(), policy_.get(), notifier_.get(),
-                                  &event_logger_));
+  factory_.reset(new PickRequestTaskFactory(policy_.get(), notifier_.get(),
+                                            &event_logger_));
+  task_ = factory_->CreatePickerTask(
+      store_.get(),
+      base::Bind(&PickRequestTaskTest::RequestPicked, base::Unretained(this)),
+      base::Bind(&PickRequestTaskTest::RequestNotPicked,
+                 base::Unretained(this)),
+      conditions, disabled_requests_);
   request_queue_not_picked_called_ = false;
+  last_picked_.reset();
 }
 
-void RequestPickerTest::PumpLoop() {
+void PickRequestTaskTest::PumpLoop() {
   task_runner_->RunUntilIdle();
 }
 
-void RequestPickerTest::AddRequestDone(RequestQueue::AddRequestResult result,
-                                       const SavePageRequest& request) {}
+void PickRequestTaskTest::AddRequestDone(ItemActionStatus status) {}
 
-void RequestPickerTest::RequestPicked(const SavePageRequest& request) {
+void PickRequestTaskTest::RequestPicked(const SavePageRequest& request) {
   last_picked_.reset(new SavePageRequest(request));
 }
 
-void RequestPickerTest::RequestNotPicked(
+void PickRequestTaskTest::RequestNotPicked(
     const bool non_user_requested_tasks_remaining) {
   request_queue_not_picked_called_ = true;
 }
 
-// Test helper to queue the two given requests and then pick one of them per
-// configured policy.
-void RequestPickerTest::QueueRequestsAndChooseOne(
-    const SavePageRequest& request1, const SavePageRequest& request2) {
+// Test helper to queue the two given requests.
+void PickRequestTaskTest::QueueRequests(const SavePageRequest& request1,
+                                        const SavePageRequest& request2) {
   DeviceConditions conditions;
   std::set<int64_t> disabled_requests;
   // Add test requests on the Queue.
-  queue_->AddRequest(request1, base::Bind(&RequestPickerTest::AddRequestDone,
+  store_->AddRequest(request1, base::Bind(&PickRequestTaskTest::AddRequestDone,
                                           base::Unretained(this)));
-  queue_->AddRequest(request2, base::Bind(&RequestPickerTest::AddRequestDone,
+  store_->AddRequest(request2, base::Bind(&PickRequestTaskTest::AddRequestDone,
                                           base::Unretained(this)));
 
   // Pump the loop to give the async queue the opportunity to do the adds.
   PumpLoop();
-
-  // Call the method under test.
-  picker_->ChooseNextRequest(
-      base::Bind(&RequestPickerTest::RequestPicked, base::Unretained(this)),
-      base::Bind(&RequestPickerTest::RequestNotPicked, base::Unretained(this)),
-      &conditions, disabled_requests);
-
-  // Pump the loop again to give the async queue the opportunity to return
-  // results from the Get operation, and for the picker to call the "picked"
-  // callback.
-  PumpLoop();
 }
 
-TEST_F(RequestPickerTest, PickFromEmptyQueue) {
+void PickRequestTaskTest::MakeFactoryAndTask() {
+  factory_.reset(new PickRequestTaskFactory(policy_.get(), notifier_.get(),
+                                            &event_logger_));
   DeviceConditions conditions;
-  std::set<int64_t> disabled_requests;
-  picker_->ChooseNextRequest(
-      base::Bind(&RequestPickerTest::RequestPicked, base::Unretained(this)),
-      base::Bind(&RequestPickerTest::RequestNotPicked, base::Unretained(this)),
-      &conditions, disabled_requests);
+  task_ = factory_->CreatePickerTask(
+      store_.get(),
+      base::Bind(&PickRequestTaskTest::RequestPicked, base::Unretained(this)),
+      base::Bind(&PickRequestTaskTest::RequestNotPicked,
+                 base::Unretained(this)),
+      conditions, disabled_requests_);
+}
+
+TEST_F(PickRequestTaskTest, PickFromEmptyQueue) {
+  task()->Run();
+  PumpLoop();
 
   // Pump the loop again to give the async queue the opportunity to return
   // results from the Get operation, and for the picker to call the "QueueEmpty"
   // callback.
   PumpLoop();
 
   EXPECT_TRUE(request_queue_not_picked_called_);
 }
 
-TEST_F(RequestPickerTest, ChooseRequestWithHigherRetryCount) {
+TEST_F(PickRequestTaskTest, ChooseRequestWithHigherRetryCount) {
+  // Set up policy to prefer higher retry count.
   policy_.reset(new OfflinerPolicy(
       kPreferUntried, kPreferEarlier, kPreferRetryCount, kMaxStartedTries,
       kMaxCompletedTries + 1, kBackgroundProcessingTimeBudgetSeconds));
-  picker_.reset(new RequestPicker(queue_.get(), policy_.get(), notifier_.get(),
-                                  &event_logger_));
+  MakeFactoryAndTask();
 
   base::Time creation_time = base::Time::Now();
-  SavePageRequest request1(
-      kRequestId1, kUrl1, kClientId1, creation_time, kUserRequested);
-  SavePageRequest request2(
-      kRequestId2, kUrl2, kClientId2, creation_time, kUserRequested);
+  SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time,
+                           kUserRequested);
+  SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time,
+                           kUserRequested);
   request2.set_completed_attempt_count(kAttemptCount);
 
-  QueueRequestsAndChooseOne(request1, request2);
+  QueueRequests(request1, request2);
+
+  task()->Run();
+  PumpLoop();
 
   EXPECT_EQ(kRequestId2, last_picked_->request_id());
   EXPECT_FALSE(request_queue_not_picked_called_);
 }
 
-TEST_F(RequestPickerTest, ChooseRequestWithSameRetryCountButEarlier) {
+TEST_F(PickRequestTaskTest, ChooseRequestWithSameRetryCountButEarlier) {
   base::Time creation_time1 =
       base::Time::Now() - base::TimeDelta::FromSeconds(10);
   base::Time creation_time2 = base::Time::Now();
   SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time1,
                            kUserRequested);
   SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time2,
                            kUserRequested);
 
-  QueueRequestsAndChooseOne(request1, request2);
+  QueueRequests(request1, request2);
+
+  task()->Run();
+  PumpLoop();
 
   EXPECT_EQ(kRequestId1, last_picked_->request_id());
   EXPECT_FALSE(request_queue_not_picked_called_);
 }
 
-TEST_F(RequestPickerTest, ChooseEarlierRequest) {
+TEST_F(PickRequestTaskTest, ChooseEarlierRequest) {
   // We need a custom policy object prefering recency to retry count.
   policy_.reset(new OfflinerPolicy(
       kPreferUntried, kPreferEarlier, !kPreferRetryCount, kMaxStartedTries,
       kMaxCompletedTries, kBackgroundProcessingTimeBudgetSeconds));
-  picker_.reset(new RequestPicker(queue_.get(), policy_.get(), notifier_.get(),
-                                  &event_logger_));
+  MakeFactoryAndTask();
 
   base::Time creation_time1 =
       base::Time::Now() - base::TimeDelta::FromSeconds(10);
   base::Time creation_time2 = base::Time::Now();
   SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time1,
                            kUserRequested);
   SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time2,
                            kUserRequested);
   request2.set_completed_attempt_count(kAttemptCount);
 
-  QueueRequestsAndChooseOne(request1, request2);
+  QueueRequests(request1, request2);
+
+  task()->Run();
+  PumpLoop();
 
   EXPECT_EQ(kRequestId1, last_picked_->request_id());
   EXPECT_FALSE(request_queue_not_picked_called_);
 }
 
-TEST_F(RequestPickerTest, ChooseSameTimeRequestWithHigherRetryCount) {
+TEST_F(PickRequestTaskTest, ChooseSameTimeRequestWithHigherRetryCount) {
   // We need a custom policy object preferring recency to retry count.
   policy_.reset(new OfflinerPolicy(
       kPreferUntried, kPreferEarlier, !kPreferRetryCount, kMaxStartedTries,
       kMaxCompletedTries + 1, kBackgroundProcessingTimeBudgetSeconds));
-  picker_.reset(new RequestPicker(queue_.get(), policy_.get(), notifier_.get(),
-                                  &event_logger_));
+  MakeFactoryAndTask();
+  ;

[fgorski, 2016/11/09 17:58:07]

nit: ;

[Pete Williamson, 2016/11/09 23:53:56]

Done.

 
   base::Time creation_time = base::Time::Now();
   SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time,
                            kUserRequested);
   SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time,
                            kUserRequested);
   request2.set_completed_attempt_count(kAttemptCount);
 
-  QueueRequestsAndChooseOne(request1, request2);
+  QueueRequests(request1, request2);
+
+  task()->Run();
+  PumpLoop();
 
   EXPECT_EQ(kRequestId2, last_picked_->request_id());
   EXPECT_FALSE(request_queue_not_picked_called_);
 }
 
-TEST_F(RequestPickerTest, ChooseRequestWithLowerRetryCount) {
+TEST_F(PickRequestTaskTest, ChooseRequestWithLowerRetryCount) {
   // We need a custom policy object preferring lower retry count.
   policy_.reset(new OfflinerPolicy(
       !kPreferUntried, kPreferEarlier, kPreferRetryCount, kMaxStartedTries,
       kMaxCompletedTries + 1, kBackgroundProcessingTimeBudgetSeconds));
-  picker_.reset(new RequestPicker(queue_.get(), policy_.get(), notifier_.get(),
-                                  &event_logger_));
+  MakeFactoryAndTask();
 
   base::Time creation_time = base::Time::Now();
   SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time,
                            kUserRequested);
   SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time,
                            kUserRequested);
   request2.set_completed_attempt_count(kAttemptCount);
 
-  QueueRequestsAndChooseOne(request1, request2);
+  QueueRequests(request1, request2);
+
+  task()->Run();
+  PumpLoop();
 
   EXPECT_EQ(kRequestId1, last_picked_->request_id());
   EXPECT_FALSE(request_queue_not_picked_called_);
 }
 
-TEST_F(RequestPickerTest, ChooseLaterRequest) {
+TEST_F(PickRequestTaskTest, ChooseLaterRequest) {
   // We need a custom policy preferring recency over retry, and later requests.
   policy_.reset(new OfflinerPolicy(
       kPreferUntried, !kPreferEarlier, !kPreferRetryCount, kMaxStartedTries,
       kMaxCompletedTries, kBackgroundProcessingTimeBudgetSeconds));
-  picker_.reset(new RequestPicker(queue_.get(), policy_.get(), notifier_.get(),
-                                  &event_logger_));
+  MakeFactoryAndTask();
 
   base::Time creation_time1 =
       base::Time::Now() - base::TimeDelta::FromSeconds(10);
   base::Time creation_time2 = base::Time::Now();
   SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time1,
                            kUserRequested);
   SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time2,
                            kUserRequested);
 
-  QueueRequestsAndChooseOne(request1, request2);
+  QueueRequests(request1, request2);
+
+  task()->Run();
+  PumpLoop();
 
   EXPECT_EQ(kRequestId2, last_picked_->request_id());
   EXPECT_FALSE(request_queue_not_picked_called_);
 }
 
-TEST_F(RequestPickerTest, ChooseNonExpiredRequest) {
+TEST_F(PickRequestTaskTest, ChooseNonExpiredRequest) {
   base::Time creation_time = base::Time::Now();
   base::Time expired_time =
       creation_time - base::TimeDelta::FromSeconds(
                           policy_->GetRequestExpirationTimeInSeconds() + 60);
   SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time,
                            kUserRequested);
   SavePageRequest request2(kRequestId2, kUrl2, kClientId2, expired_time,
                            kUserRequested);
 
-  QueueRequestsAndChooseOne(request1, request2);
+  QueueRequests(request1, request2);
 
+  task()->Run();
   PumpLoop();
 
   EXPECT_EQ(kRequestId1, last_picked_->request_id());
   EXPECT_FALSE(request_queue_not_picked_called_);
   EXPECT_EQ(kRequestId2, GetNotifier()->last_expired_request().request_id());
   EXPECT_EQ(RequestNotifier::BackgroundSavePageResult::EXPIRED,
             GetNotifier()->last_request_expiration_status());
   EXPECT_EQ(1, GetNotifier()->total_expired_requests());
 }
 
-TEST_F(RequestPickerTest, ChooseRequestThatHasNotExceededStartLimit) {
+TEST_F(PickRequestTaskTest, ChooseRequestThatHasNotExceededStartLimit) {
   base::Time creation_time1 =
       base::Time::Now() - base::TimeDelta::FromSeconds(1);
   base::Time creation_time2 = base::Time::Now();
   SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time1,
                            kUserRequested);
   SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time2,
                            kUserRequested);
 
   // With default policy settings, we should choose the earlier request.
   // However, we will make the earlier reqeust exceed the limit.
   request1.set_started_attempt_count(policy_->GetMaxStartedTries());
 
-  QueueRequestsAndChooseOne(request1, request2);
+  QueueRequests(request1, request2);
+
+  task()->Run();
+  PumpLoop();
 
   EXPECT_EQ(kRequestId2, last_picked_->request_id());
   EXPECT_FALSE(request_queue_not_picked_called_);
 }
 
-TEST_F(RequestPickerTest, ChooseRequestThatHasNotExceededCompletionLimit) {
+TEST_F(PickRequestTaskTest, ChooseRequestThatHasNotExceededCompletionLimit) {
   base::Time creation_time1 =
       base::Time::Now() - base::TimeDelta::FromSeconds(1);
   base::Time creation_time2 = base::Time::Now();
   SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time1,
                            kUserRequested);
   SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time2,
                            kUserRequested);
 
   // With default policy settings, we should choose the earlier request.
   // However, we will make the earlier reqeust exceed the limit.
   request1.set_completed_attempt_count(policy_->GetMaxCompletedTries());
 
-  QueueRequestsAndChooseOne(request1, request2);
+  QueueRequests(request1, request2);
+
+  task()->Run();
+  PumpLoop();
 
   EXPECT_EQ(kRequestId2, last_picked_->request_id());
   EXPECT_FALSE(request_queue_not_picked_called_);
 }
 
-
-TEST_F(RequestPickerTest, ChooseRequestThatIsNotDisabled) {
+TEST_F(PickRequestTaskTest, ChooseRequestThatIsNotDisabled) {
   policy_.reset(new OfflinerPolicy(
       kPreferUntried, kPreferEarlier, kPreferRetryCount, kMaxStartedTries,
       kMaxCompletedTries + 1, kBackgroundProcessingTimeBudgetSeconds));
-  picker_.reset(new RequestPicker(queue_.get(), policy_.get(), notifier_.get(),
-                                  &event_logger_));
-
-  base::Time creation_time = base::Time::Now();
-  SavePageRequest request1(
-      kRequestId1, kUrl1, kClientId1, creation_time, kUserRequested);
-  SavePageRequest request2(
-      kRequestId2, kUrl2, kClientId2, creation_time, kUserRequested);
-  request2.set_completed_attempt_count(kAttemptCount);
 
   // put request 2 on disabled list, ensure request1 picked instead,
   // even though policy would prefer 2.
-  std::set<int64_t> disabled_requests {kRequestId2};
-  DeviceConditions conditions;
+  disabled_requests_.insert(kRequestId2);
+  MakeFactoryAndTask();
+
+  base::Time creation_time = base::Time::Now();
+  SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time,
+                           kUserRequested);
+  SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time,
+                           kUserRequested);
+  request2.set_completed_attempt_count(kAttemptCount);
 
   // Add test requests on the Queue.
-  queue_->AddRequest(request1, base::Bind(&RequestPickerTest::AddRequestDone,
-                                          base::Unretained(this)));
-  queue_->AddRequest(request2, base::Bind(&RequestPickerTest::AddRequestDone,
-                                          base::Unretained(this)));
+  QueueRequests(request1, request2);
 
-  // Pump the loop to give the async queue the opportunity to do the adds.
+  task()->Run();
   PumpLoop();
 
-  // Call the method under test.
-  picker_->ChooseNextRequest(
-      base::Bind(&RequestPickerTest::RequestPicked, base::Unretained(this)),
-      base::Bind(&RequestPickerTest::RequestNotPicked, base::Unretained(this)),
-      &conditions, disabled_requests);
-
   // Pump the loop again to give the async queue the opportunity to return
   // results from the Get operation, and for the picker to call the "picked"
   // callback.
   PumpLoop();
 
   EXPECT_EQ(kRequestId1, last_picked_->request_id());
   EXPECT_FALSE(request_queue_not_picked_called_);
 }
+
 }  // namespace offline_pages