[Offline Pages] Converts MarkAttemptCompleted to use TaskQueue

components/offline_pages/background/mark_attempt_completed_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/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"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/offline_pages/background/request_queue_in_memory_store.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 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);
 
   void ChangeRequestsStateCallback(
       std::unique_ptr<UpdateRequestsResult> result);
 
   UpdateRequestsResult* last_result() const { return result_.get(); }
 
  private:
   std::unique_ptr<UpdateRequestsResult> result_;
   scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
   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();
   ASSERT_TRUE(last_result());
   EXPECT_EQ(1UL, last_result()->item_statuses.size());
   EXPECT_EQ(kRequestId1, 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());
 }
 
-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());
   EXPECT_EQ(1UL, last_result()->item_statuses.size());
   EXPECT_EQ(kRequestId1, last_result()->item_statuses.at(0).first);
   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