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.

Removes or renames many other redundant tests.

BUG=97780, 95742, 351159
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282241

[rlarocque, 2014-06-20 20:03:36]

Here's the alternate approach to https://codereview.chromium.org/315823002/. 
This version does not attempt to maintain redundant tests.

PTAL.

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

https://codereview.chromium.org/342853004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:61:
IN_PROC_BROWSER_TEST_F(TwoClientPreferencesSyncTest, Race) {
Testing races is nonesense.  I've replaced it with a test that syncs changes
from client 0 to client 1, then from client 1 to client 0.  This is equivalent
to one of the possible race scenarios from this test.

https://codereview.chromium.org/342853004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:75:
kPasswordManagerEnabled) {
Covered by testing of boolean pref sync.

https://codereview.chromium.org/342853004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:88:
kSyncKeepEverythingSynced) {
I think this is better covered by sync_prefs_unittest.

https://codereview.chromium.org/342853004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:102:
IN_PROC_BROWSER_TEST_F(LegacyTwoClientPreferencesSyncTest, DisablePreferences) {
Enable and disable tests: decided not to support these for every type.

https://codereview.chromium.org/342853004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:123:
IN_PROC_BROWSER_TEST_F(TwoClientPreferencesSyncTest, DisableSync) {
Another enable/disable test.

https://codereview.chromium.org/342853004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:149:
IN_PROC_BROWSER_TEST_F(TwoClientPreferencesSyncTest, SignInDialog) {
I don't see much value in this test.  It toggles a bunch of non-syncable but
sync-affecting preferences, then makes sure that these preferences don't match. 
I can't imagine how this could fail.

https://codereview.chromium.org/342853004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:184:
IN_PROC_BROWSER_TEST_F(TwoClientPreferencesSyncTest, kShowBookmarkBar) {
Another boolean test.

https://codereview.chromium.org/342853004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:194:
IN_PROC_BROWSER_TEST_F(TwoClientPreferencesSyncTest, kCheckDefaultBrowser) {
The first of the non-syncable boolean pref tests.  Renamed and refactored.

https://codereview.chromium.org/342853004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:215: //
TCM ID - 7297269.
More boolean tests.

https://codereview.chromium.org/342853004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:246:
IN_PROC_BROWSER_TEST_F(TwoClientPreferencesSyncTest,
Kept this as ComplexPrefs, since the overall scenario is sort of interesting. 
It also includes an integer pref, which we haven't covered yet.

https://codereview.chromium.org/342853004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:277:
IN_PROC_BROWSER_TEST_F(TwoClientPreferencesSyncTest, Privacy) {
Tests a bunch of boolean prefs.  Deleted.

https://codereview.chromium.org/342853004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:298:
IN_PROC_BROWSER_TEST_F(TwoClientPreferencesSyncTest, ClearData) {
Tests a different bunch of boolean prefs.  Aslo deleted.

https://codereview.chromium.org/342853004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:328:
IN_PROC_BROWSER_TEST_F(TwoClientPreferencesSyncTest,
The following deleted tests are each testing a single string or boolean pref.

https://codereview.chromium.org/342853004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:383:
kAutofillAuxiliaryProfilesEnabled) {
This one seemed kind of interesting because of the #ifdef.  Kept.

https://codereview.chromium.org/342853004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:404:
IN_PROC_BROWSER_TEST_F(TwoClientPreferencesSyncTest, kPromptForDownload) {
More deleted boolean and string pref tests follow.

https://codereview.chromium.org/342853004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:553:
SingleClientEnabledEncryption) {
As I explained in the previous code review
(https://codereview.chromium.org/315823002/), these encryption tests are of
questionable value.  I've refactored and kept two of them.

[pval...(no longer on Chromium), 2014-06-21 00:50:00]

ok, this is a lot to look at (happy to see deletion of questionably-valued
tests), but it looks good.

Tim, can you confirm that we want to go in this direction so that
https://codereview.chromium.org/315823002/ is no longer hanging around?

[rlarocque, 2014-06-24 16:56:31]

On 2014/06/21 00:50:00, pvalenzuela wrote:
> ok, this is a lot to look at (happy to see deletion of questionably-valued
> tests), but it looks good.
> 
> Tim, can you confirm that we want to go in this direction so that
> https://codereview.chromium.org/315823002/ is no longer hanging around?

Tim: Ping.

[rlarocque, 2014-06-25 17:52:07]

Tim: Ping again.

[rlarocque, 2014-06-26 19:56:05]

On 2014/06/25 17:52:07, rlarocque wrote:
> Tim: Ping again.

Tim: Ping.

[rlarocque, 2014-07-09 17:50:01]

Looks like Tim won't be reviewing this.

Nicolas, please take a look.

[Nicolas Zea, 2014-07-09 18:52:15]

LGTM with a question

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

https://codereview.chromium.org/342853004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:44:
DisableVerifier();
Why do we disable the verifier in these tests?

[rlarocque, 2014-07-09 19:01:18]

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

https://codereview.chromium.org/342853004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc:44:
DisableVerifier();
On 2014/07/09 18:52:15, Nicolas Zea wrote:
> Why do we disable the verifier in these tests?

In this test, I think it's out of habit.  It's based on "Race", which couldn't
use the verifier because its outcome was not deterministic.  I'll try turning it
back on.

In the test below (UnsyncableBooleanPref), it's disabled because we expect some
of the preferences won't match.

[rlarocque, 2014-07-09 20:48:54]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-07-09 20:51:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlarocque@chromium.org/342853004/20001

[commit-bot: I haz the power, 2014-07-10 00:22:27]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-10 03:45:06]

Change committed as 282241