[Sync] Add a sanity integration test for USS.

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

+// 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 "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "chrome/browser/sync/chrome_sync_client.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
+#include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
+#include "chrome/browser/sync/test/integration/status_change_checker.h"
+#include "chrome/browser/sync/test/integration/sync_integration_test_util.h"
+#include "chrome/browser/sync/test/integration/sync_test.h"
+#include "components/browser_sync/browser/profile_sync_components_factory_impl.h"
+#include "components/sync/api/fake_model_type_service.h"
+
+using browser_sync::ChromeSyncClient;
+using syncer_v2::FakeModelTypeService;
+using syncer_v2::ModelTypeService;
+using syncer_v2::SharedModelTypeProcessor;
+
+// A ChromeSyncClient that provides a ModelTypeService for PREFERENCES.
+class TestSyncClient : public ChromeSyncClient {
+ public:
+  TestSyncClient(Profile* profile, ModelTypeService* service)

[skym, 2016/09/12 19:57:49]

Could probably get away with slapping const on like all of these raw pointers in
this file.

[maxbogue, 2016/09/13 03:44:54]

Umm I've made the member vars in TestSyncClient and KeyChecker Foo* const. I
can't really make any of them const Foo* because they all do non-consty things
like AsWeakPtr, the ChromeSyncClient constructor, registering observers, etc.

+      : ChromeSyncClient(profile), service_(service) {}
+
+  base::WeakPtr<ModelTypeService> GetModelTypeServiceForType(
+      syncer::ModelType type) override {
+    if (type != syncer::PREFERENCES) {

[skym, 2016/09/12 19:57:49]

Might be more elegant as a ternary.

[maxbogue, 2016/09/13 03:44:54]

Done.

+      return ChromeSyncClient::GetModelTypeServiceForType(type);
+    }
+    return service_->AsWeakPtr();
+  }
+
+ private:
+  ModelTypeService* service_;
+};
+
+// A FakeModelTypeService that supports observing ApplySyncChanges.
+class TestModelTypeService : public FakeModelTypeService {
+ public:
+  class Observer {
+   public:
+    virtual void OnApplySyncChanges() = 0;
+  };
+
+  TestModelTypeService()
+      : FakeModelTypeService(
+            base::Bind(&SharedModelTypeProcessor::CreateAsChangeProcessor)) {}
+
+  syncer::SyncError ApplySyncChanges(
+      std::unique_ptr<syncer_v2::MetadataChangeList> metadata_changes,
+      syncer_v2::EntityChangeList entity_changes) override {
+    syncer::SyncError error = FakeModelTypeService::ApplySyncChanges(
+        std::move(metadata_changes), entity_changes);
+    NotifyObservers();
+    return error;
+  }
+
+  void OnChangeProcessorSet() override {
+    change_processor()->OnMetadataLoaded(syncer::SyncError(),
+                                         db().CreateMetadataBatch());
+  }
+
+  void AddObserver(Observer* observer) { observers_.insert(observer); }
+  void RemoveObserver(Observer* observer) { observers_.erase(observer); }
+
+ private:
+  void NotifyObservers() {
+    for (Observer* observer : observers_) {
+      observer->OnApplySyncChanges();
+    }
+  }
+
+  std::set<Observer*> observers_;
+};
+
+// A StatusChangeChecker for checking the status of keys in a
+// TestModelTypeService::Store.
+class KeyChecker : public StatusChangeChecker,
+                   public TestModelTypeService::Observer {
+ public:
+  KeyChecker(TestModelTypeService* service, const std::string& key)
+      : service_(service), key_(key) {}
+  ~KeyChecker() override {}
+
+  void OnApplySyncChanges() override { CheckExitCondition(); }
+
+  void Wait() {
+    if (IsExitConditionSatisfied()) {
+      DVLOG(1) << "Wait() -> Exit before waiting: " << GetDebugMessage();
+      return;
+    }
+
+    service_->AddObserver(this);
+    StartBlockingWait();
+    service_->RemoveObserver(this);
+  }
+
+ protected:
+  TestModelTypeService* service_;
+  const std::string key_;
+};
+
+// Wait for a key to be present.
+class KeyPresentChecker : public KeyChecker {
+ public:
+  KeyPresentChecker(TestModelTypeService* service, const std::string& key)
+      : KeyChecker(service, key) {}
+  ~KeyPresentChecker() override {}
+
+  bool IsExitConditionSatisfied() override {
+    return service_->db().HasData(key_);
+  }
+
+  std::string GetDebugMessage() const override {
+    return "Waiting for key '" + key_ + "' to be present.";
+  }
+};
+
+// Wait for a key to be absent.
+class KeyAbsentChecker : public KeyChecker {
+ public:
+  KeyAbsentChecker(TestModelTypeService* service, const std::string& key)
+      : KeyChecker(service, key) {}
+  ~KeyAbsentChecker() override {}
+
+  bool IsExitConditionSatisfied() override {
+    return !service_->db().HasData(key_);
+  }
+
+  std::string GetDebugMessage() const override {
+    return "Waiting for key '" + key_ + "' to be absent.";
+  }
+};
+
+class TwoClientUssSyncTest : public SyncTest {
+ public:
+  TwoClientUssSyncTest() : SyncTest(TWO_CLIENT) {
+    DisableVerifier();
+    sync_client_factory_ = base::Bind(&TwoClientUssSyncTest::CreateSyncClient,
+                                      base::Unretained(this));
+    ProfileSyncServiceFactory::SetSyncClientFactoryForTest(
+        &sync_client_factory_);
+    ProfileSyncComponentsFactoryImpl::OverridePrefsForUssTest(true);
+  }
+
+  ~TwoClientUssSyncTest() override {
+    ProfileSyncServiceFactory::SetSyncClientFactoryForTest(nullptr);

[skym, 2016/09/12 19:57:49]

All these test cases run in serial, right? Even when testing locally?

[maxbogue, 2016/09/13 03:44:54]

My understanding is that they run in serial within each process even if you do
multiple jobs.

+    ProfileSyncComponentsFactoryImpl::OverridePrefsForUssTest(false);

[skym, 2016/09/12 19:57:50]

Does this get called if you crash mid test? Could these pref values stay around
for other tests?

[maxbogue, 2016/09/13 03:44:54]

The flag is just in memory. If we crash mid-test it isn't persisted anywhere.

+  }
+
+  bool TestUsesSelfNotifications() override { return false; }
+
+  TestModelTypeService* GetModelTypeService(int i) {
+    return services_.at(i).get();
+  }
+
+  static bool WaitForKeyPresent(TestModelTypeService* model,

[skym, 2016/09/12 19:57:49]

Why not push these methods into the checker classes themselves?

[maxbogue, 2016/09/13 03:44:54]

I think this way reads a little better. Otherwise it'd be like
KeyPresentChecker::Wait() instead of WaitForKeyPresent(), etc.

[skym, 2016/09/13 15:54:32]

Remember that you have your arguments in there giving more context,
KeyPresentChecker::Wait(model, "key") , and you could change the function name
to WaitForKey if wanted.

Another approach would be to make Wait() return this->TimedOut();, so you could
replace WaitForKeyPresent(model, "key) with KeyPresentChecker(model,
"key").Wait().

[maxbogue, 2016/09/13 19:15:26]

I've gone with KeyPresentChecker(model, "key").Wait(). I forgot you can have
anonymous things on the stack in C++ -_-

+                                const std::string& key) {
+    KeyPresentChecker checker(model, key);
+    checker.Wait();
+    return !checker.TimedOut();
+  }
+
+  static bool WaitForKeyAbsent(TestModelTypeService* model,
+                               const std::string& key) {
+    KeyAbsentChecker checker(model, key);
+    checker.Wait();
+    return !checker.TimedOut();
+  }
+
+ protected:
+  std::unique_ptr<sync_driver::SyncClient> CreateSyncClient(Profile* profile) {
+    auto service = base::MakeUnique<TestModelTypeService>();
+    auto client = base::MakeUnique<TestSyncClient>(profile, service.get());
+    clients_.push_back(client.get());
+    services_.push_back(std::move(service));
+    return std::move(client);
+  }
+
+  ProfileSyncServiceFactory::SyncClientFactory sync_client_factory_;
+  std::vector<std::unique_ptr<TestModelTypeService>> services_;
+  std::vector<TestSyncClient*> clients_;
+
+  DISALLOW_COPY_AND_ASSIGN(TwoClientUssSyncTest);
+};
+
+IN_PROC_BROWSER_TEST_F(TwoClientUssSyncTest, Sanity) {
+  ASSERT_TRUE(SetupSync());
+  // There are three because the test infra creates a profile before the two

[skym, 2016/09/12 19:57:50]

This seems awkward, like something we may want to fix eventually.

[maxbogue, 2016/09/13 03:44:54]

Yes, but I have no way of overwriting the thing on PSSFactory at the correct
time (between the base test class's SetUp method and the test method). I've
switched to using a boolean to ignore the first call to CreateSyncClient though;
that way it's not something that shows up in each test.

+  // made for our tests. Ignore index 0 and use indexes 1 and 2.

[skym, 2016/09/12 19:57:50]

So, technically 3 client?

[maxbogue, 2016/09/13 03:44:54]

Nothing ever happens with sync in the original profile, so it doesn't affect the
test.

+  ASSERT_EQ(3U, clients_.size());
+  ASSERT_EQ(3U, services_.size());
+  TestModelTypeService* model1 = GetModelTypeService(1);
+  TestModelTypeService* model2 = GetModelTypeService(2);
+
+  model1->WriteItem("foo", "bar");
+  ASSERT_TRUE(WaitForKeyPresent(model2, "foo"));
+  EXPECT_EQ("bar", model2->db().GetValue("foo"));
+
+  model1->DeleteItem("foo");
+  ASSERT_TRUE(WaitForKeyAbsent(model2, "foo"));
+}