sync: Change progress marker checking in tests

sync: Change progress marker checking in tests

Introduces two new StatusChangeCheckers: UpdatedProgressMarkerChecker
and QuiesceStatusChangeChecker.

The first, UpdatedProgressMarkerChecker, is a simple checker for
ensuring progress markers are "fully synced".  It's a bit flaky, but no
flakier than the functions that preceded it.

The second, QuiesceStatusChangeChecker, is intended to be a replacement
for AwaitQuiescence and many other Await* methods.  The logic is
different, but the heuristic used is essentially the same.  We want to
wait until all clients pass the 'HasLatestProgressMarkers()' check, then
make sure that their progress markers match each other.  The new
implementation is a bit simpler, since it takes advantage of the
transitive properties of equality rather than doing O(N^2) comparisons.

The QuiesceStatusChangeChecker is made more complicated by the fact that
the HasLatestProgressMarkers() is not very reliable.  This becomes a
problem in practice if we call that function on some "Profile A" in
response to an event that happened in "Profile B".  So the naive
implementation of QuiesceStatusChangeChecker won't work.

The implementation in this CL uses a system of observers to make the
calls to HasLatestProgressMarker() safer.  It's an ugly hack, but IMHO
no uglier than the existing set of hacks.

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

[rlarocque, 2014-03-04 01:56:18]

Here's some more integration test refactoring.  I'm hoping this will be the last
dangerous change; the rest will be no-ops to make the code prettier.

Once this change lands, there will be almost no need for
ProfileSyncServiceHarness.  Its last remaining contribution was the
'progress_marker_partner_', which the new implementation of Quiesce checking
does not need.  The next step will be to refactor the observing of
ProfileSyncService events and start moving functions out of the harness.

This change won't eliminate any races that exist in the current implementation,
but it shouldn't make them any worse, either.

https://codereview.chromium.org/184993006/diff/40001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/profile_sync_service_harness.cc
(left):

