[Android] Check whether MockGrantCredentialsPermissionActivity can be started.

[Android] Check whether MockGrantCredentialsPermissionsActivity can be started.

BUG=406596
Committed: https://crrev.com/d2787a392a65c1172ad64e9a13e77e023d9b8d31
Cr-Commit-Position: refs/heads/master@{#292975}

[jbudorick, 2014-08-26 19:52:51]

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

[nyquist, 2014-08-27 18:19:18]

Why would we ever want this to be started?
If this is started, wouldn't that be an issue with the test not setting up the
mock account manager correctly?

If there are specific tests that test specifically how code interacts with this
external activity being started, and how that affects the current activity,
that's one thing, but I don't think that's the case for any upstream tests so
far.

[jbudorick, 2014-08-27 18:34:34]

On 2014/08/27 18:19:18, nyquist wrote:
> Why would we ever want this to be started?

MockAccountManager intents to MockGrantCredentialsPermissionActivity when it
attempts to get an auth token of a type that hasn't already been accepted. We
attempt to get that auth token in various
OAuth2TokenServiceIntegrationTest.testValidateAccounts* tests.

> If this is started, wouldn't that be an issue with the test not setting up the
> mock account manager correctly?
> 

Potentially? I'm not sure. If MockAccountManager shouldn't be sending an intent
for MockGrantCredentialsPermissionActivity, though, why does that code path
exist? Should it? This is a little out of my realm of expertise.

> If there are specific tests that test specifically how code interacts with
this
> external activity being started, and how that affects the current activity,
> that's one thing, but I don't think that's the case for any upstream tests so
> far.

[jbudorick, 2014-08-27 18:39:05]

On 2014/08/27 18:34:34, jbudorick wrote:
> On 2014/08/27 18:19:18, nyquist wrote:
> > Why would we ever want this to be started?
> 
> MockAccountManager intents to MockGrantCredentialsPermissionActivity when it
> attempts to get an auth token of a type that hasn't already been accepted. We
> attempt to get that auth token in various
> OAuth2TokenServiceIntegrationTest.testValidateAccounts* tests.
> 
> > If this is started, wouldn't that be an issue with the test not setting up
the
> > mock account manager correctly?
> > 
> 
> Potentially? I'm not sure. If MockAccountManager shouldn't be sending an
intent
> for MockGrantCredentialsPermissionActivity, though, why does that code path
> exist? Should it? This is a little out of my realm of expertise.
> 

"That code path" being the else block in MockAccountManager.getAuthTokenFuture.

> > If there are specific tests that test specifically how code interacts with
> this
> > external activity being started, and how that affects the current activity,
> > that's one thing, but I don't think that's the case for any upstream tests
so
> > far.

[jbudorick, 2014-08-27 21:20:17]

On 2014/08/27 18:39:05, jbudorick wrote:
> On 2014/08/27 18:34:34, jbudorick wrote:
> > On 2014/08/27 18:19:18, nyquist wrote:
> > > Why would we ever want this to be started?
> > 
> > MockAccountManager intents to MockGrantCredentialsPermissionActivity when it
> > attempts to get an auth token of a type that hasn't already been accepted.
We
> > attempt to get that auth token in various
> > OAuth2TokenServiceIntegrationTest.testValidateAccounts* tests.
> > 
> > > If this is started, wouldn't that be an issue with the test not setting up
> the
> > > mock account manager correctly?
> > > 
> > 
> > Potentially? I'm not sure. If MockAccountManager shouldn't be sending an
> intent
> > for MockGrantCredentialsPermissionActivity, though, why does that code path
> > exist? Should it? This is a little out of my realm of expertise.
> > 
> 
> "That code path" being the else block in
MockAccountManager.getAuthTokenFuture.
> 

Maybe that code path should be considered invalid if it's hit on the
application's main thread, i.e., @UiThreadTests should always have accounts set
up with alwaysAccept?

> > > If there are specific tests that test specifically how code interacts with
> > this
> > > external activity being started, and how that affects the current
activity,
> > > that's one thing, but I don't think that's the case for any upstream tests
> so
> > > far.

[nyquist, 2014-08-28 16:26:39]

Upstream there are no use-cases from what I know that should not set up the
accounts prior to asking for tokens.

Maybe we can change this CL to instead ensure we fail hard and early if the mock
activity is not available instead? If we do that on the right thread, the test
will crash instead of be flaky. WDYT?

The mock activity is (was?) used downstream to test flows where we would be
getting a popup in a signed build of Chrome  during first run. Currently we're
using OAuth2 tokens for everything though, so this is less of an issue.

[jbudorick, 2014-08-28 16:32:55]

On 2014/08/28 16:26:39, nyquist wrote:
> Upstream there are no use-cases from what I know that should not set up the
> accounts prior to asking for tokens.
> 
> Maybe we can change this CL to instead ensure we fail hard and early if the
mock
> activity is not available instead? If we do that on the right thread, the test
> will crash instead of be flaky. WDYT?
> 

That's fine with me. My motivation with this CL is to make sure that this code
path doesn't trip us up again with a flake, so in that regard, failing
consistently is just as good as passing consistently.

> The mock activity is (was?) used downstream to test flows where we would be
> getting a popup in a signed build of Chrome  during first run. Currently we're
> using OAuth2 tokens for everything though, so this is less of an issue.

[jbudorick, 2014-08-28 23:58:19]

Now verifies that MockAccountManager is in the same application as
MockGrantCredentialsPermissionActivity at intent creation time.

I tried waiting until the AsyncTask tried to start the activity, but this was
too late to ensure that the process was killed before it completed.

[nyquist, 2014-08-29 22:59:13]

thanks! lgtm

[jbudorick, 2014-09-02 13:57:04]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-09-02 13:57:39]

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

[commit-bot: I haz the power, 2014-09-02 19:32:52]

Committed patchset #3 (id:40001) as d3c61bf3c6b198e9a24c408fec8c5ea8b372be41

[commit-bot: I haz the power, 2014-09-10 03:19:58]

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