[Sync] Added some basic extension sync integration tests.

[Sync] Added some basic extension sync integration tests.

Refactored theme integration tests to share code with extension integration tests.

Fixed a crasher in ExtensionUpdater when run under integration tests.

BUG=53531
TEST=New integration tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=65911

[akalin, 2010-11-09 22:09:15]

+rsimha for review

I'll probably decomp the ExtensionUpdater change into its own CL.

[Raghu Simha, 2010-11-10 21:42:41]

Really nice work! I've made a few comments that mostly have to do with improving
the interface used by test cases, and reducing the need for boilerplate code in
tests.

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/live_...
File chrome/test/live_sync/live_extensions_sync_test.h (right):

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_extensions_sync_test.h:26: static bool
HasSameExtensions(Profile* profile1, Profile* profile2);
I have a suggestion here: Tests will be easier to write if this method takes
indices of profiles that need to be compared. We currently do this in
live_bookmarks_sync_test.h by providing tests with a few options for
end-of-test-case checks. This scales to single, two, multi and many client
tests. For example, we can have:

bool HasSameExtensions(int profile_a, int profile_b);
bool HasSameExtensionsAsVerifier(int profile);
bool AllProfilesHaveSameExtensions();
bool AllProfilesHaveSameExtensionsAsVerifier();

Then, test cases that use the verifier can simply call
AllProfilesHaveSameExtensions(), while tests that make racy changes (and
therefore, do not use the verifier) can call
AllProfilesHaveSameExtensionsAsVerifier().

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/live_...
File chrome/test/live_sync/live_extensions_sync_test_base.cc (right):

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_extensions_sync_test_base.cc:31: int index, bool
is_theme) {
nit: I believe the new style guide recommendation is for parameters to be one
per line and aligned.

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/live_...
File chrome/test/live_sync/live_extensions_sync_test_base.h (right):

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_extensions_sync_test_base.h:22: explicit
LiveExtensionsSyncTestBase(TestType test_type);
Perhaps this should be a private constructor? Right now, it is possible for
someone to write a test case using LiveExtensionsSyncTestBase, and we'd like to
avoid that.

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_extensions_sync_test_base.h:32:
scoped_refptr<Extension> MakeTheme() WARN_UNUSED_RESULT;
I think it'd be better to create a unique theme / extension based on an integer
index. That way, if a test needs to apply more than one theme, it can call
GetTheme(X), GetTheme(Y), etc., where X and Y are unique identifiers.

Also, I liked the original name GetTheme(). This way, the first time GetTheme(X)
is called, we make a theme with index X. The subsequent time it is called, we
can simply return the theme.

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_extensions_sync_test_base.h:36:
scoped_refptr<Extension> MakeExtension() WARN_UNUSED_RESULT;
Same comment as above.

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_extensions_sync_test_base.h:40: Profile* profile,
scoped_refptr<Extension> extension);
Tests will be a lot easier to write if you take the index of a profile instead
of a pointer to the profile object. For example, see the methods in
live_bookmarks_sync_test.h.

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_extensions_sync_test_base.h:43: void
InstallAllPendingExtensions(Profile* profile);
Same comment as above.

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/singl...
File chrome/test/live_sync/single_client_live_extensions_sync_test.cc (right):

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/singl...
chrome/test/live_sync/single_client_live_extensions_sync_test.cc:28: "Waiting
for extensions change."));
The above 3 lines can all be replaced by a single call to SetupSync(). This is
because SetupSync() will implicitly call SetupClients() if it hasn't already
been called. It also implicitly takes every client through its initial sync
cycle.

Applies to every test case in this patch.

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/two_c...
File chrome/test/live_sync/two_client_live_extensions_sync_test.cc (right):

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/two_c...
chrome/test/live_sync/two_client_live_extensions_sync_test.cc:47:
ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
In this test, since both clients need to sync stuff from each other, the correct
way to wait is to call AwaitQuiescence().

[akalin, 2010-11-12 01:20:44]

