Remove expectations on merge deltas from syncable services unit tests

A preparation for https://crrev.com/c/2087914. Expectations on the
values of SyncMergeResult::num_items_{added/deleted/...} are removed.
Coverage for the sync merge logic is still present, since the tests
already verified it by mean of other checks.

The CL complements such pre-existing checks with checks on the initial
and final size of the local model. One of the implementations of
GetAllSyncData that was removed in crrev.com/c/2083455 is re-added to
enable this. It is suffixed with ForTesting as with all other cases in
crrev.com/c/2083455.

Change-Id: If83c977b8e5dac6279b3fd7fc7517dc3d2dbc1f5
Bug: 1057577
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2091346
Commit-Queue: Victor Vianna <victorvianna@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#758773}

chrome/browser/search_engines/template_url_service_sync_unittest.cc

@@ -695,19 +695,16 @@ TEST_F(TemplateURLServiceSyncTest, ResolveSyncKeywordConflict) {
 }
 
 TEST_F(TemplateURLServiceSyncTest, StartSyncEmpty) {
+  ASSERT_TRUE(model()->GetAllSyncData(syncer::SEARCH_ENGINES).empty());
   syncer::SyncMergeResult merge_result =
       MergeAndExpectNotify(syncer::SyncDataList(), 0);
 
   EXPECT_EQ(0U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
   EXPECT_EQ(0U, processor()->change_list_size());
-  EXPECT_EQ(0, merge_result.num_items_added());
-  EXPECT_EQ(0, merge_result.num_items_modified());
-  EXPECT_EQ(0, merge_result.num_items_deleted());
-  EXPECT_EQ(0, merge_result.num_items_before_association());
-  EXPECT_EQ(0, merge_result.num_items_after_association());
 }
 
 TEST_F(TemplateURLServiceSyncTest, MergeIntoEmpty) {
+  ASSERT_TRUE(model()->GetAllSyncData(syncer::SEARCH_ENGINES).empty());
   syncer::SyncDataList initial_data = CreateInitialSyncData();
   syncer::SyncMergeResult merge_result = MergeAndExpectNotify(initial_data, 1);
 
@@ -721,13 +718,6 @@ TEST_F(TemplateURLServiceSyncTest, MergeIntoEmpty) {
   }
 
   EXPECT_EQ(0U, processor()->change_list_size());
-
-  // Locally the three new TemplateURL's should have been added.
-  EXPECT_EQ(3, merge_result.num_items_added());
-  EXPECT_EQ(0, merge_result.num_items_modified());
-  EXPECT_EQ(0, merge_result.num_items_deleted());
-  EXPECT_EQ(0, merge_result.num_items_before_association());
-  EXPECT_EQ(3, merge_result.num_items_after_association());
 }
 
 TEST_F(TemplateURLServiceSyncTest, MergeInAllNewData) {
@@ -737,6 +727,7 @@ TEST_F(TemplateURLServiceSyncTest, MergeInAllNewData) {
                                      "def"));
   model()->Add(CreateTestTemplateURL(ASCIIToUTF16("xyz.com"), "http://xyz.com",
                                      "xyz"));
+  ASSERT_EQ(3U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
   syncer::SyncDataList initial_data = CreateInitialSyncData();
   syncer::SyncMergeResult merge_result = MergeAndExpectNotify(initial_data, 1);
 
@@ -757,13 +748,6 @@ TEST_F(TemplateURLServiceSyncTest, MergeInAllNewData) {
   EXPECT_TRUE(processor()->contains_guid("abc"));
   EXPECT_TRUE(processor()->contains_guid("def"));
   EXPECT_TRUE(processor()->contains_guid("xyz"));
-
-  // Locally the three new TemplateURL's should have been added.
-  EXPECT_EQ(3, merge_result.num_items_added());
-  EXPECT_EQ(0, merge_result.num_items_modified());
-  EXPECT_EQ(0, merge_result.num_items_deleted());
-  EXPECT_EQ(3, merge_result.num_items_before_association());
-  EXPECT_EQ(6, merge_result.num_items_after_association());
 }
 
 TEST_F(TemplateURLServiceSyncTest, MergeSyncIsTheSame) {
@@ -775,6 +759,7 @@ TEST_F(TemplateURLServiceSyncTest, MergeSyncIsTheSame) {
       iter != initial_data.end(); ++iter) {
     model()->Add(Deserialize(*iter));
   }
+  ASSERT_EQ(3U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
   syncer::SyncMergeResult merge_result = MergeAndExpectNotify(initial_data, 0);
 
   EXPECT_EQ(3U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
@@ -784,13 +769,6 @@ TEST_F(TemplateURLServiceSyncTest, MergeSyncIsTheSame) {
     EXPECT_TRUE(model()->GetTemplateURLForGUID(guid));
   }
   EXPECT_EQ(0U, processor()->change_list_size());
-
-  // Locally everything should remain the same.
-  EXPECT_EQ(0, merge_result.num_items_added());
-  EXPECT_EQ(0, merge_result.num_items_modified());
-  EXPECT_EQ(0, merge_result.num_items_deleted());
-  EXPECT_EQ(3, merge_result.num_items_before_association());
-  EXPECT_EQ(3, merge_result.num_items_after_association());
 }
 
 TEST_F(TemplateURLServiceSyncTest, MergeUpdateFromSync) {
@@ -812,6 +790,7 @@ TEST_F(TemplateURLServiceSyncTest, MergeUpdateFromSync) {
   initial_data.push_back(
       TemplateURLService::CreateSyncDataFromTemplateURL(*turl2_older));
 
+  ASSERT_EQ(2U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
   syncer::SyncMergeResult merge_result = MergeAndExpectNotify(initial_data, 1);
 
   // Both were local updates, so we expect the same count.
@@ -827,13 +806,6 @@ TEST_F(TemplateURLServiceSyncTest, MergeUpdateFromSync) {
   syncer::SyncChange change = processor()->change_for_guid("xyz");
   EXPECT_TRUE(change.change_type() == syncer::SyncChange::ACTION_UPDATE);
   EXPECT_EQ("http://xyz.com", GetURL(change.sync_data()));
-
-  // Locally only the older item should have been modified.
-  EXPECT_EQ(0, merge_result.num_items_added());
-  EXPECT_EQ(1, merge_result.num_items_modified());
-  EXPECT_EQ(0, merge_result.num_items_deleted());
-  EXPECT_EQ(2, merge_result.num_items_before_association());
-  EXPECT_EQ(2, merge_result.num_items_after_association());
 }
 
 TEST_F(TemplateURLServiceSyncTest, MergeAddFromOlderSyncData) {
@@ -849,6 +821,7 @@ TEST_F(TemplateURLServiceSyncTest, MergeAddFromOlderSyncData) {
   model()->Add(CreateTestTemplateURL(ASCIIToUTF16("unique"),
                                      "http://unique.com", "ccc"));  // add
 
+  ASSERT_EQ(3U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
   syncer::SyncMergeResult merge_result =
       MergeAndExpectNotify(CreateInitialSyncData(), 1);
 
@@ -895,15 +868,6 @@ TEST_F(TemplateURLServiceSyncTest, MergeAddFromOlderSyncData) {
   ASSERT_TRUE(processor()->contains_guid("ccc"));
   EXPECT_EQ(syncer::SyncChange::ACTION_ADD,
             processor()->change_for_guid("ccc").change_type());
-
-  // All the sync items had new guids, but only one doesn't conflict and is
-  // added. The other two conflicting cases result in local modifications
-  // to override the local guids but preserve the local data.
-  EXPECT_EQ(1, merge_result.num_items_added());
-  EXPECT_EQ(2, merge_result.num_items_modified());
-  EXPECT_EQ(0, merge_result.num_items_deleted());
-  EXPECT_EQ(3, merge_result.num_items_before_association());
-  EXPECT_EQ(4, merge_result.num_items_after_association());
 }
 
 TEST_F(TemplateURLServiceSyncTest, MergeAddFromNewerSyncData) {
@@ -921,6 +885,7 @@ TEST_F(TemplateURLServiceSyncTest, MergeAddFromNewerSyncData) {
                                      "http://unique.com", "ccc", 10, false,
                                      false, 113));  // add
 
+  ASSERT_EQ(3U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
   syncer::SyncMergeResult merge_result =
       MergeAndExpectNotify(CreateInitialSyncData(), 1);
 
@@ -954,15 +919,6 @@ TEST_F(TemplateURLServiceSyncTest, MergeAddFromNewerSyncData) {
   ASSERT_TRUE(processor()->contains_guid("ccc"));
   EXPECT_EQ(syncer::SyncChange::ACTION_ADD,
             processor()->change_for_guid("ccc").change_type());
-
-  // One of the sync items is added directly without conflict. The other two
-  // conflict but are newer than the local items so are added while the local
-  // is deleted.
-  EXPECT_EQ(3, merge_result.num_items_added());
-  EXPECT_EQ(0, merge_result.num_items_modified());
-  EXPECT_EQ(2, merge_result.num_items_deleted());
-  EXPECT_EQ(3, merge_result.num_items_before_association());
-  EXPECT_EQ(4, merge_result.num_items_after_association());
 }
 
 TEST_F(TemplateURLServiceSyncTest, ProcessChangesEmptyModel) {
@@ -2001,8 +1957,10 @@ TEST_F(TemplateURLServiceSyncTest, PreSyncDeletes) {
   model()->pre_sync_deletes_.insert("aaa");
   model()->Add(CreateTestTemplateURL(ASCIIToUTF16("whatever"),
       "http://key1.com", "bbb"));
+  ASSERT_EQ(1U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
   syncer::SyncMergeResult merge_result =
       MergeAndExpectNotify(CreateInitialSyncData(), 1);
+  EXPECT_EQ(2U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
 
   // We expect the model to have GUIDs {bbb, key3} after our initial merge.
   EXPECT_TRUE(model()->GetTemplateURLForGUID("bbb"));
@@ -2016,14 +1974,6 @@ TEST_F(TemplateURLServiceSyncTest, PreSyncDeletes) {
   // The set of pre-sync deletes should be cleared so they're not reused if
   // MergeDataAndStartSyncing gets called again.
   EXPECT_TRUE(model()->pre_sync_deletes_.empty());
-
-  // Those sync items deleted via pre-sync-deletes should not get added. The
-  // remaining sync item (key3) should though.
-  EXPECT_EQ(1, merge_result.num_items_added());
-  EXPECT_EQ(0, merge_result.num_items_modified());
-  EXPECT_EQ(0, merge_result.num_items_deleted());
-  EXPECT_EQ(1, merge_result.num_items_before_association());
-  EXPECT_EQ(2, merge_result.num_items_after_association());
 }
 
 TEST_F(TemplateURLServiceSyncTest, PreSyncUpdates) {
@@ -2072,9 +2022,13 @@ TEST_F(TemplateURLServiceSyncTest, PreSyncUpdates) {
   initial_data.push_back(
       TemplateURLService::CreateSyncDataFromTemplateURL(*sync_turl));
 
+  ASSERT_EQ(prepop_turls.size(),
+            model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
   syncer::SyncMergeResult merge_result = model()->MergeDataAndStartSyncing(
       syncer::SEARCH_ENGINES,
       initial_data, PassProcessor(), CreateAndPassSyncErrorFactory());
+  EXPECT_EQ(prepop_turls.size(),
+            model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
 
   ASSERT_EQ(added_turl, model()->GetTemplateURLForKeyword(
       ASCIIToUTF16(kNewKeyword)));
@@ -2085,15 +2039,6 @@ TEST_F(TemplateURLServiceSyncTest, PreSyncUpdates) {
             change.sync_data().GetSpecifics().search_engine().keyword());
   EXPECT_EQ(new_timestamp, base::Time::FromInternalValue(
       change.sync_data().GetSpecifics().search_engine().last_modified()));
-
-  // All the sync data is old, so nothing should change locally.
-  EXPECT_EQ(0, merge_result.num_items_added());
-  EXPECT_EQ(0, merge_result.num_items_modified());
-  EXPECT_EQ(0, merge_result.num_items_deleted());
-  EXPECT_EQ(static_cast<int>(prepop_turls.size()),
-            merge_result.num_items_before_association());
-  EXPECT_EQ(static_cast<int>(prepop_turls.size()),
-            merge_result.num_items_after_association());
 }
 
 TEST_F(TemplateURLServiceSyncTest, SyncBaseURLs) {
@@ -2153,24 +2098,25 @@ TEST_F(TemplateURLServiceSyncTest, MergeInSyncTemplateURL) {
     ExpectedTemplateURL turl_uniquified;
     ExpectedTemplateURL present_in_model;
     bool keywords_conflict;
-    int merge_results[3];  // in Added, Modified, Deleted order.
+    size_t final_num_turls;
   } test_cases[] = {
-    // Both are synced and the new sync entry is better: Local is uniquified and
-    // UPDATE sent. Sync is added.
-    {SYNC, BOTH, LOCAL, LOCAL, BOTH, true, {1, 1, 0}},
-    // Both are synced and the local entry is better: Sync is uniquified and
-    // added to the model. An UPDATE is sent for it.
-    {LOCAL, BOTH, SYNC, SYNC, BOTH, true, {1, 1, 0}},
-    // Local was not known to Sync and the new sync entry is better: Sync is
-    // added. Local is removed. No updates.
-    {SYNC, SYNC, NEITHER, NEITHER, SYNC, true, {1, 0, 1}},
-    // Local was not known to sync and the local entry is better: Local is
-    // updated with sync GUID, Sync is not added. UPDATE sent for Sync.
-    {LOCAL, SYNC, SYNC, NEITHER, SYNC, true, {0, 1, 0}},
-    // No conflicting keyword. Both should be added with their original
-    // keywords, with no updates sent. Note that MergeDataAndStartSyncing is
-    // responsible for creating the ACTION_ADD for the local TemplateURL.
-    {NEITHER, SYNC, NEITHER, NEITHER, BOTH, false, {1, 0, 0}},
+      // Both are synced and the new sync entry is better: Local is uniquified
+      // and
+      // UPDATE sent. Sync is added.
+      {SYNC, BOTH, LOCAL, LOCAL, BOTH, true, 2},
+      // Both are synced and the local entry is better: Sync is uniquified and
+      // added to the model. An UPDATE is sent for it.
+      {LOCAL, BOTH, SYNC, SYNC, BOTH, true, 2},
+      // Local was not known to Sync and the new sync entry is better: Sync is
+      // added. Local is removed. No updates.
+      {SYNC, SYNC, NEITHER, NEITHER, SYNC, true, 1},
+      // Local was not known to sync and the local entry is better: Local is
+      // updated with sync GUID, Sync is not added. UPDATE sent for Sync.
+      {LOCAL, SYNC, SYNC, NEITHER, SYNC, true, 1},
+      // No conflicting keyword. Both should be added with their original
+      // keywords, with no updates sent. Note that MergeDataAndStartSyncing is
+      // responsible for creating the ACTION_ADD for the local TemplateURL.
+      {NEITHER, SYNC, NEITHER, NEITHER, BOTH, false, 2},
   };
 
   for (size_t i = 0; i < base::size(test_cases); ++i) {
@@ -2220,16 +2166,13 @@ TEST_F(TemplateURLServiceSyncTest, MergeInSyncTemplateURL) {
     syncer::SyncChangeList change_list;
     syncer::SyncMergeResult merge_result(syncer::SEARCH_ENGINES);
     test_util_a_->ResetObserverCount();
+    ASSERT_EQ(1u, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
     model()->MergeInSyncTemplateURL(sync_turl.get(), sync_data, &change_list,
                                     &initial_data, &merge_result);
+    EXPECT_EQ(test_cases[i].final_num_turls,
+              model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
     EXPECT_EQ(1, test_util_a_->GetObserverCount());
 
-    // Verify the merge results were set appropriately.
-    EXPECT_EQ(test_cases[i].merge_results[0], merge_result.num_items_added());
-    EXPECT_EQ(test_cases[i].merge_results[1],
-              merge_result.num_items_modified());
-    EXPECT_EQ(test_cases[i].merge_results[2], merge_result.num_items_deleted());
-
     // Check for expected updates, if any.
     std::string expected_update_guid;
     if (test_cases[i].update_sent == LOCAL)

chrome/browser/supervised_user/supervised_user_settings_service_unittest.cc

@@ -204,11 +204,9 @@ TEST_F(SupervisedUserSettingsServiceTest, ProcessSplitSetting) {
 
 TEST_F(SupervisedUserSettingsServiceTest, Merge) {
   syncer::SyncMergeResult result = StartSyncing(syncer::SyncDataList());
-  EXPECT_EQ(0, result.num_items_before_association());
-  EXPECT_EQ(0, result.num_items_added());
-  EXPECT_EQ(0, result.num_items_modified());
-  EXPECT_EQ(0, result.num_items_deleted());
-  EXPECT_EQ(0, result.num_items_after_association());
+  EXPECT_TRUE(settings_service_
+                  .GetAllSyncDataForTesting(syncer::SUPERVISED_USER_SETTINGS)
+                  .empty());
 
   ASSERT_TRUE(settings_);
   const base::Value* value = nullptr;
@@ -234,11 +232,10 @@ TEST_F(SupervisedUserSettingsServiceTest, Merge) {
               it.value()));
     }
     result = StartSyncing(sync_data);
-    EXPECT_EQ(0, result.num_items_before_association());
-    EXPECT_EQ(3, result.num_items_added());
-    EXPECT_EQ(0, result.num_items_modified());
-    EXPECT_EQ(0, result.num_items_deleted());
-    EXPECT_EQ(3, result.num_items_after_association());
+    EXPECT_EQ(3u,
+              settings_service_
+                  .GetAllSyncDataForTesting(syncer::SUPERVISED_USER_SETTINGS)
+                  .size());
     settings_service_.StopSyncing(syncer::SUPERVISED_USER_SETTINGS);
   }
 
@@ -264,11 +261,10 @@ TEST_F(SupervisedUserSettingsServiceTest, Merge) {
               it.value()));
     }
     result = StartSyncing(sync_data);
-    EXPECT_EQ(6, result.num_items_before_association());
-    EXPECT_EQ(0, result.num_items_added());
-    EXPECT_EQ(1, result.num_items_modified());
-    EXPECT_EQ(2, result.num_items_deleted());
-    EXPECT_EQ(4, result.num_items_after_association());
+    EXPECT_EQ(4u,
+              settings_service_
+                  .GetAllSyncDataForTesting(syncer::SUPERVISED_USER_SETTINGS)
+                  .size());
   }
 }
 

chrome/browser/supervised_user/supervised_user_whitelist_service.cc

@@ -213,6 +213,29 @@ void SupervisedUserWhitelistService::StopSyncing(syncer::ModelType type) {
   DCHECK_EQ(syncer::SUPERVISED_USER_WHITELISTS, type);
 }
 
+syncer::SyncDataList SupervisedUserWhitelistService::GetAllSyncDataForTesting(
+    syncer::ModelType type) const {
+  syncer::SyncDataList sync_data;
+  const base::DictionaryValue* whitelists =
+      prefs_->GetDictionary(prefs::kSupervisedUserWhitelists);
+  for (base::DictionaryValue::Iterator it(*whitelists); !it.IsAtEnd();
+       it.Advance()) {
+    const std::string& id = it.key();
+    const base::DictionaryValue* dict = nullptr;
+    it.value().GetAsDictionary(&dict);
+    std::string name;
+    bool result = dict->GetString(kName, &name);
+    DCHECK(result);
+    sync_pb::EntitySpecifics specifics;
+    sync_pb::ManagedUserWhitelistSpecifics* whitelist =
+        specifics.mutable_managed_user_whitelist();
+    whitelist->set_id(id);
+    whitelist->set_name(name);
+    sync_data.push_back(syncer::SyncData::CreateLocalData(id, name, specifics));
+  }
+  return sync_data;
+}
+
 syncer::SyncError SupervisedUserWhitelistService::ProcessSyncChanges(
     const base::Location& from_here,
     const syncer::SyncChangeList& change_list) {

chrome/browser/supervised_user/supervised_user_whitelist_service.h

@@ -89,6 +89,8 @@ class SupervisedUserWhitelistService : public syncer::SyncableService {
       const base::Location& from_here,
       const syncer::SyncChangeList& change_list) override;
 
+  syncer::SyncDataList GetAllSyncDataForTesting(syncer::ModelType type) const;
+
  private:
   // The following methods handle whitelist additions, updates and removals,
   // usually coming from Sync.

chrome/browser/supervised_user/supervised_user_whitelist_service_unittest.cc

@@ -169,16 +169,17 @@ class SupervisedUserWhitelistServiceTest : public testing::Test {
 TEST_F(SupervisedUserWhitelistServiceTest, MergeEmpty) {
   service_->Init();
 
+  ASSERT_TRUE(
+      service_->GetAllSyncDataForTesting(syncer::SUPERVISED_USER_WHITELISTS)
+          .empty());
   syncer::SyncMergeResult result = service_->MergeDataAndStartSyncing(
       syncer::SUPERVISED_USER_WHITELISTS, syncer::SyncDataList(),
       std::unique_ptr<syncer::SyncChangeProcessor>(),
       std::unique_ptr<syncer::SyncErrorFactory>());
+  EXPECT_TRUE(
+      service_->GetAllSyncDataForTesting(syncer::SUPERVISED_USER_WHITELISTS)
+          .empty());
   EXPECT_FALSE(result.error().IsSet());
-  EXPECT_EQ(0, result.num_items_added());
-  EXPECT_EQ(0, result.num_items_modified());
-  EXPECT_EQ(0, result.num_items_deleted());
-  EXPECT_EQ(0, result.num_items_before_association());
-  EXPECT_EQ(0, result.num_items_after_association());
 
   EXPECT_EQ(0u, installer_->registered_whitelists().size());
 }
@@ -214,16 +215,17 @@ TEST_F(SupervisedUserWhitelistServiceTest, MergeExisting) {
   initial_data.push_back(
       SupervisedUserWhitelistService::CreateWhitelistSyncData(
           "cccc", "Whitelist C"));
+  ASSERT_EQ(
+      2u, service_->GetAllSyncDataForTesting(syncer::SUPERVISED_USER_WHITELISTS)
+              .size());
   syncer::SyncMergeResult result = service_->MergeDataAndStartSyncing(
       syncer::SUPERVISED_USER_WHITELISTS, initial_data,
       std::unique_ptr<syncer::SyncChangeProcessor>(),
       std::unique_ptr<syncer::SyncErrorFactory>());
+  EXPECT_EQ(
+      2u, service_->GetAllSyncDataForTesting(syncer::SUPERVISED_USER_WHITELISTS)
+              .size());
   EXPECT_FALSE(result.error().IsSet());
-  EXPECT_EQ(1, result.num_items_added());
-  EXPECT_EQ(1, result.num_items_modified());
-  EXPECT_EQ(1, result.num_items_deleted());
-  EXPECT_EQ(2, result.num_items_before_association());
-  EXPECT_EQ(2, result.num_items_after_association());
 
   // Whitelist A (which was previously ready) should be removed now, and
   // whitelist B was never ready.