[Sync tests] Avoid usages of verifier profile in dictionary tests

Verifier profile is a bad way to specify tests expectations and its
usages should be avoided. This CL replaces all usages in dictionary
tests with actual checks of dictionary state.

Side change: AddDifferentToEach test is removed, because it was a subset
of Sanity test.

Bug: 1033848
Change-Id: Icd6a3aa25bafd7bd375c0c7daa45cf3cb413fbef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2465906
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Commit-Queue: Marc Treib <treib@chromium.org>
Auto-Submit: Maksim Moskvitin <mmoskvitin@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#816183}

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

@@ -46,11 +46,6 @@ SpellcheckCustomDictionary* GetDictionary(int index) {
       sync_datatype_helper::test()->GetProfile(index))->GetCustomDictionary();
 }
 
-SpellcheckCustomDictionary* GetVerifierDictionary() {
-  return SpellcheckServiceFactory::GetForContext(
-      sync_datatype_helper::test()->verifier())->GetCustomDictionary();
-}
-
 void LoadDictionary(SpellcheckCustomDictionary* dictionary) {
   if (dictionary->IsLoaded())
     return;
@@ -66,53 +61,25 @@ void LoadDictionary(SpellcheckCustomDictionary* dictionary) {
 
 }  // namespace
 
+const std::set<std::string>& GetDictionaryWords(int profile_index) {
+  return GetDictionary(profile_index)->GetWords();
+}
 
 void LoadDictionaries() {
-  for (int i = 0; i < sync_datatype_helper::test()->num_clients(); ++i)
+  for (int i = 0; i < sync_datatype_helper::test()->num_clients(); ++i) {
     LoadDictionary(GetDictionary(i));
-  if (sync_datatype_helper::test()->use_verifier())
-    LoadDictionary(GetVerifierDictionary());
+  }
 }
 
 size_t GetDictionarySize(int index) {
   return GetDictionary(index)->GetWords().size();
 }
 
-size_t GetVerifierDictionarySize() {
-  return GetVerifierDictionary()->GetWords().size();
-}
-
-bool DictionariesMatch() {
-  const std::set<std::string>& reference =
-      sync_datatype_helper::test()->use_verifier()
-          ? GetVerifierDictionary()->GetWords()
-          : GetDictionary(0)->GetWords();
-  for (int i = 0; i < sync_datatype_helper::test()->num_clients(); ++i) {
-    const std::set<std::string>& dictionary = GetDictionary(i)->GetWords();
-    if (reference.size() != dictionary.size() ||
-        !std::equal(reference.begin(), reference.end(), dictionary.begin())) {
-      return false;
-    }
-  }
-  return true;
-}
-
-bool DictionaryMatchesVerifier(int index) {
-  const std::set<std::string>& expected = GetVerifierDictionary()->GetWords();
-  const std::set<std::string>& actual = GetDictionary(index)->GetWords();
-  return expected.size() == actual.size() &&
-         std::equal(expected.begin(), expected.end(), actual.begin());
-}
-
 bool AddWord(int index, const std::string& word) {
   SpellcheckCustomDictionary::Change dictionary_change;
   dictionary_change.AddWord(word);
   bool result = DictionarySyncIntegrationTestHelper::ApplyChange(
       GetDictionary(index), &dictionary_change);
-  if (sync_datatype_helper::test()->use_verifier()) {
-    result &= DictionarySyncIntegrationTestHelper::ApplyChange(
-        GetVerifierDictionary(), &dictionary_change);
-  }
   return result;
 }
 
@@ -129,22 +96,25 @@ bool RemoveWord(int index, const std::string& word) {
   dictionary_change.RemoveWord(word);
   bool result = DictionarySyncIntegrationTestHelper::ApplyChange(
       GetDictionary(index), &dictionary_change);
-  if (sync_datatype_helper::test()->use_verifier()) {
-    result &= DictionarySyncIntegrationTestHelper::ApplyChange(
-        GetVerifierDictionary(), &dictionary_change);
-  }
   return result;
 }
 
-}  // namespace dictionary_helper
-
-DictionaryMatchChecker::DictionaryMatchChecker()
+DictionaryChecker::DictionaryChecker(
+    const std::vector<std::string>& expected_words)
     : MultiClientStatusChangeChecker(
-          sync_datatype_helper::test()->GetSyncServices()) {}
+          sync_datatype_helper::test()->GetSyncServices()),
+      expected_words_(expected_words.begin(), expected_words.end()) {}
+
+DictionaryChecker::~DictionaryChecker() = default;
 
