Remove initial_sync_ended bits

sync/internal_api/sync_manager_impl_unittest.cc

 // Copyright (c) 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.
 
 // Unit tests for the SyncApi. Note that a lot of the underlying
 // functionality is provided by the Syncable layer, which has its own
 // unit tests. We'll test SyncApi specific things in this harness.
 
 #include <cstddef>
 #include <map>
@@ skipping 42 matching lines @@
 #include "sync/protocol/extension_specifics.pb.h"
 #include "sync/protocol/password_specifics.pb.h"
 #include "sync/protocol/preference_specifics.pb.h"
 #include "sync/protocol/proto_value_conversions.h"
 #include "sync/protocol/sync.pb.h"
 #include "sync/sessions/sync_session.h"
 #include "sync/syncable/directory.h"
 #include "sync/syncable/entry.h"
 #include "sync/syncable/mutable_entry.h"
 #include "sync/syncable/nigori_util.h"
+#include "sync/syncable/read_transaction.h"
 #include "sync/syncable/syncable_id.h"
 #include "sync/syncable/write_transaction.h"
 #include "sync/test/callback_counter.h"
 #include "sync/test/engine/fake_sync_scheduler.h"
 #include "sync/test/fake_encryptor.h"
 #include "sync/test/fake_extensions_activity_monitor.h"
 #include "sync/util/cryptographer.h"
 #include "sync/util/extensions_activity_monitor.h"
 #include "sync/util/test_unrecoverable_error_handler.h"
 #include "sync/util/time.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::ExpectDictStringValue;
 using testing::_;
 using testing::DoAll;
 using testing::InSequence;
 using testing::Return;
 using testing::SaveArg;
 using testing::StrictMock;
 
 namespace syncer {
 
 using sessions::SyncSessionSnapshot;
+using syncable::GET_BY_HANDLE;
 using syncable::IS_DEL;
 using syncable::IS_UNSYNCED;
-using syncable::kEncryptedString;
 using syncable::NON_UNIQUE_NAME;
 using syncable::SPECIFICS;
+using syncable::kEncryptedString;
 
 namespace {
 
 const char kTestChromeVersion[] = "test chrome version";
 
 void ExpectInt64Value(int64 expected_value,
                       const DictionaryValue& value, const std::string& key) {
   std::string int64_str;
   EXPECT_TRUE(value.GetString(key, &int64_str));
   int64 val = 0;
@@ skipping 779 matching lines @@
       int64 model_version,
       const BaseTransaction* trans,
       const ImmutableChangeRecordList& changes) OVERRIDE {}
 
   virtual void OnChangesComplete(ModelType model_type) OVERRIDE {}
 
   // Helper methods.
   bool SetUpEncryption(NigoriStatus nigori_status,
                        EncryptionStatus encryption_status) {
     UserShare* share = sync_manager_.GetUserShare();
-    share->directory->set_initial_sync_ended_for_type(NIGORI, true);
 
     // We need to create the nigori node as if it were an applied server update.
     int64 nigori_id = GetIdForDataType(NIGORI);
     if (nigori_id == kInvalidId)
       return false;
 
     // Set the nigori cryptographer information.
     if (encryption_status == FULL_ENCRYPTION)
       sync_manager_.GetEncryptionHandler()->EnableEncryptEverything();
 
@@ skipping 103 matching lines @@
       sync_pb::DataTypeProgressMarker marker;
       marker.set_token("token");
       marker.set_data_type_id(GetSpecificsFieldNumberFromModelType(type));
       sync_manager_.directory()->SetDownloadProgress(type, marker);
     } else {
       sync_pb::DataTypeProgressMarker marker;
       sync_manager_.directory()->SetDownloadProgress(type, marker);
     }
   }
 
-  void SetInitialSyncEndedForType(ModelType type, bool value) {
-    sync_manager_.directory()->set_initial_sync_ended_for_type(type, value);
-  }
-
   InternalComponentsFactory::Switches GetSwitches() const {
     return switches_;
   }
 
  private:
   // Needed by |sync_manager_|.
   MessageLoop message_loop_;
   // Needed by |sync_manager_|.
   base::ScopedTempDir temp_dir_;
   // Sync Id's for the roots of the enabled datatypes.
@@ skipping 1819 matching lines @@
 
   ConfigurationParams params;
   EXPECT_CALL(*scheduler(), Start(SyncScheduler::CONFIGURATION_MODE));
   EXPECT_CALL(*scheduler(), ScheduleConfiguration(_)).
       WillOnce(DoAll(SaveArg<0>(&params), Return(true)));
 
   // Set data for all types.
   for (ModelTypeSet::Iterator iter = ModelTypeSet::All().First(); iter.Good();
        iter.Inc()) {
     SetProgressMarkerForType(iter.Get(), true);
-    SetInitialSyncEndedForType(iter.Get(), true);
   }
 
   CallbackCounter ready_task_counter, retry_task_counter;
   sync_manager_.ConfigureSyncer(
       reason,
       types_to_download,
       new_routing_info,
       base::Bind(&CallbackCounter::Callback,
                  base::Unretained(&ready_task_counter)),
       base::Bind(&CallbackCounter::Callback,
                  base::Unretained(&retry_task_counter)));
   EXPECT_EQ(0, ready_task_counter.times_called());
   EXPECT_EQ(0, retry_task_counter.times_called());
   EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::RECONFIGURATION,
             params.source);
   EXPECT_TRUE(types_to_download.Equals(params.types_to_download));
   EXPECT_EQ(new_routing_info, params.routing_info);
 
