[Sync] DeviceInfoService static method and error cleanup.

components/sync/device_info/device_info_service_unittest.cc

Index: components/sync/device_info/device_info_service_unittest.cc
diff --git a/components/sync/device_info/device_info_service_unittest.cc b/components/sync/device_info/device_info_service_unittest.cc
index 85c1040c55b881e73495ba8592e14888cbe949e2..075e48fd04e0830177a8dc03128e21b605595093 100644
--- a/components/sync/device_info/device_info_service_unittest.cc
+++ b/components/sync/device_info/device_info_service_unittest.cc
@@ -41,10 +41,40 @@ using WriteBatch = ModelTypeStore::WriteBatch;
 
 namespace {
 
-std::unique_ptr<DeviceInfo> CreateDeviceInfo() {
+const char kGuidFormat[] = "cache guid %d";
+const char kClientNameFormat[] = "client name %d";
+const char kChromeVersionFormat[] = "chrome version %d";
+const char kSyncUserAgentFormat[] = "sync user agent %d";
+const char kSigninScopedDeviceIdFormat[] = "signin scoped device id %d";
+const sync_pb::SyncEnums::DeviceType kDeviceType =
+    sync_pb::SyncEnums_DeviceType_TYPE_LINUX;
+
+// The |provider_| is first initialized with a model object created with this
+// suffix. Local suffix can be changed by setting the provider and then
+// initializing. Remote data should use other suffixes.
+const int kDefaultLocalSuffix = 0;
+
+DeviceInfoSpecifics CreateSpecifics(int suffix) {
+  DeviceInfoSpecifics specifics;
+  specifics.set_cache_guid(base::StringPrintf(kGuidFormat, suffix));
+  specifics.set_client_name(base::StringPrintf(kClientNameFormat, suffix));
+  specifics.set_device_type(sync_pb::SyncEnums_DeviceType_TYPE_LINUX);
+  specifics.set_sync_user_agent(
+      base::StringPrintf(kSyncUserAgentFormat, suffix));
+  specifics.set_chrome_version(
+      base::StringPrintf(kChromeVersionFormat, suffix));
+  specifics.set_signin_scoped_device_id(
+      base::StringPrintf(kSigninScopedDeviceIdFormat, suffix));
+  return specifics;
+}
+
+std::unique_ptr<DeviceInfo> CreateModel(int suffix) {

[maxbogue, 2016/10/28 20:20:02]

I find CreateModel to be a confusingly vague term. I dunno what is model. Maybe
CreateDeviceInfo?

[skym, 2016/10/28 20:46:16]

Why do you find create model to be confusing? There are two sides. The sync
side, and the model side. The bridge connects those two. Model represents one of
those sides, and they have objects, model objects in fact!

   return base::MakeUnique<DeviceInfo>(
-      "guid_1", "client_1", "Chromium 10k", "Chrome 10k",
-      sync_pb::SyncEnums_DeviceType_TYPE_LINUX, "device_id");
+      base::StringPrintf(kGuidFormat, suffix),
+      base::StringPrintf(kClientNameFormat, suffix),
+      base::StringPrintf(kChromeVersionFormat, suffix),
+      base::StringPrintf(kSyncUserAgentFormat, suffix), kDeviceType,
+      base::StringPrintf(kSigninScopedDeviceIdFormat, suffix));
 }
 
 void VerifyResultIsSuccess(Result result) {
@@ -71,6 +101,11 @@ void VerifyEqual(const DeviceInfoSpecifics& specifics,
             model.signin_scoped_device_id());
 }
 
+void VerifyEqual(const DeviceInfo& model,
+                 const DeviceInfoSpecifics& specifics) {
+  return VerifyEqual(specifics, model);
+}
+
 void VerifyDataBatch(std::map<std::string, DeviceInfoSpecifics> expected,
                      SyncError error,
                      std::unique_ptr<DataBatch> batch) {
@@ -160,10 +195,9 @@ class DeviceInfoServiceTest : public testing::Test,
                               public DeviceInfoTracker::Observer {
  protected:
   DeviceInfoServiceTest()
-      : change_count_(0),
-        store_(ModelTypeStoreTestUtil::CreateInMemoryStoreForTest()),
-        local_device_(new LocalDeviceInfoProviderMock()) {
-    local_device_->Initialize(CreateDeviceInfo());
+      : store_(ModelTypeStoreTestUtil::CreateInMemoryStoreForTest()),
+        provider_(new LocalDeviceInfoProviderMock()) {
+    provider_->Initialize(CreateModel(kDefaultLocalSuffix));
   }
 
   ~DeviceInfoServiceTest() override {
@@ -190,7 +224,7 @@ class DeviceInfoServiceTest : public testing::Test,
   void InitializeService() {
     ASSERT_TRUE(store_);
     service_ = base::MakeUnique<DeviceInfoService>(
-        local_device_.get(),
+        provider_.get(),
         base::Bind(&ModelTypeStoreTestUtil::MoveStoreToCallback,
                    base::Passed(&store_)),
         base::Bind(&DeviceInfoServiceTest::CreateModelTypeChangeProcessor,
@@ -205,35 +239,6 @@ class DeviceInfoServiceTest : public testing::Test,
     base::RunLoop().RunUntilIdle();
   }
 
-  // Generates a specifics object with slightly differing values. Will generate
-  // the same values on each run of a test because a simple counter is used to
-  // vary field values.
-  DeviceInfoSpecifics GenerateTestSpecifics() {
-    int label = ++generated_count_;
-    DeviceInfoSpecifics specifics;
-    specifics.set_cache_guid(base::StringPrintf("cache guid %d", label));
-    specifics.set_client_name(base::StringPrintf("client name %d", label));
-    specifics.set_device_type(sync_pb::SyncEnums_DeviceType_TYPE_LINUX);
-    specifics.set_sync_user_agent(
-        base::StringPrintf("sync user agent %d", label));
-    specifics.set_chrome_version(
-        base::StringPrintf("chrome version %d", label));
-    specifics.set_signin_scoped_device_id(
-        base::StringPrintf("signin scoped device id %d", label));
-    return specifics;
-  }
-
-  std::unique_ptr<DeviceInfoSpecifics> CopyToSpecifics(const DeviceInfo& info) {
-    return DeviceInfoService::CopyToSpecifics(info);
-  }
-
-  // Override to allow specific cache guids.
-  DeviceInfoSpecifics GenerateTestSpecifics(const std::string& guid) {
-    DeviceInfoSpecifics specifics = GenerateTestSpecifics();
-    specifics.set_cache_guid(guid);
-    return specifics;
-  }
-
   // Allows access to the store before that will ultimately be used to
   // initialize the service.
   ModelTypeStore* store() {
@@ -245,11 +250,11 @@ class DeviceInfoServiceTest : public testing::Test,
   int change_count() { return change_count_; }
 
   // Allows overriding the provider before the service is initialized.
-  void set_local_device(std::unique_ptr<LocalDeviceInfoProviderMock> provider) {
+  void set_provider(std::unique_ptr<LocalDeviceInfoProviderMock> provider) {
     ASSERT_FALSE(service_);
-    std::swap(local_device_, provider);
+    std::swap(provider_, provider);
   }
-  LocalDeviceInfoProviderMock* local_device() { return local_device_.get(); }
+  LocalDeviceInfoProviderMock* local_device() { return provider_.get(); }
 
   // Allows access to the service after InitializeService() is called.
   DeviceInfoService* service() {
@@ -280,12 +285,8 @@ class DeviceInfoServiceTest : public testing::Test,
 
   void ForcePulse() { service()->SendLocalData(); }
 
-  Time GetLastUpdateTime(const DeviceInfoSpecifics& specifics) {
-    return DeviceInfoService::GetLastUpdateTime(specifics);
-  }
-
  private:
-  int change_count_;
+  int change_count_ = 0;
 
   // In memory model type store needs a MessageLoop.
   base::MessageLoop message_loop_;
@@ -293,7 +294,9 @@ class DeviceInfoServiceTest : public testing::Test,
   // Holds the store while the service is not initialized.
   std::unique_ptr<ModelTypeStore> store_;
 
-  std::unique_ptr<LocalDeviceInfoProviderMock> local_device_;
+  // Provides information about the local device. Is initialized in each case's
+  // constructor with a model object created from |kDefaultLocalSuffix|.
+  std::unique_ptr<LocalDeviceInfoProviderMock> provider_;
 
   // Not initialized immediately (upon test's constructor). This allows each
   // test case to modify the dependencies the service will be constructed with.
@@ -302,10 +305,6 @@ class DeviceInfoServiceTest : public testing::Test,
   // A non-owning pointer to the processor given to the service. Will be nullptr
   // before being given to the service, to make ownership easier.
   RecordingModelTypeChangeProcessor* processor_ = nullptr;
-
-  // A monotonically increasing label for generated specifics objects with data
-  // that is slightly different from eachother.
-  int generated_count_ = 0;
 };
 
 namespace {
@@ -327,11 +326,11 @@ TEST_F(DeviceInfoServiceTest, EmptyDataReconciliationSlowLoad) {
 }
 
 TEST_F(DeviceInfoServiceTest, LocalProviderSubscription) {
-  set_local_device(base::MakeUnique<LocalDeviceInfoProviderMock>());
+  set_provider(base::MakeUnique<LocalDeviceInfoProviderMock>());
   InitializeAndPump();
 
   EXPECT_EQ(0u, service()->GetAllDeviceInfo().size());
-  local_device()->Initialize(CreateDeviceInfo());
+  local_device()->Initialize(CreateModel(1));
   base::RunLoop().RunUntilIdle();
 
   DeviceInfoList devices = service()->GetAllDeviceInfo();
@@ -341,12 +340,12 @@ TEST_F(DeviceInfoServiceTest, LocalProviderSubscription) {
 
 // Metadata shouldn't be loaded before the provider is initialized.
 TEST_F(DeviceInfoServiceTest, LocalProviderInitRace) {
-  set_local_device(base::MakeUnique<LocalDeviceInfoProviderMock>());
+  set_provider(base::MakeUnique<LocalDeviceInfoProviderMock>());
   InitializeAndPump();
   EXPECT_FALSE(processor()->metadata());
 
   EXPECT_EQ(0u, service()->GetAllDeviceInfo().size());
-  local_device()->Initialize(CreateDeviceInfo());
+  local_device()->Initialize(CreateModel(1));
   base::RunLoop().RunUntilIdle();
 
   DeviceInfoList devices = service()->GetAllDeviceInfo();
@@ -377,7 +376,7 @@ TEST_F(DeviceInfoServiceTest, GetClientTagEmpty) {
 
 TEST_F(DeviceInfoServiceTest, TestWithLocalData) {
   std::unique_ptr<WriteBatch> batch = store()->CreateWriteBatch();
-  DeviceInfoSpecifics specifics = GenerateTestSpecifics();
+  DeviceInfoSpecifics specifics = CreateSpecifics(1);
   store()->WriteData(batch.get(), specifics.cache_guid(),
                      specifics.SerializeAsString());
   store()->CommitWriteBatch(std::move(batch),
@@ -406,7 +405,7 @@ TEST_F(DeviceInfoServiceTest, TestWithLocalMetadata) {
 
 TEST_F(DeviceInfoServiceTest, TestWithLocalDataAndMetadata) {
   std::unique_ptr<WriteBatch> batch = store()->CreateWriteBatch();
-  DeviceInfoSpecifics specifics = GenerateTestSpecifics();
+  DeviceInfoSpecifics specifics = CreateSpecifics(1);
   store()->WriteData(batch.get(), specifics.cache_guid(),
                      specifics.SerializeAsString());
   ModelTypeState state;
@@ -427,9 +426,9 @@ TEST_F(DeviceInfoServiceTest, TestWithLocalDataAndMetadata) {
 
 TEST_F(DeviceInfoServiceTest, GetData) {
   std::unique_ptr<WriteBatch> batch = store()->CreateWriteBatch();
-  DeviceInfoSpecifics specifics1 = GenerateTestSpecifics();
-  DeviceInfoSpecifics specifics2 = GenerateTestSpecifics();
-  DeviceInfoSpecifics specifics3 = GenerateTestSpecifics();
+  DeviceInfoSpecifics specifics1 = CreateSpecifics(1);
+  DeviceInfoSpecifics specifics2 = CreateSpecifics(2);
+  DeviceInfoSpecifics specifics3 = CreateSpecifics(3);
   store()->WriteData(batch.get(), specifics1.cache_guid(),
                      specifics1.SerializeAsString());
   store()->WriteData(batch.get(), specifics2.cache_guid(),
@@ -457,8 +456,8 @@ TEST_F(DeviceInfoServiceTest, GetDataMissing) {
 
 TEST_F(DeviceInfoServiceTest, GetAllData) {
   std::unique_ptr<WriteBatch> batch = store()->CreateWriteBatch();
-  DeviceInfoSpecifics specifics1 = GenerateTestSpecifics();
-  DeviceInfoSpecifics specifics2 = GenerateTestSpecifics();
+  DeviceInfoSpecifics specifics1 = CreateSpecifics(1);
+  DeviceInfoSpecifics specifics2 = CreateSpecifics(2);
   const std::string& guid1 = specifics1.cache_guid();
   const std::string& guid2 = specifics2.cache_guid();
   store()->WriteData(batch.get(), specifics1.cache_guid(),
@@ -488,7 +487,7 @@ TEST_F(DeviceInfoServiceTest, ApplySyncChangesInMemory) {
   InitializeAndPump();
   EXPECT_EQ(1, change_count());
 
-  DeviceInfoSpecifics specifics = GenerateTestSpecifics();
+  DeviceInfoSpecifics specifics = CreateSpecifics(1);
   const SyncError error_on_add = service()->ApplySyncChanges(
       service()->CreateMetadataChangeList(), EntityAddList({specifics}));
 
@@ -512,7 +511,7 @@ TEST_F(DeviceInfoServiceTest, ApplySyncChangesStore) {
   InitializeAndPump();
   EXPECT_EQ(1, change_count());
 
-  DeviceInfoSpecifics specifics = GenerateTestSpecifics();
+  DeviceInfoSpecifics specifics = CreateSpecifics(1);
   ModelTypeState state;
   state.set_encryption_key_name("ekn");
   std::unique_ptr<MetadataChangeList> metadata_changes =
@@ -539,11 +538,9 @@ TEST_F(DeviceInfoServiceTest, ApplySyncChangesStore) {
 TEST_F(DeviceInfoServiceTest, ApplySyncChangesWithLocalGuid) {
   InitializeAndPump();
 
-  // The point of this test is to try to apply remote changes that have the same
-  // cache guid as the local device. The service should ignore these changes
-  // since only it should be performing writes on its data.
-  DeviceInfoSpecifics specifics =
-      GenerateTestSpecifics(local_device()->GetLocalDeviceInfo()->guid());
+  // The service should ignore these changes using this specifics because its
+  // guid will match the local device.
+  DeviceInfoSpecifics specifics = CreateSpecifics(kDefaultLocalSuffix);
 
   // Should have a single change from reconciliation.
   EXPECT_TRUE(
@@ -597,15 +594,14 @@ TEST_F(DeviceInfoServiceTest, MergeEmpty) {
 }
 
 TEST_F(DeviceInfoServiceTest, MergeWithData) {
+  const DeviceInfoSpecifics unique_local = CreateSpecifics(1);
+  DeviceInfoSpecifics conflict_local = CreateSpecifics(2);
+  DeviceInfoSpecifics conflict_remote = CreateSpecifics(3);
+  const DeviceInfoSpecifics unique_remote = CreateSpecifics(4);
+
   const std::string conflict_guid = "conflict_guid";
-  const DeviceInfoSpecifics unique_local =
-      GenerateTestSpecifics("unique_local_guid");
-  const DeviceInfoSpecifics conflict_local =
-      GenerateTestSpecifics(conflict_guid);
-  const DeviceInfoSpecifics conflict_remote =
-      GenerateTestSpecifics(conflict_guid);
-  const DeviceInfoSpecifics unique_remote =
-      GenerateTestSpecifics("unique_remote_guid");
+  conflict_local.set_cache_guid(conflict_guid);
+  conflict_remote.set_cache_guid(conflict_guid);
 
   std::unique_ptr<WriteBatch> batch = store()->CreateWriteBatch();
   store()->WriteData(batch.get(), unique_local.cache_guid(),
@@ -656,11 +652,11 @@ TEST_F(DeviceInfoServiceTest, MergeWithData) {
 }
 
 TEST_F(DeviceInfoServiceTest, MergeLocalGuid) {
-  const DeviceInfo* local_device_info = local_device()->GetLocalDeviceInfo();
+  const DeviceInfo* provider_info = local_device()->GetLocalDeviceInfo();
   std::unique_ptr<DeviceInfoSpecifics> specifics =

[maxbogue, 2016/10/28 20:20:02]

auto

[skym, 2016/10/28 20:46:16]

Done.

-      CopyToSpecifics(*local_device_info);
+      base::MakeUnique<DeviceInfoSpecifics>(CreateSpecifics(0));
   specifics->set_last_updated_timestamp(TimeToProtoTime(Time::Now()));
-  const std::string guid = local_device_info->guid();
+  const std::string guid = provider_info->guid();
 
   std::unique_ptr<WriteBatch> batch = store()->CreateWriteBatch();
   store()->WriteData(batch.get(), guid, specifics->SerializeAsString());
@@ -681,23 +677,11 @@ TEST_F(DeviceInfoServiceTest, MergeLocalGuid) {
   EXPECT_TRUE(processor()->put_multimap().empty());
 }
 
-TEST_F(DeviceInfoServiceTest, GetLastUpdateTime) {
-  Time time1(Time() + TimeDelta::FromDays(1));
-
-  DeviceInfoSpecifics specifics1 = GenerateTestSpecifics();
-  DeviceInfoSpecifics specifics2 = GenerateTestSpecifics();
-  specifics2.set_last_updated_timestamp(TimeToProtoTime(time1));
-
-  EXPECT_EQ(Time(), GetLastUpdateTime(specifics1));
-  EXPECT_EQ(time1, GetLastUpdateTime(specifics2));
-}
-
 TEST_F(DeviceInfoServiceTest, CountActiveDevices) {
   InitializeAndPump();
   EXPECT_EQ(1, service()->CountActiveDevices());
 
-  DeviceInfoSpecifics specifics =
-      GenerateTestSpecifics(local_device()->GetLocalDeviceInfo()->guid());
+  DeviceInfoSpecifics specifics = CreateSpecifics(0);
   service()->ApplySyncChanges(service()->CreateMetadataChangeList(),
                               EntityAddList({specifics}));
   EXPECT_EQ(1, service()->CountActiveDevices());
@@ -711,22 +695,28 @@ TEST_F(DeviceInfoServiceTest, CountActiveDevices) {
   service()->ApplySyncChanges(service()->CreateMetadataChangeList(),
                               EntityAddList({specifics}));
   EXPECT_EQ(2, service()->CountActiveDevices());
+
+  // Now set time to long ago in the past, it should not be active anymore.
+  specifics.set_last_updated_timestamp(TimeToProtoTime(Time()));
+  service()->ApplySyncChanges(service()->CreateMetadataChangeList(),
+                              EntityAddList({specifics}));
+  EXPECT_EQ(1, service()->CountActiveDevices());
 }
 
 TEST_F(DeviceInfoServiceTest, MultipleOnProviderInitialized) {
-  set_local_device(base::MakeUnique<LocalDeviceInfoProviderMock>());
+  set_provider(base::MakeUnique<LocalDeviceInfoProviderMock>());
   InitializeAndPump();
   EXPECT_EQ(nullptr, processor()->metadata());
 
   // Verify the processor was given metadata.
-  local_device()->Initialize(CreateDeviceInfo());
+  local_device()->Initialize(CreateModel(0));
   base::RunLoop().RunUntilIdle();
   const MetadataBatch* metadata = processor()->metadata();
   EXPECT_NE(nullptr, metadata);
 
   // Pointer address of metadata should remain constant because the processor
   // should not have been given new metadata.
-  local_device()->Initialize(CreateDeviceInfo());
+  local_device()->Initialize(CreateModel(0));
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(metadata, processor()->metadata());
 }
@@ -753,7 +743,7 @@ TEST_F(DeviceInfoServiceTest, DisableSync) {
   EXPECT_EQ(1u, service()->GetAllDeviceInfo().size());
   EXPECT_EQ(1, change_count());
 
-  DeviceInfoSpecifics specifics = GenerateTestSpecifics();
+  DeviceInfoSpecifics specifics = CreateSpecifics(1);
   const SyncError error = service()->ApplySyncChanges(
       service()->CreateMetadataChangeList(), EntityAddList({specifics}));