Reconcile the request queue on startup.

components/offline_pages/core/background/reconcile_task_unittest.cc

Index: components/offline_pages/core/background/reconcile_task_unittest.cc
diff --git a/components/offline_pages/core/background/reconcile_task_unittest.cc b/components/offline_pages/core/background/reconcile_task_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..1e3b61617f1127ea017b94496dd3cfc71713fb69
--- /dev/null
+++ b/components/offline_pages/core/background/reconcile_task_unittest.cc
@@ -0,0 +1,179 @@
+// 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/core/background/reconcile_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 "components/offline_pages/core/background/request_coordinator.h"
+#include "components/offline_pages/core/background/request_queue_in_memory_store.h"
+#include "components/offline_pages/core/background/request_queue_store.h"
+#include "components/offline_pages/core/background/save_page_request.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.
+const int64_t kRequestId2 = 42;
+const GURL kUrl2("http://nytimes.com");
+const ClientId kClientId2("bookmark", "5678");
+const bool kUserRequested = true;
+
+// Default request
+const SavePageRequest kEmptyRequest(0UL,

[fgorski, 2016/12/12 17:35:18]

nit: wouldn't git cl format put that on a single line below?

[Pete Williamson, 2016/12/15 18:37:43]

Git cl format seems to prefer lining these up.  I did use git cl format here.  I
don't yet quite understand how it decides to order function parameters,
sometimes it prefers lining these up under the first parameter just after the
'(', sometimes it seems to prefer starting a fresh line.  Maybe it goes for this
style if it can, and then uses the other style only if a line overflows with
this style?

+                                    GURL(""),
+                                    ClientId("", ""),
+                                    base::Time(),
+                                    true);
+}  // namespace
+
+class ReconcileTaskTest : public testing::Test {
+ public:
+  ReconcileTaskTest();
+
+  ~ReconcileTaskTest() override;
+
+  void SetUp() override;
+
+  void PumpLoop();
+
+  void AddRequestDone(ItemActionStatus status);
+
+  void GetRequestsCallback(
+      bool success,
+      std::vector<std::unique_ptr<SavePageRequest>> requests);
+
+  void ReconcileCallback(std::unique_ptr<UpdateRequestsResult> result);
+
+  void QueueRequests(const SavePageRequest& request1,
+                     const SavePageRequest& request2);
+
+  // Reset the factory and the task using the current policy.
+  void MakeTask();
+
+  ReconcileTask* task() { return task_.get(); }
+  RequestQueueStore* store() { return store_.get(); }
+  std::vector<std::unique_ptr<SavePageRequest>>& found_requests() {
+    return found_requests_;
+  }
+
+ protected:
+  void InitializeStoreDone(bool success);
+
+  std::unique_ptr<RequestQueueStore> store_;
+  std::unique_ptr<ReconcileTask> task_;
+  std::vector<std::unique_ptr<SavePageRequest>> found_requests_;
+  bool reconcile_called_;
+
+ private:
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+  base::ThreadTaskRunnerHandle task_runner_handle_;
+};
+
+ReconcileTaskTest::ReconcileTaskTest()
+    : reconcile_called_(false),
+      task_runner_(new base::TestSimpleTaskRunner),
+      task_runner_handle_(task_runner_) {}
+
+ReconcileTaskTest::~ReconcileTaskTest() {}
+
+void ReconcileTaskTest::SetUp() {
+  DeviceConditions conditions;
+  store_.reset(new RequestQueueInMemoryStore());
+  MakeTask();
+
+  store_->Initialize(base::Bind(&ReconcileTaskTest::InitializeStoreDone,
+                                base::Unretained(this)));
+  PumpLoop();
+}
+
+void ReconcileTaskTest::PumpLoop() {
+  task_runner_->RunUntilIdle();
+}
+
+void ReconcileTaskTest::AddRequestDone(ItemActionStatus status) {
+  ASSERT_EQ(ItemActionStatus::SUCCESS, status);
+}
+
+void ReconcileTaskTest::GetRequestsCallback(
+    bool success,
+    std::vector<std::unique_ptr<SavePageRequest>> requests) {
+  found_requests_ = std::move(requests);
+}
+
+void ReconcileTaskTest::ReconcileCallback(

[dougarnett, 2016/12/12 17:16:13]

Should be able to test the updated requests pass in here

[Pete Williamson, 2016/12/15 18:37:43]

Done.

+    std::unique_ptr<UpdateRequestsResult> result) {
+  reconcile_called_ = true;
+}
+
+// Test helper to queue the two given requests.
+void ReconcileTaskTest::QueueRequests(const SavePageRequest& request1,
+                                      const SavePageRequest& request2) {
+  DeviceConditions conditions;
+  std::set<int64_t> disabled_requests;
+  // Add test requests on the Queue.
+  store_->AddRequest(request1, base::Bind(&ReconcileTaskTest::AddRequestDone,
+                                          base::Unretained(this)));
+  store_->AddRequest(request2, base::Bind(&ReconcileTaskTest::AddRequestDone,
+                                          base::Unretained(this)));
+
+  // Pump the loop to give the async queue the opportunity to do the adds.
+  PumpLoop();
+}
+
+void ReconcileTaskTest::MakeTask() {
+  task_.reset(new ReconcileTask(
+      store_.get(), base::Bind(&ReconcileTaskTest::ReconcileCallback,
+                               base::Unretained(this))));
+}
+
+void ReconcileTaskTest::InitializeStoreDone(bool success) {
+  ASSERT_TRUE(success);
+}
+
+TEST_F(ReconcileTaskTest, Reconcile) {

[fgorski, 2016/12/12 17:35:18]

What about testing for case, where no need to update was found?

[Pete Williamson, 2016/12/15 18:37:43]

Done.

+  base::Time creation_time = base::Time::Now();
+  // Request2 will be expired, request1 will be current.
+  SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time,
+                           kUserRequested);
+  request1.set_request_state(SavePageRequest::RequestState::PAUSED);
+  SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time,
+                           kUserRequested);
+  request2.set_request_state(SavePageRequest::RequestState::OFFLINING);
+  QueueRequests(request1, request2);
+
+  // Initiate cleanup.
+  task()->Run();
+  PumpLoop();
+
+  // See what is left in the queue, should be just the other request.
+  store()->GetRequests(base::Bind(&ReconcileTaskTest::GetRequestsCallback,
+                                  base::Unretained(this)));
+  PumpLoop();
+  EXPECT_EQ(2UL, found_requests().size());
+
+  // in case requests come back in a different order, check which is where.
+  int request1_index = 0;
+  int request2_index = 1;
+  if (found_requests().at(0)->request_id() != kRequestId1) {
+    request1_index = 1;
+    request2_index = 0;
+  }
+  EXPECT_EQ(SavePageRequest::RequestState::PAUSED,
+            found_requests().at(request1_index)->request_state());
+  EXPECT_EQ(SavePageRequest::RequestState::AVAILABLE,
+            found_requests().at(request2_index)->request_state());
+  EXPECT_EQ(true, reconcile_called_);
+}
+
+}  // namespace offline_pages