Initial clear server data impl

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

Index: chrome/browser/sync/test/integration/sync_test.cc
diff --git a/chrome/browser/sync/test/integration/sync_test.cc b/chrome/browser/sync/test/integration/sync_test.cc
index f40d1f98fd44fbfb4f595c9aa98392743c0720e2..74fb406ea2b801ff479215b6083850aa55d453db 100644
--- a/chrome/browser/sync/test/integration/sync_test.cc
+++ b/chrome/browser/sync/test/integration/sync_test.cc
@@ -231,7 +231,9 @@ SyncTest::SyncTest(TestType test_type)
   }
 }
 
-SyncTest::~SyncTest() {}
+SyncTest::~SyncTest() {
+  sync_datatype_helper::DissociateWithTest(this);
+}
 
 void SyncTest::SetUp() {
   // Sets |server_type_| if it wasn't specified by the test.
@@ -611,8 +613,22 @@ bool SyncTest::SetupSync() {
     }
   }
 
+  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;
@@ -1139,3 +1155,13 @@ 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.

+}