Populate versions on individual nodes in sync model and native bookmark model.

chrome/browser/sync/profile_sync_service_bookmark_unittest.cc

Index: chrome/browser/sync/profile_sync_service_bookmark_unittest.cc
diff --git a/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc b/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc
index cf7e3813c50e30d008033ec4ffd3c9c41cc6328d..ead5032a852cb6f2c8f5bb252d8adda91ec03b38 100644
--- a/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc
+++ b/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc
@@ -6,6 +6,7 @@
 // BookmarkChangeProcessor.  Write unit tests for
 // BookmarkModelAssociator separately.
 
+#include <queue>
 #include <stack>
 #include <vector>
 
@@ -559,12 +560,12 @@ class ProfileSyncServiceBookmarkTest : public testing::Test {
   content::TestBrowserThread file_thread_;
 
   TestingProfile profile_;
-  scoped_ptr<TestBookmarkModelAssociator> model_associator_;
 
  protected:
   BookmarkModel* model_;
   syncer::TestUserShare test_user_share_;
   scoped_ptr<BookmarkChangeProcessor> change_processor_;
+  scoped_ptr<TestBookmarkModelAssociator> model_associator_;
   StrictMock<DataTypeErrorHandlerMock> mock_error_handler_;
 };
 
@@ -1002,6 +1003,10 @@ class ProfileSyncServiceBookmarkTestWithData
   void ExpectBookmarkModelMatchesTestData();
   void WriteTestDataToBookmarkModel();
 
+  // Verify transaction versions of bookmark nodes and sync nodes are equal
+  // recursively.
+  void ExpectTransactionVersionMatch(const BookmarkNode* node);
+
  private:
   const base::Time start_time_;
 
@@ -1134,10 +1139,10 @@ void ProfileSyncServiceBookmarkTestWithData::PopulateFromTestData(
           start_time_ + base::TimeDelta::FromMinutes(*running_count);
       model_->AddURLWithCreationTime(node, i, WideToUTF16Hack(item.title),
                                      GURL(item.url), add_time);
-      (*running_count)++;
     } else {
       model_->AddFolder(node, i, WideToUTF16Hack(item.title));
     }
+    (*running_count)++;
   }
 }
 
@@ -1165,11 +1170,11 @@ void ProfileSyncServiceBookmarkTestWithData::CompareWithTestData(
           start_time_ + base::TimeDelta::FromMinutes(*running_count);
       EXPECT_EQ(expected_time.ToInternalValue(),
                 child_node->date_added().ToInternalValue());
-      (*running_count)++;
     } else {
       EXPECT_TRUE(child_node->is_folder());
       EXPECT_FALSE(child_node->is_url());
     }
+    (*running_count)++;
   }
 }
 
@@ -1595,6 +1600,70 @@ TEST_F(ProfileSyncServiceBookmarkTestWithData, UpdateDateAdded) {
   EXPECT_EQ(node->date_added(), base::Time::FromInternalValue(30));
 }
 
+int64 GetTransactionVersion(const BookmarkNode* node) {
+  std::string version_str;
+  int64 version;
+  EXPECT_TRUE(node->GetMetaInfo(kBookmarkTransactionVersionKey, &version_str));
+  EXPECT_TRUE(base::StringToInt64(version_str, &version));
+  return version;
+}
+
+void ProfileSyncServiceBookmarkTestWithData::ExpectTransactionVersionMatch(
+    const BookmarkNode* node) {
+  std::queue<const BookmarkNode*> nodes;
+  nodes.push(node);
+  while (!nodes.empty()) {
+    const BookmarkNode* n = nodes.front();
+    nodes.pop();
+
+    syncer::ReadTransaction trans(FROM_HERE, test_user_share_.user_share());
+    syncer::ReadNode sync_node(&trans);
+    ASSERT_TRUE(model_associator_->InitSyncNodeFromChromeId(n->id(),
+                                                            &sync_node));
+    EXPECT_EQ(sync_node.GetEntry()->Get(syncer::syncable::TRANSACTION_VERSION),
+              GetTransactionVersion(n));
+    for (int i = 0; i < n->child_count(); ++i)
+      nodes.push(n->GetChild(i));
+  }
+}
+
+// Test transaction versions of model and nodes are incremented after changes
+// are applied.
+TEST_F(ProfileSyncServiceBookmarkTestWithData, UpdateTransactionVersion) {
+  LoadBookmarkModel(DELETE_EXISTING_STORAGE, DONT_SAVE_TO_STORAGE);
+  StartSync();
+  WriteTestDataToBookmarkModel();
+  MessageLoop::current()->RunUntilIdle();
+
+  // Verify model versions are equal.
+  {
+    syncer::ReadTransaction trans(FROM_HERE, test_user_share_.user_share());
+    EXPECT_EQ(trans.GetModelVersion(syncer::BOOKMARKS),
+              GetTransactionVersion(model_->root_node()));
+  }
+
+  // Verify node versions are equal.
+  ExpectTransactionVersionMatch(model_->bookmark_bar_node());
+  ExpectTransactionVersionMatch(model_->other_node());
+  ExpectTransactionVersionMatch(model_->mobile_node());
+
+  // verify version is incremented when bookmark is removed.
+  int64 old_model_version = GetTransactionVersion(model_->root_node());
+  model_->Remove(model_->bookmark_bar_node(), 0);
+  MessageLoop::current()->RunUntilIdle();
+  EXPECT_EQ(old_model_version + 1, GetTransactionVersion(model_->root_node()));
+
+  // Verify version is updated when bookmark is changed.
+  old_model_version = GetTransactionVersion(model_->root_node());
+  model_->SetTitle(model_->bookmark_bar_node()->GetChild(0),
+                   WideToUTF16Hack(L"test"));
+  MessageLoop::current()->RunUntilIdle();
+  EXPECT_EQ(old_model_version + 1, GetTransactionVersion(model_->root_node()));
+  EXPECT_EQ(old_model_version + 1,
+            GetTransactionVersion(model_->bookmark_bar_node()->GetChild(0)));
+  ExpectTransactionVersionMatch(model_->bookmark_bar_node()->GetChild(0));
+}
+

[Nicolas Zea, 2012/11/07 00:20:31]

do children get updated when a parent is modified? Perhaps include a expectation
of that here?

Also, verify that the version does not change if the node itself doesn't change
(for example by setting specifics to the same value)? Or does the version always
change?

[haitaol1, 2012/11/07 22:20:25]

Parent is not updated but siblings do because of NEXT_ID/PREV_ID under current
implementation, but not with ordinal in the future. Added expectations to verify
versions of other nodes are unchanged with hack for sibling.
The version always change in test because test uses NullDirectoryChangeDelegate,
which I changed to simply return IDs of mutations always without diffing.

 }  // namespace
 
 }  // namespace browser_sync