PyAuto hooks for Sync in TestingAutomationProvider

PyAuto hooks for Sync in TestingAutomationProvider

This patch exposes hooks for sync in TestingAutomationProvider that the chrome pyauto test suite can use. It contains the following changes:

- Partial revert of an earlier change to ProfileSyncServiceHarness. Some of its methods were made pure virtual, but this ended up being unnecessary. Also ripped out unnecessary code from LiveSyncTest.
- Minor refactor of ProfileSyncServiceHarness to allow for scenarios where the browser is restarted.
- A bunch of new methods in TestingAutomationProvider: SignInToSync, GetSyncInfo, AwaitSyncCycleCompletion, EnableSyncForDatatypes and DisableSyncForDatatypes.
- A new method in model_type.h/cc called ModelTypeFromString. Required for automation.
- New APIs in pyauto.py for sync.
- New test suite sync.py with sample tests.

BUG=53651, 60970, 56460, 61639
TEST=run pyauto sync tests

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

[Raghu Simha, 2010-10-27 04:31:49]

Nirnimesh, please review.

Note: You will need a sync username and password in order for a test to sign
into sync. For now, I have left placeholders in sync.py. Let me know how you
want to manage account credentials.

[Raghu Simha, 2010-10-27 05:05:47]

+timsteele for TestingAutomationProvider methods.

[Nirnimesh, 2010-10-27 05:36:53]

Thanks a ton for doing this. The testers are going to love these.

http://codereview.chromium.org/4096004/diff/5001/6003
File chrome/test/functional/PYAUTO_TESTS (right):

http://codereview.chromium.org/4096004/diff/5001/6003#newcode50
chrome/test/functional/PYAUTO_TESTS:50: 'sync',
You could add a test username/password in a datafile in the internal repo (in
another CL), and pull it in sync.py before enabling it.

Or else, I was going to add a private data dir and some helper accessor methods
for other pyauto scripts anyway, so you could skip enabling this.

http://codereview.chromium.org/4096004/diff/5001/6004
File chrome/test/functional/sync.py (right):

http://codereview.chromium.org/4096004/diff/5001/6004#newcode28
chrome/test/functional/sync.py:28: BOOKMARKS = 2; # See
chrome/browser/sync/syncable/model_type.h.
nit: remove ';'. add an extra space before #

http://codereview.chromium.org/4096004/diff/5001/6004#newcode31
chrome/test/functional/sync.py:31: 
nit: please leave 2 blank lines at global scope

http://codereview.chromium.org/4096004/diff/5001/6005
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/4096004/diff/5001/6005#newcode1605
chrome/test/pyautolib/pyauto.py:1605: 
The docstring doesn't say that you return True/False. Please add a Returns:
section. Also in the methods below

http://codereview.chromium.org/4096004/diff/5001/6005#newcode1616
chrome/test/pyautolib/pyauto.py:1616: def IsSyncEnabled(self):
How about adding a general call for GetSyncInfo(), which would return info about
the sync account in use (if any), if sync is enabled, if encryption is enabled,
and anything else. These will help with some verification.

http://codereview.chromium.org/4096004/diff/5001/6005#newcode1629
chrome/test/pyautolib/pyauto.py:1629: 
Please document that user needs to be signed in before using this call.
Also in the methods below

http://codereview.chromium.org/4096004/diff/5001/6005#newcode1653
chrome/test/pyautolib/pyauto.py:1653: datatype: Numeric index of the datatype
for which to enable sync.
Is there a way to specify a combination of datatypes, say BOOKMARKS and
PREFERENCES?

It'd be easier if I could pass a list of strings for which to perform this
action (same for DisableSyncForDatatype).
EnableSyncForDatatype(['BOOKMARKS', 'PREFERNCES']).

If you do this, you could merge EnableSyncForAllDatatypes with this, ie sync for
all if datatype == ["ALL"]

What do you think?

[Raghu Simha, 2010-10-28 19:44:56]

Uploading an interim patch as I continue debugging. This isn't ready for a full
review yet.

http://codereview.chromium.org/4096004/diff/5001/6003
File chrome/test/functional/PYAUTO_TESTS (right):

