App list sync unit tests

chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc

Index: chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc
diff --git a/chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc b/chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc
index e454e1b4cc618c35ae7170fc64a228a25fa86bd3..02163ae7ddf553723f3ca46e490df2cf41403258 100644
--- a/chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc
+++ b/chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc
@@ -10,6 +10,11 @@
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/crx_file/id_util.h"
+#include "components/sync/model/attachments/attachment_service_proxy_for_test.h"
+#include "components/sync/model/fake_sync_change_processor.h"
+#include "components/sync/model/sync_error_factory.h"
+#include "components/sync/model/sync_error_factory_mock.h"
+#include "components/sync/protocol/sync.pb.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/common/constants.h"
 #include "ui/app_list/app_list_item.h"
@@ -46,6 +51,87 @@ std::string CreateNextAppId(const std::string& app_id) {
   return next_app_id;
 }
 
+static const std::string UNSET = "__unset__";

[xiyuan, 2017/06/21 15:59:20]

Global variables should only be POD.
See
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

Use constexpr when possible. And const name should be like kConstName.

i.e.
  constexpr char kUnset[] = "__unset__";

[rcui, 2017/06/23 02:03:49]

Done, thanks.  Looking a lot better now :)

+
+static const std::string INVALID_ORDINALS_ID = "invalid_ordinals";
+static const std::string EMPTY_ITEM_NAME_ID = "empty_item_name";
+static const std::string EMPTY_ITEM_NAME_UNSET_ID = "empty_item_name_unset";
+static const std::string EMPTY_PARENT_ID = "empty_parent_id";
+static const std::string EMPTY_PARENT_UNSET_ID = "empty_parent_id_unset";
+static const std::string EMPTY_ORDINALS_ID = "empty_ordinals";
+static const std::string EMPTY_ORDINALS_UNSET_ID = "empty_ordinals_unset";
+static const std::string DUPE_ITEM_ID = "dupe_item_id";
+
+syncer::SyncData CreateAppRemoteData(const std::string& id,
+                                     const std::string& name,
+                                     const std::string& parent_id,
+                                     const std::string& item_ordinal,
+                                     const std::string& item_pin_ordinal) {
+  sync_pb::EntitySpecifics specifics;
+  sync_pb::AppListSpecifics* app_list = specifics.mutable_app_list();
+  if (id != UNSET)
+    app_list->set_item_id(id);
+  app_list->set_item_type(sync_pb::AppListSpecifics_AppListItemType_TYPE_APP);
+  if (name != UNSET)
+    app_list->set_item_name(name);
+  if (parent_id != UNSET)
+    app_list->set_parent_id(parent_id);
+  if (item_ordinal != UNSET)
+    app_list->set_item_ordinal(item_ordinal);
+  if (item_pin_ordinal != UNSET)
+    app_list->set_item_pin_ordinal(item_pin_ordinal);
+
+  return syncer::SyncData::CreateRemoteData(
+      std::hash<std::string>{}(id), specifics, base::Time(),
+      syncer::AttachmentIdList(),
+      syncer::AttachmentServiceProxyForTest::Create());
+}
+
+syncer::SyncDataList CreateBadAppRemoteData(const std::string& id) {
+  syncer::SyncDataList sync_list;
+  // Invalid item_ordinal and item_pin_ordinal.
+  sync_list.push_back(CreateAppRemoteData(

[khmel, 2017/06/22 00:10:11]

In case id == UNSET we create data with different ids, EMPTY_ITEM_NAME_ID,
EMPTY_ITEM_NAME_UNSET_ID, .... 
But in case we provide id != UNSET, we create data items with the same id. Is it
expected? It looks inconsistent

[rcui, 2017/06/23 02:03:49]

Good point.  I'm using a different constant for this now: kDefault.

+      id == UNSET ? INVALID_ORDINALS_ID : id, "item_name", "parent_id",
+      "$$invalid_ordinal$$", "$$invalid_ordinal$$"));

[stevenjb, 2017/06/21 23:27:17]

This should probably also be a constant

[rcui, 2017/06/23 02:03:48]

Since it's only used in this function, I think it's ok to keep as-is. Imo string
literals are preferred over constants in tests b/c they are easier to read and
validate (no indirection).  Let me know if you disagree strongly.

[stevenjb, 2017/06/23 19:46:05]

Acknowledged.

+  // Empty item name.
+  sync_list.push_back(CreateAppRemoteData(id == UNSET ? EMPTY_ITEM_NAME_ID : id,
+                                          "", "parent_id", "ordinal",
+                                          "pinordinal"));
+  sync_list.push_back(
+      CreateAppRemoteData(id == UNSET ? EMPTY_ITEM_NAME_UNSET_ID : id, UNSET,
+                          "parent_id", "ordinal", "pinordinal"));
+  // Empty parent ID.
+  sync_list.push_back(CreateAppRemoteData(id == UNSET ? EMPTY_PARENT_ID : id,
+                                          "item_name", "", "ordinal",
+                                          "pinordinal"));
+  sync_list.push_back(
+      CreateAppRemoteData(id == UNSET ? EMPTY_PARENT_UNSET_ID : id, "item_name",
+                          UNSET, "ordinal", "pinordinal"));
+  // Empty item_ordinal and item_pin_ordinal.
+  sync_list.push_back(CreateAppRemoteData(id == UNSET ? EMPTY_ORDINALS_ID : id,
+                                          "item_name", "parent_id", "", ""));
+  sync_list.push_back(
+      CreateAppRemoteData(id == UNSET ? EMPTY_ORDINALS_UNSET_ID : id,
+                          "item_name", "parent_id", UNSET, UNSET));
+  // Duplicate item_id.
+  sync_list.push_back(CreateAppRemoteData(id == UNSET ? DUPE_ITEM_ID : id,
+                                          "item_name", "parent_id", "ordinal",
+                                          "pinordinal"));
+  sync_list.push_back(CreateAppRemoteData(id == UNSET ? DUPE_ITEM_ID : id,
+                                          "item_name_dupe", "parent_id",
+                                          "ordinal", "pinordinal"));
+  // Empty item_id.
+  sync_list.push_back(CreateAppRemoteData("", "item_name", "parent_id",
+                                          "ordinal", "pinordinal"));
+  sync_list.push_back(CreateAppRemoteData(UNSET, "item_name", "parent_id",
+                                          "ordinal", "pinordinal"));
+  // All fields empty.
+  sync_list.push_back(CreateAppRemoteData("", "", "", "", ""));
+  sync_list.push_back(CreateAppRemoteData(UNSET, UNSET, UNSET, UNSET, UNSET));
+
+  return sync_list;
+}
+
 }  // namespace
 
 class AppListSyncableServiceTest : public AppListTestBase {
@@ -74,9 +160,16 @@ class AppListSyncableServiceTest : public AppListTestBase {
     return app_list_syncable_service_->GetModel();
   }
 
+  const app_list::AppListSyncableService::SyncItem* GetSyncItem(
+      std::string id) {

[xiyuan, 2017/06/21 15:59:20]

std::string -> const std::string&

And can this be a const method?

[rcui, 2017/06/23 02:03:48]

Done.

+    return app_list_syncable_service_->GetSyncItem(id);
+  }
+
+ protected:
+  std::unique_ptr<app_list::AppListSyncableService> app_list_syncable_service_;

[khmel, 2017/06/22 00:10:11]

nit: can we leave it private and add getter:?

 protected:
  app_list::AppListSyncableService* app_list_syncable_service() { return
app_list_syncable_service_.get(); } 

[rcui, 2017/06/23 02:03:48]

Done.

+
  private:
   base::ScopedTempDir temp_dir_;
-  std::unique_ptr<app_list::AppListSyncableService> app_list_syncable_service_;
 
   DISALLOW_COPY_AND_ASSIGN(AppListSyncableServiceTest);
 };
