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

sync/engine/syncer_unittest.cc

 // Copyright 2012 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.
 //
 // Syncer unit tests. Unfortunately a lot of these tests
 // are outdated and need to be reworked and updated.
 
 #include <algorithm>
 #include <limits>
 #include <list>
@@ skipping 4318 matching lines @@
     ASSERT_TRUE(tag2.GetId().ServerKnows());
     ASSERT_TRUE(id4 == tag2.GetId());
     EXPECT_FALSE(tag2.GetIsDel());
     EXPECT_FALSE(tag2.GetIsUnappliedUpdate());
     EXPECT_FALSE(tag2.GetIsUnsynced());
     EXPECT_EQ(11, tag2.GetBaseVersion());
     EXPECT_EQ("tag2", tag2.GetUniqueClientTag());
     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());
 
     syncable::Directory::Metahandles children;
     directory()->GetChildHandlesById(&trans, pref_root.GetId(), &children);
     ASSERT_EQ(2U, children.size());
   }
 
   syncable::Id id2 = TestIdFactory::MakeServer("2");
   mock_server_->AddUpdatePref(id2.GetServerId(), "", "tag1", 12, 120);
@@ skipping 22 matching lines @@
     ASSERT_EQ(id3, tag2.GetId())
         << "ID 3 should be kept, since it was less than ID 4.";
     EXPECT_FALSE(tag2.GetIsDel());
     EXPECT_FALSE(tag2.GetIsUnappliedUpdate());
     EXPECT_FALSE(tag2.GetIsUnsynced());
     EXPECT_EQ(13, tag2.GetBaseVersion());
     EXPECT_EQ("tag2", tag2.GetUniqueClientTag());
     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());
 
     syncable::Directory::Metahandles children;
     directory()->GetChildHandlesById(&trans, pref_root.GetId(), &children);
     ASSERT_EQ(2U, children.size());
   }
 }
 
 TEST_F(SyncerTest, ClientTagClashWithinBatchOfUpdates) {
@@ skipping 62 matching lines @@
     ASSERT_TRUE(tag_c.good());
     EXPECT_TRUE(tag_c.GetId().ServerKnows());
     EXPECT_EQ(ids_.FromNumber(201), tag_c.GetId());
     EXPECT_FALSE(tag_c.GetIsDel());
     EXPECT_FALSE(tag_c.GetIsUnappliedUpdate());
     EXPECT_FALSE(tag_c.GetIsUnsynced());
     EXPECT_EQ(21, tag_c.GetBaseVersion());
     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());
 
     // Verify that we have exactly 3 tagged nodes under the type root.
     syncable::Directory::Metahandles children;
     directory()->GetChildHandlesById(&trans, pref_root.GetId(), &children);
     ASSERT_EQ(3U, children.size());
   }
 }
 
 // This verifies transition to implicit permanent folders.
 TEST_F(SyncerTest, EntryWithParentIdUpdatedWithEntryWithoutParentId) {
   // Make sure SPECIFICS root exists so that we can get its parent ID.
   mock_server_->AddUpdateSpecifics(1, 0, "Folder", 10, 10, true, 1,
                                    DefaultPreferencesSpecifics());
   mock_server_->SetLastUpdateServerTag(ModelTypeToRootTag(PREFERENCES));
   EXPECT_TRUE(SyncShareNudge());
 
   Id pref_root_id;
   {
     // 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();
   }
 
   // Add a preference item with explicit parent ID.
   {
     WriteTransaction trans(FROM_HERE, UNITTEST, directory());
     MutableEntry entry(&trans, CREATE, PREFERENCES, pref_root_id, "tag");
     ASSERT_TRUE(entry.good());
@@ skipping 196 matching lines @@
   Entry n1(&trans, GET_BY_ID, node1);
   ASSERT_TRUE(n1.good());
   EXPECT_FALSE(n1.GetIsUnappliedUpdate());
 
   Entry n2(&trans, GET_BY_ID, node2);
   ASSERT_TRUE(n2.good());
   EXPECT_FALSE(n2.GetIsUnappliedUpdate());
 }
 
 // Same as the above case, but this time the second batch fails to download.
 TEST_F(SyncerTest, ConfigureFailsDontApplyUpdates) {

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

Should this be updated? AFAICT you're now actually applying updates, and
successfully configuring.

Maybe pull your logic into a separate test?

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

Good point. I removed the extra sync cycle from this test and instead updated
the test above - the happy case.

   syncable::Id node1 = ids_.NewServerId();
   syncable::Id node2 = ids_.NewServerId();
 
   // The scenario: we have two batches of updates with one update each.  A
   // normal confgure step would download all the updates one batch at a time and
   // apply them.  This configure will succeed in downloading the first batch
   // then fail when downloading the second.
   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) {
   {
     syncable::ReadTransaction rtrans(FROM_HERE, directory());
     EXPECT_TRUE(directory()->GetNigoriHandler()->NeedKeystoreKey(&rtrans));
   }
 
   SyncShareConfigure();
 
@@ skipping 13 matching lines @@
   mock_server_->SetKeystoreKey(std::string());
   SyncShareConfigure();
 
   EXPECT_NE(session_->status_controller().last_get_key_result(), SYNCER_OK);
   {
     syncable::ReadTransaction rtrans(FROM_HERE, directory());
     EXPECT_TRUE(directory()->GetNigoriHandler()->NeedKeystoreKey(&rtrans));
   }
 }
 
+// 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.
 class SyncerBookmarksTest : public SyncerTest {
  public:
   SyncerBookmarksTest() : metahandle_(syncable::kInvalidMetaHandle) {
   }
 
   void Create() {
     WriteTransaction trans(FROM_HERE, UNITTEST, directory());
@@ skipping 817 matching lines @@
     EXPECT_EQ("xyz", final_monitor_records["xyz"].extension_id);
     EXPECT_EQ(2049U, final_monitor_records["ABC"].bookmark_write_count);
     EXPECT_EQ(4U,    final_monitor_records["xyz"].bookmark_write_count);
   } else {
     EXPECT_TRUE(final_monitor_records.empty())
         << "Should not restore records after successful bookmark commit.";
   }
 }
 
 }  // namespace syncer