[Sync] Fix bookmarks reupload integration tests

Some tests related to bookmarks reupload are flaky because there is used
UpdatedProgressMarkerChecker. It doesn't guarantee that all current
changes are committed.

The better option is to wait for the actual expected data on the server
rather than for the next commit message.

For bookmarks reupload there are two possible titles in specifics:
legacy canonicalized title and full title. This CL extends the
ServerBookmarksEqualityChecker to wait until both of them are equal to
the expected title.

Bug: 1135271
Change-Id: I0a80949e30b085d94e00a82ddd8d79db39309891
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2462268Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Cr-Commit-Position: refs/heads/master@{#816154}

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

@@ -1248,12 +1248,12 @@ bool BookmarkFaviconLoadedChecker::IsExitConditionSatisfied(std::ostream* os) {
 ServerBookmarksEqualityChecker::ServerBookmarksEqualityChecker(
     syncer::ProfileSyncService* service,
     fake_server::FakeServer* fake_server,
-    const std::vector<ExpectedBookmark>& expected_bookmarks,
+    std::vector<ExpectedBookmark> expected_bookmarks,
     syncer::Cryptographer* cryptographer)
     : SingleClientStatusChangeChecker(service),
       fake_server_(fake_server),
       cryptographer_(cryptographer),
-      expected_bookmarks_(expected_bookmarks) {}
+      expected_bookmarks_(std::move(expected_bookmarks)) {}
 
 bool ServerBookmarksEqualityChecker::IsExitConditionSatisfied(
     std::ostream* os) {
@@ -1293,6 +1293,7 @@ bool ServerBookmarksEqualityChecker::IsExitConditionSatisfied(
                      [actual_specifics](const ExpectedBookmark& bookmark) {
                        return actual_specifics.legacy_canonicalized_title() ==
                                   bookmark.title &&
+                              actual_specifics.full_title() == bookmark.title &&
                               actual_specifics.url() == bookmark.url;
                      });
     if (it != expected.end()) {

chrome/browser/sync/test/integration/bookmarks_helper.h

@@ -443,10 +443,11 @@ class BookmarkFaviconLoadedChecker
 };
 
 // Checker used to block until the bookmarks on the server match a given set of
-// expected bookmarks.
+// expected bookmarks. The |title| is comapred to both legacy and full titles.
 class ServerBookmarksEqualityChecker : public SingleClientStatusChangeChecker {
  public:
   struct ExpectedBookmark {
+    // Used to check both legacy and full titles in specifics.
     std::string title;
     GURL url;
   };
@@ -454,10 +455,11 @@ class ServerBookmarksEqualityChecker : public SingleClientStatusChangeChecker {
   // If a |cryptographer| is provided (i.e. is not nullptr), it is assumed that
   // the server-side data should be encrypted, and the provided cryptographer
   // will be used to decrypt the data prior to checking for equality.
+  // |fake_server| must not be nullptr and must outlive this object.
   ServerBookmarksEqualityChecker(
       syncer::ProfileSyncService* service,
       fake_server::FakeServer* fake_server,
-      const std::vector<ExpectedBookmark>& expected_bookmarks,
+      std::vector<ExpectedBookmark> expected_bookmarks,
       syncer::Cryptographer* cryptographer);
 
   bool IsExitConditionSatisfied(std::ostream* os) override;

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

@@ -71,6 +71,10 @@ const int kSingleProfileIndex = 0;
 // fake server across PRE_MyTest and MyTest.
 const char kBookmarkGuid[] = "e397ed62-9532-4dbf-ae55-200236eba15c";
 
+// A title and a URL which are used across PRE_MyTest and MyTest.
+const char kBookmarkTitle[] = "Title";
+const char kBookmarkPageUrl[] = "http://www.foo.com/";
+
 class SingleClientBookmarksSyncTest : public SyncTest {
  public:
   SingleClientBookmarksSyncTest() : SyncTest(SINGLE_CLIENT) {}
@@ -1238,7 +1242,6 @@ IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest,
 IN_PROC_BROWSER_TEST_F(
     SingleClientBookmarksSyncTestWithDisabledReuploadBookmarks,
     PRE_ShouldNotReploadUponFaviconLoad) {
-  const GURL url = GURL("http://www.foo.com");
   fake_server::EntityBuilderFactory entity_builder_factory;
   fake_server::BookmarkEntityBuilder bookmark_builder =
       entity_builder_factory.NewBookmarkEntityBuilder("Foo Title");
@@ -1247,14 +1250,17 @@ IN_PROC_BROWSER_TEST_F(
   // that it's a legacy bookmark means any locally-produced specifics would be
   // different for this bookmark (new fields like GUID would be populated).
   std::unique_ptr<syncer::LoopbackServerEntity> bookmark =
-      bookmark_builder.BuildBookmark(url, /*is_legacy=*/true);
+      bookmark_builder.BuildBookmark(GURL(kBookmarkPageUrl),
+                                     /*is_legacy=*/true);
   ASSERT_TRUE(bookmark.get()->GetSpecifics().bookmark().guid().empty());
   fake_server_->InjectEntity(std::move(bookmark));
 
   // Start syncing.
   DisableVerifier();
   ASSERT_TRUE(SetupSync());
-  ASSERT_TRUE(BookmarksUrlChecker(kSingleProfileIndex, url, 1).Wait());
+  ASSERT_TRUE(
+      BookmarksUrlChecker(kSingleProfileIndex, GURL(kBookmarkPageUrl), 1)
+          .Wait());
 }
 
 IN_PROC_BROWSER_TEST_F(
@@ -1264,10 +1270,10 @@ IN_PROC_BROWSER_TEST_F(
 
   const std::string title = "Title";
   const std::string new_title = "New Title";
-  const GURL page_url = GURL("http://www.foo.com");
   const GURL icon_url("http://www.google.com/favicon.ico");
 
-  const BookmarkNode* bookmark = AddURL(kSingleProfileIndex, title, page_url);
+  const BookmarkNode* bookmark =
+      AddURL(kSingleProfileIndex, title, GURL(kBookmarkPageUrl));
   SetFavicon(0, bookmark, icon_url, CreateFavicon(SK_ColorWHITE),
              bookmarks_helper::FROM_UI);
 
@@ -1275,7 +1281,8 @@ IN_PROC_BROWSER_TEST_F(
       UpdatedProgressMarkerChecker(GetSyncService(kSingleProfileIndex)).Wait());
   ASSERT_TRUE(bookmarks_helper::ServerBookmarksEqualityChecker(
                   GetSyncService(kSingleProfileIndex), GetFakeServer(),
-                  {{title, page_url}}, /*cryptographer=*/nullptr)
+                  {{title, GURL(kBookmarkPageUrl)}},
+                  /*cryptographer=*/nullptr)
                   .Wait());
 
   // Stop Sync and update local entity to enter in unsynced state.
@@ -1324,7 +1331,8 @@ IN_PROC_BROWSER_TEST_F(
   ASSERT_TRUE(GetClient(kSingleProfileIndex)->AwaitEngineInitialization());
   ASSERT_TRUE(bookmarks_helper::ServerBookmarksEqualityChecker(
                   GetSyncService(kSingleProfileIndex), GetFakeServer(),
-                  {{new_title, url}}, /*cryptographer=*/nullptr)
+                  {{new_title, url}},
+                  /*cryptographer=*/nullptr)
                   .Wait());
 
   // Last commit should initiate favicon loading.
@@ -1392,15 +1400,11 @@ IN_PROC_BROWSER_TEST_F(
   fake_server_->InjectEntity(std::move(remote_folder));
 
   ASSERT_TRUE(SetupSync());
-  ASSERT_TRUE(
-      UpdatedProgressMarkerChecker(GetSyncService(kSingleProfileIndex)).Wait());
-
-  const std::vector<sync_pb::SyncEntity> server_bookmarks =
-      GetFakeServer()->GetSyncEntitiesByModelType(syncer::BOOKMARKS);
-  ASSERT_EQ(1u, CountFoldersWithTitlesMatching(kSingleProfileIndex, title));
-  ASSERT_EQ(1u, server_bookmarks.size());
-
-  EXPECT_TRUE(server_bookmarks.front().specifics().bookmark().has_full_title());
+  ASSERT_TRUE(bookmarks_helper::ServerBookmarksEqualityChecker(
+                  GetSyncService(kSingleProfileIndex), GetFakeServer(),
+                  {{title, /*url=*/GURL()}},
+                  /*cryptographer=*/nullptr)
+                  .Wait());
 }
 
 // This test looks similar to
@@ -1414,22 +1418,16 @@ IN_PROC_BROWSER_TEST_F(
   // locally.
   ASSERT_TRUE(SetupSync());
 
-  const std::string title = "Title";
-  const GURL url = GURL("http://www.foo.com");
-
   // Make an incremental remote creation of bookmark without full_title.
   fake_server::EntityBuilderFactory entity_builder_factory;
   fake_server::BookmarkEntityBuilder bookmark_builder =
-      entity_builder_factory.NewBookmarkEntityBuilder(title);
+      entity_builder_factory.NewBookmarkEntityBuilder(kBookmarkTitle);
   std::unique_ptr<syncer::LoopbackServerEntity> remote_folder =
-      bookmark_builder.BuildBookmark(url, /*is_legacy=*/false);
-  const std::string new_guid = remote_folder->GetSpecifics().bookmark().guid();
+      bookmark_builder.BuildBookmarkWithoutFullTitle(GURL(kBookmarkPageUrl));
 
-  // Makr sure that server-side specifics doesn't have full title.
-  remote_folder->GetSpecifics().mutable_bookmark()->clear_full_title();
   fake_server_->InjectEntity(std::move(remote_folder));
 
-  ASSERT_TRUE(BookmarksTitleChecker(kSingleProfileIndex, title,
+  ASSERT_TRUE(BookmarksTitleChecker(kSingleProfileIndex, kBookmarkTitle,
                                     /*expected_count=*/1)
                   .Wait());
 }
@@ -1439,16 +1437,15 @@ IN_PROC_BROWSER_TEST_F(
     ShouldReuploadFullTitleForOldClients) {
   // This test checks that the legacy bookmark which was stored locally will
   // imply reupload to the server when reupload feature is enabled.
-  const GURL url = GURL("http://www.foo.com");
-
   ASSERT_EQ(
       1u,
       GetFakeServer()->GetSyncEntitiesByModelType(syncer::BOOKMARKS).size());
-  const int64_t old_server_version =
-      GetFakeServer()
-          ->GetSyncEntitiesByModelType(syncer::BOOKMARKS)
-          .front()
-          .version();
+  ASSERT_FALSE(GetFakeServer()
+                   ->GetSyncEntitiesByModelType(syncer::BOOKMARKS)
+                   .front()
+                   .specifics()
+                   .bookmark()
+                   .has_full_title());
   ASSERT_TRUE(SetupClients());
 #if defined(OS_CHROMEOS)
   // signin::SetRefreshTokenForPrimaryAccount() is needed on ChromeOS in order
@@ -1456,20 +1453,14 @@ IN_PROC_BROWSER_TEST_F(
   GetClient(0)->SignInPrimaryAccount();
 #endif  // defined(OS_CHROMEOS)
   ASSERT_TRUE(GetClient(kSingleProfileIndex)->AwaitEngineInitialization());
-  ASSERT_TRUE(BookmarkFaviconLoadedChecker(kSingleProfileIndex, url).Wait());
   ASSERT_TRUE(
-      UpdatedProgressMarkerChecker(GetSyncService(kSingleProfileIndex)).Wait());
-  ASSERT_GT(GetFakeServer()
-                ->GetSyncEntitiesByModelType(syncer::BOOKMARKS)
-                .front()
-                .version(),
-            old_server_version);
-
-  const std::string title = "Title";
-  const std::vector<sync_pb::SyncEntity> server_bookmarks =
-      GetFakeServer()->GetSyncEntitiesByModelType(syncer::BOOKMARKS);
-  ASSERT_EQ(1u, server_bookmarks.size());
-  EXPECT_TRUE(server_bookmarks.front().specifics().bookmark().has_full_title());
+      BookmarkFaviconLoadedChecker(kSingleProfileIndex, GURL(kBookmarkPageUrl))
+          .Wait());
+  ASSERT_TRUE(bookmarks_helper::ServerBookmarksEqualityChecker(
+                  GetSyncService(kSingleProfileIndex), GetFakeServer(),
+                  {{kBookmarkTitle, GURL(kBookmarkPageUrl)}},
+                  /*cryptographer=*/nullptr)
+                  .Wait());
 }
 
 // TODO(rushans): add the same test as before with favicons.
@@ -1479,28 +1470,18 @@ IN_PROC_BROWSER_TEST_F(
     PRE_ShouldReuploadFullTitleAfterRestartOnIncrementalChange) {
   ASSERT_TRUE(SetupSync());
 
-  const std::string title = "Title";
-  const GURL url = GURL("http://www.foo.com");
-
   // Make an incremental remote creation of bookmark.
   fake_server::EntityBuilderFactory entity_builder_factory;
   fake_server::BookmarkEntityBuilder bookmark_builder =
-      entity_builder_factory.NewBookmarkEntityBuilder(title);
+      entity_builder_factory.NewBookmarkEntityBuilder(kBookmarkTitle);
   std::unique_ptr<syncer::LoopbackServerEntity> remote_folder =
-      bookmark_builder.BuildBookmarkWithoutFullTitle(url);
+      bookmark_builder.BuildBookmarkWithoutFullTitle(GURL(kBookmarkPageUrl));
   ASSERT_FALSE(remote_folder->GetSpecifics().bookmark().has_full_title());
   fake_server_->InjectEntity(std::move(remote_folder));
 
-  ASSERT_TRUE(BookmarksTitleChecker(kSingleProfileIndex, title,
+  ASSERT_TRUE(BookmarksTitleChecker(kSingleProfileIndex, kBookmarkTitle,
                                     /*expected_count=*/1)
                   .Wait());
-
-  // Check that the full title was not uploaded to the server yet.
-  const std::vector<sync_pb::SyncEntity> server_bookmarks =
-      GetFakeServer()->GetSyncEntitiesByModelType(syncer::BOOKMARKS);
-  ASSERT_EQ(1u, server_bookmarks.size());
-  EXPECT_FALSE(
-      server_bookmarks.front().specifics().bookmark().has_full_title());
 }
 
 IN_PROC_BROWSER_TEST_F(
@@ -1508,7 +1489,16 @@ IN_PROC_BROWSER_TEST_F(
     ShouldReuploadFullTitleAfterRestartOnIncrementalChange) {
   ASSERT_TRUE(SetupClients());
 
-  const GURL url = GURL("http://www.foo.com");
+  // Check that the full title was not uploaded to the server yet.
+  ASSERT_EQ(
+      1u,
+      GetFakeServer()->GetSyncEntitiesByModelType(syncer::BOOKMARKS).size());
+  ASSERT_FALSE(GetFakeServer()
+                   ->GetSyncEntitiesByModelType(syncer::BOOKMARKS)
+                   .front()
+                   .specifics()
+                   .bookmark()
+                   .has_full_title());
 
 #if defined(OS_CHROMEOS)
   // signin::SetRefreshTokenForPrimaryAccount() is needed on ChromeOS in order
@@ -1516,15 +1506,14 @@ IN_PROC_BROWSER_TEST_F(
   GetClient(0)->SignInPrimaryAccount();
 #endif  // defined(OS_CHROMEOS)
   ASSERT_TRUE(GetClient(kSingleProfileIndex)->AwaitEngineInitialization());
-  ASSERT_TRUE(BookmarkFaviconLoadedChecker(kSingleProfileIndex, url).Wait());
   ASSERT_TRUE(
-      UpdatedProgressMarkerChecker(GetSyncService(kSingleProfileIndex)).Wait());
-
-  const std::string title = "Title";
-  const std::vector<sync_pb::SyncEntity> server_bookmarks =
-      GetFakeServer()->GetSyncEntitiesByModelType(syncer::BOOKMARKS);
-  ASSERT_EQ(1u, server_bookmarks.size());
-  EXPECT_TRUE(server_bookmarks.front().specifics().bookmark().has_full_title());
+      BookmarkFaviconLoadedChecker(kSingleProfileIndex, GURL(kBookmarkPageUrl))
+          .Wait());
+  ASSERT_TRUE(bookmarks_helper::ServerBookmarksEqualityChecker(
+                  GetSyncService(kSingleProfileIndex), GetFakeServer(),
+                  {{kBookmarkTitle, GURL(kBookmarkPageUrl)}},
+                  /*cryptographer=*/nullptr)
+                  .Wait());
 }
 
 }  // namespace

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

@@ -201,18 +201,20 @@ class SingleClientCustomPassphraseUseScryptSyncTest
 
 IN_PROC_BROWSER_TEST_F(SingleClientCustomPassphraseSyncTest,
                        CommitsEncryptedData) {
+  const std::string title1 = "Hello world";
+  const std::string title2 = "Bookmark #2";
+  const GURL page_url1("https://google.com/");
+  const GURL page_url2("https://example.com/");
+
   SetEncryptionPassphraseForClient(/*index=*/0, "hunter2");
   ASSERT_TRUE(SetupSync());
 
-  ASSERT_TRUE(
-      AddURL(/*profile=*/0, "Hello world", GURL("https://google.com/")));
-  ASSERT_TRUE(
-      AddURL(/*profile=*/0, "Bookmark #2", GURL("https://example.com/")));
+  ASSERT_TRUE(AddURL(/*profile=*/0, title1, page_url1));
+  ASSERT_TRUE(AddURL(/*profile=*/0, title2, page_url2));
   ASSERT_TRUE(WaitForNigori(PassphraseType::kCustomPassphrase));
 
   EXPECT_TRUE(WaitForEncryptedServerBookmarks(
-      {{"Hello world", GURL("https://google.com/")},
-       {"Bookmark #2", GURL("https://example.com/")}},
+      {{title1, page_url1}, {title2, page_url2}},
       /*passphrase=*/"hunter2"));
 }
 
@@ -261,37 +263,39 @@ IN_PROC_BROWSER_TEST_F(SingleClientCustomPassphraseSyncTest,
 
 IN_PROC_BROWSER_TEST_F(SingleClientCustomPassphraseDoNotUseScryptSyncTest,
                        CommitsEncryptedDataUsingPbkdf2WhenScryptDisabled) {
+  const std::string title = "PBKDF2 encrypted";
+  const GURL page_url("https://google.com/pbkdf2-encrypted");
+
   SetEncryptionPassphraseForClient(/*index=*/0, "hunter2");
   ASSERT_TRUE(SetupSync());
-  ASSERT_TRUE(AddURL(/*profile=*/0, "PBKDF2 encrypted",
-                     GURL("https://google.com/pbkdf2-encrypted")));
+  ASSERT_TRUE(AddURL(/*profile=*/0, title, page_url));
 
   ASSERT_TRUE(WaitForNigori(PassphraseType::kCustomPassphrase));
   NigoriSpecifics nigori;
   EXPECT_TRUE(GetServerNigori(GetFakeServer(), &nigori));
   EXPECT_EQ(nigori.custom_passphrase_key_derivation_method(),
             sync_pb::NigoriSpecifics::PBKDF2_HMAC_SHA1_1003);
-  EXPECT_TRUE(WaitForEncryptedServerBookmarks(
-      {{"PBKDF2 encrypted", GURL("https://google.com/pbkdf2-encrypted")}},
-      /*passphrase=*/"hunter2"));
+  EXPECT_TRUE(WaitForEncryptedServerBookmarks({{title, page_url}},
+                                              /*passphrase=*/"hunter2"));
 }
 
 IN_PROC_BROWSER_TEST_F(SingleClientCustomPassphraseUseScryptSyncTest,
                        CommitsEncryptedDataUsingScryptWhenScryptEnabled) {
+  const std::string title = "scrypt encrypted";
+  const GURL page_url("https://google.com/scrypt-encrypted");
+
   SetEncryptionPassphraseForClient(/*index=*/0, "hunter2");
   ASSERT_TRUE(SetupSync());
 
-  ASSERT_TRUE(AddURL(/*profile=*/0, "scrypt encrypted",
-                     GURL("https://google.com/scrypt-encrypted")));
+  ASSERT_TRUE(AddURL(/*profile=*/0, title, page_url));
 
   ASSERT_TRUE(WaitForNigori(PassphraseType::kCustomPassphrase));
   NigoriSpecifics nigori;
   EXPECT_TRUE(GetServerNigori(GetFakeServer(), &nigori));
   EXPECT_EQ(nigori.custom_passphrase_key_derivation_method(),
             sync_pb::NigoriSpecifics::SCRYPT_8192_8_11);
-  EXPECT_TRUE(WaitForEncryptedServerBookmarks(
-      {{"scrypt encrypted", GURL("https://google.com/scrypt-encrypted")}},
-      /*passphrase=*/"hunter2"));
+  EXPECT_TRUE(WaitForEncryptedServerBookmarks({{title, page_url}},
+                                              /*passphrase=*/"hunter2"));
 }
 
 IN_PROC_BROWSER_TEST_F(SingleClientCustomPassphraseDoNotUseScryptSyncTest,
@@ -313,12 +317,13 @@ IN_PROC_BROWSER_TEST_F(SingleClientCustomPassphraseDoNotUseScryptSyncTest,
 
 IN_PROC_BROWSER_TEST_F(SingleClientCustomPassphraseDoNotUseScryptSyncTest,
                        DoesNotLeakUnencryptedData) {
+  const std::string title = "Should be encrypted";
+  const GURL page_url("https://google.com/encrypted");
   SetEncryptionPassphraseForClient(/*index=*/0, "hunter2");
   ASSERT_TRUE(SetupClients());
 
   // Create local bookmarks before sync is enabled.
-  ASSERT_TRUE(AddURL(/*profile=*/0, "Should be encrypted",
-                     GURL("https://google.com/encrypted")));
+  ASSERT_TRUE(AddURL(/*profile=*/0, title, page_url));
 
   CommittedBookmarkEntityNameObserver observer(GetFakeServer());
   ASSERT_TRUE(SetupSync());
@@ -332,19 +337,20 @@ IN_PROC_BROWSER_TEST_F(SingleClientCustomPassphraseDoNotUseScryptSyncTest,
   // once, we are certain that no bookmarks other than those we've verified to
   // be encrypted have been committed.
   EXPECT_TRUE(WaitForEncryptedServerBookmarks(
-      {{"Should be encrypted", GURL("https://google.com/encrypted")}},
+      {{title, page_url}},
       {KeyDerivationParams::CreateForPbkdf2(), "hunter2"}));
   EXPECT_THAT(observer.GetCommittedEntityNames(), ElementsAre("encrypted"));
 }
 
 IN_PROC_BROWSER_TEST_F(SingleClientCustomPassphraseDoNotUseScryptSyncTest,
                        ReencryptsDataWhenPassphraseIsSet) {
+  const std::string title = "Re-encryption is great";
+  const GURL page_url("https://google.com/re-encrypted");
   ASSERT_TRUE(SetupSync());
   ASSERT_TRUE(WaitForNigori(PassphraseType::kKeystorePassphrase));
-  ASSERT_TRUE(AddURL(/*profile=*/0, "Re-encryption is great",
-                     GURL("https://google.com/re-encrypted")));
-  std::vector<ServerBookmarksEqualityChecker::ExpectedBookmark> expected = {
-      {"Re-encryption is great", GURL("https://google.com/re-encrypted")}};
+  ASSERT_TRUE(AddURL(/*profile=*/0, title, page_url));
+  const std::vector<ServerBookmarksEqualityChecker::ExpectedBookmark> expected =
+      {{title, page_url}};
   ASSERT_TRUE(WaitForUnencryptedServerBookmarks(expected));
 
   GetSyncService()->GetUserSettings()->SetEncryptionPassphrase("hunter2");