Add more unit tests for RequestPicker

components/offline_pages/background/request_picker_unittest.cc

Index: components/offline_pages/background/request_picker_unittest.cc
diff --git a/components/offline_pages/background/request_picker_unittest.cc b/components/offline_pages/background/request_picker_unittest.cc
index e51f999a9a076e01871592ae2f63a3129dbfdd58..4a6669722cd539c0619ab8a22b9cf47a6f7737fc 100644
--- a/components/offline_pages/background/request_picker_unittest.cc
+++ b/components/offline_pages/background/request_picker_unittest.cc
@@ -27,6 +27,12 @@ const int64_t kRequestId2 = 42;
 const GURL kUrl2("http://nytimes.com");
 const ClientId kClientId2("bookmark", "5678");
 const bool kUserRequested = true;
+const int kRetryCount2 = 1;
+
+// Constants for policy values
+const bool kPreferUntried = false;

[fgorski, 2016/07/25 16:22:57]

nit: Did you intentionally set this to false? The name slightly indicates that
the value of const is true, as in "Yes, prefer untried".

Also this set of constants corresponds to the default settings of the policy, is
that correct? Is that what it is meant to do?

[Pete Williamson, 2016/07/25 19:54:21]

Your interpretation is correct, so setting kPreferUntried to false means that we
prefer requests that have already been tried (but not exceeded the retry count)
to untried requests.

Yes, these values correspond to the defaults.  When using them below, I use them
with a "!" for places we are doing the opposite of the default.  

My code should have been clear enough that you didn't have to ask, so I beefed
up the comments, see if it is clearer now.

[fgorski, 2016/07/26 16:39:14]

Ack. This is my OCD around positive sounding constants being set to false. I am
sure it bothers me more than most of people, so I think this is safe to stay
that way.

I did see your usage below and have no problems with that... It is just how it
sounds to me. Don't worry about that.

