Initial clear server data impl

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

 // Copyright (c) 2012 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/sync_test.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <limits>
@@ skipping 213 matching lines @@
     case TWO_CLIENT:
     case TWO_CLIENT_LEGACY: {
       num_clients_ = 2;
       break;
     }
     default:
       NOTREACHED() << "Invalid test type specified.";
   }
 }
 
-SyncTest::~SyncTest() {}
+SyncTest::~SyncTest() {
+  sync_datatype_helper::DissociateWithTest(this);
+}
 
 void SyncTest::SetUp() {
   // Sets |server_type_| if it wasn't specified by the test.
   DecideServerType();
 
   base::CommandLine* cl = base::CommandLine::ForCurrentProcess();
   if (cl->HasSwitch(switches::kPasswordFileForTest)) {
     ReadPasswordFile();
   } else {
     // Decide on username to use or create one.
@@ skipping 359 matching lines @@
 
 bool SyncTest::SetupSync() {
   // Create sync profiles and clients if they haven't already been created.
   if (profiles_.empty()) {
     if (!SetupClients()) {
       LOG(FATAL) << "SetupClients() failed.";
       return false;
     }
   }
 
+  int clientIndex = 0;
+  if (UsingExternalServers()) {

[skym, 2017/03/07 21:32:25]

Would be good to explain why you clear for external servers.

[wylieb, 2017/03/07 23:04:40]

Done.

+    DVLOG(1) << "Setting up first client for clear.";

[skym, 2017/03/07 21:32:25]

Would be good to explain why we need to setup before calling ClearServerData.

[wylieb, 2017/03/07 23:04:40]

Done.

+    if (!GetClient(clientIndex++)->SetupSyncForClear()) {

[skym, 2017/03/07 21:32:25]

This is clunky how you're using clientIndex++ here, and then hard coding 0 in
the ClearServerData(...) call.

[wylieb, 2017/03/07 23:04:40]

Done.

+      LOG(FATAL) << "SetupSync() failed.";
+      return false;
+    }
+
+    DVLOG(1) << "Done setting up first client for clear.";
+    ClearServerData(GetClient(0));

[skym, 2017/03/07 21:32:25]

Probably need to give this function a return value and check it as well.

[wylieb, 2017/03/07 23:04:40]

Done.

+  }
+
   // Sync each of the profiles.
-  for (int i = 0; i < num_clients_; ++i) {
+  for (int i = clientIndex; i < num_clients_; ++i) {

[skym, 2017/03/07 21:32:25]

Could reuse clientIndex instead of making a new variable

for (; clientIndex < num_clients_; ++clientIndex) {

[wylieb, 2017/03/07 23:04:40]

Done.

+    DVLOG(1) << "Setting up " << i << " client";
+

[skym, 2017/03/07 21:32:25]

I'd remove this newline to keep it consistent with the above lines.

[wylieb, 2017/03/07 23:04:40]

Done.

     if (!GetClient(i)->SetupSync()) {
       LOG(FATAL) << "SetupSync() failed.";
       return false;
     }
   }
 
   // Because clients may modify sync data as part of startup (for example local
   // session-releated data is rewritten), we need to ensure all startup-based
   // changes have propagated between the clients.
   //
@@ skipping 506 matching lines @@
 
 void SyncTest::TriggerSyncForModelTypes(int index,
                                         syncer::ModelTypeSet model_types) {
   GetSyncService(index)->TriggerRefresh(model_types);
 }
 
 void SyncTest::SetPreexistingPreferencesFileContents(
     const std::string& contents) {
   preexisting_preferences_file_contents_ = contents;
 }
+
+void SyncTest::ClearServerData(ProfileSyncServiceHarness* harness) {
+  // Start configuration, at this point our birthday is out of date.

[skym, 2017/03/07 21:32:25]

Errm, is that really true? Shouldn't our birthday only be out of date after we
call clear server data and the callback is invoked below?

[wylieb, 2017/03/07 23:04:40]

Done.

+  base::RunLoop run_loop;
+  harness->service()->ClearServerDataForTest(run_loop.QuitClosure());
+  run_loop.Run();
+
+  // Restart sync, this will get our new birthday from the server.
+  harness->RestartSyncService();

[skym, 2017/03/07 21:32:25]

It doesn't look like the success of this method is being verified.

[wylieb, 2017/03/07 23:04:40]

Done.

+}