[Offline Pages] Converts MarkAttemptCompleted to use TaskQueue

components/offline_pages/background/mark_attempt_completed_task_unittest.cc

Index: components/offline_pages/background/mark_attempt_completed_task_unittest.cc
diff --git a/components/offline_pages/background/mark_attempt_started_task_unittest.cc b/components/offline_pages/background/mark_attempt_completed_task_unittest.cc
similarity index 53%
copy from components/offline_pages/background/mark_attempt_started_task_unittest.cc
copy to components/offline_pages/background/mark_attempt_completed_task_unittest.cc
index 3e770c872a05a393a9bbeee87eadcac9d02f4366..cd738f49fa3282c0f1d115f9f51b4b513f990388 100644
--- a/components/offline_pages/background/mark_attempt_started_task_unittest.cc
+++ b/components/offline_pages/background/mark_attempt_completed_task_unittest.cc
@@ -2,9 +2,10 @@
 // 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/mark_attempt_started_task.h"
+#include "components/offline_pages/background/mark_attempt_completed_task.h"
 
 #include <memory>
+#include <utility>
 
 #include "base/bind.h"
 #include "base/test/test_simple_task_runner.h"
@@ -15,19 +16,18 @@
 namespace offline_pages {
 namespace {
 const int64_t kRequestId1 = 42;
-const int64_t kRequestId2 = 44;
 const GURL kUrl1("http://example.com");
 const ClientId kClientId1("download", "1234");
 }  // namespace
 
-class MarkAttemptStartedTaskTest : public testing::Test {
+class MarkAttemptCompletedTaskTest : public testing::Test {
  public:
-  MarkAttemptStartedTaskTest();
-  ~MarkAttemptStartedTaskTest() override;
+  MarkAttemptCompletedTaskTest();
+  ~MarkAttemptCompletedTaskTest() override;
 
   void PumpLoop();
 
-  void AddItemToStore(RequestQueueStore* store);
+  void AddStartedItemToStore(RequestQueueStore* store);
 
   void AddRequestDone(ItemActionStatus status);
 
@@ -42,40 +42,42 @@ class MarkAttemptStartedTaskTest : public testing::Test {
   base::ThreadTaskRunnerHandle task_runner_handle_;
 };
 
-MarkAttemptStartedTaskTest::MarkAttemptStartedTaskTest()
+MarkAttemptCompletedTaskTest::MarkAttemptCompletedTaskTest()
     : task_runner_(new base::TestSimpleTaskRunner),
       task_runner_handle_(task_runner_) {}
 
-MarkAttemptStartedTaskTest::~MarkAttemptStartedTaskTest() {}
+MarkAttemptCompletedTaskTest::~MarkAttemptCompletedTaskTest() {}
 
-void MarkAttemptStartedTaskTest::PumpLoop() {
+void MarkAttemptCompletedTaskTest::PumpLoop() {
   task_runner_->RunUntilIdle();
 }
 
-void MarkAttemptStartedTaskTest::AddItemToStore(RequestQueueStore* store) {
+void MarkAttemptCompletedTaskTest::AddStartedItemToStore(
+    RequestQueueStore* store) {
   base::Time creation_time = base::Time::Now();
   SavePageRequest request_1(kRequestId1, kUrl1, kClientId1, creation_time,
                             true);
+  request_1.MarkAttemptStarted(base::Time::Now());
   store->AddRequest(request_1,
-                    base::Bind(&MarkAttemptStartedTaskTest::AddRequestDone,
+                    base::Bind(&MarkAttemptCompletedTaskTest::AddRequestDone,
                                base::Unretained(this)));
   PumpLoop();
 }
 
-void MarkAttemptStartedTaskTest::AddRequestDone(ItemActionStatus status) {
+void MarkAttemptCompletedTaskTest::AddRequestDone(ItemActionStatus status) {
   ASSERT_EQ(ItemActionStatus::SUCCESS, status);
 }
 
-void MarkAttemptStartedTaskTest::ChangeRequestsStateCallback(
+void MarkAttemptCompletedTaskTest::ChangeRequestsStateCallback(
     std::unique_ptr<UpdateRequestsResult> result) {
   result_ = std::move(result);
 }
 
-TEST_F(MarkAttemptStartedTaskTest, MarkAttemptStartedWhenStoreEmpty) {
+TEST_F(MarkAttemptCompletedTaskTest, MarkAttemptCompletedWhenStoreEmpty) {
   RequestQueueInMemoryStore store;
-  MarkAttemptStartedTask task(
+  MarkAttemptCompletedTask task(
       &store, kRequestId1,
-      base::Bind(&MarkAttemptStartedTaskTest::ChangeRequestsStateCallback,
+      base::Bind(&MarkAttemptCompletedTaskTest::ChangeRequestsStateCallback,
                  base::Unretained(this)));
   task.Run();
   PumpLoop();
@@ -87,17 +89,15 @@ TEST_F(MarkAttemptStartedTaskTest, MarkAttemptStartedWhenStoreEmpty) {
   EXPECT_EQ(0UL, last_result()->updated_items.size());
 }
 
-TEST_F(MarkAttemptStartedTaskTest, MarkAttemptStartedWhenExists) {
+TEST_F(MarkAttemptCompletedTaskTest, MarkAttemptCompletedWhenExists) {
   RequestQueueInMemoryStore store;
-  AddItemToStore(&store);
+  AddStartedItemToStore(&store);
 
-  MarkAttemptStartedTask task(
+  MarkAttemptCompletedTask task(
       &store, kRequestId1,
-      base::Bind(&MarkAttemptStartedTaskTest::ChangeRequestsStateCallback,
+      base::Bind(&MarkAttemptCompletedTaskTest::ChangeRequestsStateCallback,
                  base::Unretained(this)));
 
-  // Current time for verification.
-  base::Time before_time = base::Time::Now();
   task.Run();
   PumpLoop();
   ASSERT_TRUE(last_result());
@@ -106,30 +106,9 @@ TEST_F(MarkAttemptStartedTaskTest, MarkAttemptStartedWhenExists) {
   EXPECT_EQ(ItemActionStatus::SUCCESS,
             last_result()->item_statuses.at(0).second);
   EXPECT_EQ(1UL, last_result()->updated_items.size());
-  EXPECT_LE(before_time,
-            last_result()->updated_items.at(0).last_attempt_time());
-  EXPECT_GE(base::Time::Now(),
-            last_result()->updated_items.at(0).last_attempt_time());
-  EXPECT_EQ(1, last_result()->updated_items.at(0).started_attempt_count());
-  EXPECT_EQ(SavePageRequest::RequestState::PRERENDERING,
+  EXPECT_EQ(1, last_result()->updated_items.at(0).completed_attempt_count());
+  EXPECT_EQ(SavePageRequest::RequestState::AVAILABLE,
             last_result()->updated_items.at(0).request_state());
 }
 

[Pete Williamson, 2016/10/31 21:38:26]

Should we have a test where there are multiple items in the queue, and only one
gets completed?

[dougarnett, 2016/10/31 22:51:17]

Done.

[Pete Williamson, 2016/10/31 23:01:27]

I don't see the test, did your patch get uploaded?

[Pete Williamson, 2016/10/31 23:02:04]

Nevermind, I hit refresh, and then I saw the patch.

-TEST_F(MarkAttemptStartedTaskTest, MarkAttemptStartedWhenItemMissing) {

[fgorski, 2016/10/31 20:33:49]

It's worth adding a test when the item is missing to set the expectation for
what is a proper result.

[dougarnett, 2016/10/31 22:05:52]

There already is a test for item not being found above - *WhenStoreEmpty.
There is no additional coverage nor user expectation setting in this test so
rather not bloat the test set unproductively. I went with the first
one because it was easier to read but this one is slightly more strict so
will switch to this one with some comments to try to make easier to read.

-  RequestQueueInMemoryStore store;
-  AddItemToStore(&store);
-  MarkAttemptStartedTask task(
-      &store, kRequestId2,
-      base::Bind(&MarkAttemptStartedTaskTest::ChangeRequestsStateCallback,
-                 base::Unretained(this)));
-  task.Run();
-  PumpLoop();
-  ASSERT_TRUE(last_result());
-  EXPECT_EQ(1UL, last_result()->item_statuses.size());
-  EXPECT_EQ(kRequestId2, last_result()->item_statuses.at(0).first);
-  EXPECT_EQ(ItemActionStatus::NOT_FOUND,
-            last_result()->item_statuses.at(0).second);
-  EXPECT_EQ(0UL, last_result()->updated_items.size());
-}
-
 }  // namespace offline_pages