+const bool kPreferEarlier = true;
+const bool kPreferRetryCount = true;
 }  // namespace
 
 class RequestPickerTest : public testing::Test {
@@ -90,13 +96,29 @@ void RequestPickerTest::RequestQueueEmpty() {
   request_queue_empty_called_ = true;
 }
 
-TEST_F(RequestPickerTest, ChooseNextRequest) {
+TEST_F(RequestPickerTest, PickFromEmptyQueue) {
+  DeviceConditions conditions;
+  picker_->ChooseNextRequest(
+      base::Bind(&RequestPickerTest::RequestPicked, base::Unretained(this)),
+      base::Bind(&RequestPickerTest::RequestQueueEmpty, base::Unretained(this)),
+      &conditions);
+
+  // Pump the loop again to give the async queue the opportunity to return
+  // results from the Get operation, and for the picker to call the "QueueEmpty"
+  // callback.
+  PumpLoop();
+
+  EXPECT_TRUE(request_queue_empty_called_);
+}
+
+TEST_F(RequestPickerTest, ChooseRequestWithHigherRetryCount) {
   base::Time creation_time = base::Time::Now();
   DeviceConditions conditions;
   SavePageRequest request1(
       kRequestId1, kUrl1, kClientId1, creation_time, kUserRequested);
   SavePageRequest request2(
       kRequestId2, kUrl2, kClientId2, creation_time, kUserRequested);
+  request2.set_attempt_count(kRetryCount2);

[fgorski, 2016/07/25 16:22:57]

nit: consider renaming kRetryCount2 to kAttemptCount.

[Pete Williamson, 2016/07/25 19:54:21]

Done.

   // Put some test requests on the Queue.
   queue_->AddRequest(request1, base::Bind(&RequestPickerTest::AddRequestDone,
                                           base::Unretained(this)));
@@ -120,19 +142,184 @@ TEST_F(RequestPickerTest, ChooseNextRequest) {
   EXPECT_FALSE(request_queue_empty_called_);
 }
 
-TEST_F(RequestPickerTest, PickFromEmptyQueue) {
+TEST_F(RequestPickerTest, ChooseRequestWithSameRetryCountButEarlier) {
+  base::Time creation_time1 =
+      base::Time::Now() - base::TimeDelta::FromSeconds(10);
+  base::Time creation_time2 = base::Time::Now();
   DeviceConditions conditions;
+  SavePageRequest request1(
+      kRequestId1, kUrl1, kClientId1, creation_time1, kUserRequested);
+  SavePageRequest request2(
+      kRequestId2, kUrl2, kClientId2, creation_time2, kUserRequested);
+  // Put some test requests on the Queue.

[dougarnett, 2016/07/25 18:32:10]

"Put some" => "Add  ... to ..."  here and other cases below

[Pete Williamson, 2016/07/25 19:54:21]

Done.

+  queue_->AddRequest(request1, base::Bind(&RequestPickerTest::AddRequestDone,
+                                          base::Unretained(this)));
+  queue_->AddRequest(request2, base::Bind(&RequestPickerTest::AddRequestDone,
+                                          base::Unretained(this)));
+
+  // Pump the loop to give the async queue the opportunity to do the adds.
+  PumpLoop();
+
   picker_->ChooseNextRequest(
       base::Bind(&RequestPickerTest::RequestPicked, base::Unretained(this)),
       base::Bind(&RequestPickerTest::RequestQueueEmpty, base::Unretained(this)),
       &conditions);
 
   // Pump the loop again to give the async queue the opportunity to return
-  // results from the Get operation, and for the picker to call the "QueueEmpty"
+  // results from the Get operation, and for the picker to call the "picked"
   // callback.
   PumpLoop();
 
-  EXPECT_TRUE(request_queue_empty_called_);
+  EXPECT_EQ(kRequestId1, last_picked_->request_id());
+  EXPECT_FALSE(request_queue_empty_called_);
+}
+
+TEST_F(RequestPickerTest, ChooseEarlierRequest) {
+  // We need a custom policy object prefering recency to retry count.
+  policy_.reset(
+      new OfflinerPolicy(kPreferUntried, kPreferEarlier, !kPreferRetryCount));
+  picker_.reset(new RequestPicker(queue_.get(), policy_.get()));
+
+  base::Time creation_time1 =
+      base::Time::Now() - base::TimeDelta::FromSeconds(10);
+  base::Time creation_time2 = base::Time::Now();
+  DeviceConditions conditions;
+  SavePageRequest request1(
+      kRequestId1, kUrl1, kClientId1, creation_time1, kUserRequested);
+  SavePageRequest request2(
+      kRequestId2, kUrl2, kClientId2, creation_time2, kUserRequested);
+  request2.set_attempt_count(kRetryCount2);
+  // Put some test requests on the Queue.
+  queue_->AddRequest(request1, base::Bind(&RequestPickerTest::AddRequestDone,

[dougarnett, 2016/07/25 18:32:10]

This pattern of adding requests, pump loop, choose next, pump loop looks
identical in many tests here so might help reduce clutter to factor out into
method that takes the requests to add.

[Pete Williamson, 2016/07/25 19:54:21]

Done.

+                                          base::Unretained(this)));
+  queue_->AddRequest(request2, base::Bind(&RequestPickerTest::AddRequestDone,
+                                          base::Unretained(this)));
+
+  // Pump the loop to give the async queue the opportunity to do the adds.
+  PumpLoop();
+
+  picker_->ChooseNextRequest(
+      base::Bind(&RequestPickerTest::RequestPicked, base::Unretained(this)),
+      base::Bind(&RequestPickerTest::RequestQueueEmpty, base::Unretained(this)),
+      &conditions);
+
+  // Pump the loop again to give the async queue the opportunity to return
+  // results from the Get operation, and for the picker to call the "picked"
+  // callback.
+  PumpLoop();
+
+  EXPECT_EQ(kRequestId1, last_picked_->request_id());
+  EXPECT_FALSE(request_queue_empty_called_);
+}
+
+TEST_F(RequestPickerTest, ChooseSameTimeRequestWithHigherRetryCount) {
+  // We need a custom policy object preferring recency to retry count.
+  policy_.reset(
+      new OfflinerPolicy(kPreferUntried, kPreferEarlier, !kPreferRetryCount));
+  picker_.reset(new RequestPicker(queue_.get(), policy_.get()));
+
+  base::Time creation_time = base::Time::Now();
+  DeviceConditions conditions;
+  SavePageRequest request1(
+      kRequestId1, kUrl1, kClientId1, creation_time, kUserRequested);
+  SavePageRequest request2(
+      kRequestId2, kUrl2, kClientId2, creation_time, kUserRequested);
+  request2.set_attempt_count(kRetryCount2);
+
+  // Put some test requests on the Queue.
+  queue_->AddRequest(request1, base::Bind(&RequestPickerTest::AddRequestDone,
+                                          base::Unretained(this)));
+  queue_->AddRequest(request2, base::Bind(&RequestPickerTest::AddRequestDone,
+                                          base::Unretained(this)));
+
+  // Pump the loop to give the async queue the opportunity to do the adds.
+  PumpLoop();
+
+  picker_->ChooseNextRequest(
+      base::Bind(&RequestPickerTest::RequestPicked, base::Unretained(this)),
+      base::Bind(&RequestPickerTest::RequestQueueEmpty, base::Unretained(this)),
+      &conditions);
+
+  // Pump the loop again to give the async queue the opportunity to return
+  // results from the Get operation, and for the picker to call the "picked"
+  // callback.
+  PumpLoop();
+
+  EXPECT_EQ(kRequestId2, last_picked_->request_id());
+  EXPECT_FALSE(request_queue_empty_called_);
+}
+
+TEST_F(RequestPickerTest, ChooseRequestWithLowerRetryCount) {
+  // We need a custom policy object preferring lower retry count.
+  policy_.reset(
+      new OfflinerPolicy(!kPreferUntried, kPreferEarlier, kPreferRetryCount));
+  picker_.reset(new RequestPicker(queue_.get(), policy_.get()));
+
+  base::Time creation_time = base::Time::Now();
+  DeviceConditions conditions;
+  SavePageRequest request1(
+      kRequestId1, kUrl1, kClientId1, creation_time, kUserRequested);
+  SavePageRequest request2(
+      kRequestId2, kUrl2, kClientId2, creation_time, kUserRequested);
+  request2.set_attempt_count(kRetryCount2);
+  // Put some test requests on the Queue.
+  queue_->AddRequest(request1, base::Bind(&RequestPickerTest::AddRequestDone,
+                                          base::Unretained(this)));
+  queue_->AddRequest(request2, base::Bind(&RequestPickerTest::AddRequestDone,
+                                          base::Unretained(this)));
+
+  // Pump the loop to give the async queue the opportunity to do the adds.
+  PumpLoop();
+
+  picker_->ChooseNextRequest(
+      base::Bind(&RequestPickerTest::RequestPicked, base::Unretained(this)),
+      base::Bind(&RequestPickerTest::RequestQueueEmpty, base::Unretained(this)),
+      &conditions);
+
+  // Pump the loop again to give the async queue the opportunity to return
+  // results from the Get operation, and for the picker to call the "picked"
+  // callback.
+  PumpLoop();
+
+  EXPECT_EQ(kRequestId1, last_picked_->request_id());
+  EXPECT_FALSE(request_queue_empty_called_);
+}
+
+TEST_F(RequestPickerTest, ChooseLaterRequest) {
+  // We need a custom policy preferring recency over retry, and later requests.
+  policy_.reset(
+      new OfflinerPolicy(kPreferUntried, !kPreferEarlier, !kPreferRetryCount));
+  picker_.reset(new RequestPicker(queue_.get(), policy_.get()));
+
+  base::Time creation_time1 =
+      base::Time::Now() - base::TimeDelta::FromSeconds(10);
+  base::Time creation_time2 = base::Time::Now();
+  DeviceConditions conditions;
+  SavePageRequest request1(
+      kRequestId1, kUrl1, kClientId1, creation_time1, kUserRequested);
+  SavePageRequest request2(
+      kRequestId2, kUrl2, kClientId2, creation_time2, kUserRequested);
+  // Put some test requests on the Queue.
+  queue_->AddRequest(request1, base::Bind(&RequestPickerTest::AddRequestDone,
+                                          base::Unretained(this)));
+  queue_->AddRequest(request2, base::Bind(&RequestPickerTest::AddRequestDone,
+                                          base::Unretained(this)));
+
+  // Pump the loop to give the async queue the opportunity to do the adds.
+  PumpLoop();
+
+  picker_->ChooseNextRequest(
+      base::Bind(&RequestPickerTest::RequestPicked, base::Unretained(this)),
+      base::Bind(&RequestPickerTest::RequestQueueEmpty, base::Unretained(this)),
+      &conditions);
+
+  // Pump the loop again to give the async queue the opportunity to return
+  // results from the Get operation, and for the picker to call the "picked"
+  // callback.
+  PumpLoop();
+
+  EXPECT_EQ(kRequestId2, last_picked_->request_id());
+  EXPECT_FALSE(request_queue_empty_called_);
 }
 
 }  // namespace offline_pages