Have AppSearchProviderTest sort deterministically

Before this commit, the tests were brittle, relying on the order (prior
to sorting) that apps were presented.

Also update the FetchRecommendationsWithContinueReading test code to
only call base::Time::Now() once. Now's base/time/time.h comment says:
"Watch out, the system might adjust its clock in which case time will
actually go backwards. We don't guarantee that times are increasing".

The base::Time::Now change isn't necessary to fix the test, but it makes
for a more robust test.

This fixes `unit_tests --enable-features=AppServiceAsh
--gtest_filter="AppSearchProviderTest.FetchRecommendationsWithCont*"`.
It fails before and passes after this commit.

Note that `unit_tests
--gtest_filter="AppSearchProviderTest.FetchRecommendationsWithCont*"`,
without AppServiceAsh enabled, passes both before and after this commit.

BUG=826982

Change-Id: Iaf6defc23052963e88bc9c27e759a356ef24698e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1636664
Commit-Queue: Nigel Tao <nigeltao@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664992}

chrome/browser/ui/app_list/search/app_search_provider.cc

@@ -163,10 +163,15 @@ class AppSearchProvider::App {
         installed_internally_(installed_internally) {}
   ~App() = default;
 
-  struct CompareByLastActivityTime {
+  struct CompareByLastActivityTimeAndThenAppId {
     bool operator()(const std::unique_ptr<App>& app1,
                     const std::unique_ptr<App>& app2) {
-      return app1->GetLastActivityTime() > app2->GetLastActivityTime();
+      // Sort decreasing by last activity time, then increasing by App ID.
+      base::Time t1 = app1->GetLastActivityTime();
+      base::Time t2 = app2->GetLastActivityTime();
+      if (t1 != t2)
+        return t1 > t2;
+      return app1->id_ < app2->id_;
     }
   };
 
@@ -906,9 +911,11 @@ void AppSearchProvider::MaybeRecordQueryLatencyHistogram(
 void AppSearchProvider::UpdateResults() {
   const bool show_recommendations = query_.empty();
 
-  // Presort app based on last active time in order to be able to remove
-  // duplicates from results.
-  std::sort(apps_.begin(), apps_.end(), App::CompareByLastActivityTime());
+  // Presort app based on last activity time in order to be able to remove
+  // duplicates from results. We break ties by App ID, which is arbitrary, but
+  // deterministic.
+  std::sort(apps_.begin(), apps_.end(),
+            App::CompareByLastActivityTimeAndThenAppId());
 
   if (show_recommendations) {
     // Get the map of app ids to their position in the app list, and then

chrome/browser/ui/app_list/search/tests/app_search_provider_unittest.cc

@@ -127,12 +127,7 @@ class AppSearchProviderTest : public AppListTestBase {
   }
 
   void CreateSearchWithContinueReading() {
-    clock_.SetNow(kTestCurrentTime);
-    // Create ranker here so that tests can modify feature flags.
-    ranker_ = std::make_unique<AppSearchResultRanker>(temp_dir_.GetPath(),
-                                                      kEphemeralUser);
-    app_search_ = std::make_unique<AppSearchProvider>(
-        profile_.get(), nullptr, &clock_, model_updater_.get(), ranker_.get());
+    CreateSearch();
 
     session_tracker_ = std::make_unique<sync_sessions::SyncedSessionTracker>(
         &mock_sync_sessions_client_);
@@ -412,18 +407,17 @@ TEST_F(AppSearchProviderTest, FetchRecommendationsWithContinueReading) {
   constexpr SessionID kTabId2 = SessionID::FromSerializedValue(222);
   constexpr SessionID kTabId3 = SessionID::FromSerializedValue(333);
 
+  const base::Time now = base::Time::Now();
+
   // Case 1: test that ContinueReading is recommended for the latest foreign
   // tab.
   {
     CreateSearchWithContinueReading();
     session_tracker()->InitLocalSession(kLocalSessionTag, kLocalSessionName,
                                         sync_pb::SyncEnums::TYPE_CROS);
-    const base::Time kTimestamp1 =
-        base::Time::Now() - base::TimeDelta::FromMinutes(2);
-    const base::Time kTimestamp2 =
-        base::Time::Now() - base::TimeDelta::FromMinutes(1);
-    const base::Time kTimestamp3 =
-        base::Time::Now() - base::TimeDelta::FromMinutes(3);
+    const base::Time kTimestamp1 = now - base::TimeDelta::FromMinutes(2);
+    const base::Time kTimestamp2 = now - base::TimeDelta::FromMinutes(1);
+    const base::Time kTimestamp3 = now - base::TimeDelta::FromMinutes(3);
 
     session_tracker()->PutWindowInSession(kForeignSessionTag1, kWindowId1);
     session_tracker()->PutTabInWindow(kForeignSessionTag1, kWindowId1, kTabId1);
@@ -476,8 +470,7 @@ TEST_F(AppSearchProviderTest, FetchRecommendationsWithContinueReading) {
     CreateSearchWithContinueReading();
     session_tracker()->InitLocalSession(kLocalSessionTag, kLocalSessionName,
                                         sync_pb::SyncEnums::TYPE_CROS);
-    const base::Time kTimestamp1 =
-        base::Time::Now() - base::TimeDelta::FromMinutes(1);
+    const base::Time kTimestamp1 = now - base::TimeDelta::FromMinutes(1);
 
     session_tracker()->PutWindowInSession(kLocalSessionTag, kWindowId1);
     session_tracker()->PutTabInWindow(kLocalSessionTag, kWindowId1, kTabId1);
@@ -503,8 +496,7 @@ TEST_F(AppSearchProviderTest, FetchRecommendationsWithContinueReading) {
     CreateSearchWithContinueReading();
     session_tracker()->InitLocalSession(kLocalSessionTag, kLocalSessionName,
                                         sync_pb::SyncEnums::TYPE_CROS);
-    const base::Time kTimestamp1 =
-        base::Time::Now() - base::TimeDelta::FromMinutes(121);
+    const base::Time kTimestamp1 = now - base::TimeDelta::FromMinutes(121);
 
     session_tracker()->PutWindowInSession(kForeignSessionTag1, kWindowId1);
     session_tracker()->PutTabInWindow(kForeignSessionTag1, kWindowId1, kTabId1);
@@ -530,8 +522,7 @@ TEST_F(AppSearchProviderTest, FetchRecommendationsWithContinueReading) {
     CreateSearchWithContinueReading();
     session_tracker()->InitLocalSession(kLocalSessionTag, kLocalSessionName,
                                         sync_pb::SyncEnums::TYPE_CROS);
-    const base::Time kTimestamp1 =
-        base::Time::Now() - base::TimeDelta::FromMinutes(1);
+    const base::Time kTimestamp1 = now - base::TimeDelta::FromMinutes(1);
 
     session_tracker()->PutWindowInSession(kForeignSessionTag1, kWindowId1);
     session_tracker()->PutTabInWindow(kForeignSessionTag1, kWindowId1, kTabId1);
@@ -557,8 +548,7 @@ TEST_F(AppSearchProviderTest, FetchRecommendationsWithContinueReading) {
     CreateSearchWithContinueReading();
     session_tracker()->InitLocalSession(kLocalSessionTag, kLocalSessionName,
                                         sync_pb::SyncEnums::TYPE_CROS);
-    const base::Time kTimestamp1 =
-        base::Time::Now() - base::TimeDelta::FromMinutes(1);
+    const base::Time kTimestamp1 = now - base::TimeDelta::FromMinutes(1);
 
     session_tracker()->PutWindowInSession(kForeignSessionTag1, kWindowId1);
     session_tracker()->PutTabInWindow(kForeignSessionTag1, kWindowId1, kTabId1);
@@ -584,8 +574,7 @@ TEST_F(AppSearchProviderTest, FetchRecommendationsWithContinueReading) {
     CreateSearchWithContinueReading();
     session_tracker()->InitLocalSession(kLocalSessionTag, kLocalSessionName,
                                         sync_pb::SyncEnums::TYPE_CROS);
-    const base::Time kTimestamp1 =
-        base::Time::Now() - base::TimeDelta::FromMinutes(1);
+    const base::Time kTimestamp1 = now - base::TimeDelta::FromMinutes(1);
 
     session_tracker()->PutWindowInSession(kForeignSessionTag1, kWindowId1);
     session_tracker()->PutTabInWindow(kForeignSessionTag1, kWindowId1, kTabId1);
@@ -610,8 +599,7 @@ TEST_F(AppSearchProviderTest, FetchRecommendationsWithContinueReading) {
     CreateSearchWithContinueReading();
     session_tracker()->InitLocalSession(kLocalSessionTag, kLocalSessionName,
                                         sync_pb::SyncEnums::TYPE_CROS);
-    const base::Time kTimestamp1 =
-        base::Time::Now() - base::TimeDelta::FromMinutes(1);
+    const base::Time kTimestamp1 = now - base::TimeDelta::FromMinutes(1);
 
     session_tracker()->PutWindowInSession(kForeignSessionTag1, kWindowId1);
     session_tracker()->PutTabInWindow(kForeignSessionTag1, kWindowId1, kTabId1);