sync: Refactor TwoClientPreferencesSyncTest

sync: Refactor TwoClientPreferencesSyncTest

Makes the TwoClientPreferencesSyncTests compatible with
reflection-blocking and enables reflection-blocking in its test harness.

Introduces new StatusCheckers that wait for preferences.  The new
preference status checkers work by observering preference changes rather
than sync cycle completion notifications.  Also introduces some new
status checkers to wait until encryption is enabled.

Removes DisablePreferences and DisableSync tests because we have decided
that there is not enough value in testing disable scenarios to bother
maintaining them for each type.

Removes several translation tests because they seem to not provide much
additional value and they would be particularly onerous to maintain.
Each one of these tests would have needed its own status checker or
a generic status-checker that makes use of an injected Closure.

Removes several encryption tests because they were redundant.

BUG=97780, 95742, 351159

[rlarocque, 2014-06-04 01:14:44]

+Patrick for review.
+Tim for historical perspective, review, and owners stamp.

Here's a refactoring of TwoClientPreferencesSyncTest.  I tried to stay true to
the spirit of the existing tests, even when they did not make much sense to me.

I've left some comments to explain the deleted blocks.

I'm tempted to delete much more of these tests. There are 29 of them remaining
after this CL.  Roughly 20 of them consist of setting a preference and making
sure it syncs.  The only difference between them is which preference they set.

There's also a handful of them that test certain preferences are not synced.  I
had to resort to a few hacks to support those without using AwaitQuiescence() or
its equivalents.

A few of them seem to be testing the sync setup flow.  I don't know why they're
here, mixed in with the tests of more normal preferences.

Here's what would make sense to me:
- Move tests related to encryption or setup flow somewhere else.
- Keep one vanilla preference sync test for each type (bool, string, int, etc.)
and delete the rest.
- Keep one test for unsyncable preferences and delete the rest.

If you think this is a good idea, I'll get to work on a different CL. 
Otherwise, I assert that this current CL doesn't diverge too much from the tests
as they exist today, so there would be no harm to test coverage in cleaning it
up and committing it.

https://codereview.chromium.org/315823002/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc
(left):

https://codereview.chromium.org/315823002/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:101: //
TCM ID - 3661290.
I believe these two fall under the category of disable tests, which we decided
weren't worth testing on a type-by-type basis.