@@ -131,3 +224,169 @@ TEST_F(AppListSyncableServiceTest, OEMFolderForConflictingPos) {
   EXPECT_EQ(some_app_index, web_store_app_index + 1);
   EXPECT_EQ(oem_folder_index, web_store_app_index + 2);
 }
+
+TEST_F(AppListSyncableServiceTest, InitialMerge) {
+  syncer::SyncDataList sync_list;
+  sync_list.push_back(CreateAppRemoteData(
+      "item_id1", "item_name1", "parent_id1", "ordinal", "pinordinal"));

[khmel, 2017/06/22 00:10:11]

nit: as an valid item id we probably better use id of required format.
AppListSyncService works with this, but some other code may have
DCHECK(crx_file::id_util::IdIsValid(app_id));

[khmel, 2017/06/22 00:10:12]

nit: as a valid ordinal, can we use something from ordinal?
 for example: syncer::StringOrdinal::CreateInitialOrdinal()

[rcui, 2017/06/23 02:03:48]

The ordinals we use now are valid.  They also make the tests more readable I
think, so I'd prefer to keep them.

[rcui, 2017/06/23 02:03:48]

Done.  Id's are generated with crx_file::id_util::GenerateId()

+  sync_list.push_back(CreateAppRemoteData(
+      "item_id2", "item_name2", "parent_id2", "ordinal", "pinordinal"));
+
+  app_list_syncable_service_->MergeDataAndStartSyncing(
+      syncer::APP_LIST, sync_list,
+      std::unique_ptr<syncer::SyncChangeProcessor>(
+          new syncer::FakeSyncChangeProcessor),
+      std::unique_ptr<syncer::SyncErrorFactory>(
+          new syncer::SyncErrorFactoryMock));

[xiyuan, 2017/06/21 15:59:20]

nit: use base::MakeUnique<T>() instead of std::unique_ptr<T>(new T)

[rcui, 2017/06/23 02:03:49]

Done.

+  content::RunAllBlockingPoolTasksUntilIdle();
+
+  EXPECT_TRUE(GetSyncItem("item_id1"));
+  EXPECT_EQ("item_name1", GetSyncItem("item_id1")->item_name);
+  EXPECT_EQ("parent_id1", GetSyncItem("item_id1")->parent_id);
+  EXPECT_EQ("ordinal", GetSyncItem("item_id1")->item_ordinal.ToDebugString());
+  EXPECT_EQ("pinordinal",
+            GetSyncItem("item_id1")->item_pin_ordinal.ToDebugString());
+
+  EXPECT_TRUE(GetSyncItem("item_id2"));
+  EXPECT_EQ("item_name2", GetSyncItem("item_id2")->item_name);
+  EXPECT_EQ("parent_id2", GetSyncItem("item_id2")->parent_id);
+  EXPECT_EQ("ordinal", GetSyncItem("item_id2")->item_ordinal.ToDebugString());
+  EXPECT_EQ("pinordinal",
+            GetSyncItem("item_id2")->item_pin_ordinal.ToDebugString());
+}
+
+TEST_F(AppListSyncableServiceTest, InitialMerge_BadData) {
+  syncer::SyncDataList sync_list = CreateBadAppRemoteData(UNSET);
+
+  app_list_syncable_service_->MergeDataAndStartSyncing(
+      syncer::APP_LIST, sync_list,
+      std::unique_ptr<syncer::SyncChangeProcessor>(
+          new syncer::FakeSyncChangeProcessor),
+      std::unique_ptr<syncer::SyncErrorFactory>(
+          new syncer::SyncErrorFactoryMock));
+  content::RunAllBlockingPoolTasksUntilIdle();
+
+  // Invalid item_ordinal and item_pin_ordinal.
+  EXPECT_TRUE(GetSyncItem(INVALID_ORDINALS_ID));

[khmel, 2017/06/22 00:10:12]

This should be ASSERT_TRUE(GetSyncItem(INVALID_ORDINALS_ID))
In case EXPECT_TRUE and GetSyncItem(INVALID_ORDINALS_ID) is nullptr this test
would crash on next line. In case ASSERT_* test would be stopped immediately.
nit: You may probably want to use local var:
const app_list::AppListSyncableService::SyncItem* item =
etSyncItem(INVALID_ORDINALS_ID);
ASSERT_TRUE(item)
...

[rcui, 2017/06/23 02:03:49]

Done.  I passed on using a local var as it breaks the code flow and is harder to
write..

+  EXPECT_EQ("n",
+            GetSyncItem(INVALID_ORDINALS_ID)->item_ordinal.ToDebugString());
+  EXPECT_EQ("INVALID[$$invalid_ordinal$$]",
+            GetSyncItem(INVALID_ORDINALS_ID)->item_pin_ordinal.ToDebugString());
+
+  // Empty item name.
+  EXPECT_TRUE(GetSyncItem(EMPTY_ITEM_NAME_ID));
+  EXPECT_EQ("", GetSyncItem(EMPTY_ITEM_NAME_ID)->item_name);
+  EXPECT_TRUE(GetSyncItem(EMPTY_ITEM_NAME_UNSET_ID));
+  EXPECT_EQ("", GetSyncItem(EMPTY_ITEM_NAME_UNSET_ID)->item_name);
+
+  // Empty parent ID.
+  EXPECT_TRUE(GetSyncItem(EMPTY_PARENT_ID));
+  EXPECT_EQ("", GetSyncItem(EMPTY_PARENT_ID)->parent_id);
+  EXPECT_TRUE(GetSyncItem(EMPTY_PARENT_UNSET_ID));
+  EXPECT_EQ("", GetSyncItem(EMPTY_PARENT_UNSET_ID)->parent_id);
+
+  // Empty item_ordinal and item_pin_ordinal.
+  EXPECT_TRUE(GetSyncItem(EMPTY_ORDINALS_ID));
+  EXPECT_EQ("n", GetSyncItem(EMPTY_ORDINALS_ID)->item_ordinal.ToDebugString());
+  EXPECT_EQ("INVALID[]",
+            GetSyncItem(EMPTY_ORDINALS_ID)->item_pin_ordinal.ToDebugString());
+  EXPECT_TRUE(GetSyncItem(EMPTY_ORDINALS_UNSET_ID));
+  EXPECT_EQ("n",
+            GetSyncItem(EMPTY_ORDINALS_UNSET_ID)->item_ordinal.ToDebugString());
+  EXPECT_EQ(
+      "INVALID[]",
+      GetSyncItem(EMPTY_ORDINALS_UNSET_ID)->item_pin_ordinal.ToDebugString());
+
+  // Duplicate item_id overrides previous.
+  EXPECT_TRUE(GetSyncItem(DUPE_ITEM_ID));
+  EXPECT_EQ("item_name_dupe", GetSyncItem(DUPE_ITEM_ID)->item_name);
+}
+
+TEST_F(AppListSyncableServiceTest, InitialMergeAndUpdate) {
+  syncer::SyncDataList sync_list;
+  sync_list.push_back(CreateAppRemoteData(
+      "item_id1", "item_name1", "parent_id1", "ordinal", "pinordinal"));
+  sync_list.push_back(CreateAppRemoteData(
+      "item_id2", "item_name2", "parent_id2", "ordinal", "pinordinal"));
+
+  app_list_syncable_service_->MergeDataAndStartSyncing(
+      syncer::APP_LIST, sync_list,
+      std::unique_ptr<syncer::SyncChangeProcessor>(
+          new syncer::FakeSyncChangeProcessor),
+      std::unique_ptr<syncer::SyncErrorFactory>(
+          new syncer::SyncErrorFactoryMock));
+  content::RunAllBlockingPoolTasksUntilIdle();
+
+  EXPECT_TRUE(GetSyncItem("item_id1"));
+  EXPECT_TRUE(GetSyncItem("item_id2"));
+
+  syncer::SyncChangeList change_list;
+  change_list.push_back(syncer::SyncChange(
+      FROM_HERE, syncer::SyncChange::ACTION_UPDATE,
+      CreateAppRemoteData("item_id1", "item_name1x", "parent_id1x", "ordinalx",
+                          "pinordinalx")));
+  change_list.push_back(syncer::SyncChange(
+      FROM_HERE, syncer::SyncChange::ACTION_UPDATE,
+      CreateAppRemoteData("item_id2", "item_name2x", "parent_id2x", "ordinalx",
+                          "pinordinalx")));
+
+  app_list_syncable_service_->ProcessSyncChanges(tracked_objects::Location(),
+                                                 change_list);
+  content::RunAllBlockingPoolTasksUntilIdle();
+
+  EXPECT_TRUE(GetSyncItem("item_id1"));
+  EXPECT_EQ("item_name1x", GetSyncItem("item_id1")->item_name);
+  EXPECT_EQ("parent_id1x", GetSyncItem("item_id1")->parent_id);
+  EXPECT_EQ("ordinalx", GetSyncItem("item_id1")->item_ordinal.ToDebugString());
+  EXPECT_EQ("pinordinalx",
+            GetSyncItem("item_id1")->item_pin_ordinal.ToDebugString());
+
+  EXPECT_TRUE(GetSyncItem("item_id2"));
+  EXPECT_EQ("item_name2x", GetSyncItem("item_id2")->item_name);
+  EXPECT_EQ("parent_id2x", GetSyncItem("item_id2")->parent_id);
+  EXPECT_EQ("ordinalx", GetSyncItem("item_id2")->item_ordinal.ToDebugString());
+  EXPECT_EQ("pinordinalx",
+            GetSyncItem("item_id2")->item_pin_ordinal.ToDebugString());
+}
+
+TEST_F(AppListSyncableServiceTest, InitialMergeAndUpdate_BadData) {
+  syncer::SyncDataList sync_list;
+  sync_list.push_back(CreateAppRemoteData(
+      "item_id1", "item_name1", "parent_id1", "ordinal", "pinordinal"));
+
+  app_list_syncable_service_->MergeDataAndStartSyncing(
+      syncer::APP_LIST, sync_list,
+      std::unique_ptr<syncer::SyncChangeProcessor>(
+          new syncer::FakeSyncChangeProcessor),
+      std::unique_ptr<syncer::SyncErrorFactory>(
+          new syncer::SyncErrorFactoryMock));
+  content::RunAllBlockingPoolTasksUntilIdle();
+
+  EXPECT_TRUE(GetSyncItem("item_id1"));
+
+  syncer::SyncChangeList change_list;
+  syncer::SyncDataList update_list = CreateBadAppRemoteData("item_id1");

[khmel, 2017/06/22 00:10:12]

It is not clear what we want to achieve by sending CreateBadAppRemoteData(). At
line 375 we add one more change and starting from 387 we just validate that item
was set from last update. IIUC removing lines 368-373 would not affect your
test. 
Probably you may want to move L375 before L368, but I am guessing here. Could
you please add more comments before test?

[rcui, 2017/06/23 02:03:48]

I've removed the bookmark.  This test should just be validating that bad data
can be processed without crashing.

+  for (syncer::SyncDataList::const_iterator iter = update_list.begin();
+       iter != update_list.end(); ++iter) {
+    change_list.push_back(syncer::SyncChange(
+        FROM_HERE, syncer::SyncChange::ACTION_UPDATE, *iter));
+  }
+  // Update item that acts as a bookmark.
+  change_list.push_back(syncer::SyncChange(
+      FROM_HERE, syncer::SyncChange::ACTION_UPDATE,
+      CreateAppRemoteData("item_id1", "item_name1x", "parent_id1x", "ordinalx",
+                          "pinordinalx")));
+
+  app_list_syncable_service_->ProcessSyncChanges(tracked_objects::Location(),
+                                                 change_list);
+  content::RunAllBlockingPoolTasksUntilIdle();
+
+  EXPECT_TRUE(GetSyncItem("item_id1"));
+  // Validate all items processed by checking the last update item was
+  // processed.
+  EXPECT_EQ("item_name1x", GetSyncItem("item_id1")->item_name);
+  EXPECT_EQ("parent_id1x", GetSyncItem("item_id1")->parent_id);
+  EXPECT_EQ("ordinalx", GetSyncItem("item_id1")->item_ordinal.ToDebugString());
+  EXPECT_EQ("pinordinalx",
+            GetSyncItem("item_id1")->item_pin_ordinal.ToDebugString());
+}