sync: Remove the remaining conflict sets code

chrome/browser/sync/engine/apply_updates_command_unittest.cc

Index: chrome/browser/sync/engine/apply_updates_command_unittest.cc
diff --git a/chrome/browser/sync/engine/apply_updates_command_unittest.cc b/chrome/browser/sync/engine/apply_updates_command_unittest.cc
index 19bc480c9f6c4e771dab874833d2d65a1309b34e..87562184c1b2305a66bf8fc330571641c216bad8 100644
--- a/chrome/browser/sync/engine/apply_updates_command_unittest.cc
+++ b/chrome/browser/sync/engine/apply_updates_command_unittest.cc
@@ -143,6 +143,38 @@ class ApplyUpdatesCommandTest : public SyncerCommandTest {
       *metahandle_out = entry.Get(syncable::META_HANDLE);
   }
 
+  void CreateUnappliedAndUnsyncedItem(const Id& item_id,

[akalin, 2012/02/01 01:10:24]

comment that metahandle_out can be NULL

+                                      const Id& parent_id,
+                                      const string& name,
+                                      bool is_folder,
+                                      syncable::ModelType model_type,
+                                      int64* metahandle_out) {
+    ASSERT_TRUE(item_id.ServerKnows()) << "CreateUnappliedAndUnsyncedItem()"

[akalin, 2012/02/01 01:10:24]

just to make sure you know, ASSERT_TRUE just does, roughly:

if (!condition) {
  ADD_FAILURE() << "...";
  return;
}

i.e., this check failing only halts the current function, not the entire test. 
So if this isn't what you want, you have to make this function return a bool or
something.

Although it may be okay in this case, since the test won't crash if you proceed.

[rlarocque, 2012/02/02 00:32:56]

The idea of this assert was to catch logic errors.  I will replace it with a
CHECK().  I realize that's not ideal, because it could prevent us from running
other unit tests.

In practice, though, the only time this condition will fail would be if someone
is modifying the tests.  Changes to logic in the main program should not affect
it.  

+        << " requires an 'item_id' argument that passes ServerKnows().";
+
+    // If our caller did not specify a location to store the returned
+    // metahandle, then use our own temporary storage.
+    int64 local_metahandle_out;

[akalin, 2012/02/01 01:10:24]

initialize this int64 (just paranoia)

[rlarocque, 2012/02/02 00:32:56]

Done.

+    if (!metahandle_out)
+      metahandle_out = &local_metahandle_out;
+
+    CreateUnsyncedItem(item_id, parent_id, name, is_folder,
+                       model_type, metahandle_out);
+
+    ScopedDirLookup dir(syncdb()->manager(), syncdb()->name());
+    ASSERT_TRUE(dir.good());
+    WriteTransaction trans(FROM_HERE, UNITTEST, dir);
+    MutableEntry entry(&trans, syncable::GET_BY_HANDLE, *metahandle_out);
+    ASSERT_TRUE(entry.good());
+
+    entry.Put(syncable::IS_UNAPPLIED_UPDATE, true);
+    entry.Put(syncable::SERVER_VERSION, next_revision_++);
+  }
+
+  int64 GetNextRevision() {
+    return next_revision_++;
+  }
+
   ApplyUpdatesCommand apply_updates_command_;
   TestIdFactory id_factory_;
  private:
@@ -171,6 +203,10 @@ TEST_F(ApplyUpdatesCommandTest, Simple) {
   ASSERT_TRUE(status->conflict_progress());
   EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize())
       << "Simple update shouldn't result in conflicts";
+  EXPECT_EQ(0, status->conflict_progress()->NonblockingConflictingItemsSize())
+      << "Simple update shouldn't result in conflicts";
+  EXPECT_EQ(0, status->conflict_progress()->HierarchyConflictingItemsSize())
+      << "Simple update shouldn't result in conflicts";
   EXPECT_EQ(2, status->update_progress()->SuccessfullyAppliedUpdateCount())
       << "All items should have been successfully applied";
 }
