[Sync] Fix flaky AndroidSyncSettings tests.

[Sync] Fix flaky AndroidSyncSettings tests.

These flakes were introduced in http://crrev.com/1416553003.

Apparently waitForIdleSync() is not sufficient to ensure the completion
of AsyncTasks, so this change adds a way to wait for the getAccounts
task directly.

BUG=554154, 554540
Committed: https://crrev.com/5bd7cb2d80c9efcb3662af5f49b7cb8f9e21dcaf
Cr-Commit-Position: refs/heads/master@{#360182}

[maxbogue, 2015-11-11 22:03:02]

Description was changed from

==========
[Sync] Fix flaky AndroidSyncSettings tests.

Apparently waitForIdleSync() is not sufficient to ensure the completion
of AsyncTasks, so this change adds a way to wait for the getAccounts
task directly.

BUG=554154,554540
==========

to

==========
[Sync] Fix flaky AndroidSyncSettings tests.

These flakes were introduced in http://crrev.com/1416553003.

Apparently waitForIdleSync() is not sufficient to ensure the completion
of AsyncTasks, so this change adds a way to wait for the getAccounts
task directly.

BUG=554154,554540
==========

[maxbogue, 2015-11-11 22:03:25]

maxbogue@chromium.org changed reviewers:
+ nyquist@chromium.org

[maxbogue, 2015-11-11 22:03:25]

Hey Tommy, PTAL!

[nyquist, 2015-11-11 22:44:31]

https://codereview.chromium.org/1434053002/diff/1/sync/test/android/javatests...
File
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java
(right):

https://codereview.chromium.org/1434053002/diff/1/sync/test/android/javatests...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:104:
mGetAccountsSemaphore = new Semaphore(1);
Initialize to 0?

https://codereview.chromium.org/1434053002/diff/1/sync/test/android/javatests...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:120:
@Deprecated
This doesn't seem related to this change?

https://codereview.chromium.org/1434053002/diff/1/sync/test/android/javatests...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:147:
mGetAccountsSemaphore.acquireUninterruptibly();
Remove this?

https://codereview.chromium.org/1434053002/diff/1/sync/test/android/javatests...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:165:
mGetAccountsSemaphore.release();
Remove .release() ?

[maxbogue, 2015-11-12 01:40:08]

I ran the test for a while with your changes and it flaked. My understanding of
how follows in the comments.

https://codereview.chromium.org/1434053002/diff/1/sync/test/android/javatests...
File
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java
(right):

https://codereview.chromium.org/1434053002/diff/1/sync/test/android/javatests...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:104:
mGetAccountsSemaphore = new Semaphore(1);
On 2015/11/11 22:44:31, nyquist wrote:
> Initialize to 0?

This limits the number of parallel getAccounts tasks that that can be happening
to 1.

https://codereview.chromium.org/1434053002/diff/1/sync/test/android/javatests...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:120:
@Deprecated
On 2015/11/11 22:44:31, nyquist wrote:
> This doesn't seem related to this change?

Whoops, just wanted to see if it would make one of the deprecation warnings from
before go away. Apparently forgot to remove it.

https://codereview.chromium.org/1434053002/diff/1/sync/test/android/javatests...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:147:
mGetAccountsSemaphore.acquireUninterruptibly();
On 2015/11/11 22:44:31, nyquist wrote:
> Remove this?

Without this, a separate getAccountsByType() call can leave the semaphore with a
permit, which will causes the waitForGetAccountsTask call in my test to return
immediately and the test to flake.

https://codereview.chromium.org/1434053002/diff/1/sync/test/android/javatests...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:165:
mGetAccountsSemaphore.release();
On 2015/11/11 22:44:31, nyquist wrote:
> Remove .release() ?

The release() is needed to allow future getAccounts calls to happen.

[nyquist, 2015-11-16 22:52:44]

https://codereview.chromium.org/1434053002/diff/60001/sync/test/android/javat...
File
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java
(right):

https://codereview.chromium.org/1434053002/diff/60001/sync/test/android/javat...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:666:
// Starts at 1 because waitUntilZero doesn't block with the count is 0.
I don't think this comment matches the code.
You want to start at 1 because you don't want the acquire()+release() calls of
waitUntilZero to block until there is at least one call to increment which
acquires it.

https://codereview.chromium.org/1434053002/diff/60001/sync/test/android/javat...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:673:
mSemaphore.acquireUninterruptibly();
You should probably add a long timeout here (10 seconds?)

https://codereview.chromium.org/1434053002/diff/60001/sync/test/android/javat...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:685:
mSemaphore.acquire();
You should probably add a long timeout here (10 seconds?)

[maxbogue, 2015-11-16 23:43:51]

https://codereview.chromium.org/1434053002/diff/60001/sync/test/android/javat...
File
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java
(right):

https://codereview.chromium.org/1434053002/diff/60001/sync/test/android/javat...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:666:
// Starts at 1 because waitUntilZero doesn't block with the count is 0.
On 2015/11/16 22:52:44, nyquist wrote:
> I don't think this comment matches the code.
> You want to start at 1 because you don't want the acquire()+release() calls of
> waitUntilZero to block until there is at least one call to increment which
> acquires it.

I simplified the code with a lock as you suggested offline, PTAL.

https://codereview.chromium.org/1434053002/diff/60001/sync/test/android/javat...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:673:
mSemaphore.acquireUninterruptibly();
On 2015/11/16 22:52:44, nyquist wrote:
> You should probably add a long timeout here (10 seconds?)

Acknowledged.

https://codereview.chromium.org/1434053002/diff/60001/sync/test/android/javat...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:685:
mSemaphore.acquire();
On 2015/11/16 22:52:44, nyquist wrote:
> You should probably add a long timeout here (10 seconds?)

Done.

[nyquist, 2015-11-17 20:34:39]

lgtm

https://codereview.chromium.org/1434053002/diff/80001/sync/test/android/javat...
File
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java
(right):

https://codereview.chromium.org/1434053002/diff/80001/sync/test/android/javat...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:684:
mLock.wait(10000);
Nit: Could you extract this to a constant with some nice name like
"foobarsomething_TIMEOUT_MS"?

[maxbogue, 2015-11-17 22:09:29]

https://codereview.chromium.org/1434053002/diff/80001/sync/test/android/javat...
File
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java
(right):

https://codereview.chromium.org/1434053002/diff/80001/sync/test/android/javat...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:684:
mLock.wait(10000);
On 2015/11/17 20:34:39, nyquist wrote:
> Nit: Could you extract this to a constant with some nice name like
> "foobarsomething_TIMEOUT_MS"?

Done.

[maxbogue, 2015-11-17 22:09:35]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2015-11-17 22:09:36]

The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/1434053002/#ps100001
(title: "Address comments.")

[commit-bot: I haz the power, 2015-11-17 22:10:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1434053002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1434053002/100001

[commit-bot: I haz the power, 2015-11-17 22:36:52]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-17 22:37:48]

Patchset 6 (id:??) landed as
https://crrev.com/5bd7cb2d80c9efcb3662af5f49b7cb8f9e21dcaf
Cr-Commit-Position: refs/heads/master@{#360182}