-  // Verify all the disabled types were purged.

[Nicolas Zea, 2012/12/10 22:50:38]

Doesn't this still apply?

[rlarocque, 2012/12/11 01:44:48]

Yes it does.  Fixed.

   EXPECT_TRUE(sync_manager_.InitialSyncEndedTypes().Equals(
       enabled_types));
   EXPECT_TRUE(sync_manager_.GetTypesWithEmptyProgressMarkerToken(
       ModelTypeSet::All()).Equals(disabled_types));
 }
 
 // Test that on a reconfiguration (configuration where the session context
 // already has routing info), only those recently disabled types are purged.
 TEST_F(SyncManagerTestWithMockScheduler, ReConfiguration) {
   ConfigureReason reason = CONFIGURE_REASON_RECONFIGURATION;
@@ skipping 11 matching lines @@
   EXPECT_CALL(*scheduler(), Start(SyncScheduler::CONFIGURATION_MODE));
   EXPECT_CALL(*scheduler(), ScheduleConfiguration(_)).
       WillOnce(DoAll(SaveArg<0>(&params), Return(true)));
 
   // Set data for all types except those recently disabled (so we can verify
   // only those recently disabled are purged) .
   for (ModelTypeSet::Iterator iter = ModelTypeSet::All().First(); iter.Good();
        iter.Inc()) {
     if (!disabled_types.Has(iter.Get())) {
       SetProgressMarkerForType(iter.Get(), true);
-      SetInitialSyncEndedForType(iter.Get(), true);
     } else {
       SetProgressMarkerForType(iter.Get(), false);
-      SetInitialSyncEndedForType(iter.Get(), false);
     }
   }
 
   // Set the context to have the old routing info.
   session_context()->set_routing_info(old_routing_info);
 
   CallbackCounter ready_task_counter, retry_task_counter;
   sync_manager_.ConfigureSyncer(
       reason,
       types_to_download,
       new_routing_info,
       base::Bind(&CallbackCounter::Callback,
                  base::Unretained(&ready_task_counter)),
       base::Bind(&CallbackCounter::Callback,
                  base::Unretained(&retry_task_counter)));
   EXPECT_EQ(0, ready_task_counter.times_called());
   EXPECT_EQ(0, retry_task_counter.times_called());
   EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::RECONFIGURATION,
             params.source);
   EXPECT_TRUE(types_to_download.Equals(params.types_to_download));
   EXPECT_EQ(new_routing_info, params.routing_info);
 
   // Verify only the recently disabled types were purged.
