[sync] "TwoClientBookmarksSyncTest.Sanity" is flaky

[sync] "TwoClientBookmarksSyncTest.Sanity" is flaky

Reduce flakiness in TwoClientBookmarksSyncTest by:
1) Reducing the nudge delay for sync to 1 ms
2) Switching to BookmarksMatchChecker.Wait(), which isn't prone to the
"stale snapshot" race condition.

Reducing the nudge delay applies to and should speed up all the sync
integration tests, not just the TwoClientBookmarks ones.

Only pairs of
AwaitQuiescence|AwaitMutualSyncCycleCompletion
AllModelsMatch(Verifier?)()

Were converted to use the new pattern.

BUG=660655
R=zea@chromium.org, pavely@chromium.org

Committed: https://crrev.com/3b8f13960ccb28bf854a91f05878b95529c32d79
Cr-Commit-Position: refs/heads/master@{#431376}

[Patrick Noland, 2016-11-07 20:09:22]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 20:10:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-07 21:21:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-07 21:21:38]

Dry run: This issue passed the CQ dry run.

[Patrick Noland, 2016-11-07 22:06:07]

Pavel & Zea, PTAL

[pavely, 2016-11-09 00:09:40]

lgtm

[Nicolas Zea, 2016-11-09 00:35:13]

LGTM with some nits, and a question about the cookie jar mismatch.

https://codereview.chromium.org/2479313004/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/bookmarks_helper.cc (right):

https://codereview.chromium.org/2479313004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/bookmarks_helper.cc:907:
BookmarksMatchVerifierChecker::BookmarksMatchVerifierChecker()
FYI we eventually plan to get rid of the verifier profile from tests altogether.
It's fine to keep using for now, but just a heads up.

https://codereview.chromium.org/2479313004/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
(right):

https://codereview.chromium.org/2479313004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:44: //
delays. CookieJarMismatch asserts exact histogram counts assuming that
Hmm, what's the issue with CookieJarMismatch? Ideally it shouldn't depend on
slow syncs.

https://codereview.chromium.org/2479313004/diff/1/components/sync/driver/sync...
File components/sync/driver/sync_driver_switches.cc (right):

https://codereview.chromium.org/2479313004/diff/1/components/sync/driver/sync...
components/sync/driver/sync_driver_switches.cc:29: // purpose is to speed up
integration tests.The normal delay allows coalescing
nit: "test.The" -> "test. The"

https://codereview.chromium.org/2479313004/diff/1/components/sync/driver/sync...
components/sync/driver/sync_driver_switches.cc:30: // and prevention of server
load, so don't use this unless you're really sure
nit: "server load" -> "server overload"

https://codereview.chromium.org/2479313004/diff/1/components/sync/driver/sync...
components/sync/driver/sync_driver_switches.cc:32: const char
kSyncShortNudgeDelay[] = "sync-short-nudge-delay";
I think it might be good to call this short-nudge-delay-for-test, or something
like that to make it clear it's for tests only.

https://codereview.chromium.org/2479313004/diff/1/components/sync/engine_impl...
File components/sync/engine_impl/sync_scheduler_impl.h (right):

https://codereview.chromium.org/2479313004/diff/1/components/sync/engine_impl...
components/sync/engine_impl/sync_scheduler_impl.h:89: void
SetDefaultNudgeDelay(base::TimeDelta delay_ms);
nit: Add comment explaining what this is for.

[Patrick Noland, 2016-11-09 23:31:20]

Patchset #2 (id:20001) has been deleted

[Patrick Noland, 2016-11-09 23:32:07]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 23:32:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 00:48:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-10 00:48:11]

Dry run: This issue passed the CQ dry run.

[Patrick Noland, 2016-11-10 22:13:00]

https://codereview.chromium.org/2479313004/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/bookmarks_helper.cc (right):

https://codereview.chromium.org/2479313004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/bookmarks_helper.cc:907:
BookmarksMatchVerifierChecker::BookmarksMatchVerifierChecker()
On 2016/11/09 00:35:12, Nicolas Zea wrote:
> FYI we eventually plan to get rid of the verifier profile from tests
altogether.
> It's fine to keep using for now, but just a heads up.

Acknowledged.

https://codereview.chromium.org/2479313004/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
(right):

