[sync] Eliminate Await*SyncCompletion methods in integration tests

[sync] Eliminate Await*SyncCompletion methods in integration tests

For the longest time, the sync integration tests have relied on methods
that wait for sync cycle completion by polling various internal fields
in the sync session snapshot. These methods are brittle and rely on
internal state, which is not ideal.

This cl is a first step towards eliminating the use of internal state to
detect sync completion. It does the following:

1) Removes the methods IsDataSynced, IsFullySynced, IsDataSyncedImpl,
   AwaitDataSyncCompletion and AwaitFullSyncCompletion
2) Adds a method AwaitSyncSetupCompletion which waits until a client is
   ready to push local changes to the server.
3) Adds a method AwaitCommitActivityCompletion which waits until a client
   no longer has data to commit and its progress markers are updated.
4) Updates call sites to use the new methods.

BUG=94990, 97780, 323380
TEST=sync_integration_tests
R=rlarocque@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247599

[Raghu Simha, 2014-01-27 23:56:20]

Richard, please review. Thanks.

[rlarocque, 2014-01-28 02:50:47]

Here are some comments.  Stopping now because it's late.  I'll take another look
tomorrow morning.

https://codereview.chromium.org/148723002/diff/80002/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/profile_sync_service_harness.cc
(right):

https://codereview.chromium.org/148723002/diff/80002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:55: static
const int kSyncOperationTimeoutMs = 30000;
Are you sure about this?  I think I've seen sync tests in the 40s range on the
build bots before.