-  EXPECT_TRUE(sync_manager_.InitialSyncEndedTypes().Equals(

[Nicolas Zea, 2012/12/10 22:50:38]

I guess we don't actually create roots for these tests, so can't verify they're
removed?

[rlarocque, 2012/12/11 01:44:48]

More or less, yes.

The test harness creates root nodes for some types.  See lines 850 and 870 for
more info.  Others, like AUTOFILL, are not touched at all by this test.  That
breaks the assertion on line 2936, since AUTOFILL was never InitialSyncEnded(),
yet it is not in the 'disabled_types' set.

It would be possible to try to keep this assertion, but I don't think it's worth
the effort.  I think the point of it was to ensure that the initial_sync_ended
bits remained in sync with our database status.  Following this change, such
bugs are much less likely to occur, so the assertion is less valuable.

-      Difference(ModelTypeSet::All(), disabled_types)));
   EXPECT_TRUE(sync_manager_.GetTypesWithEmptyProgressMarkerToken(
       ModelTypeSet::All()).Equals(disabled_types));
 }
 
 // Test that the retry callback is invoked on configuration failure.
 TEST_F(SyncManagerTestWithMockScheduler, ConfigurationRetry) {
   ConfigureReason reason = CONFIGURE_REASON_RECONFIGURATION;
   ModelTypeSet types_to_download(BOOKMARKS, PREFERENCES);
   ModelSafeRoutingInfo new_routing_info;
   GetModelSafeRoutingInfo(&new_routing_info);
@@ skipping 13 matching lines @@
       base::Bind(&CallbackCounter::Callback,
                  base::Unretained(&retry_task_counter)));
   EXPECT_EQ(0, ready_task_counter.times_called());
   EXPECT_EQ(1, retry_task_counter.times_called());
   EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::RECONFIGURATION,
             params.source);
   EXPECT_TRUE(types_to_download.Equals(params.types_to_download));
   EXPECT_EQ(new_routing_info, params.routing_info);
 }
 
