Request Picker task

components/offline_pages/background/request_queue_unittest.cc

Index: components/offline_pages/background/request_queue_unittest.cc
diff --git a/components/offline_pages/background/request_queue_unittest.cc b/components/offline_pages/background/request_queue_unittest.cc
index 1e6a0ddc163a8aefba6db234af096f7f99221100..77a4f3dd8a126178af908a2d6f50fd55b10c3b53 100644
--- a/components/offline_pages/background/request_queue_unittest.cc
+++ b/components/offline_pages/background/request_queue_unittest.cc
@@ -10,15 +10,19 @@
 #include "base/bind.h"
 #include "base/test/test_simple_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "components/offline_pages/background/device_conditions.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_in_memory_store.h"
 #include "components/offline_pages/background/save_page_request.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace offline_pages {
 
-using AddRequestResult = RequestQueue::AddRequestResult;
-using GetRequestsResult = RequestQueue::GetRequestsResult;
-using UpdateRequestResult = RequestQueue::UpdateRequestResult;
+using AddRequestResult = AddRequestResult;
+using GetRequestsResult = GetRequestsResult;
+using UpdateRequestResult = UpdateRequestResult;
 
 namespace {
 // Data for request 1.
@@ -31,8 +35,50 @@ const GURL kUrl2("http://test.com");
 const ClientId kClientId2("bookmark", "567");
 const bool kUserRequested = true;
 const int64_t kRequestId3 = 99;
+const int kOneWeekInSeconds = 7 * 24 * 60 * 60;
+
+// 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_;
+  int32_t total_expired_requests_;
+};
+
 // TODO(fgorski): Add tests for store failures in add/remove/get.
 class RequestQueueTest : public testing::Test {
  public:
@@ -76,6 +122,9 @@ class RequestQueueTest : public testing::Test {
     return update_requests_result_.get();
   }
 
+  void RequestPickedCallback(const SavePageRequest& request) {}
+  void RequestNotPickedCallback(bool non_user_requested_tasks_remain) {}
+
  private:
   AddRequestResult last_add_result_;
   std::unique_ptr<SavePageRequest> last_added_request_;
@@ -484,4 +533,51 @@ TEST_F(RequestQueueTest, MarkAttemptCompleted) {
             update_requests_result()->updated_items.at(0).request_state());
 }
 
+TEST_F(RequestQueueTest, CleanStaleRequests) {

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

please add a comment to this test:
Request expiration happens when PickNextRequest is called on the queue. (I see
there is one in code, but still)

also, what about tests for picking, at least some basic ones, e.g. covering
different taskComplete?

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

1. Comments added.

2. Generally I try to test something at the lowest level possible, and we
already have those tests in the pick request task unit test.  Why also add tests
here?  If it is just to test TaskComplete, it is already tested now.

+  // Create a request that is already expired.
+  base::Time creation_time =
+      base::Time::Now() - base::TimeDelta::FromSeconds(2 * kOneWeekInSeconds);
+
+  SavePageRequest original_request(kRequestId, kUrl, kClientId, creation_time,
+                                   kUserRequested);
+  queue()->AddRequest(
+      original_request,
+      base::Bind(&RequestQueueTest::AddRequestDone, base::Unretained(this)));
+  this->PumpLoop();
+  this->ClearResults();
+
+  // Set up a picker factory pointing to our fake notifier.
+  OfflinerPolicy policy;
+  RequestNotifierStub notifier;
+  RequestCoordinatorEventLogger event_logger;
+  std::unique_ptr<PickRequestTaskFactory> picker_factory(
+      new PickRequestTaskFactory(&policy, &notifier, &event_logger));
+  queue()->SetPickerFactory(std::move(picker_factory));
+
+  // Do a pick and clean operation, which will remove stale entries.
+  DeviceConditions conditions;
+  std::set<int64_t> disabled_list;
+  queue()->PickNextRequest(
+      base::Bind(&RequestQueueTest::RequestPickedCallback,
+                 base::Unretained(this)),
+      base::Bind(&RequestQueueTest::RequestNotPickedCallback,
+                 base::Unretained(this)),
+      conditions, disabled_list);
+
+  this->PumpLoop();
+
+  // Notifier should have been notified that the request was removed.
+  ASSERT_EQ(notifier.last_expired_request().request_id(), kRequestId);
+  ASSERT_EQ(notifier.last_request_expiration_status(),
+            RequestNotifier::BackgroundSavePageResult::EXPIRED);
+
+  // Doing a get should show no entries left in the queue since the expired
+  // request has been removed.
+  queue()->GetRequests(
+      base::Bind(&RequestQueueTest::GetRequestsDone, base::Unretained(this)));
+  this->PumpLoop();
+  ASSERT_EQ(GetRequestsResult::SUCCESS, this->last_get_requests_result());
+  ASSERT_TRUE(this->last_requests().empty());
+}
+
 }  // namespace offline_pages