[Sync] Make EnableDisableSingleClientTest more robust.

[Sync] Make EnableDisableSingleClientTest more robust.

EnableDisableSingleClientTest's tests enable or disable a type, and
then go verify that the root node was or was not present. This doesn't
work for USS, as USS doesn't create root nodes. So ever time we migrate
a USS type we have to update this file. However, this was a difficult
task because of the way it was set up. Full or hard coded rules about
relationships, and dependent on specific orderings of ModelType. The
most egregious violation being it would call enable/disable on non-user
selectable types, this would no-op, and then it'd verify it had the
correct affect, because typically these grouped types are preceded by
their corresponding user selectable type, and this test went through
the types in order.

To fix this, we start by making the test harness return false (failure)
if you try to enable/disable a non-selectable type. This in turns means
that EnableDisableSingleClientTest's cases need to understand some of
this type group mapping. So SyncPrefs was updated to expose
ResolvePrefGroups to allow intelligent tests. Not only does this allow
us to remove most hard coded type rules, but we are also able to test
on a more precise level, knowing typically exactly which types should
change as a result of an enable/disable. The main caveat being types
that are mapped to by multiple selectable types, which we're slightly
less strictly testing when they change.

Also updated test to guess that any new type of syncer::PRINTERS or
later will be USS. This strives to reduce the pain of adding new types,
although it is a risk in that the logic is fragile.

BUG=686172
Review-Url: https://codereview.chromium.org/2670903002
Cr-Commit-Position: refs/heads/master@{#448085}
Committed: https://chromium.googlesource.com/chromium/src/+/d09c38982b3b999ecfa67acfb8018d0b198bc4bd

[skym, 2017-02-02 20:41:49]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 20:42:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2017-02-02 20:49:25]

Description was changed from

==========
[Sync] Make EnableDisableSingleClientTest more robust.

BUG=686172
==========

to

==========
[Sync] Make EnableDisableSingleClientTest more robust.

EnableDisableSingleClientTest's tests enable or disable a type, and
then go verify that the root node was or was not present. This doesn't
work for USS, as USS doesn't create root nodes. So ever time we migrate
a USS type we have to update this file. However, this was a difficult
task because of the way it was set up. Full or hard coded rules about
relationships, and dependent on specific orderings of ModelType. The
most egregious violation being it would call enable/disable on non-user
selectable types, this would no-op, and then it'd verify it had the
correct affect, because typically these grouped types are preceded by
their corresponding user selectable type, and this test went through
the types in order.

To fix this, we start by making the test harness return false (failure)
if you try to enable/disable a non-selectable type. This in turns means
that EnableDisableSingleClientTest's cases need to understand some of
this type group mapping. So SyncPrefs was updated to expose
ResolvePrefGroups to allow intelligent tests. Not only does this allow
us to remove most hard coded type rules, but we are also able to test
on a more precise level, knowing typically exactly which types should
change as a result of an enable/disable. The main caveat being types
that are mapped to by multiple selectable types, which we're slightly
less strictly testing when they change.

Also updated test to guess that any new type of syncer::PRINTERS or
later will be USS. This strives to reduce the pain of adding new types,
although it is a risk in that the logic is fragile.

BUG=686172
==========

[skym, 2017-02-02 20:49:25]

skym@chromium.org changed reviewers:
+ maxbogue@chromium.org, pnoland@chromium.org

[skym, 2017-02-02 20:50:17]

skym@chromium.org changed reviewers:
- maxbogue@chromium.org

[skym, 2017-02-02 20:50:17]

PTAL

[commit-bot: I haz the power, 2017-02-02 21:44:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-02 21:44:42]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2670903002/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/enable_disable_test.cc (right):

https://codereview.chromium.org/2670903002/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/enable_disable_test.cc:37: void
VerifyExistence(UserShare* user_share,
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

https://codereview.chromium.org/2670903002/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/enable_disable_test.cc:61: for (int typeInt
= syncer::PRINTERS; typeInt < syncer::FIRST_PROXY_TYPE;
Should we alias PRINTERS to something analogous to FIRST_PROXY_TYPE? e.g.
FIRST_USS_TYPE. Although that example doesn't fully capture it...

https://codereview.chromium.org/2670903002/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/enable_disable_test.cc:68: // Some groups
show up in multiple groups. This means that there are at least
I don't understand the phrase "at least user selectable groups", possibly you're
missing a word?

https://codereview.chromium.org/2670903002/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/enable_disable_test.cc:68: // Some groups
show up in multiple groups. This means that there are at least
Do you mean that some types show up in multiple groups?

https://codereview.chromium.org/2670903002/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/enable_disable_test.cc:183: const
ModelTypeSet single_grouped_types =
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:34]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

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

https://codereview.chromium.org/2670903002/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/enable_disable_test.cc (right):