http://codereview.chromium.org/4096004/diff/5001/6003#newcode50
chrome/test/functional/PYAUTO_TESTS:50: 'sync',
On 2010/10/27 05:36:53, Nirnimesh wrote:
> You could add a test username/password in a datafile in the internal repo (in
> another CL), and pull it in sync.py before enabling it.
> 
> Or else, I was going to add a private data dir and some helper accessor
methods
> for other pyauto scripts anyway, so you could skip enabling this.

Sounds good. I've commented this out for now, and logged http://crbug.com/60970.

http://codereview.chromium.org/4096004/diff/5001/6004
File chrome/test/functional/sync.py (right):

http://codereview.chromium.org/4096004/diff/5001/6004#newcode28
chrome/test/functional/sync.py:28: BOOKMARKS = 2; # See
chrome/browser/sync/syncable/model_type.h.
On 2010/10/27 05:36:53, Nirnimesh wrote:
> nit: remove ';'. add an extra space before #

Done.

http://codereview.chromium.org/4096004/diff/5001/6004#newcode31
chrome/test/functional/sync.py:31: 
On 2010/10/27 05:36:53, Nirnimesh wrote:
> nit: please leave 2 blank lines at global scope

Done.

http://codereview.chromium.org/4096004/diff/5001/6005
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/4096004/diff/5001/6005#newcode1605
chrome/test/pyautolib/pyauto.py:1605: 
On 2010/10/27 05:36:53, Nirnimesh wrote:
> The docstring doesn't say that you return True/False. Please add a Returns:
> section. Also in the methods below

Done.

http://codereview.chromium.org/4096004/diff/5001/6005#newcode1629
chrome/test/pyautolib/pyauto.py:1629: 
On 2010/10/27 05:36:53, Nirnimesh wrote:
> Please document that user needs to be signed in before using this call.
> Also in the methods below

Done.

[Raghu Simha, 2010-11-02 02:55:00]

