[search_engines] Add sync integration test for duplicate keywords

This CL exercises the existing keyword uniquification logic for when
sync merges in multiple engines with the same keyword.

This soon will be changed once we get rid of uniquification.

Bug: 1022775
Change-Id: If88e5d74123c99605e95ab2ebb9c86f4f2bee43a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2569383
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833376}

chrome/browser/search_engines/template_url_service_sync_unittest.cc

@@ -185,17 +185,6 @@ class TemplateURLServiceSyncTest : public testing::Test {
   std::unique_ptr<syncer::SyncChangeProcessor> PassProcessor();
   std::unique_ptr<syncer::SyncErrorFactory> CreateAndPassSyncErrorFactory();
 
-  // Creates a TemplateURL with some test values. The caller owns the returned
-  // TemplateURL*.
-  std::unique_ptr<TemplateURL> CreateTestTemplateURL(
-      const base::string16& keyword,
-      const std::string& url,
-      const std::string& guid = std::string(),
-      time_t last_mod = 100,
-      bool safe_for_autoreplace = false,
-      bool created_by_policy = false,
-      int prepopulate_id = 999999) const;
-
   // Verifies the two TemplateURLs are equal.
   // TODO(stevet): Share this with TemplateURLServiceTest.
   void AssertEquals(const TemplateURL& expected,
@@ -289,29 +278,6 @@ TemplateURLServiceSyncTest::CreateAndPassSyncErrorFactory() {
       new syncer::SyncErrorFactoryMock());
 }
 
-std::unique_ptr<TemplateURL> TemplateURLServiceSyncTest::CreateTestTemplateURL(
-    const base::string16& keyword,
-    const std::string& url,
-    const std::string& guid,
-    time_t last_mod,
-    bool safe_for_autoreplace,
-    bool created_by_policy,
-    int prepopulate_id) const {
-  TemplateURLData data;
-  data.SetShortName(ASCIIToUTF16("unittest"));
-  data.SetKeyword(keyword);
-  data.SetURL(url);
-  data.favicon_url = GURL("http://favicon.url");
-  data.safe_for_autoreplace = safe_for_autoreplace;
-  data.date_created = Time::FromTimeT(100);
-  data.last_modified = Time::FromTimeT(last_mod);
-  data.created_by_policy = created_by_policy;
-  data.prepopulate_id = prepopulate_id;
-  if (!guid.empty())
-    data.sync_guid = guid;
-  return std::make_unique<TemplateURL>(data);
-}
-
 void TemplateURLServiceSyncTest::AssertEquals(const TemplateURL& expected,
                                               const TemplateURL& actual) const {
   ASSERT_EQ(expected.short_name(), actual.short_name());

chrome/browser/search_engines/template_url_service_test_util.cc

@@ -10,6 +10,7 @@
 #include "base/run_loop.h"
 #include "base/test/bind.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/time.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/search_engines/chrome_template_url_service_client.h"
 #include "chrome/test/base/testing_profile.h"
@@ -62,6 +63,29 @@ void RemoveManagedDefaultSearchPreferences(TestingProfile* profile) {
       DefaultSearchManager::kDefaultSearchProviderDataPrefName);
 }
 
+std::unique_ptr<TemplateURL> CreateTestTemplateURL(
+    const base::string16& keyword,
+    const std::string& url,
+    const std::string& guid,
+    time_t last_mod,
+    bool safe_for_autoreplace,
+    bool created_by_policy,
+    int prepopulate_id) {
+  TemplateURLData data;
+  data.SetShortName(base::ASCIIToUTF16("unittest"));
+  data.SetKeyword(keyword);
+  data.SetURL(url);
+  data.favicon_url = GURL("http://favicon.url");
+  data.safe_for_autoreplace = safe_for_autoreplace;
+  data.date_created = base::Time::FromTimeT(100);
+  data.last_modified = base::Time::FromTimeT(last_mod);
+  data.created_by_policy = created_by_policy;
+  data.prepopulate_id = prepopulate_id;
+  if (!guid.empty())
+    data.sync_guid = guid;
+  return std::make_unique<TemplateURL>(data);
+}
+
 TemplateURLServiceTestUtil::TemplateURLServiceTestUtil() {
   // Make unique temp directory.
   EXPECT_TRUE(temp_dir_.CreateUniqueTempDir());

chrome/browser/search_engines/template_url_service_test_util.h

@@ -30,6 +30,18 @@ void SetManagedDefaultSearchPreferences(const TemplateURLData& managed_data,
 // Removes all the managed preferences for the default search provider.
 void RemoveManagedDefaultSearchPreferences(TestingProfile* profile);
 
+// Creates a TemplateURL with some test values. The caller owns the returned
+// TemplateURL*.
+// TODO(tommycli): Change last_mod to more verbose name and make it base::Time.
+std::unique_ptr<TemplateURL> CreateTestTemplateURL(
+    const base::string16& keyword,
+    const std::string& url,
+    const std::string& guid = std::string(),
+    time_t last_mod = 100,
+    bool safe_for_autoreplace = false,
+    bool created_by_policy = false,
+    int prepopulate_id = 999999);
+
 class TemplateURLServiceTestUtil : public TemplateURLServiceObserver {
  public:
   TemplateURLServiceTestUtil();

chrome/browser/sync/test/integration/single_client_search_engines_sync_test.cc

@@ -4,13 +4,23 @@
 
 #include "base/macros.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
+#include "chrome/browser/search_engines/template_url_service_test_util.h"
 #include "chrome/browser/sync/test/integration/search_engines_helper.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/browser/sync/test/integration/updated_progress_marker_checker.h"
 #include "chrome/test/base/search_test_utils.h"
+#include "components/search_engines/template_url.h"
+#include "components/search_engines/template_url_data.h"
 #include "components/search_engines/template_url_service.h"
 #include "components/sync/driver/profile_sync_service.h"
+#include "components/sync/engine_impl/loopback_server/loopback_server_entity.h"
+#include "components/sync/engine_impl/loopback_server/persistent_unique_client_entity.h"
+#include "components/sync/model/sync_data.h"
 #include "content/public/test/browser_test.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+using search_engines_helper::HasSearchEngine;
+using testing::NotNull;
 
 class SingleClientSearchEnginesSyncTest : public SyncTest {
  public:
@@ -35,6 +45,18 @@ class SingleClientSearchEnginesSyncTest : public SyncTest {
     // TODO(crbug.com/1137771): rewrite test to not use verifier.
     return true;
   }
+
+  std::unique_ptr<syncer::LoopbackServerEntity> CreateFromTemplateURL(
+      std::unique_ptr<TemplateURL> turl) {
+    DCHECK(turl);
+    syncer::SyncData sync_data =
+        TemplateURLService::CreateSyncDataFromTemplateURL(*turl);
+    return syncer::PersistentUniqueClientEntity::CreateFromSpecificsForTesting(
+        /*non_unique_name=*/sync_data.GetTitle(),
+        /*client_tag=*/turl->sync_guid(), sync_data.GetSpecifics(),
+        /*creation_time=*/0,
+        /*last_modified_time=*/0);
+  }
 };
 
 IN_PROC_BROWSER_TEST_F(SingleClientSearchEnginesSyncTest, Sanity) {
@@ -45,3 +67,45 @@ IN_PROC_BROWSER_TEST_F(SingleClientSearchEnginesSyncTest, Sanity) {
   ASSERT_TRUE(UpdatedProgressMarkerChecker(GetSyncService(0)).Wait());
   ASSERT_TRUE(search_engines_helper::ServiceMatchesVerifier(0));
 }
+
+IN_PROC_BROWSER_TEST_F(SingleClientSearchEnginesSyncTest,
+                       DuplicateKeywordEnginesAllFromSync) {
+  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+  TemplateURLService* service =
+      search_engines_helper::GetServiceForBrowserContext(0);
+  ASSERT_FALSE(HasSearchEngine(/*profile_index=*/0, "key1"));
+  ASSERT_FALSE(service->GetTemplateURLForGUID("guid1"));
+  ASSERT_FALSE(service->GetTemplateURLForGUID("guid2"));
+  ASSERT_FALSE(service->GetTemplateURLForGUID("guid3"));
+
+  // Create two TemplateURLs with the same keyword, but different guids.
+  // "guid2" is newer, so it should be treated as better.
+  fake_server_->InjectEntity(CreateFromTemplateURL(CreateTestTemplateURL(
+      /*keyword=*/base::ASCIIToUTF16("key1"), /*url=*/"http://key1.com",
+      /*guid=*/"guid1", /*last_mod=*/10)));
+  fake_server_->InjectEntity(CreateFromTemplateURL(CreateTestTemplateURL(
+      /*keyword=*/base::ASCIIToUTF16("key1"), /*url=*/"http://key1.com",
+      /*guid=*/"guid2", /*last_mod=*/5)));
+  fake_server_->InjectEntity(CreateFromTemplateURL(CreateTestTemplateURL(
+      /*keyword=*/base::ASCIIToUTF16("key1"), /*url=*/"http://key1.com",
+      /*guid=*/"guid3", /*last_mod=*/5)));
+
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(UpdatedProgressMarkerChecker(GetSyncService(0)).Wait());
+
+  EXPECT_TRUE(HasSearchEngine(/*profile_index=*/0, "key1"));
+  TemplateURL* guid1 = service->GetTemplateURLForGUID("guid1");
+  TemplateURL* guid2 = service->GetTemplateURLForGUID("guid2");
+  TemplateURL* guid3 = service->GetTemplateURLForGUID("guid3");
+  ASSERT_THAT(guid1, NotNull());
+  ASSERT_THAT(guid2, NotNull());
+  ASSERT_THAT(guid3, NotNull());
+  EXPECT_EQ(base::ASCIIToUTF16("key1"), guid1->keyword());
+  // Due to uniquification, the keyword for "guid2" was reset to "key1.com".
+  EXPECT_EQ(base::ASCIIToUTF16("key1.com"), guid2->keyword());
+  // Due to uniquification, the keyword for "guid3" was reset to "key1_".
+  EXPECT_EQ(base::ASCIIToUTF16("key1_"), guid3->keyword());
+
+  EXPECT_EQ(guid1,
+            service->GetTemplateURLForKeyword(base::ASCIIToUTF16("key1")));
+}