Add integraton test for sync backup/rollback.

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

+// Copyright (c) 2014 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 "base/command_line.h"
+#include "base/message_loop/message_loop.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sync/profile_sync_service.h"
+#include "chrome/browser/sync/test/integration/bookmarks_helper.h"
+#include "chrome/browser/sync/test/integration/sync_integration_test_util.h"
+#include "chrome/browser/sync/test/integration/sync_test.h"
+#include "chrome/common/chrome_switches.h"
+#include "components/bookmarks/browser/bookmark_model.h"
+#include "sync/test/fake_server/fake_server_verifier.h"
+
+using bookmarks_helper::AddFolder;
+using bookmarks_helper::AddURL;
+using bookmarks_helper::GetOtherNode;
+using bookmarks_helper::ModelMatchesVerifier;
+using bookmarks_helper::Move;
+using bookmarks_helper::Remove;
+using sync_integration_test_util::AwaitCommitActivityCompletion;
+
+#if defined(OS_WIN) || defined(OS_MACOSX) || (defined(OS_LINUX) && !defined(OS_CHROMEOS))

[pval...(no longer on Chromium), 2014/06/03 22:08:18]

I think conditionally disabling the test on Chrome SS is a better pattern to
follow.

For an example, see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...

[rlarocque, 2014/06/03 22:59:48]

It would be even better if you could disable only the execution of the test,
while still compiling it for all platforms.  Large #ifdefs make it more likely
that compiles will fail on one platform but not another.  That's less of a
problem in unit tests, but it's still a problem.

I'd recommend using the pattern:
#ifdef OS_CHROMEOS
#define MAYBE_SyncClientBackupRollbackTest DISABLED_...
#else
#define MAYBE_SyncClientBackupRollbackTest SyncClientBackupRollbackTest
#endif.

[haitaol1, 2014/06/04 18:21:30]

Done.

[haitaol1, 2014/06/04 18:21:30]

Done.