https://codereview.chromium.org/2479313004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:44: //
delays. CookieJarMismatch asserts exact histogram counts assuming that
On 2016/11/09 00:35:13, Nicolas Zea wrote:
> Hmm, what's the issue with CookieJarMismatch? Ideally it shouldn't depend on
> slow syncs.
SessionsHelper waits for a title to load before deciding a tab is loaded when
calling OpenTabAndGetLocalWindows. SessionsSyncManager doesn't care if the title
is present or not, so it can plausibly commit the title-less tab, trigering the
histogram logic, then commit again once the title shows up.

https://codereview.chromium.org/2479313004/diff/1/components/sync/driver/sync...
File components/sync/driver/sync_driver_switches.cc (right):

https://codereview.chromium.org/2479313004/diff/1/components/sync/driver/sync...
components/sync/driver/sync_driver_switches.cc:29: // purpose is to speed up
integration tests.The normal delay allows coalescing
On 2016/11/09 00:35:13, Nicolas Zea wrote:
> nit: "test.The" -> "test. The"

Done.

https://codereview.chromium.org/2479313004/diff/1/components/sync/driver/sync...
components/sync/driver/sync_driver_switches.cc:30: // and prevention of server
load, so don't use this unless you're really sure
On 2016/11/09 00:35:13, Nicolas Zea wrote:
> nit: "server load" -> "server overload"

Done.

https://codereview.chromium.org/2479313004/diff/1/components/sync/driver/sync...
components/sync/driver/sync_driver_switches.cc:32: const char
kSyncShortNudgeDelay[] = "sync-short-nudge-delay";
On 2016/11/09 00:35:13, Nicolas Zea wrote:
> I think it might be good to call this short-nudge-delay-for-test, or something
> like that to make it clear it's for tests only.

Done.

https://codereview.chromium.org/2479313004/diff/1/components/sync/engine_impl...
File components/sync/engine_impl/sync_scheduler_impl.h (right):

https://codereview.chromium.org/2479313004/diff/1/components/sync/engine_impl...
components/sync/engine_impl/sync_scheduler_impl.h:89: void
SetDefaultNudgeDelay(base::TimeDelta delay_ms);
On 2016/11/09 00:35:13, Nicolas Zea wrote:
> nit: Add comment explaining what this is for.

Done.

[Patrick Noland, 2016-11-10 22:13:09]

The CQ bit was checked by pnoland@chromium.org

[Patrick Noland, 2016-11-10 22:13:09]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org,
pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2479313004/#ps40001
(title: "addressed comments")

[commit-bot: I haz the power, 2016-11-10 22:13:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 22:21:11]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-11-10 23:10:12]

Description was changed from

==========
[sync] "TwoClientBookmarksSyncTest.Sanity" is flaky

Reduce flakiness in TwoClientBookmarksSyncTest by:
1) Reducing the nudge delay for sync to 1 ms
2) Switching to BookmarksMatchChecker.Wait(), which isn't prone to the
"stale snapshot" race condition.

Reducing the nudge delay applies to and should speed up all the sync
integration tests, not just the TwoClientBookmarks ones.

Only pairs of
AwaitQuiescence|AwaitMutualSyncCycleCompletion
AllModelsMatch(Verifier?)()

Were converted to use the new pattern.

BUG=660655

R=zea@chromium.org, pavely@chromium.org
==========

to

==========
[sync] "TwoClientBookmarksSyncTest.Sanity" is flaky

Reduce flakiness in TwoClientBookmarksSyncTest by:
1) Reducing the nudge delay for sync to 1 ms
2) Switching to BookmarksMatchChecker.Wait(), which isn't prone to the
"stale snapshot" race condition.

Reducing the nudge delay applies to and should speed up all the sync
integration tests, not just the TwoClientBookmarks ones.

Only pairs of
AwaitQuiescence|AwaitMutualSyncCycleCompletion
AllModelsMatch(Verifier?)()

Were converted to use the new pattern.

BUG=660655

R=zea@chromium.org, pavely@chromium.org

Committed: https://crrev.com/3b8f13960ccb28bf854a91f05878b95529c32d79
Cr-Commit-Position: refs/heads/master@{#431376}
==========

[commit-bot: I haz the power, 2016-11-10 23:10:12]

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

[hiroshige, 2016-11-11 08:28:33]

A revert of this CL (patchset #2 id:40001) has been created in
https://codereview.chromium.org/2494633005/ by hiroshige@chromium.org.

The reason for reverting is: Suspected to cause
TwoClientUssSyncTest.ConflictResolution flaky failures:
https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=Tw...
.