I've made all the changes suggested in last week's code review. I was able to
successfully run all the tests in sync.py by plugging in my own test account
credentials.
The only work left to do is hook up the GAIA username and password
(http://crbug.com/60970).

timsteele: Please review all files under chrome/browser/sync.
nirnimesh: Please review all other files.

http://codereview.chromium.org/4096004/diff/5001/6005
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/4096004/diff/5001/6005#newcode1616
chrome/test/pyautolib/pyauto.py:1616: def IsSyncEnabled(self):
On 2010/10/27 05:36:53, Nirnimesh wrote:
> How about adding a general call for GetSyncInfo(), which would return info
about
> the sync account in use (if any), if sync is enabled, if encryption is
enabled,
> and anything else. These will help with some verification.

Done.

http://codereview.chromium.org/4096004/diff/5001/6005#newcode1653
chrome/test/pyautolib/pyauto.py:1653: datatype: Numeric index of the datatype
for which to enable sync.
On 2010/10/27 05:36:53, Nirnimesh wrote:
> Is there a way to specify a combination of datatypes, say BOOKMARKS and
> PREFERENCES?
> 
> It'd be easier if I could pass a list of strings for which to perform this
> action (same for DisableSyncForDatatype).
> EnableSyncForDatatype(['BOOKMARKS', 'PREFERNCES']).
> 
> If you do this, you could merge EnableSyncForAllDatatypes with this, ie sync
for
> all if datatype == ["ALL"]
> 
> What do you think?

Done.

[Nirnimesh, 2010-11-02 05:48:26]

Code I commented on LGTM

http://codereview.chromium.org/4096004/diff/27001/28008
File chrome/test/functional/sync.py (right):

http://codereview.chromium.org/4096004/diff/27001/28008#newcode8
chrome/test/functional/sync.py:8: import pprint
move system imports before pyauto imports

http://codereview.chromium.org/4096004/diff/27001/28010
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/4096004/diff/27001/28010#newcode1627
chrome/test/pyautolib/pyauto.py:1627: { u'authenticated': True,
Awesome!

[Raghu Simha, 2010-11-02 06:22:55]

Fixed some issues.

Tim, please review.

http://codereview.chromium.org/4096004/diff/27001/28008
File chrome/test/functional/sync.py (right):

http://codereview.chromium.org/4096004/diff/27001/28008#newcode8
chrome/test/functional/sync.py:8: import pprint
On 2010/11/02 05:48:26, Nirnimesh wrote:
> move system imports before pyauto imports

This was required only for debugging. Removed.

[Raghu Simha, 2010-11-03 00:33:03]

Added functionality that supports signing in to sync, restarting the browser,
and verifying that sync is still signed in. Also added a test called
testRestartBrowser() to sync.py, that serves as an example of how to do so.

Sorry about the multiple updates guys, but I think this is now ready for a final
review.

Thanks!

[tim (not reviewing), 2010-11-03 21:02:44]

two nits with browser/sync changes

http://codereview.chromium.org/4096004/diff/44001/37009
File chrome/browser/sync/profile_sync_service_harness.h (right):

http://codereview.chromium.org/4096004/diff/44001/37009#newcode31
chrome/browser/sync/profile_sync_service_harness.h:31: explicit
ProfileSyncServiceHarness(Profile* profile);
the style guide kinda discourages overloading constructors
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...)

In this case I'd probably say keep the explicit constructor and the
SetCredentials, and I'd add a factory CreateWithId (or something) method to
qualify the alternative.

http://codereview.chromium.org/4096004/diff/44001/37009#newcode39
chrome/browser/sync/profile_sync_service_harness.h:39: bool IsSyncEnabled();
enabled is so overloaded, and this is particularly confusing because
ProfileSyncService has IsSyncEnabled, yet this calls something else! 
IsSyncAlreadySetup()?

[Raghu Simha, 2010-11-03 21:34:55]

Tim: Changes made. Thanks for reviewing.

http://codereview.chromium.org/4096004/diff/44001/37009
File chrome/browser/sync/profile_sync_service_harness.h (right):

http://codereview.chromium.org/4096004/diff/44001/37009#newcode31
chrome/browser/sync/profile_sync_service_harness.h:31: explicit
ProfileSyncServiceHarness(Profile* profile);
On 2010/11/03 21:02:44, timsteele wrote:
> the style guide kinda discourages overloading constructors
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...)
> 
> In this case I'd probably say keep the explicit constructor and the
> SetCredentials, and I'd add a factory CreateWithId (or something) method to
> qualify the alternative.

Done.

http://codereview.chromium.org/4096004/diff/44001/37009#newcode39
chrome/browser/sync/profile_sync_service_harness.h:39: bool IsSyncEnabled();
On 2010/11/03 21:02:44, timsteele wrote:
> enabled is so overloaded, and this is particularly confusing because
> ProfileSyncService has IsSyncEnabled, yet this calls something else! 
> IsSyncAlreadySetup()?

Yikes, you're right. Done.

[Raghu Simha, 2010-11-03 22:04:42]

Tim, please take another look.

Made a final change as discussed during an in-person code review. Decided to get
rid of the explicit constructor and replace it with a factory method called
CreateAndAttach(), since it better represents what we're trying to do in code.

[tim (not reviewing), 2010-11-03 22:09:12]

LGTM, please see my last comment.

http://codereview.chromium.org/4096004/diff/66001/67007
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/4096004/diff/66001/67007#newcode108
chrome/browser/sync/profile_sync_service_harness.cc:108: Profile* profile) {
I would probably add
if (!profile->HasProfileSyncService()) {
  NOTREACHED() << "Nothing to attach to!";
  return NULL;
}
and a comment on the declaration mentioning the precondition.

[Raghu Simha, 2010-11-03 22:29:13]

Thanks for the review. Will commit after trybots go green.

http://codereview.chromium.org/4096004/diff/66001/67007
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/4096004/diff/66001/67007#newcode108
chrome/browser/sync/profile_sync_service_harness.cc:108: Profile* profile) {
On 2010/11/03 22:09:12, timsteele wrote:
> I would probably add
> if (!profile->HasProfileSyncService()) {
>   NOTREACHED() << "Nothing to attach to!";
>   return NULL;
> }
> and a comment on the declaration mentioning the precondition.

Done.