https://codereview.chromium.org/184993006/diff/40001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/profile_sync_service_harness.cc:456: if
(IsSyncDisabled()) {
This check broke a Dictionary test.  Removing it seems to have not broken
anything.

I'm happy to remove it, since I don't know why it's here in the first place. 
The test code should know which services are disabled and know how to deal with
them.  It shouldn't be AwaitStatusChange's responsibility to handle disabled
ProfileSyncServices.

The Quiesce checker, like the old AwaitQuiescence function, work around disabled
PSSs by ignoring them.

https://codereview.chromium.org/184993006/diff/40001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/single_client_status_change_checker.h
(right):

https://codereview.chromium.org/184993006/diff/40001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_status_change_checker.h:25:
virtual void InitObserver(ProfileSyncServiceHarness* obs) OVERRIDE;
This change is pretty ugly.  It was needed so we could have the
QuiesceStatusChangeChecker poke the harness so it would call back into the
QuiesceStatusChangeChecker::IsExitConditionSatisfied().

The next CL I write should remove large parts of ProfileSyncServiceHarness and
this function.

[tim (not reviewing), 2014-03-06 18:03:45]

LGTM with nits

https://codereview.chromium.org/184993006/diff/40001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/quiesce_status_change_checker.cc
(right):

https://codereview.chromium.org/184993006/diff/40001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/quiesce_status_change_checker.cc:1: //
Copyright (c) 2014 The Chromium Authors. All rights reserved.
nit - remove c

https://codereview.chromium.org/184993006/diff/40001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/quiesce_status_change_checker.cc:23: bool
ProgressMarkersMatch(const ProfileSyncService *service1,
nit - const ProfileSyncService*

https://codereview.chromium.org/184993006/diff/40001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/quiesce_status_change_checker.cc:134:
has_latest_progress_markers_ =
'latest' is misleading here for the reasons / race you point out... By latest, I
think we mean "perfmormed at least one GU after having committed all unsynced
items we may have had"?  If another client committed something, this condition
is true but we don't necessarily have the "latest" PMs.
has_latest_reflected_progress_markers_, or something to that effect?

[rlarocque, 2014-03-06 18:12:21]

https://codereview.chromium.org/184993006/diff/40001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/quiesce_status_change_checker.cc
(right):

https://codereview.chromium.org/184993006/diff/40001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/quiesce_status_change_checker.cc:134:
has_latest_progress_markers_ =
On 2014/03/06 18:03:46, timsteele wrote:
> 'latest' is misleading here for the reasons / race you point out... By latest,
I
> think we mean "perfmormed at least one GU after having committed all unsynced
> items we may have had"?  If another client committed something, this condition
> is true but we don't necessarily have the "latest" PMs.
> has_latest_reflected_progress_markers_, or something to that effect?

How about "probably_has_latest_progress_markers_"?

[tim (not reviewing), 2014-03-06 18:43:27]

On 2014/03/06 18:12:21, rlarocque wrote:
>
https://codereview.chromium.org/184993006/diff/40001/chrome/browser/sync/test...
> File chrome/browser/sync/test/integration/quiesce_status_change_checker.cc
> (right):
> 
>
https://codereview.chromium.org/184993006/diff/40001/chrome/browser/sync/test...
> chrome/browser/sync/test/integration/quiesce_status_change_checker.cc:134:
> has_latest_progress_markers_ =
> On 2014/03/06 18:03:46, timsteele wrote:
> > 'latest' is misleading here for the reasons / race you point out... By
latest,
> I
> > think we mean "perfmormed at least one GU after having committed all
unsynced
> > items we may have had"?  If another client committed something, this
condition
> > is true but we don't necessarily have the "latest" PMs.
> > has_latest_reflected_progress_markers_, or something to that effect?
> 
> How about "probably_has_latest_progress_markers_"?

:| But there's absolutely nothing in the condition that checks whether the
progress markers are "latest".  It's more of a WasSelfNotifiedOfLatestCommit()?

[rlarocque, 2014-03-06 19:23:19]

On 2014/03/06 18:43:27, timsteele wrote:
> On 2014/03/06 18:12:21, rlarocque wrote:
> >
>
https://codereview.chromium.org/184993006/diff/40001/chrome/browser/sync/test...
> > File chrome/browser/sync/test/integration/quiesce_status_change_checker.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/184993006/diff/40001/chrome/browser/sync/test...
> > chrome/browser/sync/test/integration/quiesce_status_change_checker.cc:134:
> > has_latest_progress_markers_ =
> > On 2014/03/06 18:03:46, timsteele wrote:
> > > 'latest' is misleading here for the reasons / race you point out... By
> latest,
> > I
> > > think we mean "perfmormed at least one GU after having committed all
> unsynced
> > > items we may have had"?  If another client committed something, this
> condition
> > > is true but we don't necessarily have the "latest" PMs.
> > > has_latest_reflected_progress_markers_, or something to that effect?
> > 
> > How about "probably_has_latest_progress_markers_"?
> 
> :| But there's absolutely nothing in the condition that checks whether the
> progress markers are "latest".  It's more of a
WasSelfNotifiedOfLatestCommit()?

I know.  There was a time when I wanted to rename this, too.

It's true that this function does not actually guarantee that we have the latest
progress markers.  It does not check the condition directly, because that would
be impossible.  But other functions that call this treat it as a magical black
box and they expect that it is checking this condition.  Those other functions
wouldn't make much sense if the function were more honestly named.

There's no question that the tests are broken.  As a whole, they do not do what
they claim to do.  I suppose the question is where and how do we want to expose
the inconsistency.  Do we hide the brokenness inside this function, or do we
make it clear that its callers are broken for having used it?

I'm arguing for keeping the name HasLatestProgressMarkers() in part for
historical reasons, and in part because if such a function did/could exist, we
could replace the implementation of this function and end up with correct tests.

[tim (not reviewing), 2014-03-06 22:44:06]

On 2014/03/06 19:23:19, rlarocque wrote:
> On 2014/03/06 18:43:27, timsteele wrote:
> > On 2014/03/06 18:12:21, rlarocque wrote:
> > >
> >
>
https://codereview.chromium.org/184993006/diff/40001/chrome/browser/sync/test...
> > > File chrome/browser/sync/test/integration/quiesce_status_change_checker.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/184993006/diff/40001/chrome/browser/sync/test...
> > > chrome/browser/sync/test/integration/quiesce_status_change_checker.cc:134:
> > > has_latest_progress_markers_ =
> > > On 2014/03/06 18:03:46, timsteele wrote:
> > > > 'latest' is misleading here for the reasons / race you point out... By
> > latest,
> > > I
> > > > think we mean "perfmormed at least one GU after having committed all
> > unsynced
> > > > items we may have had"?  If another client committed something, this
> > condition
> > > > is true but we don't necessarily have the "latest" PMs.
> > > > has_latest_reflected_progress_markers_, or something to that effect?
> > > 
> > > How about "probably_has_latest_progress_markers_"?
> > 
> > :| But there's absolutely nothing in the condition that checks whether the
> > progress markers are "latest".  It's more of a
> WasSelfNotifiedOfLatestCommit()?
> 
> I know.  There was a time when I wanted to rename this, too.
> 
> It's true that this function does not actually guarantee that we have the
latest
> progress markers.  It does not check the condition directly, because that
would
> be impossible.  But other functions that call this treat it as a magical black
> box and they expect that it is checking this condition.  Those other functions
> wouldn't make much sense if the function were more honestly named.
> 
> There's no question that the tests are broken.  As a whole, they do not do
what
> they claim to do.  I suppose the question is where and how do we want to
expose
> the inconsistency.  Do we hide the brokenness inside this function, or do we
> make it clear that its callers are broken for having used it?
> 
> I'm arguing for keeping the name HasLatestProgressMarkers() in part for
> historical reasons, and in part because if such a function did/could exist, we
> could replace the implementation of this function and end up with correct
tests.

ok. Personally I don't see the harm in calling out the subtleties now to avoid
confusion before we get to the future "correct tests" world. But if you feel
strongly,
LGTM.

[rlarocque, 2014-03-06 22:54:01]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-03-06 23:03:57]

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

[commit-bot: I haz the power, 2014-03-07 04:28:58]

Change committed as 255525