[Offline pages] Switching ChangeRequestsStateTask to use RequestQueueStore::GetRequestsByIds

components/offline_pages/background/change_requests_state_task_unittest.cc

Index: components/offline_pages/background/change_requests_state_task_unittest.cc
diff --git a/components/offline_pages/background/change_requests_state_task_unittest.cc b/components/offline_pages/background/change_requests_state_task_unittest.cc
index f3c2d217e0996bc808d644fca8fb5c69de969b6f..f401dd7ef3db483f642669d6d1479d3a8d1793bf 100644
--- a/components/offline_pages/background/change_requests_state_task_unittest.cc
+++ b/components/offline_pages/background/change_requests_state_task_unittest.cc
@@ -145,20 +145,27 @@ TEST_F(ChangeRequestsStateTaskTest, UpdateMultipleItems) {
   task.Run();
   PumpLoop();
   ASSERT_TRUE(last_result());
-  EXPECT_EQ(2UL, last_result()->item_statuses.size());
-  EXPECT_EQ(kRequestId1, last_result()->item_statuses.at(0).first);
+  ASSERT_EQ(2UL, last_result()->item_statuses.size());
+
+  int index_id_1 =

[Pete Williamson, 2016/12/02 01:39:06]

Why are you doing this?  I found the code easier to understand before, is it now
wrong because the algorithm returns things slightly differently?

[fgorski, 2016/12/02 22:32:03]

Yes, we did make some changes, mainly the fact that the dictionary might be
returning values in a different order on different platforms... hence this
change.

+      last_result()->item_statuses.at(0).first == kRequestId1 ? 0 : 1;
+  int index_id_2 = 1 - index_id_1;
+
+  EXPECT_EQ(kRequestId1, last_result()->item_statuses.at(index_id_1).first);
   EXPECT_EQ(ItemActionStatus::SUCCESS,
-            last_result()->item_statuses.at(0).second);
-  EXPECT_EQ(kRequestId2, last_result()->item_statuses.at(1).first);
+            last_result()->item_statuses.at(index_id_1).second);
+  EXPECT_EQ(kRequestId2, last_result()->item_statuses.at(index_id_2).first);
   EXPECT_EQ(ItemActionStatus::SUCCESS,
-            last_result()->item_statuses.at(1).second);
-  EXPECT_EQ(2UL, last_result()->updated_items.size());
-  EXPECT_EQ(kRequestId1, last_result()->updated_items.at(0).request_id());
+            last_result()->item_statuses.at(index_id_2).second);
+  ASSERT_EQ(2UL, last_result()->updated_items.size());
+  EXPECT_EQ(kRequestId1,
+            last_result()->updated_items.at(index_id_1).request_id());
   EXPECT_EQ(SavePageRequest::RequestState::PAUSED,
-            last_result()->updated_items.at(0).request_state());
-  EXPECT_EQ(kRequestId2, last_result()->updated_items.at(1).request_id());
+            last_result()->updated_items.at(index_id_1).request_state());
+  EXPECT_EQ(kRequestId2,
+            last_result()->updated_items.at(index_id_2).request_id());
   EXPECT_EQ(SavePageRequest::RequestState::PAUSED,
-            last_result()->updated_items.at(1).request_state());
+            last_result()->updated_items.at(index_id_2).request_state());
 }
 
 TEST_F(ChangeRequestsStateTaskTest, EmptyRequestsList) {
@@ -190,7 +197,7 @@ TEST_F(ChangeRequestsStateTaskTest, UpdateMissingItem) {
   task.Run();
   PumpLoop();
   ASSERT_TRUE(last_result());
-  EXPECT_EQ(2UL, last_result()->item_statuses.size());
+  ASSERT_EQ(2UL, last_result()->item_statuses.size());

[Pete Williamson, 2016/12/02 01:39:06]

Why the change from EXPECT to ASSERT?

[fgorski, 2016/12/02 22:32:03]

When we have too many, I don't have to look at the error message too long to
work out what the problem is.,

when we have too little, there will be a crash later (when accessing the first
missing index), and I prefer to crash early.

   EXPECT_EQ(kRequestId1, last_result()->item_statuses.at(0).first);
   EXPECT_EQ(ItemActionStatus::SUCCESS,
             last_result()->item_statuses.at(0).second);