[Sync] Reworking TwoClientDictionarySyncTest.Limit to correctly wait for local changes to be synced.

BUG=575316

Review URL: https://codereview.chromium.org/1580423003

Cr-Commit-Position: refs/heads/master@{#369865}

8 files changed, 190 insertions, 76 deletions

diff --git a/chrome/browser/sync/test/integration/dictionary_helper.cc b/chrome/browser/sync/test/integration/dictionary_helper.cc
index e22457d..b9937bc 100644
--- a/chrome/browser/sync/test/integration/dictionary_helper.cc
+++ b/chrome/browser/sync/test/integration/dictionary_helper.cc
@@ -10,6 +10,7 @@
 #include "base/format_macros.h"
 #include "base/macros.h"
 #include "base/run_loop.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/spellchecker/spellcheck_custom_dictionary.h"
 #include "chrome/browser/spellchecker/spellcheck_factory.h"
@@ -26,16 +27,16 @@ class DictionarySyncIntegrationTestHelper {
  public:
   // Same as SpellcheckCustomDictionary::AddWord/RemoveWord, except does not
   // write to disk.
-  static bool ApplyChange(
-      SpellcheckCustomDictionary* dictionary,
-      SpellcheckCustomDictionary::Change& change) {
-    int result = change.Sanitize(dictionary->GetWords());
-    dictionary->Apply(change);
-    dictionary->Notify(change);
-    dictionary->Sync(change);
+  static bool ApplyChange(SpellcheckCustomDictionary* dictionary,
+                          SpellcheckCustomDictionary::Change* change) {
+    int result = change->Sanitize(dictionary->GetWords());
+    dictionary->Apply(*change);
+    dictionary->Notify(*change);
+    dictionary->Sync(*change);
     return !result;
   }
 
+ private:
   DISALLOW_COPY_AND_ASSIGN(DictionarySyncIntegrationTestHelper);
 };
 
@@ -183,10 +184,18 @@ bool AddWord(int index, const std::string& word) {
   SpellcheckCustomDictionary::Change dictionary_change;
   dictionary_change.AddWord(word);
   bool result = DictionarySyncIntegrationTestHelper::ApplyChange(
-      GetDictionary(index), dictionary_change);
+      GetDictionary(index), &dictionary_change);
   if (sync_datatype_helper::test()->use_verifier()) {
     result &= DictionarySyncIntegrationTestHelper::ApplyChange(
-        GetVerifierDictionary(), dictionary_change);
+        GetVerifierDictionary(), &dictionary_change);
+  }
+  return result;
+}
+
+bool AddWords(int index, int n, const std::string& prefix) {
+  bool result = true;
+  for (int i = 0; i < n; ++i) {
+    result &= AddWord(index, prefix + base::IntToString(i));
   }
   return result;
 }
@@ -195,10 +204,10 @@ bool RemoveWord(int index, const std::string& word) {
   SpellcheckCustomDictionary::Change dictionary_change;
   dictionary_change.RemoveWord(word);
   bool result = DictionarySyncIntegrationTestHelper::ApplyChange(
-      GetDictionary(index), dictionary_change);
+      GetDictionary(index), &dictionary_change);
   if (sync_datatype_helper::test()->use_verifier()) {
     result &= DictionarySyncIntegrationTestHelper::ApplyChange(
-        GetVerifierDictionary(), dictionary_change);
+        GetVerifierDictionary(), &dictionary_change);
   }
   return result;
 }
diff --git a/chrome/browser/sync/test/integration/dictionary_helper.h b/chrome/browser/sync/test/integration/dictionary_helper.h
index 659155b..cd73291 100644
--- a/chrome/browser/sync/test/integration/dictionary_helper.h
+++ b/chrome/browser/sync/test/integration/dictionary_helper.h
@@ -43,6 +43,11 @@ bool DictionaryMatchesVerifier(int index);
 // 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.
+bool AddWords(int index, int n, const std::string& prefix);
+
 // Removes |word| from the dictionary for profile with index |index|. Also
 // removes |word| from the verifier if DisableVerifier() hasn't been called.
 // Returns true if |word| was found. Otherwise returns false.