Addressed all comments.  I want to do some more cleanup, but that can wait until
a future CL (after I get the basic apps tests checked in, too)

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/live_...
File chrome/test/live_sync/live_extensions_sync_test.h (right):

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_extensions_sync_test.h:26: static bool
HasSameExtensions(Profile* profile1, Profile* profile2);
On 2010/11/10 21:42:41, rsimha wrote:
> I have a suggestion here: Tests will be easier to write if this method takes
> indices of profiles that need to be compared. We currently do this in
> live_bookmarks_sync_test.h by providing tests with a few options for
> end-of-test-case checks. This scales to single, two, multi and many client
> tests. For example, we can have:
> 
> bool HasSameExtensions(int profile_a, int profile_b);
> bool HasSameExtensionsAsVerifier(int profile);
> bool AllProfilesHaveSameExtensions();
> bool AllProfilesHaveSameExtensionsAsVerifier();
> 
> Then, test cases that use the verifier can simply call
> AllProfilesHaveSameExtensions(), while tests that make racy changes (and
> therefore, do not use the verifier) can call
> AllProfilesHaveSameExtensionsAsVerifier().

Done, although I added the functions I actually use. (I'll add the other ones
when I need them.)

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/live_...
File chrome/test/live_sync/live_extensions_sync_test_base.cc (right):

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_extensions_sync_test_base.cc:31: int index, bool
is_theme) {
On 2010/11/10 21:42:41, rsimha wrote:
> nit: I believe the new style guide recommendation is for parameters to be one
> per line and aligned.

Done.

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/live_...
File chrome/test/live_sync/live_extensions_sync_test_base.h (right):

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_extensions_sync_test_base.h:22: explicit
LiveExtensionsSyncTestBase(TestType test_type);
On 2010/11/10 21:42:41, rsimha wrote:
> Perhaps this should be a private constructor? Right now, it is possible for
> someone to write a test case using LiveExtensionsSyncTestBase, and we'd like
to
> avoid that.

Done.

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_extensions_sync_test_base.h:32:
scoped_refptr<Extension> MakeTheme() WARN_UNUSED_RESULT;
On 2010/11/10 21:42:41, rsimha wrote:
> I think it'd be better to create a unique theme / extension based on an
integer
> index. That way, if a test needs to apply more than one theme, it can call
> GetTheme(X), GetTheme(Y), etc., where X and Y are unique identifiers.
> 
> Also, I liked the original name GetTheme(). This way, the first time
GetTheme(X)
> is called, we make a theme with index X. The subsequent time it is called, we
> can simply return the theme.

Done.

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_extensions_sync_test_base.h:36:
scoped_refptr<Extension> MakeExtension() WARN_UNUSED_RESULT;
On 2010/11/10 21:42:41, rsimha wrote:
> Same comment as above.

Done.

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_extensions_sync_test_base.h:40: Profile* profile,
scoped_refptr<Extension> extension);
On 2010/11/10 21:42:41, rsimha wrote:
> Tests will be a lot easier to write if you take the index of a profile instead
> of a pointer to the profile object. For example, see the methods in
> live_bookmarks_sync_test.h.

Given that I need to treat the verifier() separately, I think it's better to
leave it as a Profile*.  Duplicating to the verifier() automatically may work,
but it makes things less obvious.

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/singl...
File chrome/test/live_sync/single_client_live_extensions_sync_test.cc (right):

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/singl...
chrome/test/live_sync/single_client_live_extensions_sync_test.cc:28: "Waiting
for extensions change."));
On 2010/11/10 21:42:41, rsimha wrote:
> The above 3 lines can all be replaced by a single call to SetupSync(). This is
> because SetupSync() will implicitly call SetupClients() if it hasn't already
> been called. It also implicitly takes every client through its initial sync
> cycle.
> 
> Applies to every test case in this patch.

Done.

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/two_c...
File chrome/test/live_sync/two_client_live_extensions_sync_test.cc (right):

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/two_c...
chrome/test/live_sync/two_client_live_extensions_sync_test.cc:47:
ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
On 2010/11/10 21:42:41, rsimha wrote:
> In this test, since both clients need to sync stuff from each other, the
correct
> way to wait is to call AwaitQuiescence().

Converted everything to AwaitQuiescence() because that's what we really care
about.

[Raghu Simha, 2010-11-12 01:40:50]

LGTM, with some minor comments.

Thanks for adding these tests!

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/live_...
File chrome/test/live_sync/live_extensions_sync_test_base.h (right):

