[Sync] ModelTypeStore::ReadMetadataCallback now returns a MetadataBatch

components/sync/model_impl/model_type_store_impl_unittest.cc

Index: components/sync/model_impl/model_type_store_impl_unittest.cc
diff --git a/components/sync/model_impl/model_type_store_impl_unittest.cc b/components/sync/model_impl/model_type_store_impl_unittest.cc
index 6d3202ad038aad12b122b93764e8bdbca2b66da2..1d93fbad52102560dfe0d8f864d5aa4eb7325527 100644
--- a/components/sync/model_impl/model_type_store_impl_unittest.cc
+++ b/components/sync/model_impl/model_type_store_impl_unittest.cc
@@ -9,6 +9,8 @@
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
+#include "components/sync/protocol/entity_metadata.pb.h"
+#include "components/sync/protocol/model_type_state.pb.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -62,10 +64,13 @@ class ModelTypeStoreImplTest : public testing::Test {
 
   static void WriteMetadata(ModelTypeStore* store,
                             const std::string& key,
-                            const std::string& metadata) {
-    std::unique_ptr<ModelTypeStore::WriteBatch> write_batch =
-        store->CreateWriteBatch();
-    store->WriteMetadata(write_batch.get(), key, metadata);
+                            const std::string& value) {
+    sync_pb::EntityMetadata metadata;
+    metadata.set_client_tag_hash(value);
+
+    auto write_batch = store->CreateWriteBatch();
+    write_batch->GetMetadataChangeList()->UpdateMetadata(key, metadata);
+
     ModelTypeStore::Result result;
     store->CommitWriteBatch(std::move(write_batch),
                             base::Bind(&CaptureResult, &result));
@@ -75,9 +80,12 @@ class ModelTypeStoreImplTest : public testing::Test {
 
   static void WriteGlobalMetadata(ModelTypeStore* store,
                                   const std::string& metadata) {
-    std::unique_ptr<ModelTypeStore::WriteBatch> write_batch =
-        store->CreateWriteBatch();
-    store->WriteGlobalMetadata(write_batch.get(), metadata);
+    sync_pb::ModelTypeState state;
+    state.set_encryption_key_name(metadata);
+
+    auto write_batch = store->CreateWriteBatch();
+    write_batch->GetMetadataChangeList()->UpdateModelTypeState(state);
+
     ModelTypeStore::Result result;
     store->CommitWriteBatch(std::move(write_batch),
                             base::Bind(&CaptureResult, &result));
@@ -95,17 +103,17 @@ class ModelTypeStoreImplTest : public testing::Test {
   static void ReadStoreContents(
       ModelTypeStore* store,
       std::unique_ptr<ModelTypeStore::RecordList>* data_records,
-      std::unique_ptr<ModelTypeStore::RecordList>* metadata_records,
-      std::string* global_metadata) {
+      std::unique_ptr<MetadataBatch>* metadata_batch) {
     ModelTypeStore::Result result;
     store->ReadAllData(
-        base::Bind(&CaptureResultWithRecords, &result, data_records));
+        base::Bind(&CaptureResultAndRecords, &result, data_records));
     PumpLoop();
     ASSERT_EQ(ModelTypeStore::Result::SUCCESS, result);
-    store->ReadAllMetadata(base::Bind(&CaptureResutRecordsAndString, &result,
-                                      metadata_records, global_metadata));
+    SyncError sync_error;
+    store->ReadAllMetadata(base::Bind(&CaptureSyncErrorAndMetadataBatch,
+                                      &sync_error, metadata_batch));
     PumpLoop();
-    ASSERT_EQ(ModelTypeStore::Result::SUCCESS, result);
+    ASSERT_FALSE(sync_error.IsSet());

[skym, 2016/11/05 00:45:43]

Doesn't seem like there a case that expects an actual error.

[maxbogue, 2016/11/08 03:58:55]

Actually did this now, whoops.

   }
 
   // Following functions capture parameters passed to callbacks into variables
@@ -116,7 +124,7 @@ class ModelTypeStoreImplTest : public testing::Test {
     *dst = result;
   }
 
-  static void CaptureResultWithRecords(
+  static void CaptureResultAndRecords(
       ModelTypeStore::Result* dst_result,
       std::unique_ptr<ModelTypeStore::RecordList>* dst_records,
       ModelTypeStore::Result result,
@@ -125,19 +133,16 @@ class ModelTypeStoreImplTest : public testing::Test {
     *dst_records = std::move(records);
   }
 
-  static void CaptureResutRecordsAndString(
-      ModelTypeStore::Result* dst_result,
-      std::unique_ptr<ModelTypeStore::RecordList>* dst_records,
-      std::string* dst_value,
-      ModelTypeStore::Result result,
-      std::unique_ptr<ModelTypeStore::RecordList> records,
-      const std::string& value) {
-    *dst_result = result;
-    *dst_records = std::move(records);
-    *dst_value = value;
+  static void CaptureSyncErrorAndMetadataBatch(
+      SyncError* dst_sync_error,
+      std::unique_ptr<MetadataBatch>* dst_batch,
+      SyncError sync_error,
+      std::unique_ptr<MetadataBatch> batch) {
+    *dst_sync_error = sync_error;
+    *dst_batch = std::move(batch);
   }
 
-  static void CaptureResutRecordsAndIdList(
+  static void CaptureResultRecordsAndIdList(
       ModelTypeStore::Result* dst_result,
       std::unique_ptr<ModelTypeStore::RecordList>* dst_records,
       std::unique_ptr<ModelTypeStore::IdList>* dst_id_list,
@@ -149,6 +154,21 @@ class ModelTypeStoreImplTest : public testing::Test {
     *dst_id_list = std::move(missing_id_list);
   }
 
+  void ExpectMetadata(std::unique_ptr<MetadataBatch> batch,

[skym, 2016/11/05 00:45:43]

Verify? :)

[maxbogue, 2016/11/07 17:03:39]

Whoops, forgot. Done!

+                      const std::string& global_metadata,
+                      std::map<std::string, std::string> expected_metadata) {

[skym, 2016/11/05 00:45:43]

expected_storage_key_to_client_tag_hash ?

[maxbogue, 2016/11/07 17:03:39]

I'm keeping the weird way I'm stuffing the strings into the protos transparent
to the test cases.

+    EXPECT_EQ(global_metadata,
+              batch->GetModelTypeState().encryption_key_name());

[skym, 2016/11/05 00:45:43]

This seems wrong. Should be something like
batch->GetModelTypeState().AsSerializedString(). Or |global_metadata| needs a
rename.

[maxbogue, 2016/11/07 17:03:40]

Same as above. WriteGlobalMetadata just stuffs a string into that field because
it's the one it can. The tests don't need to care.

+    EntityMetadataMap actual_metadata = batch->TakeAllMetadata();
+    for (auto kv : expected_metadata) {

[skym, 2016/11/05 00:45:42]

kv&

[maxbogue, 2016/11/07 17:03:39]

const auto&, but done.

+      auto it = actual_metadata.find(kv.first);
+      ASSERT_TRUE(it != actual_metadata.end());
+      EXPECT_EQ(kv.second, it->second.client_tag_hash());
+      actual_metadata.erase(it);
+    }
+    EXPECT_EQ(0U, actual_metadata.size());
+  }
+
  private:
   base::MessageLoop message_loop_;
   std::unique_ptr<ModelTypeStore> store_;
@@ -175,13 +195,11 @@ TEST_F(ModelTypeStoreImplTest, ReadEmptyStore) {
   CreateStore();
 
   std::unique_ptr<ModelTypeStore::RecordList> data_records;
-  std::unique_ptr<ModelTypeStore::RecordList> metadata_records;
-  std::string global_metadata;
-  ReadStoreContents(store(), &data_records, &metadata_records,
-                    &global_metadata);
+  std::unique_ptr<MetadataBatch> metadata_batch;
+  ReadStoreContents(store(), &data_records, &metadata_batch);
   ASSERT_TRUE(data_records->empty());
-  ASSERT_TRUE(metadata_records->empty());
-  ASSERT_TRUE(global_metadata.empty());
+  ExpectMetadata(std::move(metadata_batch), "",
+                 std::map<std::string, std::string>());
 }
 
 // Test that records that are written to store later can be read from it.
@@ -190,16 +208,13 @@ TEST_F(ModelTypeStoreImplTest, WriteThenRead) {
   WriteTestData();
 
   std::unique_ptr<ModelTypeStore::RecordList> data_records;
-  std::unique_ptr<ModelTypeStore::RecordList> metadata_records;
-  std::string global_metadata;
-  ReadStoreContents(store(), &data_records, &metadata_records,
-                    &global_metadata);
+  std::unique_ptr<MetadataBatch> metadata_batch;
+  ReadStoreContents(store(), &data_records, &metadata_batch);
   ASSERT_THAT(*data_records,
               testing::UnorderedElementsAre(RecordMatches("id1", "data1"),
                                             RecordMatches("id2", "data2")));
-  ASSERT_THAT(*metadata_records,
-              testing::ElementsAre(RecordMatches("id1", "metadata1")));
-  ASSERT_EQ("global_metadata", global_metadata);
+  ExpectMetadata(std::move(metadata_batch), "global_metadata",
+                 {{"id1", "metadata1"}});
 }
 
 // Test that if global metadata is not set then ReadAllMetadata still succeeds
@@ -218,15 +233,13 @@ TEST_F(ModelTypeStoreImplTest, MissingGlobalMetadata) {
   PumpLoop();
   ASSERT_EQ(ModelTypeStore::Result::SUCCESS, result);
 
-  std::unique_ptr<ModelTypeStore::RecordList> records;
-  std::string global_metadata;
-  store()->ReadAllMetadata(base::Bind(&CaptureResutRecordsAndString, &result,
-                                      &records, &global_metadata));
+  SyncError error;
+  std::unique_ptr<MetadataBatch> metadata_batch;
+  store()->ReadAllMetadata(
+      base::Bind(&CaptureSyncErrorAndMetadataBatch, &error, &metadata_batch));
   PumpLoop();
-  ASSERT_EQ(ModelTypeStore::Result::SUCCESS, result);
-  ASSERT_THAT(*records,
-              testing::UnorderedElementsAre(RecordMatches("id1", "metadata1")));
-  ASSERT_EQ(std::string(), global_metadata);
+  ASSERT_FALSE(error.IsSet());
+  ExpectMetadata(std::move(metadata_batch), "", {{"id1", "metadata1"}});
 }
 
 // Test that when reading data records by id, if one of the ids is missing
@@ -244,7 +257,7 @@ TEST_F(ModelTypeStoreImplTest, ReadMissingDataRecords) {
   std::unique_ptr<ModelTypeStore::RecordList> records;
   std::unique_ptr<ModelTypeStore::IdList> missing_id_list;
 
-  store()->ReadData(id_list, base::Bind(&CaptureResutRecordsAndIdList, &result,
+  store()->ReadData(id_list, base::Bind(&CaptureResultRecordsAndIdList, &result,
                                         &records, &missing_id_list));
   PumpLoop();
   ASSERT_EQ(ModelTypeStore::Result::SUCCESS, result);
@@ -270,26 +283,19 @@ TEST_F(ModelTypeStoreImplTest, TwoStoresWithSharedBackend) {
   WriteGlobalMetadata(store_2.get(), "global2");
 
   std::unique_ptr<ModelTypeStore::RecordList> data_records;
-  std::unique_ptr<ModelTypeStore::RecordList> metadata_records;
-  std::string global_metadata;
+  std::unique_ptr<MetadataBatch> metadata_batch;
 
-  ReadStoreContents(store_1.get(), &data_records, &metadata_records,
-                    &global_metadata);
+  ReadStoreContents(store_1.get(), &data_records, &metadata_batch);
 
   EXPECT_THAT(*data_records,
               testing::ElementsAre(RecordMatches("key", "data1")));
-  EXPECT_THAT(*metadata_records,
-              testing::ElementsAre(RecordMatches("key", "metadata1")));
-  EXPECT_EQ("global1", global_metadata);
+  ExpectMetadata(std::move(metadata_batch), "global1", {{"key", "metadata1"}});
 
-  ReadStoreContents(store_2.get(), &data_records, &metadata_records,
-                    &global_metadata);
+  ReadStoreContents(store_2.get(), &data_records, &metadata_batch);
 
   EXPECT_THAT(*data_records,
               testing::ElementsAre(RecordMatches("key", "data2")));
-  EXPECT_THAT(*metadata_records,
-              testing::ElementsAre(RecordMatches("key", "metadata2")));
-  EXPECT_EQ("global2", global_metadata);
+  ExpectMetadata(std::move(metadata_batch), "global2", {{"key", "metadata2"}});
 }
 
 }  // namespace syncer