Deflake preference sync integration test

This includes:
1. Factoring out preference-specific logic away from the shared SyncTest
   class.
2. Improved failure messages for the case where engine initialization
   fails.
3. Rewrite the test to just test the intended behavior under test,
   which is about two clients having a mismatching preference type.

Bug: 1012688
Change-Id: I7679a6c2b7963bdb181a35e2f327944320781896
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1898655
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712563}

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

@@ -108,14 +108,8 @@ void ChangeListPref(int index,
 }
 
 scoped_refptr<PrefStore> BuildPrefStoreFromPrefsFile(Profile* profile) {
-  profile->GetPrefs()->CommitPendingWrite();
-  // Writes are scheduled on the IO thread. The JsonPrefStore requires all
-  // access (construction, Get, Set, ReadPrefs) to be made from the same thread.
-  // So instead of reading the file from the IO thread, we simply schedule a
-  // dummy task to avoid races with writing the file and reading it.
   base::RunLoop run_loop;
-  profile->GetIOTaskRunner()->PostTask(FROM_HERE,
-                                       base::BindOnce(run_loop.QuitClosure()));
+  profile->GetPrefs()->CommitPendingWrite(run_loop.QuitClosure());
   run_loop.Run();
 
   auto pref_store = base::MakeRefCounted<JsonPrefStore>(
@@ -124,6 +118,7 @@ scoped_refptr<PrefStore> BuildPrefStoreFromPrefsFile(Profile* profile) {
   if (pref_store->ReadPrefs() != PersistentPrefStore::PREF_READ_ERROR_NONE) {
     ADD_FAILURE() << " Failed reading the prefs file into the store.";
   }
+
   return pref_store;
 }
 

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

@@ -68,11 +68,16 @@ class EngineInitializeChecker : public SingleClientStatusChangeChecker {
     if (service()->IsEngineInitialized())
       return true;
     // Engine initialization is blocked by an auth error.
-    if (HasAuthError(service()))
+    if (HasAuthError(service())) {
+      LOG(WARNING) << "Sync engine initialization blocked by auth error";
       return true;
+    }
     // Engine initialization is blocked by a failure to fetch Oauth2 tokens.
-    if (service()->IsRetryingAccessTokenFetchForTest())
+    if (service()->IsRetryingAccessTokenFetchForTest()) {
+      LOG(WARNING) << "Sync engine initialization blocked by failure to fetch "
+                      "access tokens";
       return true;
+    }
     // Still waiting on engine initialization.
     return false;
   }
@@ -460,13 +465,18 @@ bool ProfileSyncServiceHarness::AwaitEngineInitialization() {
     return false;
   }
 
-  if (!service()->IsEngineInitialized()) {
-    LOG(ERROR) << "Service engine not initialized.";
+  if (HasAuthError(service())) {
+    LOG(ERROR) << "Credentials were rejected. Sync cannot proceed.";
     return false;
   }
 
-  if (HasAuthError(service())) {
-    LOG(ERROR) << "Credentials were rejected. Sync cannot proceed.";
+  if (service()->IsRetryingAccessTokenFetchForTest()) {
+    LOG(ERROR) << "Failed to fetch access token. Sync cannot proceed.";
+    return false;
+  }
+
+  if (!service()->IsEngineInitialized()) {
+    LOG(ERROR) << "Service engine not initialized.";
     return false;
   }
 

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

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <map>
+
 #include "base/macros.h"
 #include "base/strings/stringprintf.h"
 #include "base/test/metrics/histogram_tester.h"
@@ -11,6 +13,7 @@
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/browser/sync/test/integration/updated_progress_marker_checker.h"
+#include "chrome/common/chrome_constants.h"
 #include "chrome/common/pref_names.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/json_pref_store.h"
@@ -34,7 +37,33 @@ class SingleClientPreferencesSyncTest : public SyncTest {
 
   ~SingleClientPreferencesSyncTest() override {}
 
+  // If non-empty, |contents| will be written to the Preferences file of the
+  // profile at |index| before that Profile object is created.
+  void SetPreexistingPreferencesFileContents(int index,
+                                             const std::string& contents) {
+    preexisting_preferences_file_contents_[index] = contents;
+  }
+
+ protected:
+  void BeforeSetupClient(int index,
+                         const base::FilePath& profile_path) override {
+    const std::string& contents = preexisting_preferences_file_contents_[index];
+    if (contents.empty()) {
+      return;
+    }
+
+    base::FilePath pref_path(profile_path.Append(chrome::kPreferencesFilename));
+    ASSERT_TRUE(base::CreateDirectory(profile_path));
+    ASSERT_NE(-1,
+              base::WriteFile(pref_path, contents.c_str(), contents.size()));
+  }
+
  private:
+  // The contents to be written to a profile's Preferences file before the
+  // Profile object is created. If empty, no preexisting file will be written.
+  // The map key corresponds to the profile's index.
+  std::map<int, std::string> preexisting_preferences_file_contents_;
+
   DISALLOW_COPY_AND_ASSIGN(SingleClientPreferencesSyncTest);
 };
 

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

@@ -348,7 +348,8 @@ bool SyncTest::CreateGaiaAccount(const std::string& username,
   return entry->GetHttpStatusCode() == 200;
 }
 
-void SyncTest::BeforeSetupClient(int index) {}
+void SyncTest::BeforeSetupClient(int index,
+                                 const base::FilePath& profile_path) {}
 
 bool SyncTest::CreateProfile(int index) {
   base::FilePath profile_path;
@@ -383,6 +384,8 @@ bool SyncTest::CreateProfile(int index) {
         base::StringPrintf("SyncIntegrationTestClient%d", index));
   }
 
+  BeforeSetupClient(index, profile_path);
+
   if (UsingExternalServers()) {
     // If running against an EXTERNAL_LIVE_SERVER, we signin profiles using real
     // GAIA server. This requires creating profiles with no test hooks.
@@ -435,27 +438,6 @@ Profile* SyncTest::MakeProfileForUISignin(base::FilePath profile_path) {
 }
 
 Profile* SyncTest::MakeTestProfile(base::FilePath profile_path, int index) {
-  const auto& preference_contents_it =
-      preexisting_preferences_file_contents_.find(index);
-  if (preference_contents_it != preexisting_preferences_file_contents_.end() &&
-      !preference_contents_it->second.empty()) {
-    // The profile directory might not exist yet (e.g. for the verifier_
-    // profile).
-    if (!base::PathExists(profile_path) &&
-        !base::CreateDirectory(profile_path)) {
-      LOG(FATAL) << "Could not create profile directory: " << profile_path;
-    }
-    base::FilePath pref_path(profile_path.Append(chrome::kPreferencesFilename));
-    int write_result =
-        base::WriteFile(pref_path, preference_contents_it->second.c_str(),
-                        preference_contents_it->second.size());
-    if (write_result !=
-        static_cast<int>(preference_contents_it->second.size())) {
-      LOG(FATAL) << "Preexisting Preferences file could not be written to "
-                 << pref_path;
-    }
-  }
-
   std::unique_ptr<Profile> profile =
       Profile::CreateProfile(profile_path, profile_delegates_[index].get(),
                              Profile::CREATE_MODE_SYNCHRONOUS);
@@ -598,7 +580,6 @@ bool SyncTest::SetupClients() {
 #endif
 
   for (int i = 0; i < num_clients_; ++i) {
-    BeforeSetupClient(i);
     if (!CreateProfile(i)) {
       return false;
     }
@@ -1203,9 +1184,3 @@ arc::SyncArcPackageHelper* SyncTest::sync_arc_helper() {
   return nullptr;
 #endif
 }
-
-void SyncTest::SetPreexistingPreferencesFileContents(
-    int index,
-    const std::string& contents) {
-  preexisting_preferences_file_contents_[index] = contents;
-}

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

@@ -289,18 +289,14 @@ class SyncTest : public InProcessBrowserTest {
 
   void OnWillCreateBrowserContextServices(content::BrowserContext* context);
 
-  virtual void BeforeSetupClient(int index);
+  // Invoked immediately before creating profile |index| under |profile_path|.
+  virtual void BeforeSetupClient(int index, const base::FilePath& profile_path);
 
   // Implementations of the EnableNotifications() and DisableNotifications()
   // functions defined above.
   void DisableNotificationsImpl();
   void EnableNotificationsImpl();
 
-  // If non-empty, |contents| will be written to the Preferences file of the
-  // profile at |index| before that Profile object is created.
-  void SetPreexistingPreferencesFileContents(int index,
-                                             const std::string& contents);
-
   // Helper to ProfileManager::CreateProfileAsync that creates a new profile
   // used for UI Signin. Blocks until profile is created.
   static Profile* MakeProfileForUISignin(base::FilePath profile_path);
@@ -360,9 +356,8 @@ class SyncTest : public InProcessBrowserTest {
       instance_id::InstanceIDDriver* instance_id_driver,
       content::BrowserContext* context);
 
-  // Helper to Profile::CreateProfile that handles path creation, setting up
-  // preexisting pref files, and registering the created profile  as a testing
-  // profile.
+  // Helper to Profile::CreateProfile that handles path creation. It creates
+  // a profile then registers it as a testing profile.
   Profile* MakeTestProfile(base::FilePath profile_path, int index);
 
   // Helper method used to create a Gaia account at runtime.
@@ -511,11 +506,6 @@ class SyncTest : public InProcessBrowserTest {
   // creation via http requests.
   bool create_gaia_account_at_runtime_;
 
-  // The contents to be written to a profile's Preferences file before the
-  // Profile object is created. If empty, no preexisting file will be written.
-  // The map key corresponds to the profile's index.
-  std::map<int, std::string> preexisting_preferences_file_contents_;
-
   // Disable extension install verification.
   extensions::ScopedInstallVerifierBypassForTest ignore_install_verification_;
 

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

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <map>
 #include <string>
 
 #include "base/guid.h"
@@ -194,34 +195,36 @@ class TwoClientPreferencesSyncTestWithSelfNotifications : public SyncTest {
 IN_PROC_BROWSER_TEST_F(TwoClientPreferencesSyncTestWithSelfNotifications,
                        DISABLED_ShouldKeepLocalDataOnTypeMismatch) {
   ResetSyncForPrimaryAccount();
-  // Client 1 has type-conflicting data in it's pref file. Verify that incoming
-  // values from sync of other type do not modify the local state.
-  SetPreexistingPreferencesFileContents(
-      1, "{\"testing\":{\"my-test-preference\": \"some-string\"}}");
   ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
 
   constexpr char pref_name[] = "testing.my-test-preference";
+  constexpr char string_value[] = "some-string";
+
+  // Client 0 registers a boolean preference, client 1 registers a string.
   GetRegistry(GetProfile(0))
       ->RegisterBooleanPref(pref_name, false,
                             user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
-  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  GetRegistry(GetProfile(1))
+      ->RegisterStringPref(pref_name, "",
+                           user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
 
+  // Set non-default values on both clients.
   ChangeBooleanPref(0, pref_name);
+  ChangeStringPref(1, pref_name, string_value);
   ASSERT_THAT(GetPrefs(0)->GetBoolean(pref_name), Eq(true));
-  GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1));
+  ASSERT_THAT(GetPrefs(1)->GetString(pref_name), Eq(string_value));
 
-  // Verify the value got not stored at client1 (because of type mismatch).
-  scoped_refptr<PrefStore> pref_store =
-      BuildPrefStoreFromPrefsFile(GetProfile(1));
-  const base::Value* result;
-  ASSERT_TRUE(pref_store->GetValue("testing.my-test-preference", &result));
-  EXPECT_THAT(result->GetString(), Eq("some-string"));
+  // Start sync and await until they sync mutually.
+  base::HistogramTester histogram_tester;
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
-  // Verify reads at client1 get served the default value.
-  GetRegistry(GetProfile(1))
-      ->RegisterBooleanPref(pref_name, false,
-                            user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
-  EXPECT_THAT(GetPrefs(1)->GetBoolean(pref_name), Eq(false));
+  // Verify that neither of the clients got updated, because of type mismatch.
+  EXPECT_THAT(GetPrefs(0)->GetBoolean(pref_name), Eq(true));
+  EXPECT_THAT(GetPrefs(1)->GetString(pref_name), Eq(string_value));
+
+  // Only one of the two clients sees the mismatch, the one sync-ing last.
+  histogram_tester.ExpectTotalCount("Sync.Preferences.RemotePrefTypeMismatch",
+                                    1);
 }
 
 // Verifies that priority synced preferences and regular sycned preferences are