http://codereview.chromium.org/4732005/diff/11001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_extensions_sync_test_base.h:40: Profile* profile,
scoped_refptr<Extension> extension);
On 2010/11/12 01:20:44, akalin wrote:
> On 2010/11/10 21:42:41, rsimha wrote:
> > Tests will be a lot easier to write if you take the index of a profile
instead
> > of a pointer to the profile object. For example, see the methods in
> > live_bookmarks_sync_test.h.
> 
> Given that I need to treat the verifier() separately, I think it's better to
> leave it as a Profile*.  Duplicating to the verifier() automatically may work,
> but it makes things less obvious.

You're right. SGTM.

http://codereview.chromium.org/4732005/diff/22001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_extensions_sync_test_base.h:32:
scoped_refptr<Extension> MakeTheme() WARN_UNUSED_RESULT;
Awesome :)

http://codereview.chromium.org/4732005/diff/22001/chrome/test/live_sync/singl...
File chrome/test/live_sync/single_client_live_extensions_sync_test.cc (right):

http://codereview.chromium.org/4732005/diff/22001/chrome/test/live_sync/singl...
chrome/test/live_sync/single_client_live_extensions_sync_test.cc:40:
ASSERT_TRUE(AwaitQuiescence());
This can stay as AwaitSyncCycleCompletion. AwaitQuiescence is an expensive call,
and can sometimes slow down a test if called when only one client is making a
change.

http://codereview.chromium.org/4732005/diff/22001/chrome/test/live_sync/singl...
chrome/test/live_sync/single_client_live_extensions_sync_test.cc:55:
ASSERT_TRUE(AwaitQuiescence());
Same comment as above.

http://codereview.chromium.org/4732005/diff/22001/chrome/test/live_sync/two_c...
File chrome/test/live_sync/two_client_live_themes_sync_test.cc (right):

http://codereview.chromium.org/4732005/diff/22001/chrome/test/live_sync/two_c...
chrome/test/live_sync/two_client_live_themes_sync_test.cc:83:
ASSERT_TRUE(AwaitQuiescence());
This can stay as AwaitMutualSyncCycleCompletion for the same reason as I
mentioned in another comment.

[Raghu Simha, 2010-11-12 01:43:42]

Correcting an earlier comment. Still LGTM!

http://codereview.chromium.org/4732005/diff/22001/chrome/test/live_sync/singl...
File chrome/test/live_sync/single_client_live_extensions_sync_test.cc (right):

http://codereview.chromium.org/4732005/diff/22001/chrome/test/live_sync/singl...
chrome/test/live_sync/single_client_live_extensions_sync_test.cc:40:
ASSERT_TRUE(AwaitQuiescence());
On 2010/11/12 01:40:50, rsimha wrote:
> This can stay as AwaitSyncCycleCompletion. AwaitQuiescence is an expensive
call,
> and can sometimes slow down a test if called when only one client is making a
> change.

Oh crap, AwaitQuiescence is fine here because it follows a call to SetupSync().
But my earlier comment applies in cases where both clients are synced, and only
one of them makes a change.

:)

[akalin, 2010-11-12 02:19:03]

Addresssed all comments, made one more pass over calls to AwaitQuiescence, et
al.  Can you take one final look?

http://codereview.chromium.org/4732005/diff/22001/chrome/test/live_sync/singl...
File chrome/test/live_sync/single_client_live_extensions_sync_test.cc (right):

http://codereview.chromium.org/4732005/diff/22001/chrome/test/live_sync/singl...
chrome/test/live_sync/single_client_live_extensions_sync_test.cc:40:
ASSERT_TRUE(AwaitQuiescence());
Not needed at all!

On 2010/11/12 01:43:42, rsimha wrote:
> On 2010/11/12 01:40:50, rsimha wrote:
> > This can stay as AwaitSyncCycleCompletion. AwaitQuiescence is an expensive
> call,
> > and can sometimes slow down a test if called when only one client is making
a
> > change.
> 
> Oh crap, AwaitQuiescence is fine here because it follows a call to
SetupSync().
> But my earlier comment applies in cases where both clients are synced, and
only
> one of them makes a change.
> 
> :)

http://codereview.chromium.org/4732005/diff/22001/chrome/test/live_sync/singl...
chrome/test/live_sync/single_client_live_extensions_sync_test.cc:55:
ASSERT_TRUE(AwaitQuiescence());
On 2010/11/12 01:40:50, rsimha wrote:
> Same comment as above.

