Deflake and reenable bookmark sync tests

GUID-related tests have been flaky since introduction and recent
investigations suggest that the underlying issue is FakeServer's lack of
sending sync invalidations for certain APIs like InjectEntity().

This patch modifies FakeServer to actually issue invalidations and
removes some now redundant (and undesired) calls to
TriggerSyncForModelTypes().

Assumes this fixes the flakiness, all tests are reenabled.

Bug: 1014086
Change-Id: Ie0f597c024646586b9ec6c3dfbb761a86dadaad2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1879293Reviewed-by: Maksim Moskvitin <mmoskvitin@google.com>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#710271}

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

@@ -439,9 +439,6 @@ IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest, DownloadDeletedBookmark) {
       syncer::PersistentTombstoneEntity::CreateNew(entity_id, std::string()));
   GetFakeServer()->InjectEntity(std::move(tombstone));
 
-  const syncer::ModelTypeSet kBookmarksType(syncer::BOOKMARKS);
-  TriggerSyncForModelTypes(kSingleProfileIndex, kBookmarksType);
-
   const int kExpectedCountAfterDeletion = 0;
   ASSERT_TRUE(BookmarksTitleChecker(kSingleProfileIndex, title,
                                     kExpectedCountAfterDeletion)
@@ -478,9 +475,6 @@ IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest,
   bookmark_specifics->set_url(updated_url.spec());
   ASSERT_TRUE(GetFakeServer()->ModifyEntitySpecifics(entity_id, specifics));
 
-  const syncer::ModelTypeSet kBookmarksType(syncer::BOOKMARKS);
-  TriggerSyncForModelTypes(kSingleProfileIndex, kBookmarksType);
-
   ASSERT_TRUE(BookmarksUrlChecker(kSingleProfileIndex, updated_url, 1).Wait());
   ASSERT_EQ(0u,
             CountBookmarksWithUrlsMatching(kSingleProfileIndex, original_url));
@@ -788,10 +782,8 @@ IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest,
                                          /*REMOTE_INITIAL_UPDATE=*/5));
 }
 
-// TODO(crbug.com/1014086): re-enable this test on all builders once flakiness
-// is addressed.
 IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest,
-                       DISABLED_ApplyRemoteCreationWithValidGUID) {
+                       ApplyRemoteCreationWithValidGUID) {
   // Start syncing.
   DisableVerifier();
   ASSERT_TRUE(SetupSync());
@@ -818,10 +810,8 @@ IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest,
       GetBookmarkBarNode(kSingleProfileIndex)->children()[0].get()->guid());
 }
 
-// TODO(crbug.com/1014086): re-enable this test on all builders once flakiness
-// is addressed.
 IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest,
-                       DISABLED_ApplyRemoteCreationWithoutValidGUID) {
+                       ApplyRemoteCreationWithoutValidGUID) {
   // Start syncing.
   DisableVerifier();
   ASSERT_TRUE(SetupSync());
@@ -854,10 +844,8 @@ IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest,
       GetBookmarkBarNode(kSingleProfileIndex)->children()[0].get()->guid());
 }
 
-// TODO(crbug.com/1014086): re-enable this test on all builders once flakiness
-// is addressed.
 IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest,
-                       DISABLED_ApplyRemoteCreationWithoutValidGUIDOrOCII) {
+                       ApplyRemoteCreationWithoutValidGUIDOrOCII) {
   // Start syncing.
   DisableVerifier();
   ASSERT_TRUE(SetupSync());
@@ -974,10 +962,8 @@ IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest,
                                             originator_client_item_id));
 }
 
-// TODO(crbug.com/1014086): re-enable this test on all builders once flakiness
-// is addressed.
 IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest,
-                       DISABLED_ApplyRemoteUpdateWithValidGUID) {
+                       ApplyRemoteUpdateWithValidGUID) {
   // Create a bookmark.
   const GURL url = GURL("https://foo.com");
   fake_server::EntityBuilderFactory entity_builder_factory;

components/sync/test/fake_server/fake_server.cc

@@ -340,7 +340,14 @@ void FakeServer::InjectEntity(std::unique_ptr<LoopbackServerEntity> entity) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(entity->GetModelType() != syncer::AUTOFILL_WALLET_DATA)
       << "Wallet data must be injected via SetWalletData()";
+
+  const ModelType model_type = entity->GetModelType();
+
   loopback_server_->SaveEntity(std::move(entity));
+
+  // Notify observers so invalidations are mimic-ed.
+  OnCommit(/*committer_id=*/std::string(),
+           /*committed_model_types=*/{model_type});
 }
 
 void FakeServer::SetWalletData(
@@ -352,21 +359,42 @@ void FakeServer::SetWalletData(
         << "The sync server doesn not provide a client tag for wallet entries";
     DCHECK(!entity.id_string().empty()) << "server id required!";
   }
+
   wallet_entities_ = wallet_entities;
+
+  // TODO(crbug.com/1019108): This function should issue OnCommit() calls to
+  // observers in order to mimic invalidations, such that tests don't need to
+  // call TriggerSyncForModelTypes().
 }
 
 bool FakeServer::ModifyEntitySpecifics(
     const std::string& id,
     const sync_pb::EntitySpecifics& updated_specifics) {
-  return loopback_server_->ModifyEntitySpecifics(id, updated_specifics);
+  if (!loopback_server_->ModifyEntitySpecifics(id, updated_specifics)) {
+    return false;
+  }
+
+  // Notify observers so invalidations are mimic-ed.
+  OnCommit(/*committer_id=*/std::string(), /*committed_model_types=*/{
+               GetModelTypeFromSpecifics(updated_specifics)});
+
+  return true;
 }
 
 bool FakeServer::ModifyBookmarkEntity(
     const std::string& id,
     const std::string& parent_id,
     const sync_pb::EntitySpecifics& updated_specifics) {
-  return loopback_server_->ModifyBookmarkEntity(id, parent_id,
-                                                updated_specifics);
+  if (!loopback_server_->ModifyBookmarkEntity(id, parent_id,
+                                              updated_specifics)) {
+    return false;
+  }
+
+  // Notify observers so invalidations are mimic-ed.
+  OnCommit(/*committer_id=*/std::string(),
+           /*committed_model_types=*/{syncer::BOOKMARKS});
+
+  return true;
 }
 
 void FakeServer::ClearServerData() {