[Sync] Add unit tests for remote update handler.

This patch introduces unit tests for the client tag based remote update
handler. Most of cases are copied from the model type processor unit
tests.

Bug: 947044
Change-Id: Iac2bde9c59425760dad5c6847e7b398dec8dfe3a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2152786
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#760439}

components/sync/BUILD.gn

@@ -630,6 +630,7 @@ source_set("unit_tests") {
     "model/sync_data_unittest.cc",
     "model/sync_error_unittest.cc",
     "model_impl/client_tag_based_model_type_processor_unittest.cc",
+    "model_impl/client_tag_based_remote_update_handler_unittest.cc",
     "model_impl/in_memory_metadata_change_list_unittest.cc",
     "model_impl/model_type_store_backend_unittest.cc",
     "model_impl/model_type_store_impl_unittest.cc",

components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc

@@ -1105,56 +1105,6 @@ TEST_F(ClientTagBasedModelTypeProcessorTest, ShouldIgnoreRedundantLocalUpdate) {
   EXPECT_EQ(mtime, type_processor()->GetEntityModificationTime(kKey1));
 }
 
-// Thoroughly tests the data generated by a server item creation.
-TEST_F(ClientTagBasedModelTypeProcessorTest, ShouldProcessRemoteCreation) {
-  InitializeToReadyState();
-  worker()->UpdateFromServer(GetHash(kKey1), GenerateSpecifics(kKey1, kValue1));
-  EXPECT_EQ(1U, db()->data_count());
-  EXPECT_EQ(1U, db()->metadata_count());
-  EXPECT_EQ(1U, ProcessorEntityCount());
-  EXPECT_EQ(0U, worker()->GetNumPendingCommits());
-
-  const EntityData& data = db()->GetData(kKey1);
-  EXPECT_FALSE(data.id.empty());
-  EXPECT_EQ(kKey1, data.specifics.preference().name());
-  EXPECT_EQ(kValue1, data.specifics.preference().value());
-  EXPECT_FALSE(data.creation_time.is_null());
-  EXPECT_FALSE(data.modification_time.is_null());
-  EXPECT_EQ(kKey1, data.name);
-  EXPECT_FALSE(data.is_deleted());
-
-  const EntityMetadata metadata = db()->GetMetadata(kKey1);
-  EXPECT_TRUE(metadata.has_client_tag_hash());
-  EXPECT_TRUE(metadata.has_server_id());
-  EXPECT_FALSE(metadata.is_deleted());
-  EXPECT_EQ(0, metadata.sequence_number());
-  EXPECT_EQ(0, metadata.acked_sequence_number());
-  EXPECT_EQ(1, metadata.server_version());
-  EXPECT_TRUE(metadata.has_creation_time());
-  EXPECT_TRUE(metadata.has_modification_time());
-  EXPECT_TRUE(metadata.has_specifics_hash());
-}
-
-TEST_F(ClientTagBasedModelTypeProcessorTest,
-       ShouldIgnoreRemoteUpdatesForRootNodes) {
-  InitializeToReadyState();
-  UpdateResponseDataList update;
-  update.push_back(worker()->GenerateTypeRootUpdateData(ModelType::SESSIONS));
-
-  worker()->UpdateFromServer(std::move(update));
-  // Root node update should be filtered out.
-  EXPECT_EQ(0U, ProcessorEntityCount());
-}
-
-TEST_F(ClientTagBasedModelTypeProcessorTest,
-       ShouldIgnoreRemoteUpdatesWithUnexpectedClientTagHash) {
-  InitializeToReadyState();
-  worker()->UpdateFromServer(GetHash(kKey2), GenerateSpecifics(kKey1, kValue1));
-  EXPECT_EQ(0U, db()->data_count());
-  EXPECT_EQ(0U, db()->metadata_count());
-  EXPECT_EQ(0U, ProcessorEntityCount());
-}
-
 // Test that an error applying changes from a server update is
 // propagated to the error handler.
 TEST_F(ClientTagBasedModelTypeProcessorTest, ShouldReportErrorApplyingUpdate) {
@@ -1164,27 +1114,6 @@ TEST_F(ClientTagBasedModelTypeProcessorTest, ShouldReportErrorApplyingUpdate) {
   worker()->UpdateFromServer(GetHash(kKey1), GenerateSpecifics(kKey1, kValue1));
 }
 
-// Thoroughly tests the data generated by a server item creation.
-TEST_F(ClientTagBasedModelTypeProcessorTest, ShouldProcessRemoteUpdate) {
-  InitializeToReadyState();
-
-  // Local add writes data and metadata; ack writes metadata again.
-  WriteItemAndAck(kKey1, kValue1);
-  EXPECT_EQ(1U, db()->data_change_count());
-  EXPECT_EQ(2U, db()->metadata_change_count());
-
-  // Redundant update from server doesn't write data but updates metadata.
-  worker()->UpdateFromServer(GetHash(kKey1), GenerateSpecifics(kKey1, kValue1));
-  EXPECT_EQ(1U, db()->data_change_count());
-  EXPECT_EQ(3U, db()->metadata_change_count());
-
-  // A reflection (update already received) is ignored completely.
-  worker()->UpdateFromServer(GetHash(kKey1), GenerateSpecifics(kKey1, kValue1),
-                             0 /* version_offset */);
-  EXPECT_EQ(1U, db()->data_change_count());
-  EXPECT_EQ(3U, db()->metadata_change_count());
-}
-
 // Tests locally deleting an acknowledged item.
 TEST_F(ClientTagBasedModelTypeProcessorTest, ShouldCommitLocalDeletion) {
   InitializeToReadyState();
@@ -1326,18 +1255,6 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
   EXPECT_EQ(0U, worker()->GetNumPendingCommits());
 }
 
-// Deletes an item we've never seen before.
-// Should have no effect and not crash.
-TEST_F(ClientTagBasedModelTypeProcessorTest,
-       ShouldIgnoreRemoteDeletionOfUnknownEntity) {
-  InitializeToReadyState();
-  worker()->TombstoneFromServer(GetHash(kKey1));
-  EXPECT_EQ(0U, db()->data_count());
-  EXPECT_EQ(0U, db()->metadata_count());
-  EXPECT_EQ(0U, ProcessorEntityCount());
-  EXPECT_EQ(0U, worker()->GetNumPendingCommits());
-}
-
 // Tests that after committing entity fails, processor includes this entity in
 // consecutive commits.
 TEST_F(ClientTagBasedModelTypeProcessorTest,

components/sync/model_impl/client_tag_based_remote_update_handler_unittest.cc

+// Copyright 2020 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 "components/sync/model_impl/client_tag_based_remote_update_handler.h"
+
+#include <utility>
+
+#include "components/sync/model/fake_model_type_sync_bridge.h"
+#include "components/sync/model/metadata_batch.h"
+#include "components/sync/model/mock_model_type_change_processor.h"
+#include "components/sync/model_impl/processor_entity_tracker.h"
+#include "components/sync/test/engine/mock_model_type_processor.h"
+#include "components/sync/test/engine/mock_model_type_worker.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+using syncer::FakeModelTypeSyncBridge;
+using syncer::UpdateResponseData;
+using syncer::UpdateResponseDataList;
+
+const char kKey1[] = "key1";
+const char kKey2[] = "key2";
+const char kValue1[] = "value1";
+
+sync_pb::ModelTypeState GenerateModelTypeState() {
+  sync_pb::ModelTypeState model_type_state;
+  model_type_state.set_initial_sync_done(true);
+  return model_type_state;
+}
+
+class ClientTagBasedRemoteUpdateHandlerTest : public ::testing::Test {
+ public:
+  ClientTagBasedRemoteUpdateHandlerTest()
+      : processor_entity_tracker_(GenerateModelTypeState(),
+                                  syncer::EntityMetadataMap()),
+        model_type_sync_bridge_(change_processor_.CreateForwardingProcessor()),
+        remote_update_handler_(syncer::PREFERENCES,
+                               &model_type_sync_bridge_,
+                               &processor_entity_tracker_),
+        worker_(GenerateModelTypeState(), &model_type_processor_) {}
+
+  ~ClientTagBasedRemoteUpdateHandlerTest() override = default;
+
+  void ProcessSingleUpdate(UpdateResponseData update) {
+    UpdateResponseDataList updates;
+    updates.push_back(std::move(update));
+    remote_update_handler_.ProcessIncrementalUpdate(GenerateModelTypeState(),
+                                                    std::move(updates));
+  }
+
+  syncer::UpdateResponseData GenerateUpdate(const std::string& key,
+                                            const std::string& value) {
+    const syncer::ClientTagHash client_tag_hash =
+        FakeModelTypeSyncBridge::TagHashFromKey(key);
+    return GenerateUpdate(client_tag_hash, key, value);
+  }
+
+  syncer::UpdateResponseData GenerateUpdate(
+      const syncer::ClientTagHash& client_tag_hash,
+      const std::string& key,
+      const std::string& value) {
+    return worker()->GenerateUpdateData(
+        client_tag_hash,
+        FakeModelTypeSyncBridge::GenerateSpecifics(key, value));
+  }
+
+  syncer::UpdateResponseData GenerateUpdate(const std::string& key,
+                                            const std::string& value,
+                                            int64_t version_offset) {
+    const syncer::ClientTagHash client_tag_hash =
+        FakeModelTypeSyncBridge::TagHashFromKey(key);
+    const sync_pb::ModelTypeState model_type_state = GenerateModelTypeState();
+    const sync_pb::EntitySpecifics specifics =
+        FakeModelTypeSyncBridge::GenerateSpecifics(key, value);
+    return worker()->GenerateUpdateData(client_tag_hash, specifics,
+                                        version_offset,
+                                        model_type_state.encryption_key_name());
+  }
+
+  size_t ProcessorEntityCount() const {
+    return processor_entity_tracker_.GetAllEntitiesIncludingTombstones().size();
+  }
+
+  FakeModelTypeSyncBridge* bridge() { return &model_type_sync_bridge_; }
+  syncer::ClientTagBasedRemoteUpdateHandler* remote_update_handler() {
+    return &remote_update_handler_;
+  }
+  FakeModelTypeSyncBridge::Store* db() { return bridge()->mutable_db(); }
+  syncer::ProcessorEntityTracker* entity_tracker() {
+    return &processor_entity_tracker_;
+  }
+  testing::NiceMock<syncer::MockModelTypeChangeProcessor>* change_processor() {
+    return &change_processor_;
+  }
+  syncer::MockModelTypeWorker* worker() { return &worker_; }
+
+ private:
+  testing::NiceMock<syncer::MockModelTypeChangeProcessor> change_processor_;
+  syncer::ProcessorEntityTracker processor_entity_tracker_;
+  FakeModelTypeSyncBridge model_type_sync_bridge_;
+  syncer::ClientTagBasedRemoteUpdateHandler remote_update_handler_;
+  testing::NiceMock<syncer::MockModelTypeProcessor> model_type_processor_;
+  syncer::MockModelTypeWorker worker_;
+};
+
+// Thoroughly tests the data generated by a server item creation.
+TEST_F(ClientTagBasedRemoteUpdateHandlerTest, ShouldProcessRemoteCreation) {
+  ProcessSingleUpdate(GenerateUpdate(kKey1, kValue1));
+  EXPECT_EQ(1u, db()->data_count());
+  EXPECT_EQ(1u, db()->metadata_count());
+
+  const syncer::EntityData& data = db()->GetData(kKey1);
+  EXPECT_FALSE(data.id.empty());
+  EXPECT_EQ(kKey1, data.specifics.preference().name());
+  EXPECT_EQ(kValue1, data.specifics.preference().value());
+  EXPECT_FALSE(data.creation_time.is_null());
+  EXPECT_FALSE(data.modification_time.is_null());
+  EXPECT_EQ(kKey1, data.name);
+  EXPECT_FALSE(data.is_deleted());
+
+  const sync_pb::EntityMetadata& metadata = db()->GetMetadata(kKey1);
+  EXPECT_TRUE(metadata.has_client_tag_hash());
+  EXPECT_TRUE(metadata.has_server_id());
+  EXPECT_FALSE(metadata.is_deleted());
+  EXPECT_EQ(0, metadata.sequence_number());
+  EXPECT_EQ(0, metadata.acked_sequence_number());
+  EXPECT_EQ(1, metadata.server_version());
+  EXPECT_TRUE(metadata.has_creation_time());
+  EXPECT_TRUE(metadata.has_modification_time());
+  EXPECT_TRUE(metadata.has_specifics_hash());
+}
+
+TEST_F(ClientTagBasedRemoteUpdateHandlerTest,
+       ShouldIgnoreRemoteUpdatesForRootNodes) {
+  ASSERT_EQ(0U, ProcessorEntityCount());
+  ProcessSingleUpdate(
+      worker()->GenerateTypeRootUpdateData(syncer::ModelType::SESSIONS));
+  // Root node update should be filtered out.
+  EXPECT_EQ(0U, db()->data_count());
+  EXPECT_EQ(0U, db()->metadata_count());
+  EXPECT_EQ(0U, ProcessorEntityCount());
+}
+
+TEST_F(ClientTagBasedRemoteUpdateHandlerTest,
+       ShouldIgnoreRemoteUpdatesWithUnexpectedClientTagHash) {
+  ASSERT_EQ(0U, ProcessorEntityCount());
+  ProcessSingleUpdate(GenerateUpdate(
+      FakeModelTypeSyncBridge::TagHashFromKey(kKey2), kKey1, kValue1));
+  EXPECT_EQ(0U, db()->data_count());
+  EXPECT_EQ(0U, db()->metadata_count());
+  EXPECT_EQ(0U, ProcessorEntityCount());
+}
+
+// Thoroughly tests the data generated by a server item creation.
+TEST_F(ClientTagBasedRemoteUpdateHandlerTest, ShouldProcessRemoteUpdate) {
+  ProcessSingleUpdate(GenerateUpdate(kKey1, kValue1));
+  ASSERT_EQ(1U, ProcessorEntityCount());
+  ASSERT_EQ(1U, db()->data_change_count());
+  ASSERT_EQ(1U, db()->metadata_change_count());
+
+  // Redundant update from server doesn't write data but updates metadata.
+  ProcessSingleUpdate(GenerateUpdate(kKey1, kValue1));
+  EXPECT_EQ(1U, db()->data_change_count());
+  EXPECT_EQ(2U, db()->metadata_change_count());
+
+  // A reflection (update already received) is ignored completely.
+  ProcessSingleUpdate(GenerateUpdate(kKey1, kValue1, /*version_offset=*/0));
+  EXPECT_EQ(1U, db()->data_change_count());
+  EXPECT_EQ(2U, db()->metadata_change_count());
+}
+
+TEST_F(ClientTagBasedRemoteUpdateHandlerTest, ShouldProcessRemoteDeletion) {
+  ProcessSingleUpdate(GenerateUpdate(kKey1, kValue1));
+  ASSERT_EQ(1U, ProcessorEntityCount());
+  ASSERT_EQ(1U, db()->data_change_count());
+  ASSERT_EQ(1U, db()->metadata_change_count());
+
+  ProcessSingleUpdate(worker()->GenerateTombstoneUpdateData(
+      FakeModelTypeSyncBridge::TagHashFromKey(kKey1)));
+  // Delete from server should clear the data and all the metadata.
+  EXPECT_EQ(0U, db()->data_count());
+  EXPECT_EQ(0U, db()->metadata_count());
+  EXPECT_EQ(0U, ProcessorEntityCount());
+}
+
+// Deletes an item we've never seen before. Should have no effect and not crash.
+TEST_F(ClientTagBasedRemoteUpdateHandlerTest,
+       ShouldIgnoreRemoteDeletionOfUnknownEntity) {
+  ASSERT_EQ(0U, ProcessorEntityCount());
+  ProcessSingleUpdate(worker()->GenerateTombstoneUpdateData(
+      FakeModelTypeSyncBridge::TagHashFromKey(kKey1)));
+  EXPECT_EQ(0U, db()->data_count());
+  EXPECT_EQ(0U, db()->metadata_count());
+  EXPECT_EQ(0U, ProcessorEntityCount());
+}
+
+TEST_F(ClientTagBasedRemoteUpdateHandlerTest,
+       ShouldNotTreatMatchingChangesAsConflict) {
+  UpdateResponseData update = GenerateUpdate(kKey1, kValue1);
+  sync_pb::EntitySpecifics specifics = update.entity.specifics;
+  ProcessSingleUpdate(std::move(update));
+  ASSERT_EQ(1U, ProcessorEntityCount());
+  ASSERT_EQ(1U, db()->data_change_count());
+  ASSERT_EQ(1U, db()->metadata_change_count());
+  ASSERT_EQ(1U, db()->GetMetadata(kKey1).server_version());
+
+  // Mark local entity as changed.
+  entity_tracker()->IncrementSequenceNumberForAllExcept({});
+  ASSERT_TRUE(entity_tracker()->HasLocalChanges());
+
+  update = GenerateUpdate(kKey1, kValue1);
+  // Make sure to have the same specifics.
+  update.entity.specifics = specifics;
+  // Changes match doesn't call ResolveConflict.
+  ProcessSingleUpdate(std::move(update));
+
+  EXPECT_EQ(1U, db()->data_change_count());
+  EXPECT_EQ(2U, db()->GetMetadata(kKey1).server_version());
+  EXPECT_EQ(1U, ProcessorEntityCount());
+  EXPECT_FALSE(entity_tracker()->HasLocalChanges());
+}
+
+}  // namespace

components/sync/test/engine/mock_model_type_worker.cc

@@ -210,7 +210,8 @@ syncer::UpdateResponseData MockModelTypeWorker::GenerateTypeRootUpdateData(
   return response_data;
 }
 
-void MockModelTypeWorker::TombstoneFromServer(const ClientTagHash& tag_hash) {
+syncer::UpdateResponseData MockModelTypeWorker::GenerateTombstoneUpdateData(
+    const ClientTagHash& tag_hash) {
   int64_t old_version = GetServerVersion(tag_hash);
   int64_t version = old_version + 1;
   SetServerVersion(tag_hash, version);
@@ -229,9 +230,12 @@ void MockModelTypeWorker::TombstoneFromServer(const ClientTagHash& tag_hash) {
   response_data.entity = std::move(data);
   response_data.response_version = version;
   response_data.encryption_key_name = model_type_state_.encryption_key_name();
+  return response_data;
+}
 
+void MockModelTypeWorker::TombstoneFromServer(const ClientTagHash& tag_hash) {
   UpdateResponseDataList list;
-  list.push_back(std::move(response_data));
+  list.push_back(GenerateTombstoneUpdateData(tag_hash));
   processor_->OnUpdateReceived(model_type_state_, std::move(list));
 }
 

components/sync/test/engine/mock_model_type_worker.h

@@ -105,6 +105,11 @@ class MockModelTypeWorker : public CommitQueue {
   syncer::UpdateResponseData GenerateTypeRootUpdateData(
       const ModelType& model_type);
 
+  // Returns an UpdateResponseData representing an update received from
+  // the server for a deleted entity.
+  syncer::UpdateResponseData GenerateTombstoneUpdateData(
+      const ClientTagHash& tag_hash);
+
   // Triggers a server-side deletion of the entity with |tag_hash|; updates
   // server state accordingly.
   void TombstoneFromServer(const ClientTagHash& tag_hash);