diff --git a/chrome/browser/sync/test/integration/fake_server_match_status_checker.cc b/chrome/browser/sync/test/integration/fake_server_match_status_checker.cc
new file mode 100644
index 0000000..d45b01d
--- /dev/null
+++ b/chrome/browser/sync/test/integration/fake_server_match_status_checker.cc
@@ -0,0 +1,39 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/sync/test/integration/fake_server_match_status_checker.h"
+#include "chrome/browser/sync/test/integration/sync_datatype_helper.h"
+#include "chrome/browser/sync/test/integration/sync_test.h"
+
+namespace fake_server {
+
+FakeServerMatchStatusChecker::FakeServerMatchStatusChecker()
+    : fake_server_(sync_datatype_helper::test()->GetFakeServer()) {
+  DCHECK(fake_server_);
+}
+
+void FakeServerMatchStatusChecker::Wait() {
+  DVLOG(1) << "Await: " << GetDebugMessage();
+
+  if (IsExitConditionSatisfied()) {
+    DVLOG(1) << "Await -> Exit before waiting: " << GetDebugMessage();
+    return;
+  }
+
+  fake_server_->AddObserver(this);
+  StartBlockingWait();
+  fake_server_->RemoveObserver(this);
+}
+
+void FakeServerMatchStatusChecker::OnCommit(
+    const std::string& committer_id,
+    syncer::ModelTypeSet committed_model_types) {
+  CheckExitCondition();
+}
+
+fake_server::FakeServer* FakeServerMatchStatusChecker::fake_server() const {
+  return fake_server_;
+}
+
+}  // namespace fake_server
diff --git a/chrome/browser/sync/test/integration/fake_server_match_status_checker.h b/chrome/browser/sync/test/integration/fake_server_match_status_checker.h
new file mode 100644
index 0000000..02f0ae3
--- /dev/null
+++ b/chrome/browser/sync/test/integration/fake_server_match_status_checker.h
@@ -0,0 +1,36 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_SYNC_TEST_INTEGRATION_FAKE_SERVER_MATCH_STATUS_CHECKER_H_
+#define CHROME_BROWSER_SYNC_TEST_INTEGRATION_FAKE_SERVER_MATCH_STATUS_CHECKER_H_
+
+#include <string>
+
+#include "chrome/browser/sync/test/integration/status_change_checker.h"
+#include "sync/internal_api/public/base/model_type.h"
+#include "sync/test/fake_server/fake_server.h"
+
+namespace fake_server {
+
+// A matcher that checks a generic condition against the fake server. This class
+// is abstract where any subclass will be responsible for implementing some of
+// StatusChangeChecker's virtual methods.
+class FakeServerMatchStatusChecker : public StatusChangeChecker,
+                                     public FakeServer::Observer {
+ public:
+  FakeServerMatchStatusChecker();
+  void Wait();
+  void OnCommit(const std::string& committer_id,
+                syncer::ModelTypeSet committed_model_types) override;
+
+ protected:
+  FakeServer* fake_server() const;
+
+ private:
+  FakeServer* fake_server_;
+};
+
+}  // namespace fake_server
+
+#endif  // CHROME_BROWSER_SYNC_TEST_INTEGRATION_FAKE_SERVER_MATCH_STATUS_CHECKER_H_
diff --git a/chrome/browser/sync/test/integration/sync_integration_test_util.cc b/chrome/browser/sync/test/integration/sync_integration_test_util.cc
index c84e948..896d969 100644
--- a/chrome/browser/sync/test/integration/sync_integration_test_util.cc
+++ b/chrome/browser/sync/test/integration/sync_integration_test_util.cc
@@ -4,11 +4,40 @@
 
 #include "chrome/browser/sync/test/integration/sync_integration_test_util.h"
 
+#include <string>
+
+#include "base/strings/stringprintf.h"
+#include "chrome/browser/sync/test/integration/fake_server_match_status_checker.h"
 #include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
 #include "chrome/browser/sync/test/integration/updated_progress_marker_checker.h"
 #include "components/browser_sync/browser/profile_sync_service.h"
 
