Sync: fix for the code that checks whether the initial download has completed

sync/engine/syncer_unittest.cc

Index: sync/engine/syncer_unittest.cc
diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc
index c630fe4aa38db8fdb885786594590d0198858682..f2bbc639b1d9139fb374266c4ece05c745ad01b5 100644
--- a/sync/engine/syncer_unittest.cc
+++ b/sync/engine/syncer_unittest.cc
@@ -4336,6 +4336,8 @@ TEST_F(SyncerTest, ClientTagUpdateClashesWithLocalEntry) {
     tag2_metahandle = tag2.GetMetahandle();
 
     // Preferences type root should have been created by the updates above.
+    ASSERT_TRUE(directory()->InitialSyncEndedForType(&trans, PREFERENCES));
+
     Entry pref_root(&trans, GET_TYPE_ROOT, PREFERENCES);
     ASSERT_TRUE(pref_root.good());
 
@@ -4378,6 +4380,8 @@ TEST_F(SyncerTest, ClientTagUpdateClashesWithLocalEntry) {
     EXPECT_EQ(tag2_metahandle, tag2.GetMetahandle());
 
     // Preferences type root should have been created by the updates above.
+    ASSERT_TRUE(directory()->InitialSyncEndedForType(&trans, PREFERENCES));
+
     Entry pref_root(&trans, GET_TYPE_ROOT, PREFERENCES);
     ASSERT_TRUE(pref_root.good());
 
@@ -4460,6 +4464,8 @@ TEST_F(SyncerTest, ClientTagClashWithinBatchOfUpdates) {
     EXPECT_EQ("tag c", tag_c.GetUniqueClientTag());
 
     // Preferences type root should have been created by the updates above.
+    ASSERT_TRUE(directory()->InitialSyncEndedForType(&trans, PREFERENCES));
+
     Entry pref_root(&trans, GET_TYPE_ROOT, PREFERENCES);
     ASSERT_TRUE(pref_root.good());
 
@@ -4483,6 +4489,9 @@ TEST_F(SyncerTest, EntryWithParentIdUpdatedWithEntryWithoutParentId) {
     // Preferences type root should have been created by the update above.
     // We need it in order to get its ID.
     syncable::ReadTransaction trans(FROM_HERE, directory());
+
+    ASSERT_TRUE(directory()->InitialSyncEndedForType(&trans, PREFERENCES));
+
     Entry pref_root(&trans, GET_TYPE_ROOT, PREFERENCES);
     ASSERT_TRUE(pref_root.good());
     pref_root_id = pref_root.GetId();
@@ -4710,30 +4719,36 @@ TEST_F(SyncerTest, ConfigureFailsDontApplyUpdates) {
   mock_server_->FailNthPostBufferToPathCall(2);
 
   // Construct the first GetUpdates response.
-  mock_server_->AddUpdateDirectory(node1, ids_.root(), "one", 1, 10,
-                                   foreign_cache_guid(), "-1");
+  mock_server_->AddUpdatePref(node1.GetServerId(), "", "one", 1, 10);
   mock_server_->SetChangesRemaining(1);
   mock_server_->NextUpdateBatch();
 
-  // Consutrct the second GetUpdates response.
-  mock_server_->AddUpdateDirectory(node2, ids_.root(), "two", 1, 20,
-                                   foreign_cache_guid(), "-2");
+  // Construct the second GetUpdates response.
+  mock_server_->AddUpdatePref(node2.GetServerId(), "", "two", 2, 20);
 
   SyncShareConfigure();
 
-  syncable::ReadTransaction trans(FROM_HERE, directory());
+  // The type shouldn't be marked as having the initial sync completed.
+  EXPECT_FALSE(directory()->InitialSyncEndedForType(PREFERENCES));
 
-  // The first node was downloaded, but not applied.
-  Entry n1(&trans, GET_BY_ID, node1);
-  ASSERT_TRUE(n1.good());
-  EXPECT_TRUE(n1.GetIsUnappliedUpdate());
+  {
+    syncable::ReadTransaction trans(FROM_HERE, directory());
 
-  // The second node was not downloaded.
-  Entry n2(&trans, GET_BY_ID, node2);
-  EXPECT_FALSE(n2.good());
+    // The first node was downloaded, but not applied.
+    Entry n1(&trans, GET_BY_ID, node1);
+    ASSERT_TRUE(n1.good());
+    EXPECT_TRUE(n1.GetIsUnappliedUpdate());
 
-  // One update remains undownloaded.
-  mock_server_->ClearUpdatesQueue();
+    // The second node was not downloaded.
+    Entry n2(&trans, GET_BY_ID, node2);
+    EXPECT_FALSE(n2.good());
+  }
+
+  // Finish the download and verify that the type is now marked as
+  // having the initial sync completed.
+  SyncShareNudge();
+
+  EXPECT_TRUE(directory()->InitialSyncEndedForType(PREFERENCES));
 }
 
 TEST_F(SyncerTest, GetKeySuccess) {
@@ -4767,6 +4782,20 @@ TEST_F(SyncerTest, GetKeyEmpty) {
   }
 }
 
+// Trigger an update that contains a progress marker only and verify that
+// the type gets marked as having initial sync complete.
+TEST_F(SyncerTest, ProgressMarkerOnlyUpdate) {

[Nicolas Zea, 2015/10/13 17:51:37]

This is to test the the implicit permanent folder is created right? Maybe add a
comment about that?

[stanisc, 2015/10/13 21:23:11]

Right. Added a comment. Also added a check that verifies that the root folder
was created.

+  EXPECT_FALSE(directory()->InitialSyncEndedForType(PREFERENCES));
+  sync_pb::DataTypeProgressMarker* marker =
+      mock_server_->AddUpdateProgressMarker();
+  marker->set_data_type_id(GetSpecificsFieldNumberFromModelType(PREFERENCES));
+  marker->set_token("foobar");
+
+  SyncShareNudge();
+
+  EXPECT_TRUE(directory()->InitialSyncEndedForType(PREFERENCES));
+}
+
 // Tests specifically related to bookmark (and therefore no client tags) sync
 // logic. Entities without client tags have custom logic in parts of the code,
 // and hence are not covered by e.g. the Undeletion tests below.