https://codereview.chromium.org/2670903002/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/enable_disable_test.cc:37: void
VerifyExistence(UserShare* user_share,
On 2017/02/02 22:11:37, Patrick Noland wrote:
> 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

Done.

https://codereview.chromium.org/2670903002/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/enable_disable_test.cc:61: for (int typeInt
= syncer::PRINTERS; typeInt < syncer::FIRST_PROXY_TYPE;
On 2017/02/02 22:11:37, Patrick Noland wrote:
> Should we alias PRINTERS to something analogous to FIRST_PROXY_TYPE? e.g.
> FIRST_USS_TYPE. Although that example doesn't fully capture it...

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.

https://codereview.chromium.org/2670903002/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/enable_disable_test.cc:68: // Some groups
show up in multiple groups. This means that there are at least
On 2017/02/02 22:11:37, Patrick Noland wrote:
> Do you mean that some types show up in multiple groups? 

Yes, done.

https://codereview.chromium.org/2670903002/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/enable_disable_test.cc:68: // Some groups
show up in multiple groups. This means that there are at least
On 2017/02/02 22:11:37, Patrick Noland wrote:
> I don't understand the phrase "at least user selectable groups", possibly
you're
> missing a word?

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

https://codereview.chromium.org/2670903002/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/enable_disable_test.cc:183: const
ModelTypeSet single_grouped_types =
On 2017/02/02 22:11:37, Patrick Noland wrote:
> Is the calculation of grouped, single_Grouped, and multi_grouped types common
> enough to lift into helpers? I think it would help readability

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.

[commit-bot: I haz the power, 2017-02-02 23:38:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patrick Noland, 2017-02-03 00:12:41]

lgtm

[skym, 2017-02-03 00:18:16]

skym@chromium.org changed reviewers:
+ maxbogue@chromium.org

[skym, 2017-02-03 00:18:17]

Adding max for committer lgtm.

[commit-bot: I haz the power, 2017-02-03 00:34:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-03 00:34:07]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2017-02-03 01:09:03]

rs lgtm

[skym, 2017-02-03 21:27:40]

The CQ bit was checked by skym@chromium.org

[commit-bot: I haz the power, 2017-02-03 21:28:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 22:09:06]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486157260899960,
"parent_rev": "f5b9f2202525189a68dae03f3fc9adf1f299b8fc", "commit_rev":
"d09c38982b3b999ecfa67acfb8018d0b198bc4bd"}

[commit-bot: I haz the power, 2017-02-03 22:09:42]

Description was changed from

==========
[Sync] Make EnableDisableSingleClientTest more robust.

EnableDisableSingleClientTest's tests enable or disable a type, and
then go verify that the root node was or was not present. This doesn't
work for USS, as USS doesn't create root nodes. So ever time we migrate
a USS type we have to update this file. However, this was a difficult
task because of the way it was set up. Full or hard coded rules about
relationships, and dependent on specific orderings of ModelType. The
most egregious violation being it would call enable/disable on non-user
selectable types, this would no-op, and then it'd verify it had the
correct affect, because typically these grouped types are preceded by
their corresponding user selectable type, and this test went through
the types in order.

To fix this, we start by making the test harness return false (failure)
if you try to enable/disable a non-selectable type. This in turns means
that EnableDisableSingleClientTest's cases need to understand some of
this type group mapping. So SyncPrefs was updated to expose
ResolvePrefGroups to allow intelligent tests. Not only does this allow
us to remove most hard coded type rules, but we are also able to test
on a more precise level, knowing typically exactly which types should
change as a result of an enable/disable. The main caveat being types
that are mapped to by multiple selectable types, which we're slightly
less strictly testing when they change.

Also updated test to guess that any new type of syncer::PRINTERS or
later will be USS. This strives to reduce the pain of adding new types,
although it is a risk in that the logic is fragile.

BUG=686172
==========

to

==========
[Sync] Make EnableDisableSingleClientTest more robust.

EnableDisableSingleClientTest's tests enable or disable a type, and
then go verify that the root node was or was not present. This doesn't
work for USS, as USS doesn't create root nodes. So ever time we migrate
a USS type we have to update this file. However, this was a difficult
task because of the way it was set up. Full or hard coded rules about
relationships, and dependent on specific orderings of ModelType. The
most egregious violation being it would call enable/disable on non-user
selectable types, this would no-op, and then it'd verify it had the
correct affect, because typically these grouped types are preceded by
their corresponding user selectable type, and this test went through
the types in order.

To fix this, we start by making the test harness return false (failure)
if you try to enable/disable a non-selectable type. This in turns means
that EnableDisableSingleClientTest's cases need to understand some of
this type group mapping. So SyncPrefs was updated to expose
ResolvePrefGroups to allow intelligent tests. Not only does this allow
us to remove most hard coded type rules, but we are also able to test
on a more precise level, knowing typically exactly which types should
change as a result of an enable/disable. The main caveat being types
that are mapped to by multiple selectable types, which we're slightly
less strictly testing when they change.

Also updated test to guess that any new type of syncer::PRINTERS or
later will be USS. This strives to reduce the pain of adding new types,
although it is a risk in that the logic is fragile.

BUG=686172

Review-Url: https://codereview.chromium.org/2670903002
Cr-Commit-Position: refs/heads/master@{#448085}
Committed:
https://chromium.googlesource.com/chromium/src/+/d09c38982b3b999ecfa67acfb801...
==========

[commit-bot: I haz the power, 2017-02-03 22:09:44]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/d09c38982b3b999ecfa67acfb801...