sync: Refactor migration integration tests

sync: Refactor migration integration tests

Refactors the migration tests to no longer reuse the same
StatusChangeChecker.

Splits the previous MigrationChecker into two separate classes:
MigrationWaiter and MigrationWatcher.  The waiter is long-lived
and observes migrations.  The waiter exists only while we're
waiting for a migration to complete.

BUG=97780, 95742
Committed: https://crrev.com/23f8f38115c398503d045e174d76b3bdef94744b
Cr-Commit-Position: refs/heads/master@{#292720}

[rlarocque, 2014-08-20 00:45:15]

This started as an attempt to refactor bookmarks integration tests.  You can see
that effort at https://codereview.chromium.org/474273003/.

I think that refactoring flakily fails because of some overlapping attempts to
pump the message loop.  The obvious solution would be to use RunLoop instead.

It turns out that using RunLoop in StatusChangeChecker breaks the migration
tests.  So I need to refactor them first.

Which brings us to this CL.  Please review.

[pval...(no longer on Chromium), 2014-08-20 19:11:37]

I'll be out for a month, so I'll let resetswitch@ look at this one to become
familiar with some of the code.

[Nicolas Zea, 2014-08-20 22:24:10]

LGTM with some nits and a comment

https://codereview.chromium.org/490643004/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/migration_waiter.h (right):

https://codereview.chromium.org/490643004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/migration_waiter.h:22:
MigrationWaiter(MigrationWatcher* watcher,
nit: swap param order?

https://codereview.chromium.org/490643004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/migration_waiter.h:39: MigrationWatcher*
watcher_;
nit: make const pointer?

https://codereview.chromium.org/490643004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/migration_waiter.h:42: syncer::ModelTypeSet
expected_types_;
const?

https://codereview.chromium.org/490643004/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/migration_watcher.cc (right):

https://codereview.chromium.org/490643004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/migration_watcher.cc:47:
migrated_types_.PutAll(pending_types_);
Does migrated_types_  == ProtocolTypes().Difference(pending_types_)? If so, I
don't think you need pending types around as a member.

https://codereview.chromium.org/490643004/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/migration_watcher.h (right):

https://codereview.chromium.org/490643004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/migration_watcher.h:38:
ProfileSyncServiceHarness* harness_;
nit: make these three const?

[rlarocque, 2014-08-21 00:41:20]

Patch updated.  PTAL.

https://codereview.chromium.org/490643004/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/migration_waiter.h (right):

https://codereview.chromium.org/490643004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/migration_waiter.h:22:
MigrationWaiter(MigrationWatcher* watcher,
On 2014/08/20 22:24:10, Nicolas Zea wrote:
> nit: swap param order?

Done.

https://codereview.chromium.org/490643004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/migration_waiter.h:39: MigrationWatcher*
watcher_;
On 2014/08/20 22:24:10, Nicolas Zea wrote:
> nit: make const pointer?

Done.

https://codereview.chromium.org/490643004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/migration_waiter.h:42: syncer::ModelTypeSet
expected_types_;
On 2014/08/20 22:24:10, Nicolas Zea wrote:
> const?

Done.

https://codereview.chromium.org/490643004/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/migration_watcher.cc (right):

https://codereview.chromium.org/490643004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/migration_watcher.cc:47:
migrated_types_.PutAll(pending_types_);
On 2014/08/20 22:24:10, Nicolas Zea wrote:
> Does migrated_types_  == ProtocolTypes().Difference(pending_types_)? If so, I
> don't think you need pending types around as a member.

No, that won't work.  This MigrationWatcher can live a long time and observe
many migrations.  If we migrate bookmarks, then migrate themes separately, the
migrated_types_ should contain both themes and bookmarks.

https://codereview.chromium.org/490643004/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/migration_watcher.h (right):

https://codereview.chromium.org/490643004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/migration_watcher.h:38:
ProfileSyncServiceHarness* harness_;
On 2014/08/20 22:24:10, Nicolas Zea wrote:
> nit: make these three const?

pending_types_, migrated_types_, and MigrationWaiter: no, these are mutable.

ProfileSyncServiceHarness: const ptr'ed.

[David Abrahams, 2014-08-22 18:54:41]

lgtm

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

https://codereview.chromium.org/490643004/diff/40001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/migration_waiter.h:39: MigrationWatcher*
const watcher_;
Nit: Feels a little weird that a Watcher has a Waiter and a Waiter has a
Watcher. But I can deal with it if it makes splitting the classses much simpler.

[rlarocque, 2014-08-29 21:50:02]

I just remembered that this exists.

I think this should be safe to commit, but I won't be around to babysit it as it
lands and watch for flakiness.  It would be irresponsible of me to hit the CQ
button if I won't be around to handle the consequences.

Feel free to try to commit this yourselves.  I don't expect it will cause
problems.

[Nicolas Zea, 2014-08-29 22:36:21]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-08-29 22:43:39]

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

[commit-bot: I haz the power, 2014-08-29 23:52:05]

Committed patchset #3 (id:40001) as 74407c7942d4cf1207827417858bc34576b0fc18

[commit-bot: I haz the power, 2014-09-10 03:11:50]

Patchset 3 (id:??) landed as
https://crrev.com/23f8f38115c398503d045e174d76b3bdef94744b
Cr-Commit-Position: refs/heads/master@{#292720}