Change database scheme - add state and start tracking

components/offline_pages/background/save_page_request_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/save_page_request.h"
 
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace offline_pages {
 
@@ skipping 14 matching lines @@
 TEST_F(SavePageRequestTest, CreatePendingReqeust) {
   base::Time creation_time = base::Time::Now();
   SavePageRequest request(
       kRequestId, kUrl, kClientId, creation_time, kUserRequested);
   ASSERT_EQ(kRequestId, request.request_id());
   ASSERT_EQ(kUrl, request.url());
   ASSERT_EQ(kClientId, request.client_id());
   ASSERT_EQ(creation_time, request.creation_time());
   ASSERT_EQ(creation_time, request.activation_time());
   ASSERT_EQ(base::Time(), request.last_attempt_time());
-  ASSERT_EQ(0, request.attempt_count());
+  ASSERT_EQ(0, request.completed_attempt_count());

[fgorski, 2016/08/08 17:48:05]

check for state and started count here as well, please.

[Pete Williamson, 2016/08/08 18:44:29]

Done.

 }
 
 TEST_F(SavePageRequestTest, StartAndCompleteRequest) {
   base::Time creation_time = base::Time::Now();
   base::Time activation_time = creation_time + base::TimeDelta::FromHours(6);
   SavePageRequest request(kRequestId, kUrl, kClientId, creation_time,
                           activation_time, kUserRequested);
 
   base::Time start_time = activation_time + base::TimeDelta::FromHours(3);
   request.MarkAttemptStarted(start_time);
 
   // Most things don't change about the request.
   ASSERT_EQ(kRequestId, request.request_id());
   ASSERT_EQ(kUrl, request.url());
   ASSERT_EQ(kClientId, request.client_id());
   ASSERT_EQ(creation_time, request.creation_time());
   ASSERT_EQ(activation_time, request.activation_time());
 
   // Attempt time, attempt count and status will though.
   ASSERT_EQ(start_time, request.last_attempt_time());
-  ASSERT_EQ(1, request.attempt_count());
+  ASSERT_EQ(1, request.started_attempt_count());
+  ASSERT_EQ(SavePageRequest::RequestState::PRERENDERING,
+            request.request_state());
 
   request.MarkAttemptCompleted();
 
   // Again, most things don't change about the request.
   ASSERT_EQ(kRequestId, request.request_id());
   ASSERT_EQ(kUrl, request.url());
   ASSERT_EQ(kClientId, request.client_id());
   ASSERT_EQ(creation_time, request.creation_time());
   ASSERT_EQ(activation_time, request.activation_time());
 
   // Last attempt time and status are updated.
-  ASSERT_EQ(base::Time(), request.last_attempt_time());
-  ASSERT_EQ(1, request.attempt_count());
+  ASSERT_EQ(base::Time(), request.last_attempt_time()); // TODO: flaky?

[fgorski, 2016/08/08 17:48:06]

what is happening? Could you explain it?

[Pete Williamson, 2016/08/08 18:44:29]

Sure - Sorry, I meant to investigate this before uploading the change, and
missed the TODO that I left for myself.

My concern here was that if time passes between when we set last_attempt_time
and when we verify that the time might not be what we expect.  It was related to
not being sure if we were using the real clock here or a test clock.  If we used
a real clock, this would be flaky if any of the test takes more than 0 time to
complete.  I wanted to make sure I investigated and that we were using a test
clock.

Looking at it, I don't see a test clock in use, so I'm not sure why this test
always passes unless it always happens very fast and never loses the time slice.

[Pete Williamson, 2016/08/08 18:53:12]

Ah, I see it is comparing to base::Time(), not base::Time::Now().  In that case,
the code is safe, comment removed.

+  ASSERT_EQ(1, request.completed_attempt_count());
+  ASSERT_EQ(SavePageRequest::RequestState::AVAILABLE, request.request_state());
 }
 
 TEST_F(SavePageRequestTest, StartAndAbortRequest) {
   base::Time creation_time = base::Time::Now();
   SavePageRequest request(kRequestId, kUrl, kClientId, creation_time,
                           kUserRequested);
-  int start_attempt_count = 2;
-  request.set_attempt_count(start_attempt_count);
 
   base::Time start_time = creation_time + base::TimeDelta::FromHours(3);
   request.MarkAttemptStarted(start_time);
 
   // Most things don't change about the request.
   ASSERT_EQ(kRequestId, request.request_id());
   ASSERT_EQ(kUrl, request.url());
   ASSERT_EQ(kClientId, request.client_id());
   ASSERT_EQ(creation_time, request.creation_time());
 
   // Attempt time and attempt count will though.
   ASSERT_EQ(start_time, request.last_attempt_time());
-  ASSERT_EQ(start_attempt_count + 1, request.attempt_count());
+  ASSERT_EQ(1, request.started_attempt_count());
+  ASSERT_EQ(SavePageRequest::RequestState::PRERENDERING,
+            request.request_state());
 
   request.MarkAttemptAborted();
 
   // Again, most things don't change about the request.
   ASSERT_EQ(kRequestId, request.request_id());
   ASSERT_EQ(kUrl, request.url());
   ASSERT_EQ(kClientId, request.client_id());
   ASSERT_EQ(creation_time, request.creation_time());
 
-  // Last attempt time is updated and attempt count decremented.
+  // Last attempt time is updated and completed attempt count did not rise.
   ASSERT_EQ(base::Time(), request.last_attempt_time());
-  ASSERT_EQ(start_attempt_count, request.attempt_count());
+  ASSERT_EQ(0, request.completed_attempt_count());
+  ASSERT_EQ(SavePageRequest::RequestState::AVAILABLE, request.request_state());

[fgorski, 2016/08/08 17:48:06]

This is unrelated to this line, but affects this file:
Can we have a scenario when we MarkAttemptPaused(), please?
* From AVAILABLE (with no attempts), if that is possible
* From AVAILABLE, which has been attempted in the past
* From PRERENDERING, (make sure to check last_attempt_time_).

[Pete Williamson, 2016/08/08 18:44:29]

Tests for the state changes are coming in next changelist (I've already coded
it, but I'm waiting for this to land before sending it out for review so we
don't have to review code twice)

[fgorski, 2016/08/08 22:35:33]

Acknowledged.

 }
 
 }  // namespace offline_pages