diff options
author | rlarocque@chromium.org <rlarocque@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-02-21 01:16:11 +0000 |
---|---|---|
committer | rlarocque@chromium.org <rlarocque@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-02-21 01:16:11 +0000 |
commit | 5436bd2533a236ca3c6ff6cc7ce7b9a89e9ab10d (patch) | |
tree | 15f78cd81f547a5ba963e16842b699b5d4eaa72e /sync/internal_api/sync_manager_impl_unittest.cc | |
parent | 8cf3e8e05c35bbd056b7848eb059f773f329f703 (diff) | |
download | chromium_src-5436bd2533a236ca3c6ff6cc7ce7b9a89e9ab10d.zip chromium_src-5436bd2533a236ca3c6ff6cc7ce7b9a89e9ab10d.tar.gz chromium_src-5436bd2533a236ca3c6ff6cc7ce7b9a89e9ab10d.tar.bz2 |
Add unit tests for SyncManager change handling
During the code review for UniquePositions [1], we found a fairly serious
issue in code that was only tested by the integration tests. This set
of unit tests attempts to close that hole in our unit test coverage.
[1] See https://codereview.chromium.org/11885024/.
BUG=145412
Review URL: https://chromiumcodereview.appspot.com/12294043
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@183692 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'sync/internal_api/sync_manager_impl_unittest.cc')
-rw-r--r-- | sync/internal_api/sync_manager_impl_unittest.cc | 299 |
1 files changed, 299 insertions, 0 deletions
diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc index cc84e17..a0bec40 100644 --- a/sync/internal_api/sync_manager_impl_unittest.cc +++ b/sync/internal_api/sync_manager_impl_unittest.cc @@ -67,6 +67,7 @@ #include "sync/syncable/syncable_write_transaction.h" #include "sync/test/callback_counter.h" #include "sync/test/engine/fake_sync_scheduler.h" +#include "sync/test/engine/test_id_factory.h" #include "sync/test/fake_encryptor.h" #include "sync/test/fake_extensions_activity_monitor.h" #include "sync/util/cryptographer.h" @@ -3047,4 +3048,302 @@ TEST_F(SyncManagerTest, PurgeDisabledTypes) { sync_manager_.GetTypesWithEmptyProgressMarkerToken(ModelTypeSet::All()))); } +// A test harness to exercise the code that processes and passes changes from +// the "SYNCER"-WriteTransaction destructor, through the SyncManager, to the +// ChangeProcessor. +class SyncManagerChangeProcessingTest : public SyncManagerTest { + public: + virtual void OnChangesApplied( + ModelType model_type, + int64 model_version, + const BaseTransaction* trans, + const ImmutableChangeRecordList& changes) OVERRIDE { + last_changes_ = changes; + } + + virtual void OnChangesComplete(ModelType model_type) OVERRIDE {} + + const ImmutableChangeRecordList& GetRecentChangeList() { + return last_changes_; + } + + UserShare* share() { + return sync_manager_.GetUserShare(); + } + + // Set some flags so our nodes reasonably approximate the real world scenario + // and can get past CheckTreeInvariants. + // + // It's never going to be truly accurate, since we're squashing update + // receipt, processing and application into a single transaction. + void SetNodeProperties(syncable::MutableEntry *entry) { + entry->Put(syncable::ID, id_factory_.NewServerId()); + entry->Put(syncable::BASE_VERSION, 10); + entry->Put(syncable::SERVER_VERSION, 10); + } + + // Looks for the given change in the list. Returns the index at which it was + // found. Returns -1 on lookup failure. + size_t FindChangeInList(int64 id, ChangeRecord::Action action) { + SCOPED_TRACE(id); + for (size_t i = 0; i < last_changes_.Get().size(); ++i) { + if (last_changes_.Get()[i].id == id + && last_changes_.Get()[i].action == action) { + return i; + } + } + ADD_FAILURE() << "Failed to find specified change"; + return -1; + } + + // Returns the current size of the change list. + // + // Note that spurious changes do not necessarily indicate a problem. + // Assertions on change list size can help detect problems, but it may be + // necessary to reduce their strictness if the implementation changes. + size_t GetChangeListSize() { + return last_changes_.Get().size(); + } + + protected: + ImmutableChangeRecordList last_changes_; + TestIdFactory id_factory_; +}; + +// Test creation of a folder and a bookmark. +TEST_F(SyncManagerChangeProcessingTest, AddBookmarks) { + int64 type_root = GetIdForDataType(BOOKMARKS); + int64 folder_id = kInvalidId; + int64 child_id = kInvalidId; + + // Create a folder and a bookmark under it. + { + syncable::WriteTransaction trans( + FROM_HERE, syncable::SYNCER, share()->directory.get()); + syncable::Entry root(&trans, syncable::GET_BY_HANDLE, type_root); + ASSERT_TRUE(root.good()); + + syncable::MutableEntry folder(&trans, syncable::CREATE, + BOOKMARKS, root.Get(syncable::ID), "folder"); + ASSERT_TRUE(folder.good()); + SetNodeProperties(&folder); + folder.Put(syncable::IS_DIR, true); + folder_id = folder.Get(syncable::META_HANDLE); + + syncable::MutableEntry child(&trans, syncable::CREATE, + BOOKMARKS, folder.Get(syncable::ID), "child"); + ASSERT_TRUE(child.good()); + SetNodeProperties(&child); + child_id = child.Get(syncable::META_HANDLE); + } + + // The closing of the above scope will delete the transaction. Its processed + // changes should be waiting for us in a member of the test harness. + EXPECT_EQ(2UL, GetChangeListSize()); + + // We don't need to check these return values here. The function will add a + // non-fatal failure if these changes are not found. + size_t folder_change_pos = + FindChangeInList(folder_id, ChangeRecord::ACTION_ADD); + size_t child_change_pos = + FindChangeInList(child_id, ChangeRecord::ACTION_ADD); + + // Parents are delivered before children. + EXPECT_LT(folder_change_pos, child_change_pos); +} + +// Test moving a bookmark into an empty folder. +TEST_F(SyncManagerChangeProcessingTest, MoveBookmarkIntoEmptyFolder) { + int64 type_root = GetIdForDataType(BOOKMARKS); + int64 folder_b_id = kInvalidId; + int64 child_id = kInvalidId; + + // Create two folders. Place a child under folder A. + { + syncable::WriteTransaction trans( + FROM_HERE, syncable::SYNCER, share()->directory.get()); + syncable::Entry root(&trans, syncable::GET_BY_HANDLE, type_root); + ASSERT_TRUE(root.good()); + + syncable::MutableEntry folder_a(&trans, syncable::CREATE, + BOOKMARKS, root.Get(syncable::ID), "folderA"); + ASSERT_TRUE(folder_a.good()); + SetNodeProperties(&folder_a); + folder_a.Put(syncable::IS_DIR, true); + + syncable::MutableEntry folder_b(&trans, syncable::CREATE, + BOOKMARKS, root.Get(syncable::ID), "folderB"); + ASSERT_TRUE(folder_b.good()); + SetNodeProperties(&folder_b); + folder_b.Put(syncable::IS_DIR, true); + folder_b_id = folder_b.Get(syncable::META_HANDLE); + + syncable::MutableEntry child(&trans, syncable::CREATE, + BOOKMARKS, folder_a.Get(syncable::ID), + "child"); + ASSERT_TRUE(child.good()); + SetNodeProperties(&child); + child_id = child.Get(syncable::META_HANDLE); + } + + // Close that transaction. The above was to setup the initial scenario. The + // real test starts now. + + // Move the child from folder A to folder B. + { + syncable::WriteTransaction trans( + FROM_HERE, syncable::SYNCER, share()->directory.get()); + + syncable::Entry folder_b(&trans, syncable::GET_BY_HANDLE, folder_b_id); + syncable::MutableEntry child(&trans, syncable::GET_BY_HANDLE, child_id); + + child.Put(syncable::PARENT_ID, folder_b.Get(syncable::ID)); + } + + EXPECT_EQ(1UL, GetChangeListSize()); + + // Verify that this was detected as a real change. An early version of the + // UniquePosition code had a bug where moves from one folder to another were + // ignored unless the moved node's UniquePosition value was also changed in + // some way. + FindChangeInList(child_id, ChangeRecord::ACTION_UPDATE); +} + +// Test moving a bookmark into a non-empty folder. +TEST_F(SyncManagerChangeProcessingTest, MoveIntoPopulatedFolder) { + int64 type_root = GetIdForDataType(BOOKMARKS); + int64 child_a_id = kInvalidId; + int64 child_b_id = kInvalidId; + + // Create two folders. Place one child each under folder A and folder B. + { + syncable::WriteTransaction trans( + FROM_HERE, syncable::SYNCER, share()->directory.get()); + syncable::Entry root(&trans, syncable::GET_BY_HANDLE, type_root); + ASSERT_TRUE(root.good()); + + syncable::MutableEntry folder_a(&trans, syncable::CREATE, + BOOKMARKS, root.Get(syncable::ID), "folderA"); + ASSERT_TRUE(folder_a.good()); + SetNodeProperties(&folder_a); + folder_a.Put(syncable::IS_DIR, true); + + syncable::MutableEntry folder_b(&trans, syncable::CREATE, + BOOKMARKS, root.Get(syncable::ID), "folderB"); + ASSERT_TRUE(folder_b.good()); + SetNodeProperties(&folder_b); + folder_b.Put(syncable::IS_DIR, true); + + syncable::MutableEntry child_a(&trans, syncable::CREATE, + BOOKMARKS, folder_a.Get(syncable::ID), + "childA"); + ASSERT_TRUE(child_a.good()); + SetNodeProperties(&child_a); + child_a_id = child_a.Get(syncable::META_HANDLE); + + syncable::MutableEntry child_b(&trans, syncable::CREATE, + BOOKMARKS, folder_b.Get(syncable::ID), + "childB"); + SetNodeProperties(&child_b); + child_b_id = child_b.Get(syncable::META_HANDLE); + + } + + // Close that transaction. The above was to setup the initial scenario. The + // real test starts now. + + { + syncable::WriteTransaction trans( + FROM_HERE, syncable::SYNCER, share()->directory.get()); + + syncable::MutableEntry child_a(&trans, syncable::GET_BY_HANDLE, child_a_id); + syncable::MutableEntry child_b(&trans, syncable::GET_BY_HANDLE, child_b_id); + + // Move child A from folder A to folder B and update its position. + child_a.Put(syncable::PARENT_ID, child_b.Get(syncable::PARENT_ID)); + child_a.PutPredecessor(child_b.Get(syncable::ID)); + } + + EXPECT_EQ(2UL, GetChangeListSize()); + + // Verify that both child A and child B are in the change list, even though + // only child A was moved. The rules state that all siblings of + // position-modified items must be included in the list of changes. + int64 child_a_pos = FindChangeInList(child_a_id, ChangeRecord::ACTION_UPDATE); + int64 child_b_pos = FindChangeInList(child_b_id, ChangeRecord::ACTION_UPDATE); + + // Siblings should appear in left-to-right order. + EXPECT_LT(child_b_pos, child_a_pos); +} + +// Tests the ordering of deletion changes. +TEST_F(SyncManagerChangeProcessingTest, DeletionsAndChanges) { + int64 type_root = GetIdForDataType(BOOKMARKS); + int64 folder_a_id = kInvalidId; + int64 folder_b_id = kInvalidId; + int64 child_id = kInvalidId; + + // Create two folders. Place a child under folder A. + { + syncable::WriteTransaction trans( + FROM_HERE, syncable::SYNCER, share()->directory.get()); + syncable::Entry root(&trans, syncable::GET_BY_HANDLE, type_root); + ASSERT_TRUE(root.good()); + + syncable::MutableEntry folder_a(&trans, syncable::CREATE, + BOOKMARKS, root.Get(syncable::ID), "folderA"); + ASSERT_TRUE(folder_a.good()); + SetNodeProperties(&folder_a); + folder_a.Put(syncable::IS_DIR, true); + folder_a_id = folder_a.Get(syncable::META_HANDLE); + + syncable::MutableEntry folder_b(&trans, syncable::CREATE, + BOOKMARKS, root.Get(syncable::ID), "folderB"); + ASSERT_TRUE(folder_b.good()); + SetNodeProperties(&folder_b); + folder_b.Put(syncable::IS_DIR, true); + folder_b_id = folder_b.Get(syncable::META_HANDLE); + + syncable::MutableEntry child(&trans, syncable::CREATE, + BOOKMARKS, folder_a.Get(syncable::ID), + "child"); + ASSERT_TRUE(child.good()); + SetNodeProperties(&child); + child_id = child.Get(syncable::META_HANDLE); + } + + // Close that transaction. The above was to setup the initial scenario. The + // real test starts now. + + { + syncable::WriteTransaction trans( + FROM_HERE, syncable::SYNCER, share()->directory.get()); + + syncable::MutableEntry folder_a( + &trans, syncable::GET_BY_HANDLE, folder_a_id); + syncable::MutableEntry folder_b( + &trans, syncable::GET_BY_HANDLE, folder_b_id); + syncable::MutableEntry child(&trans, syncable::GET_BY_HANDLE, child_id); + + // Delete folder B and its child. + child.Put(syncable::IS_DEL, true); + folder_b.Put(syncable::IS_DEL, true); + + // Make an unrelated change to folder A. + folder_a.Put(syncable::NON_UNIQUE_NAME, "NewNameA"); + } + + EXPECT_EQ(3UL, GetChangeListSize()); + + size_t folder_a_pos = + FindChangeInList(folder_a_id, ChangeRecord::ACTION_UPDATE); + size_t folder_b_pos = + FindChangeInList(folder_b_id, ChangeRecord::ACTION_DELETE); + size_t child_pos = FindChangeInList(child_id, ChangeRecord::ACTION_DELETE); + + // Deletes should appear before updates. + EXPECT_LT(child_pos, folder_a_pos); + EXPECT_LT(folder_b_pos, folder_a_pos); +} + } // namespace |