+
+class SingleClientBackupRollbackTest : public SyncTest {
+ public:
+  SingleClientBackupRollbackTest() : SyncTest(SINGLE_CLIENT) {}
+  virtual ~SingleClientBackupRollbackTest() {}
+
+  virtual void SetUp() OVERRIDE {
+    CommandLine::ForCurrentProcess()->AppendSwitch(
+          switches::kSyncEnableBackupRollback);
+    SyncTest::SetUp();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SingleClientBackupRollbackTest);
+};
+
+class BackupModeChecker {

[rlarocque, 2014/06/03 22:59:48]

I think there are better ways to test than this.

Polling tends to be flaky.  Speaking abstractly, it's possible that the state
could briefly change from X to Y to X again in between polls and this test would
never notice most of the time.  Except that every now and then there would be
one lucky trybot or buildbot that polls at state Y and behaves differently.

The second issue is that we should avoid verifications against internal state. 
It makes it harder to refactor that internal state later on.  More importantly,
this internal state is not visible to users.

I can see that you're basing several expectations on bookmarks appearing and
disappearing.  Could you make a checker that registers as a bookmarks observer
and verifies that state instead?  That way we'd have more confidence that the
bookmarks the user sees are being modified as expected.

The ProfilesMatchStatusChecker in autofill_helper.cc provides an example of how
this sort of test can be implemented.

[haitaol1, 2014/06/04 18:21:30]

The exit condition here is not bookmark change. I want to wait until PSS has
backed up all user's data, which is true when association is finished.

On 2014/06/03 22:59:48, rlarocque wrote:

+ public:
+  explicit BackupModeChecker(ProfileSyncService* service,
+                             base::TimeDelta timeout)
+      : pss_(service),
+        expiration_(base::TimeTicks::Now() + timeout) {}
+
+  bool Wait() {
+    base::MessageLoop::current()->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&BackupModeChecker::PeriodicCheck, base::Unretained(this)),
+        base::TimeDelta::FromSeconds(1));
+    base::MessageLoop::current()->Run();
+    return IsBackupComplete();
+  }
+
+ private:
+  void PeriodicCheck() {
+    if (IsBackupComplete() || base::TimeTicks::Now() > expiration_) {
+      base::MessageLoop::current()->Quit();
+    } else {
+      base::MessageLoop::current()->PostDelayedTask(
+          FROM_HERE,
+          base::Bind(&BackupModeChecker::PeriodicCheck, base::Unretained(this)),
+          base::TimeDelta::FromSeconds(1));
+    }
+  }
+
+  bool IsBackupComplete() {
+    return pss_->backend_mode() == ProfileSyncService::BACKUP &&

[pval...(no longer on Chromium), 2014/06/03 22:08:18]

Richard knows more about verifying client state than I do, so I'll let him
comment on this. My concern here is that we're modifying PSS only for test
verification. Is there any other existing way we can verify that backup/rollback
has completed? 

[haitaol1, 2014/06/04 18:21:30]

Yes, Richard pointed to an existing funciton.

On 2014/06/03 22:08:18, pvalenzuela wrote:

+        pss_->is_syncing();

[pval...(no longer on Chromium), 2014/06/03 22:08:18]

Based on the changes to PSS in this CL, doesn't pss_->is_syncing() return false
when backup is complete?

[haitaol1, 2014/06/04 18:21:30]

Backup is a continuous process as long as user's not signed in.

On 2014/06/03 22:08:18, pvalenzuela wrote:

+  }
+
+  ProfileSyncService* pss_;
+  base::TimeTicks expiration_;
+};
+
+IN_PROC_BROWSER_TEST_F(SingleClientBackupRollbackTest, Sanity) {
+  logging::SetMinLogLevel(-1);

[rlarocque, 2014/06/04 18:30:29]

Remove this before commit?

+  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+
+  // Starting state:
+  // other_node
+  //    -> top
+  //      -> tier1_a
+  //        -> http://mail.google.com  "tier1_a_url0"
+  //      -> tier1_b
+  //        -> http://www.nhl.com "tier1_b_url0"
+  const BookmarkNode* top = AddFolder(0, GetOtherNode(0), 0, L"top");
+  const BookmarkNode* tier1_a = AddFolder(0, top, 0, L"tier1_a");
+  const BookmarkNode* tier1_b = AddFolder(0, top, 1, L"tier1_b");
+  AddURL(0, tier1_a, 0, L"tier1_a_url0", GURL("http://mail.google.com"));
+  AddURL(0, tier1_b, 0, L"tier1_b_url0", GURL("http://www.nhl.com"));
+
+  BackupModeChecker checker(GetSyncService(0),
+                            base::TimeDelta::FromSeconds(15));
+  ASSERT_TRUE(checker.Wait());

[pval...(no longer on Chromium), 2014/06/03 22:08:18]

Why wait for the checker here? As far as I can tell no rollback is happening at
this point.

[haitaol1, 2014/06/04 18:21:30]

To make sure backup is finished in order to set rollback expections.

On 2014/06/03 22:08:18, pvalenzuela wrote:

[rlarocque, 2014/06/04 18:30:29]

In the real world, outside of tests, what would stop a user from signing in and
setting up sync before the backup is finished?

+
+  // Setup sync, wait for its completion, and make sure changes were synced.
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(AwaitCommitActivityCompletion(GetSyncService((0))));
+  ASSERT_TRUE(ModelMatchesVerifier(0));
+
+  // Made bookmark changes while sync is on.
+  Move(0, tier1_a->GetChild(0), tier1_b, 1);
+  Remove(0, tier1_b, 0);
+  AddFolder(0, tier1_b, 1, L"tier2_c");
+  ASSERT_TRUE(AwaitCommitActivityCompletion(GetSyncService((0))));
+  ASSERT_TRUE(ModelMatchesVerifier(0));
+
+  // Let server to return rolllback command on next sync request.
+  GetFakeServer()->TriggerError(sync_pb::SyncEnums::USER_ROLLBACK);
+
+  // Make another change to trigger downloading of rollback command.
+  Remove(0, tier1_b, 0);
+
+  // Wait for sync to switch to backup mode after finishing rollback.
+  ASSERT_TRUE(checker.Wait());
+
+  // Verify bookmarks are restored.
+  ASSERT_EQ(1, tier1_a->child_count());
+  const BookmarkNode* url1 = tier1_a->GetChild(0);
+  ASSERT_EQ(GURL("http://mail.google.com"), url1->url());
+
+  ASSERT_EQ(1, tier1_b->child_count());
+  const BookmarkNode* url2 = tier1_b->GetChild(0);
+  ASSERT_EQ(GURL("http://www.nhl.com"), url2->url());
+}
+
+#endif