sync: Refactor themes sync integration tests

sync: Refactor themes sync integration tests

Rewrites the themes sync integration tests so they no longer depend on
AwaitQuiescence() or related functions.

Removes racy themes tests.  They don't offer significantly better
coverage than non-racy tests.  Most of them were flaky and partially
disabled.

Adds or modifies existing tests so we have some coverage of both model
association and ProcessSyncChanges().

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

[rlarocque, 2014-07-15 20:26:21]

Here's another integration test refactoring.  Please review.

[pval...(no longer on Chromium), 2014-07-15 20:34:47]

https://codereview.chromium.org/395503004/diff/20001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/themes_helper.cc (right):

https://codereview.chromium.org/395503004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/themes_helper.cc:85: // but they do
occasionally check that the ThemeService is attempts to install
s/attempts/attempting/ ?

https://codereview.chromium.org/395503004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/themes_helper.cc:250: VLOG(1) << "Current
theme ID is: " << GetThemeID(profile);
should this be *D*VLOG?

https://codereview.chromium.org/395503004/diff/20001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/two_client_themes_sync_test.cc
(right):

https://codereview.chromium.org/395503004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/two_client_themes_sync_test.cc:32: class
LegacyTwoClientThemesSyncTest : public SyncTest {
remove this (woo)

https://codereview.chromium.org/395503004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/two_client_themes_sync_test.cc:56: //
TODO(sync): Add functions to simulate when a pending extension
it may not have been exactly what akalin had in mind, but doesn't your CL
basically accomplish the first part of this TODO?

[rlarocque, 2014-07-15 20:47:01]

Thanks for the quick review.

Patch uploaded.  PTAL.

https://codereview.chromium.org/395503004/diff/20001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/themes_helper.cc (right):

https://codereview.chromium.org/395503004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/themes_helper.cc:85: // but they do
occasionally check that the ThemeService is attempts to install
On 2014/07/15 20:34:47, pvalenzuela wrote:
> s/attempts/attempting/ ?

I removed the preceding "is".

https://codereview.chromium.org/395503004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/themes_helper.cc:250: VLOG(1) << "Current
theme ID is: " << GetThemeID(profile);
On 2014/07/15 20:34:47, pvalenzuela wrote:
> should this be *D*VLOG?

Actually, I should just remove them.  I was using them to debug an issue in the
themes code itself.

https://codereview.chromium.org/395503004/diff/20001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/two_client_themes_sync_test.cc
(right):

https://codereview.chromium.org/395503004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/two_client_themes_sync_test.cc:32: class
LegacyTwoClientThemesSyncTest : public SyncTest {
On 2014/07/15 20:34:47, pvalenzuela wrote:
> remove this (woo)

Done.

https://codereview.chromium.org/395503004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/two_client_themes_sync_test.cc:56: //
TODO(sync): Add functions to simulate when a pending extension
On 2014/07/15 20:34:47, pvalenzuela wrote:
> it may not have been exactly what akalin had in mind, but doesn't your CL
> basically accomplish the first part of this TODO?

No, not really.

Custom themes are implemented as extensions, and these extensions are supposed
to be downloaded from the app store.  In the extensions integration tests, we
create fake extensions and install them manually to simulate this.

In the themes tests, we never install nor fail to install themes.  We just check
that these themes/extensions are *pending* for install and consider that good
enough.

We could fake it in the same way as we do for the extensions tests, but:
1. That's outside the scope of this change.
2. It's of questionable value, since it would be using some hacks that don't
really reflect how these things are installed in the real world.

[rlarocque, 2014-07-15 23:41:16]

https://codereview.chromium.org/395503004/diff/60001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/two_client_themes_sync_test.cc
(right):

https://codereview.chromium.org/395503004/diff/60001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/two_client_themes_sync_test.cc:27: 
I just noticed that I forgot to set the flag to disable self-notificaions here. 
Will fix in the next upload.

[pval...(no longer on Chromium), 2014-07-16 00:03:46]

lgtm

https://codereview.chromium.org/395503004/diff/20001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/two_client_themes_sync_test.cc
(right):

https://codereview.chromium.org/395503004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/two_client_themes_sync_test.cc:56: //
TODO(sync): Add functions to simulate when a pending extension
On 2014/07/15 20:47:01, rlarocque wrote:
> On 2014/07/15 20:34:47, pvalenzuela wrote:
> > it may not have been exactly what akalin had in mind, but doesn't your CL
> > basically accomplish the first part of this TODO?
> 
> No, not really.
> 
> Custom themes are implemented as extensions, and these extensions are supposed
> to be downloaded from the app store.  In the extensions integration tests, we
> create fake extensions and install them manually to simulate this.
> 
> In the themes tests, we never install nor fail to install themes.  We just
check
> that these themes/extensions are *pending* for install and consider that good
> enough.
> 
> We could fake it in the same way as we do for the extensions tests, but:
> 1. That's outside the scope of this change.
> 2. It's of questionable value, since it would be using some hacks that don't
> really reflect how these things are installed in the real world.
K; makes sense.

[rlarocque, 2014-07-16 00:59:11]

Self-notifications are now disabled.

Haitao: ping.

[haitaol1, 2014-07-16 16:37:53]

lgtm

[rlarocque, 2014-07-16 17:14:40]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-07-16 17:16:39]

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

[commit-bot: I haz the power, 2014-07-16 22:27:09]

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

[commit-bot: I haz the power, 2014-07-17 03:51:05]

Change committed as 283629