@@ -210,7 +246,195 @@ TEST_F(ApplyUpdatesCommandTest, UpdateWithChildrenBeforeParents) {
       << "All updates should have been successfully applied";
 }
 
-TEST_F(ApplyUpdatesCommandTest, NestedItemsWithUnknownParent) {
+TEST_F(ApplyUpdatesCommandTest, SimpleConflict) {

[akalin, 2012/02/01 01:10:24]

Please describe what the test does in a comment above TEST_F

+  string root_server_id = syncable::GetNullId().GetServerId();
+  CreateUnappliedAndUnsyncedItem(id_factory_.MakeServer("item"),
+                                 id_factory_.root(), "item", false,
+                                 syncable::BOOKMARKS, NULL);
+
+  ExpectGroupToChange(apply_updates_command_, GROUP_UI);
+  apply_updates_command_.ExecuteImpl(session());
+
+  sessions::StatusController* status = session()->mutable_status_controller();
+  sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI);
+  ASSERT_TRUE(status->conflict_progress());
+  EXPECT_EQ(1, status->conflict_progress()->ConflictingItemsSize())
+      << "Unsynced and unapplied item should be a simple conflict";
+}
+
+TEST_F(ApplyUpdatesCommandTest, HierarchyAndSimpleConflict) {

[akalin, 2012/02/01 01:10:24]

here too

+  int64 handle;

[akalin, 2012/02/01 01:10:24]

initialize (paranoia, although if CreateUnapplied... fails, then you can get
non-deterministic behavior since you use handle)

[rlarocque, 2012/02/02 00:32:56]

Done.

+  // Create a simply-conflicting item.  It will start with valid parent ids.
+  CreateUnappliedAndUnsyncedItem(id_factory_.MakeServer("orphaned_by_server"),
+                                 id_factory_.root(), "orphaned_by_server",
+                                 false, syncable::BOOKMARKS, &handle);
+  {
+    // Manually set the SERVER_PARENT_ID to bad value.
+    // A bad parent indicates a hierarchy conflict.
+    ScopedDirLookup dir(syncdb()->manager(), syncdb()->name());
+    ASSERT_TRUE(dir.good());
+    WriteTransaction trans(FROM_HERE, UNITTEST, dir);
+    MutableEntry entry(&trans, syncable::GET_BY_HANDLE, handle);
+    ASSERT_TRUE(entry.good());
+
+    entry.Put(syncable::SERVER_PARENT_ID,
+              id_factory_.MakeServer("bogus_parent"));
+  }
+
+  ExpectGroupToChange(apply_updates_command_, GROUP_UI);
+  apply_updates_command_.ExecuteImpl(session());
+
+  sessions::StatusController* status = session()->mutable_status_controller();
+  sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI);
+
+  EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize());
+
+  // An update that is both a simple conflict and a hierarchy conflict should be
+  // treated as a hierarchy conflict.
+  ASSERT_TRUE(status->conflict_progress());
+  EXPECT_EQ(1, status->conflict_progress()->HierarchyConflictingItemsSize());
+  EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize());
+}
+
+TEST_F(ApplyUpdatesCommandTest, HierarchyConflictDirectoryLoop) {

[akalin, 2012/02/01 01:10:24]

describe test

+  // Item 'X' locally has parent of 'root'.  Server is updating it to have
+  // parent of 'Y'.
+  {
+    ScopedDirLookup dir(syncdb()->manager(), syncdb()->name());
+    ASSERT_TRUE(dir.good());
+    WriteTransaction trans(FROM_HERE, UNITTEST, dir);
+
+    syncable::Id parent_id(id_factory_.root());
+    MutableEntry entry(&trans, syncable::CREATE, parent_id, "X");
+    ASSERT_TRUE(entry.good());
+
+    entry.Put(syncable::ID, id_factory_.MakeServer("X"));
+    entry.Put(syncable::BASE_VERSION, GetNextRevision());
+    entry.Put(syncable::IS_UNSYNCED, false);
+    entry.Put(syncable::NON_UNIQUE_NAME, "X");
+    entry.Put(syncable::IS_DIR, true);
+    entry.Put(syncable::IS_DEL, false);
+    entry.Put(syncable::PARENT_ID, parent_id);
+
+    CHECK(entry.PutPredecessor(id_factory_.root()));
+    entry.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics());
+
+    entry.Put(syncable::SERVER_VERSION, GetNextRevision());
+    entry.Put(syncable::IS_UNAPPLIED_UPDATE, true);
+    entry.Put(syncable::SERVER_NON_UNIQUE_NAME, "X");
+    entry.Put(syncable::SERVER_PARENT_ID, id_factory_.MakeServer("Y"));
+    entry.Put(syncable::SERVER_IS_DIR, true);
+    entry.Put(syncable::SERVER_SPECIFICS, DefaultBookmarkSpecifics());
+  }
+
+  // Item 'Y' is child of 'X'.
+  CreateUnsyncedItem(id_factory_.MakeServer("Y"), id_factory_.MakeServer("X"),
+                     "Y", true, syncable::BOOKMARKS, NULL);
+
+  // If the server's update were applied, we would have X be a child of Y, and Y
+  // as a child of X.  That's a directory loop.  The UpdateApplicator should
+  // prevent the update from being applied and note that this is a hierarchy
+  // conflict.
+
+  ExpectGroupToChange(apply_updates_command_, GROUP_UI);
+  apply_updates_command_.ExecuteImpl(session());
+
+  sessions::StatusController* status = session()->mutable_status_controller();
+  sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI);
+
+  EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize());
+
+  // This should count as a hierarchy conflict.
+  ASSERT_TRUE(status->conflict_progress());
+  EXPECT_EQ(1, status->conflict_progress()->HierarchyConflictingItemsSize());
+  EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize());
+}
+
+TEST_F(ApplyUpdatesCommandTest, HierarchyConflictDeletedParent) {

[akalin, 2012/02/01 01:10:24]

here too

+  // Create a locally deleted parent item.
+  int64 parent_handle;
+  CreateUnsyncedItem(Id::CreateFromServerId("parent"), id_factory_.root(),
+                     "parent", true, syncable::BOOKMARKS, &parent_handle);
+  {
+    ScopedDirLookup dir(syncdb()->manager(), syncdb()->name());
+    ASSERT_TRUE(dir.good());
+    WriteTransaction trans(FROM_HERE, UNITTEST, dir);
+    MutableEntry entry(&trans, syncable::GET_BY_HANDLE, parent_handle);
+    entry.Put(syncable::IS_DEL, true);
+  }
+
+  // Create an incoming child from the server.
+  CreateUnappliedNewItemWithParent("child", DefaultBookmarkSpecifics(),
+                                   "parent");
+
+  // The server's update may seem valid to some other client, but on this client
+  // that new item's parent no longer exists.  The update should not be applied
+  // and the update applicator should indicate this is a hierarchy conflict.
+
+  ExpectGroupToChange(apply_updates_command_, GROUP_UI);
+  apply_updates_command_.ExecuteImpl(session());
+
+  sessions::StatusController* status = session()->mutable_status_controller();
+  sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI);
+
+  // This should count as a hierarchy conflict.
+  ASSERT_TRUE(status->conflict_progress());
+  EXPECT_EQ(1, status->conflict_progress()->HierarchyConflictingItemsSize());
+  EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize());
+}
+
+TEST_F(ApplyUpdatesCommandTest, HierarchyConflictDeleteNonEmptyDirectory) {

[akalin, 2012/02/01 01:10:24]

here too

+  // Create a server-deleted directory.
+  {
+    ScopedDirLookup dir(syncdb()->manager(), syncdb()->name());
+    ASSERT_TRUE(dir.good());
+    WriteTransaction trans(FROM_HERE, UNITTEST, dir);
+
+    syncable::Id parent_id(id_factory_.root());
+    MutableEntry entry(&trans, syncable::CREATE, parent_id, "parent");
+    ASSERT_TRUE(entry.good());
+
+    entry.Put(syncable::ID, id_factory_.MakeServer("parent"));
+    entry.Put(syncable::BASE_VERSION, GetNextRevision());
+    entry.Put(syncable::IS_UNSYNCED, false);
+    entry.Put(syncable::NON_UNIQUE_NAME, "parent");
+    entry.Put(syncable::IS_DIR, true);
+    entry.Put(syncable::IS_DEL, false);
+    entry.Put(syncable::PARENT_ID, parent_id);
+
+    CHECK(entry.PutPredecessor(id_factory_.root()));
+    entry.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics());
+
+    entry.Put(syncable::SERVER_VERSION, GetNextRevision());
+    entry.Put(syncable::IS_UNAPPLIED_UPDATE, true);
+    entry.Put(syncable::SERVER_NON_UNIQUE_NAME, "parent");
+    entry.Put(syncable::SERVER_PARENT_ID, parent_id);
+    entry.Put(syncable::SERVER_IS_DIR, true);
+    entry.Put(syncable::SERVER_IS_DEL, true);
+    entry.Put(syncable::SERVER_SPECIFICS, DefaultBookmarkSpecifics());
+  }
+
+  // Create a local child of the server-deleted directory.
+  CreateUnsyncedItem(id_factory_.MakeServer("child"),
+                     id_factory_.MakeServer("parent"), "child", false,
+                     syncable::BOOKMARKS, NULL);
+
+  // The server's request to delete the directory must be ignored, otherwise our
+  // unsynced new child would be orphaned.  This is a hierarchy conflict.
+
+  ExpectGroupToChange(apply_updates_command_, GROUP_UI);
+  apply_updates_command_.ExecuteImpl(session());
+
+  sessions::StatusController* status = session()->mutable_status_controller();
+  sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI);
+
+  // This should count as a hierarchy conflict.
+  ASSERT_TRUE(status->conflict_progress());
+  EXPECT_EQ(1, status->conflict_progress()->HierarchyConflictingItemsSize());
+  EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize());
+}
+
+TEST_F(ApplyUpdatesCommandTest, HierarchyConflictUnknownParent) {

[akalin, 2012/02/01 01:10:24]

here too (since you're touching it)

   // We shouldn't be able to do anything with either of these items.
   CreateUnappliedNewItemWithParent("some_item",
                                    DefaultBookmarkSpecifics(),
@@ -228,7 +452,10 @@ TEST_F(ApplyUpdatesCommandTest, NestedItemsWithUnknownParent) {
   EXPECT_EQ(2, status->update_progress()->AppliedUpdatesSize())
       << "All updates should have been attempted";
   ASSERT_TRUE(status->conflict_progress());
-  EXPECT_EQ(2, status->conflict_progress()->ConflictingItemsSize())
+  EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize())
+      << "Updates with unknown parent should not be treated as 'simple'"
+      << " conflicts";
+  EXPECT_EQ(2, status->conflict_progress()->HierarchyConflictingItemsSize())
       << "All updates with an unknown ancestors should be in conflict";
   EXPECT_EQ(0, status->update_progress()->SuccessfullyAppliedUpdateCount())
       << "No item with an unknown ancestor should be applied";
@@ -265,7 +492,7 @@ TEST_F(ApplyUpdatesCommandTest, ItemsBothKnownAndUnknown) {
   EXPECT_EQ(6, status->update_progress()->AppliedUpdatesSize())
       << "All updates should have been attempted";
   ASSERT_TRUE(status->conflict_progress());
-  EXPECT_EQ(2, status->conflict_progress()->ConflictingItemsSize())
+  EXPECT_EQ(2, status->conflict_progress()->HierarchyConflictingItemsSize())
       << "The updates with unknown ancestors should be in conflict";
   EXPECT_EQ(4, status->update_progress()->SuccessfullyAppliedUpdateCount())
       << "The updates with known ancestors should be successfully applied";