Stabilize sync integration tests

Stabilize sync integration tests

This patch contains more updates to the ProfileSyncServiceTestHarness class, in light of recent changes to the sync startup sequence, and new tests that require support for offline scenarios.

BUG=53858, 54743, 52007
TEST=sync_integration_tests

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

[Raghu Simha, 2010-09-14 04:28:40]

Sending this out for preliminary review by timsteele. Might have to revisit
startup logic.

[Paweł Hajdan Jr., 2010-09-14 17:11:25]

Drive-by with some test comments. Please do not commit without my explicit
"LGTM".

http://codereview.chromium.org/3399002/diff/8001/9005
File chrome/test/live_sync/profile_sync_service_test_harness.cc (right):

http://codereview.chromium.org/3399002/diff/8001/9005#newcode183
chrome/test/live_sync/profile_sync_service_test_harness.cc:183:
AwaitStatusChangeWithTimeout(60, reason);
Please use chrome/test/test_timeouts, do not hardcode the timeout values.

http://codereview.chromium.org/3399002/diff/8001/9005#newcode300
chrome/test/live_sync/profile_sync_service_test_harness.cc:300: if
(!AwaitStatusChangeWithTimeout(30,
Same here, and all other places.

http://codereview.chromium.org/3399002/diff/8001/9008
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/3399002/diff/8001/9008#newcode626
net/tools/testserver/chromiumsync.py:626: # print 'Authenticate'
Do not comment out code. Could you delete it?

[Raghu Simha, 2010-09-14 21:15:08]

This is now ready for review by timsteele for startup logic changes, and phajdan
for changes to the timeout mechanism.

http://codereview.chromium.org/3399002/diff/8001/9005
File chrome/test/live_sync/profile_sync_service_test_harness.cc (right):

http://codereview.chromium.org/3399002/diff/8001/9005#newcode183
chrome/test/live_sync/profile_sync_service_test_harness.cc:183:
AwaitStatusChangeWithTimeout(60, reason);
On 2010/09/14 17:11:26, Paweł Hajdan Jr. wrote:
> Please use chrome/test/test_timeouts, do not hardcode the timeout values.

Done.

http://codereview.chromium.org/3399002/diff/8001/9005#newcode300
chrome/test/live_sync/profile_sync_service_test_harness.cc:300: if
(!AwaitStatusChangeWithTimeout(30,
On 2010/09/14 17:11:26, Paweł Hajdan Jr. wrote:
> Same here, and all other places.

Done.

http://codereview.chromium.org/3399002/diff/8001/9008
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/3399002/diff/8001/9008#newcode626
net/tools/testserver/chromiumsync.py:626: # print 'Authenticate'
On 2010/09/14 17:11:26, Paweł Hajdan Jr. wrote:
> Do not comment out code. Could you delete it?

Done.

[Paweł Hajdan Jr., 2010-09-14 21:20:35]

Just a small naming comment.

http://codereview.chromium.org/3399002/diff/15001/16011
File chrome/test/test_timeouts.h (right):

http://codereview.chromium.org/3399002/diff/15001/16011#newcode48
chrome/test/test_timeouts.h:48: // Timeout to wait for a sync operation to
complete.
Can we make the name more generic, like live_test_timeout_ms?

Then the comment would say "Timeout for tests that access live services.".

[Raghu Simha, 2010-09-14 21:27:45]

http://codereview.chromium.org/3399002/diff/15001/16011
File chrome/test/test_timeouts.h (right):

http://codereview.chromium.org/3399002/diff/15001/16011#newcode48
chrome/test/test_timeouts.h:48: // Timeout to wait for a sync operation to
complete.
The timeouts are technically not for entire tests, but for specific operations.
A test can perform multiple operations of this sort. I've renamed the method to
"live_operation_timeout_ms()".

On 2010/09/14 21:20:35, Paweł Hajdan Jr. wrote:
> Can we make the name more generic, like live_test_timeout_ms?
> 
> Then the comment would say "Timeout for tests that access live services.".

[tim (not reviewing), 2010-09-14 21:34:58]

http://codereview.chromium.org/3399002/diff/15001/16005
File chrome/test/live_sync/profile_sync_service_test_harness.cc (right):

http://codereview.chromium.org/3399002/diff/15001/16005#newcode136
chrome/test/live_sync/profile_sync_service_test_harness.cc:136: if
(!GetStatus().server_reachable) {
what makes this error case special?

http://codereview.chromium.org/3399002/diff/15001/16005#newcode187
chrome/test/live_sync/profile_sync_service_test_harness.cc:187: } else {
maybe expect_eq( sync_initialized() )

http://codereview.chromium.org/3399002/diff/15001/16007
File chrome/test/live_sync/single_client_live_bookmarks_sync_test.cc (right):

http://codereview.chromium.org/3399002/diff/15001/16007#newcode140
chrome/test/live_sync/single_client_live_bookmarks_sync_test.cc:140:
IN_PROC_BROWSER_TEST_F(SingleClientLiveBookmarksSyncTest, DISABLED_GetUpdates) {
I think we can safely relieve this test of duty.

[Paweł Hajdan Jr., 2010-09-14 21:43:44]

Code I commented in the drive-by LGTM with two comments.

http://codereview.chromium.org/3399002/diff/22001/23010
File chrome/test/test_timeouts.cc (right):

http://codereview.chromium.org/3399002/diff/22001/23010#newcode56
chrome/test/test_timeouts.cc:56:
InitializeTimeout(switches::kLiveOperationTimeout,
nit: For consistency, this should be moved to the end of this function.

http://codereview.chromium.org/3399002/diff/22001/23011
File chrome/test/test_timeouts.h (right):

http://codereview.chromium.org/3399002/diff/22001/23011#newcode48
chrome/test/test_timeouts.h:48: // Timeout to wait for a live operation to
complete.
nit: Add a comment that live operation means accessing sites on the internet
etc.

[Raghu Simha, 2010-09-14 22:16:43]

http://codereview.chromium.org/3399002/diff/15001/16005
File chrome/test/live_sync/profile_sync_service_test_harness.cc (right):

http://codereview.chromium.org/3399002/diff/15001/16005#newcode136
chrome/test/live_sync/profile_sync_service_test_harness.cc:136: if
(!GetStatus().server_reachable) {
On 2010/09/14 21:34:58, timsteele wrote:
> what makes this error case special?

Spoke to Tim about this. I've annotated the entire state change machine with
comments that clarify what's going on in each state.

http://codereview.chromium.org/3399002/diff/15001/16005#newcode187
chrome/test/live_sync/profile_sync_service_test_harness.cc:187: } else {
On 2010/09/14 21:34:58, timsteele wrote:
> maybe expect_eq( sync_initialized() ) 

Done.

http://codereview.chromium.org/3399002/diff/15001/16007
File chrome/test/live_sync/single_client_live_bookmarks_sync_test.cc (right):

http://codereview.chromium.org/3399002/diff/15001/16007#newcode140
chrome/test/live_sync/single_client_live_bookmarks_sync_test.cc:140:
IN_PROC_BROWSER_TEST_F(SingleClientLiveBookmarksSyncTest, DISABLED_GetUpdates) {
On 2010/09/14 21:34:58, timsteele wrote:
> I think we can safely relieve this test of duty.

Done.

http://codereview.chromium.org/3399002/diff/22001/23010
File chrome/test/test_timeouts.cc (right):

http://codereview.chromium.org/3399002/diff/22001/23010#newcode56
chrome/test/test_timeouts.cc:56:
InitializeTimeout(switches::kLiveOperationTimeout,
On 2010/09/14 21:43:46, Paweł Hajdan Jr. wrote:
> nit: For consistency, this should be moved to the end of this function.

Done.

http://codereview.chromium.org/3399002/diff/22001/23011
File chrome/test/test_timeouts.h (right):

http://codereview.chromium.org/3399002/diff/22001/23011#newcode48
chrome/test/test_timeouts.h:48: // Timeout to wait for a live operation to
complete.
On 2010/09/14 21:43:46, Paweł Hajdan Jr. wrote:
> nit: Add a comment that live operation means accessing sites on the internet
> etc.

Done.

[tim (not reviewing), 2010-09-14 22:23:58]

LGTM