https://codereview.chromium.org/148723002/diff/80002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:491: bool
ProfileSyncServiceHarness::AwaitSteadyState() {
It's not clear to me what we can infer about the state of the system when we run
out of callbacks.  This condition is likely to be very racy.  Why should we use
it instead of AwaitStatusChange?

https://codereview.chromium.org/148723002/diff/80002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:554: bool
ProfileSyncServiceHarness::IsCommitActivityComplete() const {
I think HasFullySyncedProgressMarkers() might be a better name for this.

I was a bit confused about what IsCommitActivityComplete() meant.  The way I see
it, the client is done committing when the last sync cycle that included commits
is done.  That occurs long before the follow-up, self-notify-induced sync cycle
that this function tests for.

[Raghu Simha, 2014-01-28 21:13:11]

Here are responses to your initial comments. Updated CL coming up.

https://codereview.chromium.org/148723002/diff/80002/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/profile_sync_service_harness.cc
(right):

https://codereview.chromium.org/148723002/diff/80002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:55: static
const int kSyncOperationTimeoutMs = 30000;
On 2014/01/28 02:50:48, rlarocque wrote:
> Are you sure about this?  I think I've seen sync tests in the 40s range on the
> build bots before.

In that case, I'll undo this change and leave it for its own CL. Having said
that, the point I make is still valid: For the most part, we'd like tests that
timeout to have a few seconds to log their failure cause before the test runner
kills the test.

https://codereview.chromium.org/148723002/diff/80002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:491: bool
ProfileSyncServiceHarness::AwaitSteadyState() {
On 2014/01/28 02:50:48, rlarocque wrote:
> It's not clear to me what we can infer about the state of the system when we
run
> out of callbacks.  This condition is likely to be very racy.  Why should we
use
> it instead of AwaitStatusChange?

AwaitStatusChange will run the message loop forever until an exit criterion is
satisfied. This relies on the fact that ProfileSyncService will nudge the
harness, and can (and does) cause tests to timeout. I added AwaitSteadyState as
a means to pump the message loop for a particular client before moving on to
other clients.

I accept your point about it potentially being racy. Stay tuned for an updated
CL that reuses AwaitStatusChange in these cases.

https://codereview.chromium.org/148723002/diff/80002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:554: bool
ProfileSyncServiceHarness::IsCommitActivityComplete() const {
On 2014/01/28 02:50:48, rlarocque wrote:
> I think HasFullySyncedProgressMarkers() might be a better name for this.
> 
> I was a bit confused about what IsCommitActivityComplete() meant.  The way I
see
> it, the client is done committing when the last sync cycle that included
commits
> is done.  That occurs long before the follow-up, self-notify-induced sync
cycle
> that this function tests for.  

I've renamed it to HasLatestProgressMarkers.

[Raghu Simha, 2014-01-28 21:50:47]

Richard, I've uploaded a new patch set for review. PTAL.

[rlarocque, 2014-01-28 23:08:28]

I think I'm getting a better idea of what this patch includes now.  The idea is
to remove all existing wait methods and replace them with a new wait method
based almost entirely on progress markers?

If so, I'm not sure this will work.
 
- Are there no cases where the tests rely on the existing 'await' methods to
exit if they encounter a serious failure?  If the test was expecting its 'Await'
function to exit early if an unrecoverable error is encountered (I think the
current test functions still do stuff like that, right?) then they could be
waiting a long time.  

- IIRC, we disable self-notifications in every test harness where that's an
option.  We have no guarantee that progress markers will ever become fully
synced in those tests.

https://codereview.chromium.org/148723002/diff/150001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/profile_sync_service_harness.cc
(right):

https://codereview.chromium.org/148723002/diff/150001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:257: if
(!AwaitSyncSetupCompletion()) {
As I understand it, this will loop until ShouldPushChanges() returns true.  At
this point in init, that's likely to be blocked on the condition

  data_type_manager_->state() == DataTypeManager::CONFIGURED;

That's OK, I guess.  It means we'll keep looping util we're ready to commit
changes.

However, if the server were to reject our credentials or we fail to decrypt the
data we receive, I don't think we'll ever reach the ShouldPushChanges() state. 
This makes me wonder what the if statements on lines 264 and 271 are for.  

Also, we won't exit if we hit an unrecoverable error, either.  I'm not sure if
that's a problem.

https://codereview.chromium.org/148723002/diff/150001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:366: bool
ProfileSyncServiceHarness::AwaitCommitActivityCompletion() {
You should change the name of this function, too.

https://codereview.chromium.org/148723002/diff/150001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:399: if
(ServiceIsPushingChanges()) {
If this is likely to become a pattern (ie. many StatusChangeCheckers will have
special conditions to check before they start looping), you should add support
for it in the StatusChangeChecker class.

[Raghu Simha, 2014-01-29 00:28:43]

Richard, PTAL.

On 2014/01/28 23:08:28, rlarocque wrote:
> I think I'm getting a better idea of what this patch includes now.  The idea
is
> to remove all existing wait methods and replace them with a new wait method
> based almost entirely on progress markers?

For the most part, yes. There still are cases like initial sync setup or
enable/disable tests in which we don't check progress markers. Also, note that
this patch doesn't add a dependency on progress markers at any spot that didn't
already depend on progress markers (among other things.) 

> - Are there no cases where the tests rely on the existing 'await' methods to
> exit if they encounter a serious failure?  If the test was expecting its
'Await'
> function to exit early if an unrecoverable error is encountered (I think the
> current test functions still do stuff like that, right?) then they could be
> waiting a long time.

The only tests where we expect the wait for commit activity to fail are tests
that disable sync. The error checks in AwaitStatusChange exit early in these
cases.

> - IIRC, we disable self-notifications in every test harness where that's an
> option.  We have no guarantee that progress markers will ever become fully
> synced in those tests.

These tests don't call on AwaitCommitActivityCompletion. To reiterate, I don't
believe this CL adds a dependency on progress markers to any spot that didn't
already depend on them.

https://codereview.chromium.org/148723002/diff/150001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/profile_sync_service_harness.cc
(right):

https://codereview.chromium.org/148723002/diff/150001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:257: if
(!AwaitSyncSetupCompletion()) {
On 2014/01/28 23:08:29, rlarocque wrote:
> However, if the server were to reject our credentials or we fail to decrypt
the
> data we receive, I don't think we'll ever reach the ShouldPushChanges() state.

> This makes me wonder what the if statements on lines 264 and 271 are for.  
> 
> Also, we won't exit if we hit an unrecoverable error, either.  I'm not sure if
> that's a problem.

Fair point. I've covered these error conditions in DoneWaitingForSyncSetup,
which causes AwaitSyncSetupCompletion to exit early and return false if any
errors were hit.

https://codereview.chromium.org/148723002/diff/150001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:366: bool
ProfileSyncServiceHarness::AwaitCommitActivityCompletion() {
On 2014/01/28 23:08:29, rlarocque wrote:
> You should change the name of this function, too.

Actually, here's why I'd like to keep this name instead of renaming it to
something like AwaitLatestProgressMarkers. Numerous tests call this method when
a client changes data, in order to wait for its commit activity to end. I don't
think tests should have to know about or worry about progress markers. In
future, once the C++ server is implemented, we should re-implement this function
using knowledge of global state and remove references to progress markers.

I've made a note of this in a TODO.

https://codereview.chromium.org/148723002/diff/150001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:399: if
(ServiceIsPushingChanges()) {
On 2014/01/28 23:08:29, rlarocque wrote:
> If this is likely to become a pattern (ie. many StatusChangeCheckers will have
> special conditions to check before they start looping), you should add support
> for it in the StatusChangeChecker class.

The following checks are now in AwaitStatusChange. They needn't be repeated in
other Await* methods, so I've removed them.
- If sync is disabled, return false
- If the exit criterion is already satisfied, return true

[rlarocque, 2014-01-29 00:58:12]

I think you're right about IsDataSynced() and comparing progress markers.  It
turns out that IsDataSynced() is used much more rarely than I thought it was.

LGTM.

[Raghu Simha, 2014-01-29 02:35:16]

Committed patchset #3 manually as r247599 (presubmit successful).