-namespace sync_integration_test_util {
+namespace {
+
+// Helper class to block until the server has a given number of entities.
+class ServerCountMatchStatusChecker
+    : public fake_server::FakeServerMatchStatusChecker {
+ public:
+  ServerCountMatchStatusChecker(syncer::ModelType type, size_t count)
+      : type_(type), count_(count) {}
+  ~ServerCountMatchStatusChecker() override {}
+
+  bool IsExitConditionSatisfied() override {
+    return count_ == fake_server()->GetSyncEntitiesByModelType(type_).size();
+  }
+
+  std::string GetDebugMessage() const override {
+    return base::StringPrintf(
+        "Waiting for fake server entity count %zu to match expected count %zu "
+        "for type %d",
+        (size_t)fake_server()->GetSyncEntitiesByModelType(type_).size(), count_,
+        type_);
+  }
+
+ private:
+  const syncer::ModelType type_;
+  const size_t count_;
+};
 
 class PassphraseRequiredChecker : public SingleClientStatusChangeChecker {
  public:
@@ -35,6 +64,10 @@ class PassphraseAcceptedChecker : public SingleClientStatusChangeChecker {
   std::string GetDebugMessage() const override { return "Passhrase Accepted"; }
 };
 
+}  // namespace
+
+namespace sync_integration_test_util {
+
 bool AwaitPassphraseRequired(ProfileSyncService* service) {
   PassphraseRequiredChecker checker(service);
   checker.Wait();
@@ -53,4 +86,10 @@ bool AwaitCommitActivityCompletion(ProfileSyncService* service) {
   return !checker.TimedOut();
 }
 
+bool AwaitServerCount(syncer::ModelType type, size_t count) {
+  ServerCountMatchStatusChecker checker(type, count);
+  checker.Wait();
+  return !checker.TimedOut();
+}
+
 }  // namespace sync_integration_test_util
diff --git a/chrome/browser/sync/test/integration/sync_integration_test_util.h b/chrome/browser/sync/test/integration/sync_integration_test_util.h
index db43c96..fcbd222 100644
--- a/chrome/browser/sync/test/integration/sync_integration_test_util.h
+++ b/chrome/browser/sync/test/integration/sync_integration_test_util.h
@@ -5,8 +5,14 @@
 #ifndef CHROME_BROWSER_SYNC_TEST_INTEGRATION_SYNC_INTEGRATION_TEST_UTIL_H_
 #define CHROME_BROWSER_SYNC_TEST_INTEGRATION_SYNC_INTEGRATION_TEST_UTIL_H_
 
+#include "sync/internal_api/public/base/model_type.h"
+
 class ProfileSyncService;
 
+namespace fake_server {
+class FakeServer;
+}  // namespace fake_server
+
 namespace sync_integration_test_util {
 
 // Wait until the provided |service| is blocked waiting for a passphrase.
@@ -19,6 +25,9 @@ bool AwaitPassphraseAccepted(ProfileSyncService* service);
 // This can be a bit flaky.  See UpdatedProgressMarkerChecker for details.
 bool AwaitCommitActivityCompletion(ProfileSyncService* service);
 
+// Wait until the fake server has a specific count for the given type.
+bool AwaitServerCount(syncer::ModelType type, size_t count);
+
 }  // namespace sync_integration_test_util
 
 #endif  // CHROME_BROWSER_SYNC_TEST_INTEGRATION_SYNC_INTEGRATION_TEST_UTIL_H_
diff --git a/chrome/browser/sync/test/integration/two_client_dictionary_sync_test.cc b/chrome/browser/sync/test/integration/two_client_dictionary_sync_test.cc
index 518dc99..85a4c8c0 100644
--- a/chrome/browser/sync/test/integration/two_client_dictionary_sync_test.cc
+++ b/chrome/browser/sync/test/integration/two_client_dictionary_sync_test.cc
@@ -9,10 +9,12 @@
 #include "chrome/browser/sync/test/integration/sync_integration_test_util.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/common/spellcheck_common.h"
+#include "sync/internal_api/public/base/model_type.h"
 
 using chrome::spellcheck_common::MAX_SYNCABLE_DICTIONARY_WORDS;
 using dictionary_helper::AwaitNumDictionaryEntries;
 using sync_integration_test_util::AwaitCommitActivityCompletion;
+using sync_integration_test_util::AwaitServerCount;
 
 class TwoClientDictionarySyncTest : public SyncTest {
  public:
@@ -112,73 +114,46 @@ IN_PROC_BROWSER_TEST_F(TwoClientDictionarySyncTest, RemoveOnAAddOnB) {
   ASSERT_EQ(1UL, dictionary_helper::GetDictionarySize(0));
 }
 
-// Tests the case where the Nth client pushes the server beyond its
+// Tests the case where a client has more words added than the
 // MAX_SYNCABLE_DICTIONARY_WORDS limit.
-// Used to crash on Windows; now failing on Linux after conversion to
-// 2-client test. See crbug.com/575316
-IN_PROC_BROWSER_TEST_F(TwoClientDictionarySyncTest, DISABLED_Limit) {
+IN_PROC_BROWSER_TEST_F(TwoClientDictionarySyncTest, Limit) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   dictionary_helper::LoadDictionaries();
   ASSERT_TRUE(dictionary_helper::AwaitDictionariesMatch());
 
-  const int n = num_clients();
-
-  // Pick a number of initial words per client such that
-  // (num-clients()-1) * initial_words
-  //     < MAX_SYNCABLE_DICTIONARY_WORDS
-  //     < num_clients() * initial_words
-  size_t initial_words =
-      (MAX_SYNCABLE_DICTIONARY_WORDS + n) / n;
-
-  // Add |initial_words| words to each of the clients before sync.
-  for (int i = 0; i < n; ++i) {
-    GetClient(i)->DisableSyncForAllDatatypes();
-    for (size_t j = 0; j < initial_words; ++j) {
-      ASSERT_TRUE(dictionary_helper::AddWord(
-          i, "foo-" + base::IntToString(i) + "-" + base::Uint64ToString(j)));
-    }
-    ASSERT_EQ(initial_words, dictionary_helper::GetDictionarySize(i));
-  }
-
-  // As long as we don't get involved in any race conditions where two clients
-  // are committing at once, we should be able to guarantee that the server has
-  // at most MAX_SYNCABLE_DICTIONARY_WORDS words.  Every client will be able to
-  // sync these items.  Clients are allowed to have more words if they're
-  // available locally, but they won't be able to commit any words once the
-  // server is full.
-  //
-  // As we enable clients one-by-one, all but the (N-1)th client should be able
-  // to commit all of their items.  The last one will have some local data left
-  // over.
-
-  // Open the floodgates.   Allow N-1 clients to sync their items.
-  for (int i = 0; i < n-1; ++i) {
-    SCOPED_TRACE(i);
-
-    // Client #i has |initial_words| words before sync.
-    ASSERT_EQ(initial_words, dictionary_helper::GetDictionarySize(i));
-    ASSERT_TRUE(GetClient(i)->EnableSyncForAllDatatypes());
-  }
-
-  // Wait for clients to catch up.  All should be in sync with the server
-  // and have exactly (initial_words * (N-1)) words in their dictionaries.
-  for (int i = 0; i < n-1; ++i) {
-    SCOPED_TRACE(i);
-    ASSERT_TRUE(AwaitNumDictionaryEntries(i, initial_words*(n-1)));
-  }
-
-  // Add the client that has engough new words to cause an overflow.
-  ASSERT_EQ(initial_words, dictionary_helper::GetDictionarySize(n-1));
-  ASSERT_TRUE(GetClient(n-1)->EnableSyncForAllDatatypes());
-
-  // The Nth client will receive the initial_words * (n-1) entries that were on
-  // the server.  It will commit some of the entries it had locally.  And it
-  // will have a few uncommittable items left over.
-  ASSERT_TRUE(AwaitNumDictionaryEntries(n-1, initial_words*n));
-
-  // Everyone else should be at the limit.
-  for (int i = 0; i < n-1; ++i) {
-    SCOPED_TRACE(i);
-    ASSERT_TRUE(AwaitNumDictionaryEntries(i, MAX_SYNCABLE_DICTIONARY_WORDS));
-  }
+  // Disable client #1 before client #0 starts adding anything.
+  GetClient(1)->DisableSyncForAllDatatypes();
+
+  // Pick a size between 1/2 and 1/3 of MAX_SYNCABLE_DICTIONARY_WORDS. This will
+  // allow the test to verify that while we crossed the limit the client not
+  // actively making changes is still recieving sync updates but stops exactly
+  // on the limit.
+  size_t chunk_size = MAX_SYNCABLE_DICTIONARY_WORDS * 2 / 5;
+
+  ASSERT_TRUE(dictionary_helper::AddWords(0, chunk_size, "foo-0-"));
+  ASSERT_EQ(chunk_size, dictionary_helper::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
+  // clients because one needs to be disconnected during this process, because
+  // part of what we're testing is that when it comes online it pulls remote
+  // changes before pushing local changes, with the limit in mind. So we check
+  // the server count here to make sure client #0 is done pushing its changes
+  // out. In there real world there's a race condition here, if multiple clients
+  // are adding words simultaneously then we're go over the limit slightly,
+  // though we'd expect this to be relatively small.
+  AwaitServerCount(syncer::DICTIONARY, chunk_size);
+
+  ASSERT_TRUE(dictionary_helper::AddWords(1, 2 * chunk_size, "foo-1-"));
+  ASSERT_EQ(2 * chunk_size, dictionary_helper::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
+  // client #1 then tries to commit changes, that is when it obeys the limit
+  // and will cause client #0 to only see the limit worth of words.
+  ASSERT_TRUE(GetClient(1)->EnableSyncForAllDatatypes());
+  ASSERT_TRUE(AwaitNumDictionaryEntries(1, 3 * chunk_size));
+  ASSERT_TRUE(
+      AwaitServerCount(syncer::DICTIONARY, MAX_SYNCABLE_DICTIONARY_WORDS));
+  ASSERT_TRUE(AwaitNumDictionaryEntries(0, MAX_SYNCABLE_DICTIONARY_WORDS));
 }
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index bd008e7..19309d9 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1492,6 +1492,8 @@
       'browser/sync/test/integration/extensions_helper.h',
       'browser/sync/test/integration/fake_server_invalidation_service.cc',
       'browser/sync/test/integration/fake_server_invalidation_service.h',
+      'browser/sync/test/integration/fake_server_match_status_checker.cc',
+      'browser/sync/test/integration/fake_server_match_status_checker.h',
       'browser/sync/test/integration/migration_waiter.cc',
       'browser/sync/test/integration/migration_waiter.h',
       'browser/sync/test/integration/migration_watcher.cc',