It's a single-client test, so AwaitQuiescence is okay as we discussed.

http://codereview.chromium.org/4732005/diff/22001/chrome/test/live_sync/two_c...
File chrome/test/live_sync/two_client_live_themes_sync_test.cc (right):

http://codereview.chromium.org/4732005/diff/22001/chrome/test/live_sync/two_c...
chrome/test/live_sync/two_client_live_themes_sync_test.cc:83:
ASSERT_TRUE(AwaitQuiescence());
On 2010/11/12 01:40:50, rsimha wrote:
> This can stay as AwaitMutualSyncCycleCompletion for the same reason as I
> mentioned in another comment.

Done, also fixed other places in here

http://codereview.chromium.org/4732005/diff/38001/chrome/test/live_sync/two_c...
File chrome/test/live_sync/two_client_live_extensions_sync_test.cc (right):

http://codereview.chromium.org/4732005/diff/38001/chrome/test/live_sync/two_c...
chrome/test/live_sync/two_client_live_extensions_sync_test.cc:73:
ASSERT_TRUE(AwaitQuiescence());
Added one here since we have two clients making changes

http://codereview.chromium.org/4732005/diff/38001/chrome/test/live_sync/two_c...
chrome/test/live_sync/two_client_live_extensions_sync_test.cc:96:
ASSERT_TRUE(AwaitQuiescence());
Ditto

[Raghu Simha, 2010-11-12 02:32:20]

Responses to your comments are below. No need to wait for another review.

http://codereview.chromium.org/4732005/diff/22001/chrome/test/live_sync/singl...
File chrome/test/live_sync/single_client_live_extensions_sync_test.cc (right):

http://codereview.chromium.org/4732005/diff/22001/chrome/test/live_sync/singl...
chrome/test/live_sync/single_client_live_extensions_sync_test.cc:55:
ASSERT_TRUE(AwaitQuiescence());
On 2010/11/12 02:19:03, akalin wrote:
> On 2010/11/12 01:40:50, rsimha wrote:
> > Same comment as above.
> 
> It's a single-client test, so AwaitQuiescence is okay as we discussed.

Yeah, in this case, it's equivalent to a call to AwaitSyncCycleCompletion in
theory, because there's only one client. However, I'd recommend directly calling
AwaitSyncCycleCompletion in single client tests. This will avoid the overhead of
2 levels of function calls, and will make the logs (when verbose logging is
enabled) easier to understand, by not spewing 3 lines of logging when only one
is sufficient.

http://codereview.chromium.org/4732005/diff/38001/chrome/test/live_sync/two_c...
File chrome/test/live_sync/two_client_live_extensions_sync_test.cc (right):

http://codereview.chromium.org/4732005/diff/38001/chrome/test/live_sync/two_c...
chrome/test/live_sync/two_client_live_extensions_sync_test.cc:73:
ASSERT_TRUE(AwaitQuiescence());
On 2010/11/12 02:19:03, akalin wrote:
> Added one here since we have two clients making changes

Agree.

[akalin, 2010-11-12 02:47:10]

Okay, checking in as soon as trybots pass!

http://codereview.chromium.org/4732005/diff/22001/chrome/test/live_sync/singl...
File chrome/test/live_sync/single_client_live_extensions_sync_test.cc (right):

http://codereview.chromium.org/4732005/diff/22001/chrome/test/live_sync/singl...
chrome/test/live_sync/single_client_live_extensions_sync_test.cc:55:
ASSERT_TRUE(AwaitQuiescence());
On 2010/11/12 02:32:20, rsimha wrote:
> On 2010/11/12 02:19:03, akalin wrote:
> > On 2010/11/12 01:40:50, rsimha wrote:
> > > Same comment as above.
> > 
> > It's a single-client test, so AwaitQuiescence is okay as we discussed.
> 
> Yeah, in this case, it's equivalent to a call to AwaitSyncCycleCompletion in
> theory, because there's only one client. However, I'd recommend directly
calling
> AwaitSyncCycleCompletion in single client tests. This will avoid the overhead
of
> 2 levels of function calls, and will make the logs (when verbose logging is
> enabled) easier to understand, by not spewing 3 lines of logging when only one
> is sufficient.

Done.