https://codereview.chromium.org/315823002/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:414:
IN_PROC_BROWSER_TEST_F(TwoClientPreferencesSyncTest,
These tests would be particularly difficult to maintain going forward.  They'd
require their own set of status checkers.  They have no obvious hooks for
observers, so we'd probably have to trigger the checks based on sync cycles,
rather than observing the model directly.

If you'd like to keep them, it can be done.  I just figured I'd offer you the
choice to delete them now, rather than doing all the work to support them only
to find that everyone agrees they're redundant and should be deleted anyway.

https://codereview.chromium.org/315823002/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:564:
SingleClientEnabledEncryption) {
These tests are a not very useful.  Here's what I think may have happened:

- At some point in the past, EnableEncryption would enable encryption but would
not wait for it.
- Then something changed and EnableEncryption was setup so that it would always
wait for encryption to be completed.  I'm not sure if this happened before or
after we started untangling the ProfileSyncServiceHarness state machine
hairball.  I'm guessing before.
- Today, the IsEncryptionComplete() is redundant. 
ASSERT_TRUE(EnableEncryption(0)) by definition will not return until
IsEncryptionComplete(0) is true.

So if we remove the redundant lines from SingleClientEnabledEncryption, and
replaced the MutualSyncCycleCompletion() check with something based on a
StatusChecker, we would have:

ASSERT_TRUE(SetupSync());
ASSERT_TRUE(EnableEncryption(0);
ASSERT_TRUE(AwaitEncryptionComplete(1));

Which doesn't look much like a preferences test.  It's also functionally
identical to the similarly refactor-able BothClientsEnabledEncryption.

Since none of this makes sense to me, I decided to delete most of these tests
and leave the two that looked least trivial.

[pval...(no longer on Chromium), 2014-06-04 18:06:30]

https://codereview.chromium.org/315823002/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/preferences_helper.cc (right):

https://codereview.chromium.org/315823002/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/preferences_helper.cc:277: }
add blank line after this one

https://codereview.chromium.org/315823002/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/preferences_helper.cc:370: bool
AwaitBooleanPrefMatches(const char* pref_name) {
It's unfortunate that the PrefService design forces us to have all these
per-type verification methods, but I don't see any other way to do this.

Any crazy ideas of alternatives?

https://codereview.chromium.org/315823002/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc
(left):

https://codereview.chromium.org/315823002/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:101: //
TCM ID - 3661290.
On 2014/06/04 01:14:44, rlarocque wrote:
> I believe these two fall under the category of disable tests, which we decided
> weren't worth testing on a type-by-type basis.

roger

https://codereview.chromium.org/315823002/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:414:
IN_PROC_BROWSER_TEST_F(TwoClientPreferencesSyncTest,
On 2014/06/04 01:14:44, rlarocque wrote:
> These tests would be particularly difficult to maintain going forward.  They'd
> require their own set of status checkers.  They have no obvious hooks for
> observers, so we'd probably have to trigger the checks based on sync cycles,
> rather than observing the model directly.
> 
> If you'd like to keep them, it can be done.  I just figured I'd offer you the
> choice to delete them now, rather than doing all the work to support them only
> to find that everyone agrees they're redundant and should be deleted anyway.

I'd rather delete costly/complicated tests now and revisit them in the future.
This deletion sounds find to me.

https://codereview.chromium.org/315823002/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:564:
SingleClientEnabledEncryption) {
On 2014/06/04 01:14:43, rlarocque wrote:
> These tests are a not very useful.  Here's what I think may have happened:
> 
> - At some point in the past, EnableEncryption would enable encryption but
would
> not wait for it.
> - Then something changed and EnableEncryption was setup so that it would
always
> wait for encryption to be completed.  I'm not sure if this happened before or
> after we started untangling the ProfileSyncServiceHarness state machine
> hairball.  I'm guessing before.
> - Today, the IsEncryptionComplete() is redundant. 
> ASSERT_TRUE(EnableEncryption(0)) by definition will not return until
> IsEncryptionComplete(0) is true.
> 
> So if we remove the redundant lines from SingleClientEnabledEncryption, and
> replaced the MutualSyncCycleCompletion() check with something based on a
> StatusChecker, we would have:
> 
> ASSERT_TRUE(SetupSync());
> ASSERT_TRUE(EnableEncryption(0);
> ASSERT_TRUE(AwaitEncryptionComplete(1));
> 
> Which doesn't look much like a preferences test.  It's also functionally
> identical to the similarly refactor-able BothClientsEnabledEncryption.
> 
> Since none of this makes sense to me, I decided to delete most of these tests
> and leave the two that looked least trivial.

roger

https://codereview.chromium.org/315823002/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc
(right):

https://codereview.chromium.org/315823002/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:146: //
This pref is syncable.
Why have a syncable pref here at all? What about just verifying that no sync
cycle was initiated?

This question applies to other non-syncable pref tests here as well.

[rlarocque, 2014-06-04 18:24:40]

https://codereview.chromium.org/315823002/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/preferences_helper.cc (right):

https://codereview.chromium.org/315823002/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/preferences_helper.cc:277: }
On 2014/06/04 18:06:31, pvalenzuela wrote:
> add blank line after this one

Done.

https://codereview.chromium.org/315823002/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/preferences_helper.cc:370: bool
AwaitBooleanPrefMatches(const char* pref_name) {
On 2014/06/04 18:06:31, pvalenzuela wrote:
> It's unfortunate that the PrefService design forces us to have all these
> per-type verification methods, but I don't see any other way to do this.
> 
> Any crazy ideas of alternatives?

Templates: Too complicated.  Often requires placing lots of code in the header
file.  The original PrefService would have had even greater reason to use
templates than we do here.  They wisely chose not to.

Delegate classes:

class PrefChecker {
  PrefChecker();
  virtual CheckEqual(const char* pref_name, PrefService* service1, PrefService*
service2) = 0;
}

then subclass for each type and inject them.  This gets ugly fast.  Defining
each of these classes still requires lots of boilerplate and would confuse the
implementation of the StatusChangeChecker.  Also, these would need to be passed
around by reference or pointer in order to take advantage of the vptr, so
lifetimes may become an issue.  Where exactly would we allocate these things?

In short, this is a bad idea, too.

https://codereview.chromium.org/315823002/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc
(right):

https://codereview.chromium.org/315823002/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:146: //
This pref is syncable.
On 2014/06/04 18:06:31, pvalenzuela wrote:
> Why have a syncable pref here at all? What about just verifying that no sync
> cycle was initiated?
> 
> This question applies to other non-syncable pref tests here as well.

It's in accordance with the idea that sync cycles are internal to the sync
engine.  An integration test should not directly observe them.

Verifying that no sync cycle is initiated is hard.  We have nudge delays that
prevent things from syncing for a while after a change is made.  If we did an
EXPECT() right away, or even an EXPECT() after a short sleep, it might not
actually prove anything.

The idea here is that if a syncable pref has synced, then it's probably safe to
infer that a sync cycle has happened and that the syncer could have synced the
non-syncable pref by now if it wanted to.

This is the hack that I referred to in the comment at the top of the review
request.

[pval...(no longer on Chromium), 2014-06-04 23:00:41]

lgtm

I support the deletion of the superfluous pref tests as long as we're confident
testing by type (e.g., bool) provides the same coverage. It sounds like it does.

https://codereview.chromium.org/315823002/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/preferences_helper.cc (right):

https://codereview.chromium.org/315823002/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/preferences_helper.cc:370: bool
AwaitBooleanPrefMatches(const char* pref_name) {
On 2014/06/04 18:24:40, rlarocque wrote:
> On 2014/06/04 18:06:31, pvalenzuela wrote:
> > It's unfortunate that the PrefService design forces us to have all these
> > per-type verification methods, but I don't see any other way to do this.
> > 
> > Any crazy ideas of alternatives?
> 
> Templates: Too complicated.  Often requires placing lots of code in the header
> file.  The original PrefService would have had even greater reason to use
> templates than we do here.  They wisely chose not to.
> 
> Delegate classes:
> 
> class PrefChecker {
>   PrefChecker();
>   virtual CheckEqual(const char* pref_name, PrefService* service1,
PrefService*
> service2) = 0;
> }
> 
> then subclass for each type and inject them.  This gets ugly fast.  Defining
> each of these classes still requires lots of boilerplate and would confuse the
> implementation of the StatusChangeChecker.  Also, these would need to be
passed
> around by reference or pointer in order to take advantage of the vptr, so
> lifetimes may become an issue.  Where exactly would we allocate these things?
> 
> In short, this is a bad idea, too.

Sounds reasonable; 3 more methods isn't too bad.

https://codereview.chromium.org/315823002/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc
(right):

https://codereview.chromium.org/315823002/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:146: //
This pref is syncable.
On 2014/06/04 18:24:40, rlarocque wrote:
> On 2014/06/04 18:06:31, pvalenzuela wrote:
> > Why have a syncable pref here at all? What about just verifying that no sync
> > cycle was initiated?
> > 
> > This question applies to other non-syncable pref tests here as well.
> 
> It's in accordance with the idea that sync cycles are internal to the sync
> engine.  An integration test should not directly observe them.
> 
> Verifying that no sync cycle is initiated is hard.  We have nudge delays that
> prevent things from syncing for a while after a change is made.  If we did an
> EXPECT() right away, or even an EXPECT() after a short sleep, it might not
> actually prove anything.
> 
> The idea here is that if a syncable pref has synced, then it's probably safe
to
> infer that a sync cycle has happened and that the syncer could have synced the
> non-syncable pref by now if it wanted to.
> 
> This is the hack that I referred to in the comment at the top of the review
> request.

Ok, I think this is good.

[rlarocque, 2014-06-05 17:46:33]

Tim, what are your thoughts on deleting the tests that each test a single
preference, and instead keeping one for each preference type?