-// Test that PurgePartiallySyncedTypes purges only those types that don't
-// have empty progress marker and don't have initial sync ended set.
+// Test that PurgePartiallySyncedTypes purges only those types that have not
+// fully completed their initial download and apply.
 TEST_F(SyncManagerTest, PurgePartiallySyncedTypes) {
+  ModelSafeRoutingInfo routing_info;
+  GetModelSafeRoutingInfo(&routing_info);
+  ModelTypeSet enabled_types = GetRoutingInfoTypes(routing_info);
+
   UserShare* share = sync_manager_.GetUserShare();
 
-  // Set Nigori and Bookmarks to be partial types.
-  sync_pb::DataTypeProgressMarker nigori_marker;
-  nigori_marker.set_data_type_id(
-      GetSpecificsFieldNumberFromModelType(NIGORI));
-  nigori_marker.set_token("token");
-  sync_pb::DataTypeProgressMarker bookmark_marker;
-  bookmark_marker.set_data_type_id(
-      GetSpecificsFieldNumberFromModelType(BOOKMARKS));
-  bookmark_marker.set_token("token");
-  share->directory->SetDownloadProgress(NIGORI, nigori_marker);
-  share->directory->SetDownloadProgress(BOOKMARKS, bookmark_marker);
+  // The test harness automatically initializes all types in the routing info.
+  // Check that autofill is not among them.
+  ASSERT_FALSE(enabled_types.Has(AUTOFILL));
 
-  // Set Preferences to be a full type.
-  sync_pb::DataTypeProgressMarker pref_marker;
-  pref_marker.set_data_type_id(
+  // Further ensure that the test harness did not create its root node.
+  {
+    syncable::ReadTransaction trans(FROM_HERE, share->directory.get());
+    syncable::Entry autofill_root_node(&trans, syncable::GET_BY_SERVER_TAG,
+                                       ModelTypeToRootTag(AUTOFILL));
+    ASSERT_FALSE(autofill_root_node.good());
+  }
+
+  // One more redundant check.
+  ASSERT_FALSE(sync_manager_.InitialSyncEndedTypes().Has(AUTOFILL));
+
+  // Give autofill a progress marker.
+  sync_pb::DataTypeProgressMarker autofill_marker;
+  autofill_marker.set_data_type_id(
+      GetSpecificsFieldNumberFromModelType(AUTOFILL));
+  autofill_marker.set_token("token");
+  share->directory->SetDownloadProgress(AUTOFILL, autofill_marker);
+
+  int autofill_meta;
+  {
+    syncable::WriteTransaction trans(FROM_HERE, syncable::UNITTEST,
+                                     share->directory.get());
+    // Also add a pending autofill update from the server.

[Nicolas Zea, 2012/12/10 22:50:38]

Can you use the test entry factory for this?

[rlarocque, 2012/12/11 01:44:48]

That would be the first use of TestEntryFactory from this file.  I agree it's a
good idea, though.

Done.

+    sync_pb::EntitySpecifics specifics;
+    AddDefaultFieldValue(AUTOFILL, &specifics);
+    syncable::Id node_id = syncable::Id::CreateFromServerId("xyz");
+    syncable::MutableEntry entry(&trans, syncable::CREATE_NEW_UPDATE_ITEM,
+                                 node_id);
+    EXPECT_TRUE(entry.good());
+    // Make it look like sort of like a pending creation from the server.
+    // The SERVER_PARENT_ID and UNIQUE_CLIENT_TAG aren't quite right, but
+    // it's good enough for our purposes.
+    entry.Put(syncable::SERVER_VERSION, 1);
+    entry.Put(syncable::IS_UNAPPLIED_UPDATE, true);
+    entry.Put(syncable::SERVER_IS_DIR, false);
+    entry.Put(syncable::SERVER_PARENT_ID, syncable::GetNullId());
+    entry.Put(syncable::SERVER_SPECIFICS, specifics);
+    entry.Put(syncable::NON_UNIQUE_NAME, "xyz");
+
+    autofill_meta = entry.Get(syncable::META_HANDLE);
+  }
+
+  // Preferences is an enabled type.  Check that the harness initialized it.
+  ASSERT_TRUE(enabled_types.Has(PREFERENCES));
+  ASSERT_TRUE(sync_manager_.InitialSyncEndedTypes().Has(PREFERENCES));
+
+  // Give preferencse a progress marker.
+  sync_pb::DataTypeProgressMarker prefs_marker;
+  prefs_marker.set_data_type_id(
       GetSpecificsFieldNumberFromModelType(PREFERENCES));
-  pref_marker.set_token("token");
-  share->directory->SetDownloadProgress(PREFERENCES, pref_marker);
-  share->directory->set_initial_sync_ended_for_type(PREFERENCES, true);
+  prefs_marker.set_token("token");
+  share->directory->SetDownloadProgress(PREFERENCES, prefs_marker);
 
-  ModelTypeSet partial_types =
-      sync_manager_.GetTypesWithEmptyProgressMarkerToken(ModelTypeSet::All());
-  EXPECT_FALSE(partial_types.Has(NIGORI));
-  EXPECT_FALSE(partial_types.Has(BOOKMARKS));
-  EXPECT_FALSE(partial_types.Has(PREFERENCES));
+  // Add a fully synced preferences node under the root.
+  std::string pref_client_tag = "prefABC";
+  std::string pref_hashed_tag = "hashXYZ";
+  sync_pb::EntitySpecifics pref_specifics;
+  AddDefaultFieldValue(PREFERENCES, &pref_specifics);
+  int pref_meta = MakeServerNode(
+      share, PREFERENCES, pref_client_tag, pref_hashed_tag, pref_specifics);
 
+  // And now, the purge.
   EXPECT_TRUE(sync_manager_.PurgePartiallySyncedTypes());
 
-  // Ensure only bookmarks and nigori lost their progress marker. Preferences
-  // should still have it.
-  partial_types =
+  // Ensure that autofill lost its progress marker, but preferences did not.
+  ModelTypeSet empty_tokens =
       sync_manager_.GetTypesWithEmptyProgressMarkerToken(ModelTypeSet::All());
-  EXPECT_TRUE(partial_types.Has(NIGORI));
-  EXPECT_TRUE(partial_types.Has(BOOKMARKS));
-  EXPECT_FALSE(partial_types.Has(PREFERENCES));
+  EXPECT_TRUE(empty_tokens.Has(AUTOFILL));
+  EXPECT_FALSE(empty_tokens.Has(PREFERENCES));
+
+  // Ensure that autofill lots its node, but preferences did not.
+  {
+    syncable::ReadTransaction trans(FROM_HERE, share->directory.get());
+    syncable::Entry autofill_node(&trans, GET_BY_HANDLE, autofill_meta);
+    syncable::Entry pref_node(&trans, GET_BY_HANDLE, pref_meta);
+    EXPECT_FALSE(autofill_node.good());
+    EXPECT_TRUE(pref_node.good());
+  }
 }
 
 // Test CleanupDisabledTypes properly purges all disabled types as specified
 // by the previous and current enabled params. Enabled partial types should not

[Nicolas Zea, 2012/12/10 22:50:38]

nit: Last sentence no longer applies.

[rlarocque, 2012/12/11 01:44:48]

Done.

 // be purged.
 // Fails on Windows: crbug.com/139726
 #if defined(OS_WIN)
 #define MAYBE_PurgeDisabledTypes DISABLED_PurgeDisabledTypes
 #else
 #define MAYBE_PurgeDisabledTypes PurgeDisabledTypes
 #endif
 TEST_F(SyncManagerTest, MAYBE_PurgeDisabledTypes) {
   ModelSafeRoutingInfo routing_info;
   GetModelSafeRoutingInfo(&routing_info);
   ModelTypeSet enabled_types = GetRoutingInfoTypes(routing_info);
   ModelTypeSet disabled_types = Difference(ModelTypeSet::All(), enabled_types);
-  ModelTypeSet partial_enabled_types(PASSWORDS);
 
-  // Set data for all non-partial types.
+  // The harness should have initialized the enabled_types for us.
+  EXPECT_TRUE(enabled_types.Equals(sync_manager_.InitialSyncEndedTypes()));
+
+  // Set progress markers for all types.
   for (ModelTypeSet::Iterator iter = ModelTypeSet::All().First(); iter.Good();
        iter.Inc()) {
     SetProgressMarkerForType(iter.Get(), true);
-    if (!partial_enabled_types.Has(iter.Get()))
-      SetInitialSyncEndedForType(iter.Get(), true);
   }
 
   // Verify all the enabled types remain after cleanup, and all the disabled
   // types were purged.
   sync_manager_.PurgeDisabledTypes(ModelTypeSet::All(), enabled_types);
-  EXPECT_TRUE(enabled_types.Equals(
-      Union(sync_manager_.InitialSyncEndedTypes(), partial_enabled_types)));
+  EXPECT_TRUE(enabled_types.Equals(sync_manager_.InitialSyncEndedTypes()));
   EXPECT_TRUE(disabled_types.Equals(
       sync_manager_.GetTypesWithEmptyProgressMarkerToken(ModelTypeSet::All())));
 
   // Disable some more types.
   disabled_types.Put(BOOKMARKS);
   disabled_types.Put(PREFERENCES);
   ModelTypeSet new_enabled_types =
       Difference(ModelTypeSet::All(), disabled_types);
 
   // Verify only the non-disabled types remain after cleanup.
   sync_manager_.PurgeDisabledTypes(enabled_types, new_enabled_types);
-  EXPECT_TRUE(new_enabled_types.Equals(
-      Union(sync_manager_.InitialSyncEndedTypes(), partial_enabled_types)));
+  EXPECT_TRUE(new_enabled_types.Equals(sync_manager_.InitialSyncEndedTypes()));
   EXPECT_TRUE(disabled_types.Equals(
       sync_manager_.GetTypesWithEmptyProgressMarkerToken(ModelTypeSet::All())));
 }
 
 }  // namespace