[Sync] Test SCF control states and a regression

[Sync] Test SCF control states and a regression

Four new tests for SyncCustomizationFragment:

- SCF state is correct starting from sync on and transitioning to off.
- SCF state is correct starting from sync off and transitioning to on.
- SCF's data type controls respond correctly to the sync everything switch.
- SCF opening and closing does not start sync.

In order to prevent flakiness, this CL makes some additional changes.

- Remove MockAccountManager.postAsyncAccountChangedEvent().
  Justification: we have no control about when Android performs
  the callback from it. The callback coming in at weird points
  during other test cases was making tests flaky. Testing mechanisms
  should be predictable.
- SCF now caches the backend initialized state in order to filter
  syncStateChanged() calls down to only changes to that state.

BUG=480604
Committed: https://crrev.com/a465d93b75f40e550a6362c5639d78c6c14b9642
Cr-Commit-Position: refs/heads/master@{#329740}

[maxbogue, 2015-05-04 21:16:38]

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

[maxbogue, 2015-05-04 21:16:39]

Hey Tommy and Nick, please review this change.

I'm not super happy with the solution of removing mAndroidSyncSettings from the
various classes, but it was the simplest and safest. I think the best possible
solution would be a more involved refactor of how all the singleton sync classes
are initialized and managed.

[maniscalco, 2015-05-04 22:02:58]

On 2015/05/04 21:16:39, maxbogue wrote:
> Hey Tommy and Nick, please review this change.
> 
> I'm not super happy with the solution of removing mAndroidSyncSettings from
the
> various classes, but it was the simplest and safest. I think the best possible
> solution would be a more involved refactor of how all the singleton sync
classes
> are initialized and managed.

Can you say more about the flakiness you saw.  Which test cases were flaky?  Do
you know the cause?

[maxbogue, 2015-05-05 00:36:22]

On 2015/05/04 22:02:58, maniscalco wrote:
> On 2015/05/04 21:16:39, maxbogue wrote:
> > Hey Tommy and Nick, please review this change.
> > 
> > I'm not super happy with the solution of removing mAndroidSyncSettings from
> the
> > various classes, but it was the simplest and safest. I think the best
possible
> > solution would be a more involved refactor of how all the singleton sync
> classes
> > are initialized and managed.
> 
> Can you say more about the flakiness you saw.  Which test cases were flaky? 
Do
> you know the cause?

First, all four tests would occasionally flake due to some of the components (I
believe it actually manifested through ChromeSigninController) having a
reference
to the wrong AndroidSyncSettings. Either somehow it was getting constructed
and its reference stored before the overwrite happened, or a reference was
hanging around between tests.

Actually, more investigation reveals something strangely in between those two:
https://paste.googleplex.com/5983722163666944?raw

Note where the "started: testDefaultControlStatesWithSyncOnThenOff" happens. I
do
not know what's causing that, but it seems to be something with the test runner
happening in a strange order. I don't like that AccountChangedReceiver can kick
off initializing sync stuff.

Second, the testSyncEverythingAndDataTypes test would flake occasionally due
to a syncStateChanged() call happening between when I disabled the sync
everything switch and checked the data type switch states. This was representing
an actual potential problem in the UI, though in practice it was not observed
because the sync state settles very quickly. Either way, the solution was to
only make the syncStateChanged call do anything if the state that we actually
care about is what changed.

[pval...(no longer on Chromium), 2015-05-05 18:16:36]

pvalenzuela@chromium.org changed reviewers:
+ pvalenzuela@chromium.org

[pval...(no longer on Chromium), 2015-05-05 18:16:37]

https://codereview.chromium.org/1118833002/diff/60001/chrome/android/sync_she...
File
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java
(right):

https://codereview.chromium.org/1118833002/diff/60001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java:128:
private void startSync() throws InterruptedException {
should these three methods (startSync, stopSync, waitForSyncInitialized) go in
SyncTestBase of SyncTestUtils?

https://codereview.chromium.org/1118833002/diff/60001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java:215:
// The sync switch should be on and enabled.
This sort of explanation would be more useful in the assertion message (instead
of a comment).

e.g.,
assertTrue("The sync switch should be on.",
getSyncSwitch(fragment).isChecked());
assertTrue("The sync switch should be enabled.",
getSyncSwitch(fragment).isEnabled());

https://codereview.chromium.org/1118833002/diff/60001/chrome/android/sync_she...
File
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTestBase.java
(right):

https://codereview.chromium.org/1118833002/diff/60001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTestBase.java:45:
setUpMockAndroidSyncSettings();
does this need to be moved? (if there's a dependency, we should document it)

[maniscalco, 2015-05-05 20:50:53]

On 2015/05/05 00:36:22, maxbogue wrote:
> On 2015/05/04 22:02:58, maniscalco wrote:
> > On 2015/05/04 21:16:39, maxbogue wrote:
> > > Hey Tommy and Nick, please review this change.
> > > 
> > > I'm not super happy with the solution of removing mAndroidSyncSettings
from
> > the
> > > various classes, but it was the simplest and safest. I think the best
> possible
> > > solution would be a more involved refactor of how all the singleton sync
> > classes
> > > are initialized and managed.
> > 
> > Can you say more about the flakiness you saw.  Which test cases were flaky? 
> Do
> > you know the cause?
> 
> First, all four tests would occasionally flake due to some of the components
(I
> believe it actually manifested through ChromeSigninController) having a
> reference
> to the wrong AndroidSyncSettings. Either somehow it was getting constructed
> and its reference stored before the overwrite happened, or a reference was
> hanging around between tests.
> 
> Actually, more investigation reveals something strangely in between those two:
> https://paste.googleplex.com/5983722163666944?raw
> 
> Note where the "started: testDefaultControlStatesWithSyncOnThenOff" happens. I
> do
> not know what's causing that, but it seems to be something with the test
runner
> happening in a strange order. I don't like that AccountChangedReceiver can
kick
> off initializing sync stuff.
> 
> Second, the testSyncEverythingAndDataTypes test would flake occasionally due
> to a syncStateChanged() call happening between when I disabled the sync
> everything switch and checked the data type switch states. This was
representing
> an actual potential problem in the UI, though in practice it was not observed
> because the sync state settles very quickly. Either way, the solution was to
> only make the syncStateChanged call do anything if the state that we actually
> care about is what changed.

I'll restate to make sure I understand.  It sounds like the flakiness you saw
was caused by some objects caching a reference to the AndroidSyncSettings
singleton and the "singleton" begin replaced with a new one via its
overrideForTests method.  Essentially, some code called get() which created an
AndroidSyncSettings.  Some objects then cached the resulting reference in member
variables for later use.  Later on, overrideForTests was invoked which created a
new AndroidSyncSettings, replacing the one created earlier.  However, the
objects that cached the old one never learn of the new one so we end up with a
singleton that's not really a singleton.  Did I get that right?

I thinking about how we could prevent this kind of issue in the future.  I
wonder if we should make overrideForTests assert/fail if sAndroidSyncSettings is
non-null.  We'd then also make sure the tests call overrideForTests *before* any
other class calls get().  WDYT?

[maxbogue, 2015-05-05 21:30:05]

On 2015/05/05 20:50:53, maniscalco wrote:
> On 2015/05/05 00:36:22, maxbogue wrote:
> > On 2015/05/04 22:02:58, maniscalco wrote:
> > > On 2015/05/04 21:16:39, maxbogue wrote:
> > > > Hey Tommy and Nick, please review this change.
> > > > 
> > > > I'm not super happy with the solution of removing mAndroidSyncSettings
> from
> > > the
> > > > various classes, but it was the simplest and safest. I think the best
> > possible
> > > > solution would be a more involved refactor of how all the singleton sync
> > > classes
> > > > are initialized and managed.
> > > 
> > > Can you say more about the flakiness you saw.  Which test cases were
flaky? 
> > Do
> > > you know the cause?
> > 
> > First, all four tests would occasionally flake due to some of the components
> (I
> > believe it actually manifested through ChromeSigninController) having a
> > reference
> > to the wrong AndroidSyncSettings. Either somehow it was getting constructed
> > and its reference stored before the overwrite happened, or a reference was
> > hanging around between tests.
> > 
> > Actually, more investigation reveals something strangely in between those
two:
> > https://paste.googleplex.com/5983722163666944?raw
> > 
> > Note where the "started: testDefaultControlStatesWithSyncOnThenOff" happens.
I
> > do
> > not know what's causing that, but it seems to be something with the test
> runner
> > happening in a strange order. I don't like that AccountChangedReceiver can
> kick
> > off initializing sync stuff.
> > 
> > Second, the testSyncEverythingAndDataTypes test would flake occasionally due
> > to a syncStateChanged() call happening between when I disabled the sync
> > everything switch and checked the data type switch states. This was
> representing
> > an actual potential problem in the UI, though in practice it was not
observed
> > because the sync state settles very quickly. Either way, the solution was to
> > only make the syncStateChanged call do anything if the state that we
actually
> > care about is what changed.
> 
> I'll restate to make sure I understand.  It sounds like the flakiness you saw
> was caused by some objects caching a reference to the AndroidSyncSettings
> singleton and the "singleton" begin replaced with a new one via its
> overrideForTests method.  Essentially, some code called get() which created an
> AndroidSyncSettings.  Some objects then cached the resulting reference in
member
> variables for later use.  Later on, overrideForTests was invoked which created
a
> new AndroidSyncSettings, replacing the one created earlier.  However, the
> objects that cached the old one never learn of the new one so we end up with a
> singleton that's not really a singleton.  Did I get that right?
> 
> I thinking about how we could prevent this kind of issue in the future.  I
> wonder if we should make overrideForTests assert/fail if sAndroidSyncSettings
is
> non-null.  We'd then also make sure the tests call overrideForTests *before*
any
> other class calls get().  WDYT?

Yes, your summary is correct. The problem with your suggested solution is making
sure that overrideForTests() is called before the first .get(). If I knew how to
do that part, I would have already done what you suggested. I thought we already
HAD done that part, but apparently not.

Personally, I would vastly prefer we change the singleton's to have
.init(context)
and .get() so that we explicitly know when they are being created. I think this
is
important because they manage an important set of event hooks for the system
that
we need to know are in place at the correct time. However, that would be a much
more difficult project and is out of scope for this CL.

[maxbogue, 2015-05-05 21:33:11]

https://codereview.chromium.org/1118833002/diff/60001/chrome/android/sync_she...
File
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java
(right):

https://codereview.chromium.org/1118833002/diff/60001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java:128:
private void startSync() throws InterruptedException {
On 2015/05/05 18:16:37, pvalenzuela wrote:
> should these three methods (startSync, stopSync, waitForSyncInitialized) go in
> SyncTestBase of SyncTestUtils?

Done.

https://codereview.chromium.org/1118833002/diff/60001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java:215:
// The sync switch should be on and enabled.
On 2015/05/05 18:16:37, pvalenzuela wrote:
> This sort of explanation would be more useful in the assertion message
(instead
> of a comment).
> 
> e.g.,
> assertTrue("The sync switch should be on.",
> getSyncSwitch(fragment).isChecked());
> assertTrue("The sync switch should be enabled.",
> getSyncSwitch(fragment).isEnabled());

Done, thanks. Forgot that was a thing you could do.

https://codereview.chromium.org/1118833002/diff/60001/chrome/android/sync_she...
File
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTestBase.java
(right):

https://codereview.chromium.org/1118833002/diff/60001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTestBase.java:45:
setUpMockAndroidSyncSettings();
On 2015/05/05 18:16:37, pvalenzuela wrote:
> does this need to be moved? (if there's a dependency, we should document it)

No, I don't believe there is. I originally moved it in an unsuccessful attempt
to solve the AndroidSyncSettings flakiness issue, but decided to leave it in
anyways because I think it make more sense this way.

[pval...(no longer on Chromium), 2015-05-06 00:21:46]

thanks for resolving these issues. I'll let the real reviewers do their thing.
:-)

[maniscalco, 2015-05-06 15:34:50]

On 2015/05/05 21:30:05, maxbogue wrote:
> On 2015/05/05 20:50:53, maniscalco wrote:
> > On 2015/05/05 00:36:22, maxbogue wrote:
> > > On 2015/05/04 22:02:58, maniscalco wrote:
> > > > On 2015/05/04 21:16:39, maxbogue wrote:
> > > > > Hey Tommy and Nick, please review this change.
> > > > > 
> > > > > I'm not super happy with the solution of removing mAndroidSyncSettings
> > from
> > > > the
> > > > > various classes, but it was the simplest and safest. I think the best
> > > possible
> > > > > solution would be a more involved refactor of how all the singleton
sync
> > > > classes
> > > > > are initialized and managed.
> > > > 
> > > > Can you say more about the flakiness you saw.  Which test cases were
> flaky? 
> > > Do
> > > > you know the cause?
> > > 
> > > First, all four tests would occasionally flake due to some of the
components
> > (I
> > > believe it actually manifested through ChromeSigninController) having a
> > > reference
> > > to the wrong AndroidSyncSettings. Either somehow it was getting
constructed
> > > and its reference stored before the overwrite happened, or a reference was
> > > hanging around between tests.
> > > 
> > > Actually, more investigation reveals something strangely in between those
> two:
> > > https://paste.googleplex.com/5983722163666944?raw
> > > 
> > > Note where the "started: testDefaultControlStatesWithSyncOnThenOff"
happens.
> I
> > > do
> > > not know what's causing that, but it seems to be something with the test
> > runner
> > > happening in a strange order. I don't like that AccountChangedReceiver can
> > kick
> > > off initializing sync stuff.
> > > 
> > > Second, the testSyncEverythingAndDataTypes test would flake occasionally
due
> > > to a syncStateChanged() call happening between when I disabled the sync
> > > everything switch and checked the data type switch states. This was
> > representing
> > > an actual potential problem in the UI, though in practice it was not
> observed
> > > because the sync state settles very quickly. Either way, the solution was
to
> > > only make the syncStateChanged call do anything if the state that we
> actually
> > > care about is what changed.
> > 
> > I'll restate to make sure I understand.  It sounds like the flakiness you
saw
> > was caused by some objects caching a reference to the AndroidSyncSettings
> > singleton and the "singleton" begin replaced with a new one via its
> > overrideForTests method.  Essentially, some code called get() which created
an
> > AndroidSyncSettings.  Some objects then cached the resulting reference in
> member
> > variables for later use.  Later on, overrideForTests was invoked which
created
> a
> > new AndroidSyncSettings, replacing the one created earlier.  However, the
> > objects that cached the old one never learn of the new one so we end up with
a
> > singleton that's not really a singleton.  Did I get that right?
> > 
> > I thinking about how we could prevent this kind of issue in the future.  I
> > wonder if we should make overrideForTests assert/fail if
sAndroidSyncSettings
> is
> > non-null.  We'd then also make sure the tests call overrideForTests *before*
> any
> > other class calls get().  WDYT?
> 
> Yes, your summary is correct. The problem with your suggested solution is
making
> sure that overrideForTests() is called before the first .get(). If I knew how
to
> do that part, I would have already done what you suggested. I thought we
already
> HAD done that part, but apparently not.
> 
> Personally, I would vastly prefer we change the singleton's to have
> .init(context)
> and .get() so that we explicitly know when they are being created. I think
this
> is
> important because they manage an important set of event hooks for the system
> that
> we need to know are in place at the correct time. However, that would be a
much
> more difficult project and is out of scope for this CL.

Max and I talked more about this yesterday.  While requiring that users of
AndroidSyncSettings never cache the result of get() resolves the duplicate
singleton issue, it's error prone because future users of the class may
reasonably think it's OK to cache the reference.  He's going to see if he can
find the cause of the first non-override instantiation and resolve the issue by
ensuring the overrideForTests() method is called before get().

[maxbogue, 2015-05-06 18:32:46]

PTAL at the latest patch. I switched my approach to the AndroidSyncSettings
issue; instead of preventing components from storing a reference to it, I'm
simply preventing AccountsChangedReceiver from instantiating
ChromeSigninController (and therefore AndroidSyncSettings).

[maniscalco, 2015-05-06 19:26:33]

On 2015/05/06 18:32:46, maxbogue wrote:
> PTAL at the latest patch. I switched my approach to the AndroidSyncSettings
> issue; instead of preventing components from storing a reference to it, I'm
> simply preventing AccountsChangedReceiver from instantiating
> ChromeSigninController (and therefore AndroidSyncSettings).

Great.  Did you ever figure out what exactly was triggering the pre-override
get()?

[maxbogue, 2015-05-07 20:56:05]

On 2015/05/06 19:26:33, maniscalco wrote:
> On 2015/05/06 18:32:46, maxbogue wrote:
> > PTAL at the latest patch. I switched my approach to the AndroidSyncSettings
> > issue; instead of preventing components from storing a reference to it, I'm
> > simply preventing AccountsChangedReceiver from instantiating
> > ChromeSigninController (and therefore AndroidSyncSettings).
> 
> Great.  Did you ever figure out what exactly was triggering the pre-override
> get()?

It was a call in MockAccountManager, which I have removed in the most recent
patch. I think Tommy will not like this approach, but I see no value in leaving
that call in for testing purposes. As far as I could see, the callback would
never happen during the actual test that triggered it. (For five tests
running I would see all five "sends" and then the five "receives" coming in
after the tests were done.)

[maniscalco, 2015-05-08 15:51:00]

On 2015/05/07 20:56:05, maxbogue wrote:
> On 2015/05/06 19:26:33, maniscalco wrote:
> > On 2015/05/06 18:32:46, maxbogue wrote:
> > > PTAL at the latest patch. I switched my approach to the
AndroidSyncSettings
> > > issue; instead of preventing components from storing a reference to it,
I'm
> > > simply preventing AccountsChangedReceiver from instantiating
> > > ChromeSigninController (and therefore AndroidSyncSettings).
> > 
> > Great.  Did you ever figure out what exactly was triggering the pre-override
> > get()?
> 
> It was a call in MockAccountManager, which I have removed in the most recent
> patch. I think Tommy will not like this approach, but I see no value in
leaving
> that call in for testing purposes. As far as I could see, the callback would
> never happen during the actual test that triggered it. (For five tests
> running I would see all five "sends" and then the five "receives" coming in
> after the tests were done.)

So there are several issues that combine to create flakiness.  Restating for
posterity and to make sure we're all on the same page.

1. Lazy instantiation of AndroidSyncSettings - The tests need to ensure a mock
instance is created, however, the first call to get() will create a real
instance so we must ensure the test has a chance to override before get() is
called.

2. Tests may receive broadcasts before any test setup code has a chance to
execute - Because we register for "account changed" broadcasts in the manifest,
we may end up invoking get() before the test setup code runs and has a chance to
override AndroidSyncSettings.

3. Broadcasts cross test invocations - From what Max observed, it sounds like a
broadcast can be triggered by one test invocation and be delivered in a
subsequent invocation.  That's bad because it means we don't have test
isolation, which makes it hard to create reliable tests.

Given then above I don't see a lot of great options.  Tommy, WDYT?

A. Would it be reasonable to change the manifest to not automatically subscribe
to to "account changed" (or whichever broadcast is getting us into trouble)? 
I'm assuming the answer is no, but wanted to verify.  With this approach we
modify the test to subscribe only after it has successfully overridden
AndroidSyncSettings.

B. We could go back to the approach of never caching a ref to
AndroidSyncSettings (you must always call get()), but that approach is still
racy given #3 above.

C. The approach in patchset #7 seems like it just mitigates the issue in #3
above by not triggering the broadcast.  However, couldn't some other test end up
triggering the broadcast some day?

Happy for us to get together and go over this if that's easier.

[nyquist, 2015-05-11 22:24:34]

https://codereview.chromium.org/1118833002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java
(right):

https://codereview.chromium.org/1118833002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java:150:
if (AndroidSyncSettings.get(mContext).isSyncEnabled()) {
Since we're now not supposed to cache the AndroidSyncSettings object, would it
make sense to make all the methods take a Context, and never expose the instead
of getting the instance based on the context? I do think it's easier to
understand how you have it now, but that makes it easy to in the future store a
reference to it.

https://codereview.chromium.org/1118833002/diff/80001/chrome/android/sync_she...
File
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java
(right):

https://codereview.chromium.org/1118833002/diff/80001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java:168:
assertTrue("The sync switch is on.", getSyncSwitch(fragment).isChecked());
I believe all these messages are the opposite of what they should be. I believe
they are displayed to the engineer when a test fails, not when it works.

https://codereview.chromium.org/1118833002/diff/80001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java:174:
assertTrue("Data types are checked.", dataType.isChecked());
"Data types" doesn't really help the engineer here. Is there any way we can find
out which? Same on next helper method.

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

https://codereview.chromium.org/1118833002/diff/120001/chrome/android/shell/j...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/signin/AccountsChangedReceiver.java
(left):

https://codereview.chromium.org/1118833002/diff/120001/chrome/android/shell/j...
chrome/android/shell/java/src/org/chromium/chrome/shell/signin/AccountsChangedReceiver.java:33:
ChromeSigninController.get(context).getSignedInUser();
totally fine to keep this, but I'd probably remove this file from the CL

[nyquist, 2015-05-11 23:37:21]

On 2015/05/08 15:51:00, maniscalco wrote:
> On 2015/05/07 20:56:05, maxbogue wrote:
> > On 2015/05/06 19:26:33, maniscalco wrote:
> > > On 2015/05/06 18:32:46, maxbogue wrote:
> > > > PTAL at the latest patch. I switched my approach to the
> AndroidSyncSettings
> > > > issue; instead of preventing components from storing a reference to it,
> I'm
> > > > simply preventing AccountsChangedReceiver from instantiating
> > > > ChromeSigninController (and therefore AndroidSyncSettings).
> > > 
> > > Great.  Did you ever figure out what exactly was triggering the
pre-override
> > > get()?
> > 
> > It was a call in MockAccountManager, which I have removed in the most recent
> > patch. I think Tommy will not like this approach, but I see no value in
> leaving
> > that call in for testing purposes. As far as I could see, the callback would
> > never happen during the actual test that triggered it. (For five tests
> > running I would see all five "sends" and then the five "receives" coming in
> > after the tests were done.)
> 
> So there are several issues that combine to create flakiness.  Restating for
> posterity and to make sure we're all on the same page.
> 
> 1. Lazy instantiation of AndroidSyncSettings - The tests need to ensure a mock
> instance is created, however, the first call to get() will create a real
> instance so we must ensure the test has a chance to override before get() is
> called.
> 
> 2. Tests may receive broadcasts before any test setup code has a chance to
> execute - Because we register for "account changed" broadcasts in the
manifest,
> we may end up invoking get() before the test setup code runs and has a chance
to
> override AndroidSyncSettings.
> 3. Broadcasts cross test invocations - From what Max observed, it sounds like
a
> broadcast can be triggered by one test invocation and be delivered in a
> subsequent invocation.  That's bad because it means we don't have test
> isolation, which makes it hard to create reliable tests.
> 
> Given then above I don't see a lot of great options.  Tommy, WDYT?

Yeah. You could do ugly things such as adding the Java Object ID of the current
application context when you publish the notification in
MockAccountManager.java, and then in the receiver check if the object ID matches
the current application context. You'd need a way to inject the listeners
specifically for test though. Kind of like using a factory to create a listener,
and call that from prod code, and override the implementation of the factory for
test.

> 
> A. Would it be reasonable to change the manifest to not automatically
subscribe
> to to "account changed" (or whichever broadcast is getting us into trouble)? 
> I'm assuming the answer is no, but wanted to verify.  With this approach we
> modify the test to subscribe only after it has successfully overridden
> AndroidSyncSettings.

Possibly. We do something like this for cache invalidation, where we specify the
class name in the manifest. We could use the chrome shell for sync test manifest
for example and do something new and fun there. However, in the
cacheinvalidation case we can easily refer to things because we only need the
string for the class name. Here we might have to instantiate things, which might
be a little bit more work.

> 
> B. We could go back to the approach of never caching a ref to
> AndroidSyncSettings (you must always call get()), but that approach is still
> racy given #3 above.

We could always pass in a context, and never store a reference anywhere, except
in AndroidSyncSettings itself. So basically, all methods in AndroidSyncSettings
would take a Context, and then it is impossible to cache the AndroidSyncSettings
at all. No get-method.
You still need to init it from the test though. But at least no stale
references.

> 
> C. The approach in patchset #7 seems like it just mitigates the issue in #3
> above by not triggering the broadcast.  However, couldn't some other test end
up
> triggering the broadcast some day?

Yeah, something similar could happen.

> 
> Happy for us to get together and go over this if that's easier.

[maxbogue, 2015-05-12 00:25:02]

On 2015/05/11 23:37:21, nyquist wrote:
> On 2015/05/08 15:51:00, maniscalco wrote:
> > On 2015/05/07 20:56:05, maxbogue wrote:
> > > On 2015/05/06 19:26:33, maniscalco wrote:
> > > > On 2015/05/06 18:32:46, maxbogue wrote:
> > > > > PTAL at the latest patch. I switched my approach to the
> > AndroidSyncSettings
> > > > > issue; instead of preventing components from storing a reference to
it,
> > I'm
> > > > > simply preventing AccountsChangedReceiver from instantiating
> > > > > ChromeSigninController (and therefore AndroidSyncSettings).
> > > > 
> > > > Great.  Did you ever figure out what exactly was triggering the
> pre-override
> > > > get()?
> > > 
> > > It was a call in MockAccountManager, which I have removed in the most
recent
> > > patch. I think Tommy will not like this approach, but I see no value in
> > leaving
> > > that call in for testing purposes. As far as I could see, the callback
would
> > > never happen during the actual test that triggered it. (For five tests
> > > running I would see all five "sends" and then the five "receives" coming
in
> > > after the tests were done.)
> > 
> > So there are several issues that combine to create flakiness.  Restating for
> > posterity and to make sure we're all on the same page.
> > 
> > 1. Lazy instantiation of AndroidSyncSettings - The tests need to ensure a
mock
> > instance is created, however, the first call to get() will create a real
> > instance so we must ensure the test has a chance to override before get() is
> > called.
> > 
> > 2. Tests may receive broadcasts before any test setup code has a chance to
> > execute - Because we register for "account changed" broadcasts in the
> manifest,
> > we may end up invoking get() before the test setup code runs and has a
chance
> to
> > override AndroidSyncSettings.
> > 3. Broadcasts cross test invocations - From what Max observed, it sounds
like
> a
> > broadcast can be triggered by one test invocation and be delivered in a
> > subsequent invocation.  That's bad because it means we don't have test
> > isolation, which makes it hard to create reliable tests.
> > 
> > Given then above I don't see a lot of great options.  Tommy, WDYT?
> 
> Yeah. You could do ugly things such as adding the Java Object ID of the
current
> application context when you publish the notification in
> MockAccountManager.java, and then in the receiver check if the object ID
matches
> the current application context. You'd need a way to inject the listeners
> specifically for test though. Kind of like using a factory to create a
listener,
> and call that from prod code, and override the implementation of the factory
for
> test.

I tried this. The ID never once matched in all my tests, which is why I decided
it wasn't
worth sticking with. Gets us nothing unless we could force waiting for them too.

> We could always pass in a context, and never store a reference anywhere,
except
> in AndroidSyncSettings itself. So basically, all methods in
AndroidSyncSettings
> would take a Context, and then it is impossible to cache the
AndroidSyncSettings
> at all. No get-method.
> You still need to init it from the test though. But at least no stale
> references.

Not a big fan of this solution due to the extra argument everything would need.

Neither of you have expressed your opinion on the current solution in this
change,
which I would certainly appreciate. I think it's the simplest we're likely to
find and
I don't see the downside. Another very simple solution I can think of is simply
not
caching the AndroidSyncSettings object in ChromeSigninController.

I think the broader issue is how unpredictable the initialization of these
classes is,
but I do not think addressing that is in the scope of this change. These tests
are a
high priority and that is not.

[maxbogue, 2015-05-12 00:25:26]

https://codereview.chromium.org/1118833002/diff/80001/chrome/android/sync_she...
File
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java
(right):

https://codereview.chromium.org/1118833002/diff/80001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java:168:
assertTrue("The sync switch is on.", getSyncSwitch(fragment).isChecked());
On 2015/05/11 22:24:34, nyquist wrote:
> I believe all these messages are the opposite of what they should be. I
believe
> they are displayed to the engineer when a test fails, not when it works.

Whoops, I misunderstood how they should be phrased. Fixed.

https://codereview.chromium.org/1118833002/diff/80001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java:174:
assertTrue("Data types are checked.", dataType.isChecked());
On 2015/05/11 22:24:34, nyquist wrote:
> "Data types" doesn't really help the engineer here. Is there any way we can
find
> out which? Same on next helper method.

Done.

https://codereview.chromium.org/1118833002/diff/120001/chrome/android/shell/j...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/signin/AccountsChangedReceiver.java
(left):

https://codereview.chromium.org/1118833002/diff/120001/chrome/android/shell/j...
chrome/android/shell/java/src/org/chromium/chrome/shell/signin/AccountsChangedReceiver.java:33:
ChromeSigninController.get(context).getSignedInUser();
On 2015/05/11 22:31:31, nyquist wrote:
> totally fine to keep this, but I'd probably remove this file from the CL

Done.

[maniscalco, 2015-05-12 15:52:18]

On 2015/05/12 00:25:02, maxbogue wrote:
> On 2015/05/11 23:37:21, nyquist wrote:
> > On 2015/05/08 15:51:00, maniscalco wrote:
> > > On 2015/05/07 20:56:05, maxbogue wrote:
> > > > On 2015/05/06 19:26:33, maniscalco wrote:
> > > > > On 2015/05/06 18:32:46, maxbogue wrote:
> > > > > > PTAL at the latest patch. I switched my approach to the
> > > AndroidSyncSettings
> > > > > > issue; instead of preventing components from storing a reference to
> it,
> > > I'm
> > > > > > simply preventing AccountsChangedReceiver from instantiating
> > > > > > ChromeSigninController (and therefore AndroidSyncSettings).
> > > > > 
> > > > > Great.  Did you ever figure out what exactly was triggering the
> > pre-override
> > > > > get()?
> > > > 
> > > > It was a call in MockAccountManager, which I have removed in the most
> recent
> > > > patch. I think Tommy will not like this approach, but I see no value in
> > > leaving
> > > > that call in for testing purposes. As far as I could see, the callback
> would
> > > > never happen during the actual test that triggered it. (For five tests
> > > > running I would see all five "sends" and then the five "receives" coming
> in
> > > > after the tests were done.)
> > > 
> > > So there are several issues that combine to create flakiness.  Restating
for
> > > posterity and to make sure we're all on the same page.
> > > 
> > > 1. Lazy instantiation of AndroidSyncSettings - The tests need to ensure a
> mock
> > > instance is created, however, the first call to get() will create a real
> > > instance so we must ensure the test has a chance to override before get()
is
> > > called.
> > > 
> > > 2. Tests may receive broadcasts before any test setup code has a chance to
> > > execute - Because we register for "account changed" broadcasts in the
> > manifest,
> > > we may end up invoking get() before the test setup code runs and has a
> chance
> > to
> > > override AndroidSyncSettings.
> > > 3. Broadcasts cross test invocations - From what Max observed, it sounds
> like
> > a
> > > broadcast can be triggered by one test invocation and be delivered in a
> > > subsequent invocation.  That's bad because it means we don't have test
> > > isolation, which makes it hard to create reliable tests.
> > > 
> > > Given then above I don't see a lot of great options.  Tommy, WDYT?
> > 
> > Yeah. You could do ugly things such as adding the Java Object ID of the
> current
> > application context when you publish the notification in
> > MockAccountManager.java, and then in the receiver check if the object ID
> matches
> > the current application context. You'd need a way to inject the listeners
> > specifically for test though. Kind of like using a factory to create a
> listener,
> > and call that from prod code, and override the implementation of the factory
> for
> > test.
> 
> I tried this. The ID never once matched in all my tests, which is why I
decided
> it wasn't
> worth sticking with. Gets us nothing unless we could force waiting for them
too.
> 
> > We could always pass in a context, and never store a reference anywhere,
> except
> > in AndroidSyncSettings itself. So basically, all methods in
> AndroidSyncSettings
> > would take a Context, and then it is impossible to cache the
> AndroidSyncSettings
> > at all. No get-method.
> > You still need to init it from the test though. But at least no stale
> > references.
> 
> Not a big fan of this solution due to the extra argument everything would
need.
> 
> Neither of you have expressed your opinion on the current solution in this
> change,
> which I would certainly appreciate. I think it's the simplest we're likely to
> find and
> I don't see the downside. Another very simple solution I can think of is
simply
> not
> caching the AndroidSyncSettings object in ChromeSigninController.

I commented on the approach of patchset 7 in comment #14, paragraph C above. 
Just because this test doesn't trigger the broadcast doesn't mean other tests
won't.

I prefer Tommy's suggestion of passing in the context and prohibiting caching of
the AndroidSyncSettings ref.  The benefit of that approach is that it mitigates
the impact of the initialization race.  I'm not just interested in making the
current tests work reliably.  I'm also interested in ensuring future tests (that
haven't yet been written) won't run into the same race condition you've run
into.

Maybe I don't understand your objection to passing an extra argument.  It seems
like a relatively easy change to make (fairly mechanical) and while it's a
little tedious to use, it would mitigate the initialization race.  Seems like a
win to me.


> I think the broader issue is how unpredictable the initialization of these
> classes is,
> but I do not think addressing that is in the scope of this change. These tests
> are a
> high priority and that is not.

[maniscalco, 2015-05-12 15:54:53]

https://codereview.chromium.org/1118833002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java
(right):

https://codereview.chromium.org/1118833002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java:121:
public View onCreateView(LayoutInflater inflater, ViewGroup container,
mIsSyncInitialized is not explicitly initialized so it's going to start off as
false regardless of what mProfileSyncService.isSyncInitialized() would return if
invoked.  Should we be setting mIsSyncInitialized to the return value of
.isSyncInitialized() in onCreateView since that's effectively our initialization
method?

[maxbogue, 2015-05-12 18:00:55]

We reached a consensus offline. In this CL I will disable the broadcasts that
were causing trouble but leave them in as a flag in case future tests want them.

In a future CL I will refactor AndroidSyncSettings so that nobody can store a
reference to it and recreate this issue.

https://codereview.chromium.org/1118833002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java
(right):

https://codereview.chromium.org/1118833002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java:121:
public View onCreateView(LayoutInflater inflater, ViewGroup container,
On 2015/05/12 15:54:52, maniscalco wrote:
> mIsSyncInitialized is not explicitly initialized so it's going to start off as
> false regardless of what mProfileSyncService.isSyncInitialized() would return
if
> invoked.  Should we be setting mIsSyncInitialized to the return value of
> .isSyncInitialized() in onCreateView since that's effectively our
initialization
> method?

Might help, can't hurt. Done.

[maniscalco, 2015-05-12 20:10:58]

The plan to refactor AndroidSyncSettings in a follow up CL to prevent caching
SGTM.

Patch set 9 LGTM % one comment.

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

https://codereview.chromium.org/1118833002/diff/160001/sync/test/android/java...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:149:
public boolean addAccountHolderExplicitly(AccountHolder accountHolder,
Please document the new parameter.  Also, it looks like you'll need to update
some existing call sites to pass the new parameter.

[maxbogue, 2015-05-12 21:52:05]

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

https://codereview.chromium.org/1118833002/diff/160001/sync/test/android/java...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:149:
public boolean addAccountHolderExplicitly(AccountHolder accountHolder,
On 2015/05/12 20:10:58, maniscalco wrote:
> Please document the new parameter.  Also, it looks like you'll need to update
> some existing call sites to pass the new parameter.

Done. I added versions of the methods without the parameter that default to it
being false, both so I wouldn't have to change call sites and to discourage
calling it with true.

[nyquist, 2015-05-12 23:54:22]

lgtm

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

https://codereview.chromium.org/1118833002/diff/200001/sync/test/android/java...
sync/test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java:157:
* @param broadcaseEvent whether to broadcast an AccountChangedEvent
Nit: broadcastEvent here and below

[maxbogue, 2015-05-13 21:39:58]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2015-05-13 21:39:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
maniscalco@chromium.org, nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/1118833002/#ps220001
(title: "Fix typo.")

[commit-bot: I haz the power, 2015-05-13 21:40:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1118833002/220001

[commit-bot: I haz the power, 2015-05-13 21:45:44]

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

[commit-bot: I haz the power, 2015-05-13 21:45:46]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[maxbogue, 2015-05-13 22:10:46]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2015-05-13 22:10:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, maniscalco@chromium.org
Link to the patchset: https://codereview.chromium.org/1118833002/#ps240001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-05-13 22:12:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1118833002/240001

[commit-bot: I haz the power, 2015-05-13 22:43:56]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-05-13 22:45:43]

Patchset 13 (id:??) landed as
https://crrev.com/a465d93b75f40e550a6362c5639d78c6c14b9642
Cr-Commit-Position: refs/heads/master@{#329740}