-bool DictionaryMatchChecker::IsExitConditionSatisfied(std::ostream* os) {
+bool DictionaryChecker::IsExitConditionSatisfied(std::ostream* os) {
   *os << "Waiting for matching dictionaries";
-  return dictionary_helper::DictionariesMatch();
+  for (int i = 0; i < sync_datatype_helper::test()->num_clients(); ++i) {
+    if (GetDictionaryWords(/*profile_index=*/i) != expected_words_) {
+      return false;
+    }
+  }
+  return true;
 }
 
 NumDictionaryEntriesChecker::NumDictionaryEntriesChecker(int index,
@@ -155,8 +125,10 @@ NumDictionaryEntriesChecker::NumDictionaryEntriesChecker(int index,
       num_words_(num_words) {}
 
 bool NumDictionaryEntriesChecker::IsExitConditionSatisfied(std::ostream* os) {
-  size_t actual_size = dictionary_helper::GetDictionarySize(index_);
+  size_t actual_size = GetDictionarySize(index_);
   *os << "Waiting for client " << index_ << ": " << actual_size << " / "
       << num_words_ << " words downloaded";
   return actual_size == num_words_;
 }
+
+}  // namespace dictionary_helper

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

@@ -7,6 +7,7 @@
 
 #include <stddef.h>
 
+#include <set>
 #include <string>
 
 #include "chrome/browser/sync/test/integration/multi_client_status_change_checker.h"
@@ -14,33 +15,22 @@
 
 namespace dictionary_helper {
 
-// Synchronously loads the dictionaries across all profiles. Also loads the
-// dictionary for the verifier if DisableVerifier() hasn't been called. Returns
-// only after the dictionaries have finished to load.
+// Returns set of words stored in dictionary for given |profile_index|.
+const std::set<std::string>& GetDictionaryWords(int profile_index);
+
+// Synchronously loads the dictionaries across all profiles. Returns only after
+// the dictionaries have finished to load.
 void LoadDictionaries();
 
 // Used to check the size of the dictionary within a particular sync profile.
 size_t GetDictionarySize(int index);
 
-// Used to check the size of the dictionary within the verifier sync profile.
-size_t GetVerifierDictionarySize();
-
-// Used to verify that dictionaries match across all profiles. Also checks
-// verifier if DisableVerifier() hasn't been called.
-bool DictionariesMatch();
-
-// Used to verify that the dictionary within a particular sync profile matches
-// the dictionary within the verifier sync profile.
-bool DictionaryMatchesVerifier(int index);
-
-// Adds |word| to the dictionary for profile with index |index|. Also adds
-// |word| to the verifier if DisableVerifier() hasn't been called. Returns true
+// Adds |word| to the dictionary for profile with index |index|. Returns true
 // if |word| is valid and not a duplicate. Otherwise returns false.
 bool AddWord(int index, const std::string& word);
 
-// Add |n| words with the given |prefix| to the specified client |index|. Also
-// adds to the verifier if not disAbled. Return value is true iff all words are
-// not duplicates and valid.
+// Add |n| words with the given |prefix| to the specified client |index|. Return
+// value is true iff all words are not duplicates and valid.
 bool AddWords(int index, int n, const std::string& prefix);
 
 // Removes |word| from the dictionary for profile with index |index|. Also
@@ -48,15 +38,17 @@ bool AddWords(int index, int n, const std::string& prefix);
 // Returns true if |word| was found. Otherwise returns false.
 bool RemoveWord(int index, const std::string& word);
 
-}  // namespace dictionary_helper
-
-// Checker to block until all services have matching dictionaries.
-class DictionaryMatchChecker : public MultiClientStatusChangeChecker {
+// Checker to block until all clients have expected dictionaries.
+class DictionaryChecker : public MultiClientStatusChangeChecker {
  public:
-  DictionaryMatchChecker();
+  explicit DictionaryChecker(const std::vector<std::string>& expected_words);
+  ~DictionaryChecker() override;
 
   // StatusChangeChecker implementation.
   bool IsExitConditionSatisfied(std::ostream* os) override;
+
+ private:
+  std::set<std::string> expected_words_;
 };
 
 // Checker to block until the number of dictionary entries to equal to an
@@ -64,6 +56,7 @@ class DictionaryMatchChecker : public MultiClientStatusChangeChecker {
 class NumDictionaryEntriesChecker : public SingleClientStatusChangeChecker {
  public:
   NumDictionaryEntriesChecker(int index, size_t num_words);
+  ~NumDictionaryEntriesChecker() override = default;
 
   // StatusChangeChecker implementation.
   bool IsExitConditionSatisfied(std::ostream* os) override;
@@ -73,4 +66,6 @@ class NumDictionaryEntriesChecker : public SingleClientStatusChangeChecker {
   size_t num_words_;
 };
 
+}  // namespace dictionary_helper
+
 #endif  // CHROME_BROWSER_SYNC_TEST_INTEGRATION_DICTIONARY_HELPER_H_

chrome/browser/sync/test/integration/performance/dictionary_sync_perf_test.cc

@@ -34,7 +34,7 @@ perf_test::PerfResultReporter SetUpReporter(const std::string& story) {
 class DictionarySyncPerfTest : public SyncTest {
  public:
   DictionarySyncPerfTest() : SyncTest(TWO_CLIENT) {}
-  ~DictionarySyncPerfTest() override {}
+  ~DictionarySyncPerfTest() override = default;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(DictionarySyncPerfTest);
@@ -43,7 +43,8 @@ class DictionarySyncPerfTest : public SyncTest {
 IN_PROC_BROWSER_TEST_F(DictionarySyncPerfTest, P0) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   dictionary_helper::LoadDictionaries();
-  ASSERT_TRUE(dictionary_helper::DictionariesMatch());
+  ASSERT_TRUE(
+      dictionary_helper::DictionaryChecker(/*expected_words=*/{}).Wait());
 
   auto reporter = SetUpReporter(
       base::NumberToString(spellcheck::kMaxSyncableDictionaryWords) + "_words");

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

@@ -8,32 +8,32 @@
 #include "chrome/browser/sync/test/integration/updated_progress_marker_checker.h"
 #include "components/sync/driver/profile_sync_service.h"
 #include "content/public/test/browser_test.h"
+#include "testing/gmock/include/gmock/gmock.h"
 
 namespace {
 
+using testing::ElementsAre;
+using testing::IsEmpty;
+
 class SingleClientDictionarySyncTest : public SyncTest {
  public:
   SingleClientDictionarySyncTest() : SyncTest(SINGLE_CLIENT) {}
-
-  ~SingleClientDictionarySyncTest() override {}
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(SingleClientDictionarySyncTest);
+  ~SingleClientDictionarySyncTest() override = default;
 };
 
 IN_PROC_BROWSER_TEST_F(SingleClientDictionarySyncTest, Sanity) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   dictionary_helper::LoadDictionaries();
-  ASSERT_TRUE(dictionary_helper::DictionariesMatch());
+  EXPECT_THAT(dictionary_helper::GetDictionaryWords(0), IsEmpty());
 
-  std::string word = "foo";
-  ASSERT_TRUE(dictionary_helper::AddWord(0, word));
-  ASSERT_TRUE(UpdatedProgressMarkerChecker(GetSyncService(0)).Wait());
-  ASSERT_TRUE(dictionary_helper::DictionariesMatch());
+  const std::string word = "foo";
+  EXPECT_TRUE(dictionary_helper::AddWord(0, word));
+  EXPECT_TRUE(UpdatedProgressMarkerChecker(GetSyncService(0)).Wait());
+  EXPECT_THAT(dictionary_helper::GetDictionaryWords(0), ElementsAre(word));
 
-  ASSERT_TRUE(dictionary_helper::RemoveWord(0, word));
-  ASSERT_TRUE(UpdatedProgressMarkerChecker(GetSyncService(0)).Wait());
-  ASSERT_TRUE(dictionary_helper::DictionariesMatch());
+  EXPECT_TRUE(dictionary_helper::RemoveWord(0, word));
+  EXPECT_TRUE(UpdatedProgressMarkerChecker(GetSyncService(0)).Wait());
+  EXPECT_THAT(dictionary_helper::GetDictionaryWords(0), IsEmpty());
 }
 
 }  // namespace

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

@@ -16,122 +16,106 @@
 
 namespace {
 
+using dictionary_helper::AddWord;
+using dictionary_helper::AddWords;
+using dictionary_helper::DictionaryChecker;
+using dictionary_helper::GetDictionarySize;
+using dictionary_helper::LoadDictionaries;
+using dictionary_helper::NumDictionaryEntriesChecker;
+using dictionary_helper::RemoveWord;
 using spellcheck::kMaxSyncableDictionaryWords;
 
 class TwoClientDictionarySyncTest : public SyncTest {
  public:
   TwoClientDictionarySyncTest() : SyncTest(TWO_CLIENT) {}
-
-  ~TwoClientDictionarySyncTest() override {}
+  ~TwoClientDictionarySyncTest() override = default;
 
   bool TestUsesSelfNotifications() override { return false; }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(TwoClientDictionarySyncTest);
 };
 
 IN_PROC_BROWSER_TEST_F(TwoClientDictionarySyncTest, E2E_ENABLED(Sanity)) {
   ResetSyncForPrimaryAccount();
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
-  dictionary_helper::LoadDictionaries();
-  ASSERT_TRUE(DictionaryMatchChecker().Wait());
+  LoadDictionaries();
+  EXPECT_TRUE(DictionaryChecker(/*expected_words=*/{}).Wait());
 
-  std::vector<std::string> words;
-  words.push_back("foo");
-  words.push_back("bar");
+  const std::vector<std::string> words{"foo", "bar"};
   ASSERT_EQ(num_clients(), static_cast<int>(words.size()));
 
   for (int i = 0; i < num_clients(); ++i) {
-    ASSERT_TRUE(dictionary_helper::AddWord(i, words[i]));
+    EXPECT_TRUE(AddWord(i, words[i]));
+  }
+  EXPECT_TRUE(DictionaryChecker(words).Wait());
+
+  for (int i = 0; i < num_clients(); ++i) {
+    EXPECT_TRUE(RemoveWord(i, words[i]));
   }
-  ASSERT_TRUE(DictionaryMatchChecker().Wait());
-  ASSERT_EQ(words.size(), dictionary_helper::GetDictionarySize(0));
+  EXPECT_TRUE(DictionaryChecker(/*expected_words=*/{}).Wait());
 
   for (int i = 0; i < num_clients(); ++i) {
-    ASSERT_TRUE(dictionary_helper::RemoveWord(i, words[i]));
+    EXPECT_TRUE(AddWord(i, words[i]));
   }
-  ASSERT_TRUE(DictionaryMatchChecker().Wait());
-  ASSERT_EQ(0UL, dictionary_helper::GetDictionarySize(0));
-
-  DisableVerifier();
-  for (int i = 0; i < num_clients(); ++i)
-    ASSERT_TRUE(dictionary_helper::AddWord(i, words[i]));
-  ASSERT_TRUE(DictionaryMatchChecker().Wait());
-  ASSERT_EQ(words.size(), dictionary_helper::GetDictionarySize(0));
+  EXPECT_TRUE(DictionaryChecker(words).Wait());
 }
 
 IN_PROC_BROWSER_TEST_F(TwoClientDictionarySyncTest,
                        E2E_ENABLED(SimultaneousAdd)) {
   ResetSyncForPrimaryAccount();
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
-  dictionary_helper::LoadDictionaries();
-  ASSERT_TRUE(DictionaryMatchChecker().Wait());
+  LoadDictionaries();
+  ASSERT_TRUE(DictionaryChecker(/*expected_words=*/{}).Wait());
 
-  for (int i = 0; i < num_clients(); ++i)
-    dictionary_helper::AddWord(i, "foo");
-  ASSERT_TRUE(DictionaryMatchChecker().Wait());
-  ASSERT_EQ(1UL, dictionary_helper::GetDictionarySize(0));
+  const std::string word = "foo";
+  for (int i = 0; i < num_clients(); ++i) {
+    dictionary_helper::AddWord(i, word);
+  }
+  EXPECT_TRUE(DictionaryChecker(/*expected_words=*/{word}).Wait());
 }
 
 IN_PROC_BROWSER_TEST_F(TwoClientDictionarySyncTest,
                        E2E_ENABLED(SimultaneousRemove)) {
   ResetSyncForPrimaryAccount();
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
-  dictionary_helper::LoadDictionaries();
-  ASSERT_TRUE(DictionaryMatchChecker().Wait());
-
-  for (int i = 0; i < num_clients(); ++i)
-    dictionary_helper::AddWord(i, "foo");
-  ASSERT_TRUE(DictionaryMatchChecker().Wait());
-  ASSERT_EQ(1UL, dictionary_helper::GetDictionarySize(0));
-
-  for (int i = 0; i < num_clients(); ++i)
-    dictionary_helper::RemoveWord(i, "foo");
-  ASSERT_TRUE(DictionaryMatchChecker().Wait());
-  ASSERT_EQ(0UL, dictionary_helper::GetDictionarySize(0));
-}
-
-IN_PROC_BROWSER_TEST_F(TwoClientDictionarySyncTest,
-                       E2E_ENABLED(AddDifferentToEach)) {
-  ResetSyncForPrimaryAccount();
-  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
-  dictionary_helper::LoadDictionaries();
-  ASSERT_TRUE(DictionaryMatchChecker().Wait());
+  LoadDictionaries();
+  ASSERT_TRUE(DictionaryChecker(/*expected_words=*/{}).Wait());
 
-  for (int i = 0; i < num_clients(); ++i)
-    dictionary_helper::AddWord(i, "foo" + base::NumberToString(i));
+  const std::string word = "foo";
+  for (int i = 0; i < num_clients(); ++i) {
+    AddWord(i, word);
+  }
+  ASSERT_TRUE(DictionaryChecker(/*expected_words=*/{word}).Wait());
 
-  ASSERT_TRUE(DictionaryMatchChecker().Wait());
-  ASSERT_EQ(num_clients(),
-            static_cast<int>(dictionary_helper::GetDictionarySize(0)));
+  for (int i = 0; i < num_clients(); ++i) {
+    RemoveWord(i, word);
+  }
+  EXPECT_TRUE(DictionaryChecker(/*expected_words=*/{}).Wait());
 }
 
 IN_PROC_BROWSER_TEST_F(TwoClientDictionarySyncTest,
                        E2E_ENABLED(RemoveOnAAddOnB)) {
   ResetSyncForPrimaryAccount();
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
-  dictionary_helper::LoadDictionaries();
-  ASSERT_TRUE(DictionaryMatchChecker().Wait());
+  LoadDictionaries();
+  ASSERT_TRUE(DictionaryChecker(/*expected_words=*/{}).Wait());
 
-  std::string word = "foo";
+  const std::string word = "foo";
   // Add on client A, check it appears on B.
-  ASSERT_TRUE(dictionary_helper::AddWord(0, word));
-  ASSERT_TRUE(DictionaryMatchChecker().Wait());
+  ASSERT_TRUE(AddWord(0, word));
+  EXPECT_TRUE(DictionaryChecker(/*expected_words=*/{word}).Wait());
   // Remove on client A, check it disappears on B.
-  ASSERT_TRUE(dictionary_helper::RemoveWord(0, word));
-  ASSERT_TRUE(DictionaryMatchChecker().Wait());
+  EXPECT_TRUE(RemoveWord(0, word));
+  EXPECT_TRUE(DictionaryChecker(/*expected_words=*/{}).Wait());
   // Add on client B, check it appears on A.
-  ASSERT_TRUE(dictionary_helper::AddWord(1, word));
-  ASSERT_TRUE(DictionaryMatchChecker().Wait());
-  ASSERT_EQ(1UL, dictionary_helper::GetDictionarySize(0));
+  EXPECT_TRUE(AddWord(1, word));
+  EXPECT_TRUE(DictionaryChecker(/*expected_words=*/{word}).Wait());
 }
 
 // Tests the case where a client has more words added than the
 // kMaxSyncableDictionaryWords limit.
 IN_PROC_BROWSER_TEST_F(TwoClientDictionarySyncTest, Limit) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
-  dictionary_helper::LoadDictionaries();
-  ASSERT_TRUE(DictionaryMatchChecker().Wait());
+  LoadDictionaries();
+  ASSERT_TRUE(DictionaryChecker(/*expected_words=*/{}).Wait());
 
   // Disable client #1 before client #0 starts adding anything.
   GetClient(1)->DisableSyncForAllDatatypes();
@@ -142,8 +126,8 @@ IN_PROC_BROWSER_TEST_F(TwoClientDictionarySyncTest, Limit) {
   // on the limit.
   size_t chunk_size = kMaxSyncableDictionaryWords * 2 / 5;
 
-  ASSERT_TRUE(dictionary_helper::AddWords(0, chunk_size, "foo-0-"));
-  ASSERT_EQ(chunk_size, dictionary_helper::GetDictionarySize(0));
+  ASSERT_TRUE(AddWords(0, chunk_size, "foo-0-"));
+  ASSERT_EQ(chunk_size, GetDictionarySize(0));
 
   // We must wait for the server here. This test was originally an n-client test
   // where n-1 clients waited to have the same state. We cannot do that on 2
@@ -157,8 +141,8 @@ IN_PROC_BROWSER_TEST_F(TwoClientDictionarySyncTest, Limit) {
   ASSERT_TRUE(
       ServerCountMatchStatusChecker(syncer::DICTIONARY, chunk_size).Wait());
 
-  ASSERT_TRUE(dictionary_helper::AddWords(1, 2 * chunk_size, "foo-1-"));
-  ASSERT_EQ(2 * chunk_size, dictionary_helper::GetDictionarySize(1));
+  ASSERT_TRUE(AddWords(1, 2 * chunk_size, "foo-1-"));
+  ASSERT_EQ(2 * chunk_size, GetDictionarySize(1));
 
   // Client #1 should first pull remote changes, apply them, without capping at
   // any sort of limit. This will cause client #1 to have 3 * chunk_size. When