[Sync] Make EnableDisableSingleClientTest more robust.

chrome/browser/sync/test/integration/enable_disable_test.cc

Index: chrome/browser/sync/test/integration/enable_disable_test.cc
diff --git a/chrome/browser/sync/test/integration/enable_disable_test.cc b/chrome/browser/sync/test/integration/enable_disable_test.cc
index 177a5edbc47fa58357d16bf23328562ab96233ae..113cef9960dfd2b1939669a9ed7f0892d82d73d2 100644
--- a/chrome/browser/sync/test/integration/enable_disable_test.cc
+++ b/chrome/browser/sync/test/integration/enable_disable_test.cc
@@ -7,39 +7,43 @@
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "components/browser_sync/profile_sync_service.h"
 #include "components/sync/base/model_type.h"
+#include "components/sync/base/sync_prefs.h"
 #include "components/sync/driver/sync_driver_switches.h"
 #include "components/sync/syncable/read_node.h"
 #include "components/sync/syncable/read_transaction.h"
 
-// This file contains tests that exercise enabling and disabling data
-// types.
-
-namespace {
-
 using base::FeatureList;
+using syncer::ModelType;
 using syncer::ModelTypeSet;
+using syncer::ModelTypeToString;
+using syncer::ProxyTypes;
+using syncer::SyncPrefs;
+using syncer::UserSelectableTypes;
+using syncer::UserShare;
 
-class EnableDisableSingleClientTest : public SyncTest {
- public:
-  EnableDisableSingleClientTest() : SyncTest(SINGLE_CLIENT) {}
-  ~EnableDisableSingleClientTest() override {}
+namespace {
 
-  // Don't use self-notifications as they can trigger additional sync cycles.
-  bool TestUsesSelfNotifications() override { return false; }
+bool DoesTopLevelNodeExist(UserShare* user_share, ModelType type) {
+  syncer::ReadTransaction trans(FROM_HERE, user_share);
+  syncer::ReadNode node(&trans);
+  return node.InitTypeRoot(type) == syncer::BaseNode::INIT_OK;
+}
 
- private:
-  DISALLOW_COPY_AND_ASSIGN(EnableDisableSingleClientTest);
-};
+void VerifyExistence(UserShare* user_share, bool exists, ModelType type) {
+  EXPECT_EQ(exists, DoesTopLevelNodeExist(user_share, type))
+      << ModelTypeToString(type);
+}
 
-bool DoesTopLevelNodeExist(syncer::UserShare* user_share,
-                           syncer::ModelType type) {
-    syncer::ReadTransaction trans(FROM_HERE, user_share);
-    syncer::ReadNode node(&trans);
-    return node.InitTypeRoot(type) == syncer::BaseNode::INIT_OK;
+void VerifyExistence(UserShare* user_share,

[Patrick Noland, 2017/02/02 22:11:37]

Can you give the VerifyExistence functions more descriptive names? The
difference between the two isn't all that apparent from just the signatures, and
it's not apparent that you EXPECT that exists equals DoesTopLevelNodeExist

[skym, 2017/02/02 23:37:52]

Done.

+                     bool exists,
+                     ModelType grouped,
+                     ModelType selectable) {
+  EXPECT_EQ(exists, DoesTopLevelNodeExist(user_share, grouped))
+      << ModelTypeToString(selectable) << "->" << ModelTypeToString(grouped);
 }
 
 bool IsUnready(const syncer::DataTypeStatusTable& data_type_status_table,
-               syncer::ModelType type) {
+               ModelType type) {
   return data_type_status_table.GetUnreadyErrorTypes().Has(type);
 }
 
@@ -53,135 +57,143 @@ ModelTypeSet UnifiedSyncServiceTypes() {
   if (FeatureList::IsEnabled(switches::kSyncUSSDeviceInfo)) {
     set.Put(syncer::DEVICE_INFO);
   }
+  // All new types should be USS. This logic is fragile to reordering ModelType.
+  for (int typeInt = syncer::PRINTERS; typeInt < syncer::FIRST_PROXY_TYPE;

[Patrick Noland, 2017/02/02 22:11:37]

Should we alias PRINTERS to something analogous to FIRST_PROXY_TYPE? e.g.
FIRST_USS_TYPE. Although that example doesn't fully capture it...

[skym, 2017/02/02 23:37:52]

How about I expand the comment here to include that? I don't think we want
actual code to know this, but I'm more okay with integ tests doing such.

+       typeInt++) {
+    set.Put(static_cast<ModelType>(typeInt));
+  }
   return set;
 }
 
+// Some groups show up in multiple groups. This means that there are at least

[Patrick Noland, 2017/02/02 22:11:37]

Do you mean that some types show up in multiple groups? 

[Patrick Noland, 2017/02/02 22:11:37]

I don't understand the phrase "at least user selectable groups", possibly you're
missing a word?

[skym, 2017/02/02 23:37:52]

You're correct, should be "...at least _two_ user selectable...", done.

[skym, 2017/02/02 23:37:52]

Yes, done.

+// user selectable groups that will cause these types to become enabled. This
+// affects our tests because we cannot assume that before enabling a multi type
+// it will be disabled, because the other selectable type(s) could already be
+// enabling it. And vice versa for disabling.
+ModelTypeSet MultiGroupTypes(const SyncPrefs& sync_prefs,
+                             const ModelTypeSet& registered_types) {
+  const ModelTypeSet selectable_types = UserSelectableTypes();
+  ModelTypeSet seen;
+  ModelTypeSet multi;
+  for (ModelTypeSet::Iterator si = selectable_types.First(); si.Good();
+       si.Inc()) {
+    const ModelTypeSet grouped_types =
+        sync_prefs.ResolvePrefGroups(registered_types, ModelTypeSet(si.Get()));
+    for (ModelTypeSet::Iterator gi = grouped_types.First(); gi.Good();
+         gi.Inc()) {
+      if (seen.Has(gi.Get())) {
+        multi.Put(gi.Get());
+      } else {
+        seen.Put(gi.Get());
+      }
+    }
+  }
+  return multi;
+}
+
+// This test enables and disables types and verifies the type is sufficiently
+// affected by checking for existence of a root node.
+class EnableDisableSingleClientTest : public SyncTest {
+ public:
+  EnableDisableSingleClientTest() : SyncTest(SINGLE_CLIENT) {}
+  ~EnableDisableSingleClientTest() override {}
+
+  // Don't use self-notifications as they can trigger additional sync cycles.
+  bool TestUsesSelfNotifications() override { return false; }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(EnableDisableSingleClientTest);
+};
+
 IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, EnableOneAtATime) {
   ASSERT_TRUE(SetupClients());
-
+  SyncPrefs sync_prefs(GetProfile(0)->GetPrefs());
   // Setup sync with no enabled types.
   ASSERT_TRUE(GetClient(0)->SetupSync(ModelTypeSet()));
-
-  syncer::UserShare* user_share = GetSyncService(0)->GetUserShare();
-  const syncer::DataTypeStatusTable& data_type_status_table =
-      GetSyncService(0)->data_type_status_table();
+  UserShare* user_share = GetSyncService(0)->GetUserShare();
 
   const ModelTypeSet registered_types =
       GetSyncService(0)->GetRegisteredDataTypes();
-  const ModelTypeSet registered_directory_types =
-      Difference(registered_types, UnifiedSyncServiceTypes());
-  const ModelTypeSet registered_directory_user_types =
-      Intersection(registered_types, syncer::UserSelectableTypes());
-  for (ModelTypeSet::Iterator it = registered_directory_user_types.First();
-       it.Good(); it.Inc()) {
-    ASSERT_TRUE(GetClient(0)->EnableSyncForDatatype(it.Get()));
-
-    // AUTOFILL_PROFILE is lumped together with AUTOFILL.
-    // SESSIONS is lumped together with PROXY_TABS and
-    // HISTORY_DELETE_DIRECTIVES.
-    // Favicons are lumped together with PROXY_TABS and
-    // HISTORY_DELETE_DIRECTIVES.
-    if (it.Get() == syncer::AUTOFILL_PROFILE || it.Get() == syncer::SESSIONS) {
-      continue;
+  const ModelTypeSet multi_grouped_types =
+      MultiGroupTypes(sync_prefs, registered_types);
+  const ModelTypeSet selectable_types =
+      Intersection(registered_types, UserSelectableTypes());
+  for (ModelTypeSet::Iterator si = selectable_types.First(); si.Good();
+       si.Inc()) {
+    const ModelTypeSet grouped_types =
+        Difference(Difference(sync_prefs.ResolvePrefGroups(
+                                  registered_types, ModelTypeSet(si.Get())),
+                              ProxyTypes()),
+                   UnifiedSyncServiceTypes());
+    const ModelTypeSet single_grouped_types =
+        Difference(grouped_types, multi_grouped_types);
+    for (ModelTypeSet::Iterator sgi = single_grouped_types.First(); sgi.Good();
+         sgi.Inc()) {
+      VerifyExistence(user_share, false, sgi.Get(), si.Get());
     }
 
-    if (!syncer::ProxyTypes().Has(it.Get())) {
-      ASSERT_TRUE(DoesTopLevelNodeExist(user_share, it.Get()) ||
-                  IsUnready(data_type_status_table, it.Get()))
-          << syncer::ModelTypeToString(it.Get());
-    }
+    EXPECT_TRUE(GetClient(0)->EnableSyncForDatatype(si.Get()));
 
-    // AUTOFILL_PROFILE is lumped together with AUTOFILL.
-    if (it.Get() == syncer::AUTOFILL) {
-      ASSERT_TRUE(DoesTopLevelNodeExist(user_share,
-                                        syncer::AUTOFILL_PROFILE));
-    } else if (it.Get() == syncer::HISTORY_DELETE_DIRECTIVES ||
-               it.Get() == syncer::PROXY_TABS) {
-      ASSERT_TRUE(DoesTopLevelNodeExist(user_share,
-                                        syncer::SESSIONS));
+    for (ModelTypeSet::Iterator gi = grouped_types.First(); gi.Good();
+         gi.Inc()) {
+      VerifyExistence(user_share, true, gi.Get(), si.Get());
     }
   }
 }
 
 IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, DisableOneAtATime) {
   ASSERT_TRUE(SetupClients());
-
+  SyncPrefs sync_prefs(GetProfile(0)->GetPrefs());
   // Setup sync with no disabled types.
   ASSERT_TRUE(GetClient(0)->SetupSync());
-
-  const ModelTypeSet registered_types =
-      GetSyncService(0)->GetRegisteredDataTypes();
-  const ModelTypeSet registered_directory_types =
-      Difference(registered_types, UnifiedSyncServiceTypes());
-
-  syncer::UserShare* user_share = GetSyncService(0)->GetUserShare();
-
+  UserShare* user_share = GetSyncService(0)->GetUserShare();
   const syncer::DataTypeStatusTable& data_type_status_table =
       GetSyncService(0)->data_type_status_table();
 
+  const ModelTypeSet registered_types =
+      GetSyncService(0)->GetRegisteredDataTypes();
+  const ModelTypeSet multi_grouped_types =
+      MultiGroupTypes(sync_prefs, registered_types);
+  const ModelTypeSet registered_directory_types = Difference(
+      Difference(registered_types, UnifiedSyncServiceTypes()), ProxyTypes());
   // Make sure all top-level nodes exist first.
-  for (ModelTypeSet::Iterator it = registered_directory_types.First();
-       it.Good(); it.Inc()) {
-    if (!syncer::ProxyTypes().Has(it.Get())) {
-      ASSERT_TRUE(DoesTopLevelNodeExist(user_share, it.Get()) ||
-                  IsUnready(data_type_status_table, it.Get()));
-    }
+  for (ModelTypeSet::Iterator rdi = registered_directory_types.First();
+       rdi.Good(); rdi.Inc()) {
+    EXPECT_TRUE(DoesTopLevelNodeExist(user_share, rdi.Get()) ||
+                IsUnready(data_type_status_table, rdi.Get()))
+        << ModelTypeToString(rdi.Get());
   }
 
-  for (ModelTypeSet::Iterator it = registered_directory_types.First();
-       it.Good(); it.Inc()) {
-    // SUPERVISED_USERS and SUPERVISED_USER_SHARED_SETTINGS are always synced.
-    if (it.Get() == syncer::SUPERVISED_USERS ||
-        it.Get() == syncer::SUPERVISED_USER_SHARED_SETTINGS ||
-        it.Get() == syncer::SYNCED_NOTIFICATIONS ||
-        it.Get() == syncer::SYNCED_NOTIFICATION_APP_INFO)
-      continue;
-
-    // Device info cannot be disabled.
-    if (it.Get() == syncer::DEVICE_INFO)
-      continue;
-
-    ASSERT_TRUE(GetClient(0)->DisableSyncForDatatype(it.Get()));
-
-    // AUTOFILL_PROFILE is lumped together with AUTOFILL.
-    // SESSIONS is lumped together with PROXY_TABS and TYPED_URLS.
-    // HISTORY_DELETE_DIRECTIVES is lumped together with TYPED_URLS.
-    // PRIORITY_PREFERENCES is lumped together with PREFERENCES.
-    // Favicons are lumped together with PROXY_TABS and
-    // HISTORY_DELETE_DIRECTIVES.
-    if (it.Get() == syncer::AUTOFILL_PROFILE ||
-        it.Get() == syncer::SESSIONS ||
-        it.Get() == syncer::HISTORY_DELETE_DIRECTIVES ||
-        it.Get() == syncer::PRIORITY_PREFERENCES ||
-        it.Get() == syncer::FAVICON_IMAGES ||
-        it.Get() == syncer::FAVICON_TRACKING) {
-      continue;
+  const ModelTypeSet selectable_types =
+      Intersection(registered_types, UserSelectableTypes());
+  for (ModelTypeSet::Iterator si = selectable_types.First(); si.Good();
+       si.Inc()) {
+    const ModelTypeSet grouped_types =
+        Difference(Difference(sync_prefs.ResolvePrefGroups(
+                                  registered_types, ModelTypeSet(si.Get())),
+                              ProxyTypes()),
+                   UnifiedSyncServiceTypes());
+    for (ModelTypeSet::Iterator gi = grouped_types.First(); gi.Good();
+         gi.Inc()) {
+      VerifyExistence(user_share, true, gi.Get(), si.Get());
     }
 
-    syncer::UserShare* user_share =
-        GetSyncService(0)->GetUserShare();
-
-    ASSERT_FALSE(DoesTopLevelNodeExist(user_share, it.Get()))
-        << syncer::ModelTypeToString(it.Get());
-
-    if (it.Get() == syncer::AUTOFILL) {
-      // AUTOFILL_PROFILE is lumped together with AUTOFILL.
-      ASSERT_FALSE(DoesTopLevelNodeExist(user_share, syncer::AUTOFILL_PROFILE));
-    } else if (it.Get() == syncer::TYPED_URLS) {
-      ASSERT_FALSE(DoesTopLevelNodeExist(user_share,
-                                         syncer::HISTORY_DELETE_DIRECTIVES));
-      // SESSIONS should be enabled only if PROXY_TABS is.
-      ASSERT_EQ(GetClient(0)->IsTypePreferred(syncer::PROXY_TABS),
-                DoesTopLevelNodeExist(user_share, syncer::SESSIONS));
-    } else if (it.Get() == syncer::PROXY_TABS) {
-      // SESSIONS should be enabled only if TYPED_URLS is.
-      ASSERT_EQ(GetClient(0)->IsTypePreferred(syncer::TYPED_URLS),
-                DoesTopLevelNodeExist(user_share, syncer::SESSIONS));
-    } else if (it.Get() == syncer::PREFERENCES) {
-      ASSERT_FALSE(DoesTopLevelNodeExist(user_share,
-                                         syncer::PRIORITY_PREFERENCES));
+    EXPECT_TRUE(GetClient(0)->DisableSyncForDatatype(si.Get()));
+
+    const ModelTypeSet single_grouped_types =

[Patrick Noland, 2017/02/02 22:11:37]

Is the calculation of grouped, single_Grouped, and multi_grouped types common
enough to lift into helpers? I think it would help readability

[skym, 2017/02/02 23:37:52]

I tried to simplify things. I'm not sure it helped very much, I really just
moved around where things are defined for some of the more generic sets so we
don't repeat ourselves as much between the two cases.

The thing here is that something like set3 = Difference(set1, set2) is already
pretty concise/terse.

Lemme know what you think of the updates.

+        Difference(grouped_types, multi_grouped_types);
+    for (ModelTypeSet::Iterator sgi = single_grouped_types.First(); sgi.Good();
+         sgi.Inc()) {
+      VerifyExistence(user_share, false, sgi.Get(), si.Get());
     }
   }
+
+  // Lastly make sure that all the multi grouped times are all gone, since we
+  // did not check these after disabling inside the above loop.
+  for (ModelTypeSet::Iterator mgi = multi_grouped_types.First(); mgi.Good();
+       mgi.Inc()) {
+    VerifyExistence(user_share, false, mgi.Get());
+  }
 }
 
 }  // namespace