New issue
Advanced search Search tips

Issue 798699 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 796544



Sign in to add a comment

Create infrastructure to interact with IdentityManager in test contexts

Project Member Reported by blundell@chromium.org, Jan 3 2018

Issue description

Unittests and browsertests that use fakes of the signin classes (e.g., FakeSigninManager) will need to be converted to interact with and drive the IdentityManager. To do this, we'll create an IdentityTestEnvironment class that creates an IdentityManager and provides interfaces to drive that IdentityManager as needed by tests (e.g., signing in or granting access tokens).
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/95c480200bb51e33c85b7b9c37d9e4fdeb121f55

commit 95c480200bb51e33c85b7b9c37d9e4fdeb121f55
Author: Colin Blundell <blundell@chromium.org>
Date: Fri Jan 12 09:11:21 2018

Add IdentityManager diagnostics observer

We will shortly be converting tests that make use of
OAuth2TokenService::DiagnosticsObserver to interact with the
IdentityManager. As part of supporting this conversion, this CL
introduces IdentityManager::DiagnosticsObserver.

Bug: 798699
Change-Id: Ief1f862afc89c29f52cc014744ef14751222f8d0
Reviewed-on: https://chromium-review.googlesource.com/848992
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528909}
[modify] https://crrev.com/95c480200bb51e33c85b7b9c37d9e4fdeb121f55/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/95c480200bb51e33c85b7b9c37d9e4fdeb121f55/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/95c480200bb51e33c85b7b9c37d9e4fdeb121f55/services/identity/public/cpp/identity_manager_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Jan 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/817390734cc9a98941c6b61ab22a011d091ebd96

commit 817390734cc9a98941c6b61ab22a011d091ebd96
Author: Colin Blundell <blundell@chromium.org>
Date: Wed Jan 17 14:27:26 2018

Introduce IdentityTestEnvironment and example usage

This CL introduces infrastructure to support converting unittests
that use fakes of the signin internal classes to instead interact
with IdentityManager. IdentityTestEnvironment brings up an
IdentityManager instance and allows its client to interact with this
instance, both directly and by driving the state of signin (e.g.,
signing the user in).

This IdentityManager instance is currently backed by fakes of its
dependencies on signin internal classes. When IdentityManager is
backed by the Identity Service, IdentityTestEnvironment will internally
create a mock Identity Service implementation and have its
IdentityManager instance connect to that mock implementation.

IdentityTestEnvironment starts off with the user signed out, similar to
the fakes of the internal signin classes.

Bug: 798699
Change-Id: Iec01fe3b1f721a8aa5ea372a433b4160dd45075f
Reviewed-on: https://chromium-review.googlesource.com/857396
Reviewed-by: Mathieu Perreault <mathp@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529733}
[modify] https://crrev.com/817390734cc9a98941c6b61ab22a011d091ebd96/components/suggestions/BUILD.gn
[modify] https://crrev.com/817390734cc9a98941c6b61ab22a011d091ebd96/components/suggestions/suggestions_service_impl_unittest.cc
[modify] https://crrev.com/817390734cc9a98941c6b61ab22a011d091ebd96/services/identity/public/cpp/BUILD.gn
[modify] https://crrev.com/817390734cc9a98941c6b61ab22a011d091ebd96/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/817390734cc9a98941c6b61ab22a011d091ebd96/services/identity/public/cpp/identity_manager.h
[add] https://crrev.com/817390734cc9a98941c6b61ab22a011d091ebd96/services/identity/public/cpp/identity_test_environment.cc
[add] https://crrev.com/817390734cc9a98941c6b61ab22a011d091ebd96/services/identity/public/cpp/identity_test_environment.h

The above provides a solution for unittests. Leaving this bug open as it's not yet clear what our approach will be for browsertests.
Project Member

Comment 4 by bugdroid1@chromium.org, Feb 5 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8a04c82f8bc640a82413e984118f0b1f6c7e0143

commit 8a04c82f8bc640a82413e984118f0b1f6c7e0143
Author: Colin Blundell <blundell@chromium.org>
Date: Mon Feb 05 13:13:41 2018

IdentityManager: Fire observer callbacks on test signin/out for non-CrOS

Currently, the Identity Service client lib infrastructure for making the
primary account available in unittests does not result in firing the
IdentityManager or SigninManager signin/out observer callbacks.
An upcoming unittest being converted will need this functionality (i.e.,
it tests a production flow that assumes the firing of these callbacks).

As far as we know, there is no use case for test signin *not* firing
these callbacks in a non-ChromeOS context (note that on ChromeOS these
callbacks are never fired in production). This CL changes the Identity
Service client lib to fire these callbacks in its test infrastructure
for making the primary account available and clearing the primary
account.

Bug: 798699
Change-Id: Ic70dc7ebcce73f914a65416c39b2bd18d26f3ba9
Reviewed-on: https://chromium-review.googlesource.com/888754
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534374}
[modify] https://crrev.com/8a04c82f8bc640a82413e984118f0b1f6c7e0143/components/ntp_snippets/contextual/contextual_suggestions_fetcher_impl_unittest.cc
[modify] https://crrev.com/8a04c82f8bc640a82413e984118f0b1f6c7e0143/components/ntp_snippets/remote/remote_suggestions_fetcher_impl_unittest.cc
[modify] https://crrev.com/8a04c82f8bc640a82413e984118f0b1f6c7e0143/components/suggestions/suggestions_service_impl_unittest.cc
[modify] https://crrev.com/8a04c82f8bc640a82413e984118f0b1f6c7e0143/services/identity/public/cpp/BUILD.gn
[modify] https://crrev.com/8a04c82f8bc640a82413e984118f0b1f6c7e0143/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/8a04c82f8bc640a82413e984118f0b1f6c7e0143/services/identity/public/cpp/identity_test_environment.cc
[modify] https://crrev.com/8a04c82f8bc640a82413e984118f0b1f6c7e0143/services/identity/public/cpp/identity_test_environment.h
[add] https://crrev.com/8a04c82f8bc640a82413e984118f0b1f6c7e0143/services/identity/public/cpp/identity_test_utils.cc
[add] https://crrev.com/8a04c82f8bc640a82413e984118f0b1f6c7e0143/services/identity/public/cpp/identity_test_utils.h

Project Member

Comment 5 by bugdroid1@chromium.org, Jun 4 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0605d7411404879ba447ba889f31201d51d23579

commit 0605d7411404879ba447ba889f31201d51d23579
Author: Colin Blundell <blundell@chromium.org>
Date: Mon Jun 04 11:13:17 2018

IdentityManager: Add test APIs for setting primary account/refresh token

To date, IdentityManager's test APIs
(IdentityTestEnvironment/identity_test_utils.h) have intentionally only
supported making the primary account available, i.e., signed in with a
refresh token. However, PrimaryAccountAccessTokenFetcher unittests have
a legitimate use case for wanting these steps separate: there are tests
that test PrimaryAccountAccessTokenFetcher's correct operation in the
case where a fetcher is started with the primary account set but with
no token available for the primary account (with the token potentially
being made available later).

To prepare for porting PrimaryAccountAccessTokenFetcher, this CL adds
IdentityManager test APIs for setting just the primary account and
adding a refresh token for a given account. This CL also takes the
opportunity to make the test APIs that set the primary account return
the account ID, as this is useful to consumers that want to sign in
the primary account and then do followup operations on that account;
these consumers will generally need to know the primary account ID.

Bug:  848642 , 798699
Change-Id: I249e6851779545346b5f521afe18150bec20e69f
Reviewed-on: https://chromium-review.googlesource.com/1082197
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564055}
[modify] https://crrev.com/0605d7411404879ba447ba889f31201d51d23579/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/0605d7411404879ba447ba889f31201d51d23579/services/identity/public/cpp/identity_test_environment.cc
[modify] https://crrev.com/0605d7411404879ba447ba889f31201d51d23579/services/identity/public/cpp/identity_test_environment.h
[modify] https://crrev.com/0605d7411404879ba447ba889f31201d51d23579/services/identity/public/cpp/identity_test_utils.cc
[modify] https://crrev.com/0605d7411404879ba447ba889f31201d51d23579/services/identity/public/cpp/identity_test_utils.h

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8591177cea52489a23d013b08467c43e602b0053

commit 8591177cea52489a23d013b08467c43e602b0053
Author: Colin Blundell <blundell@chromium.org>
Date: Tue Jun 12 13:00:58 2018

IdentityTestEnvironment: Sharpen API for setting refresh token

IdentityTestEnvironment (and identity_test_utils.h) have an API for
setting a refresh token for a given account. However, this API has a
hidden gotcha: IdentityManager assumes that when a refresh token is
updated, AccountTrackerService knows about the account. This assumption
is valid in production, but IdentityTestEnvironment doesn't make any
attempt to ensure it in this testing context. The only reason that this
gotcha hasn't yet been tripped is because all uses of this API are
currently for the primary account, which AccountTrackerService *does*
already know about.

This CL sharpens this API to be just about setting a refresh token for
the primary account, with the constraint that the primary account must
itself already be set. A followup CL will add an API for adding an
account, as this will shortly be needed for porting
PrimaryAccountAccessTokenFetcher unittests.

Bug: 798699
Change-Id: Id734e582eb9841b3150b380486556b046212269c
Reviewed-on: https://chromium-review.googlesource.com/1095108
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566392}
[modify] https://crrev.com/8591177cea52489a23d013b08467c43e602b0053/components/browser_sync/profile_sync_service_startup_unittest.cc
[modify] https://crrev.com/8591177cea52489a23d013b08467c43e602b0053/components/browser_sync/sync_auth_manager_unittest.cc
[modify] https://crrev.com/8591177cea52489a23d013b08467c43e602b0053/services/identity/public/cpp/identity_test_environment.cc
[modify] https://crrev.com/8591177cea52489a23d013b08467c43e602b0053/services/identity/public/cpp/identity_test_environment.h
[modify] https://crrev.com/8591177cea52489a23d013b08467c43e602b0053/services/identity/public/cpp/identity_test_utils.cc
[modify] https://crrev.com/8591177cea52489a23d013b08467c43e602b0053/services/identity/public/cpp/identity_test_utils.h

Project Member

Comment 7 by bugdroid1@chromium.org, Jun 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f363b1dd4851d89bb926c764661c3c26d897b030

commit f363b1dd4851d89bb926c764661c3c26d897b030
Author: Colin Blundell <blundell@chromium.org>
Date: Wed Jun 13 09:00:50 2018

IdentityTestEnvironment: Sharpen APIs for interacting with refresh token

This CL follows the motivations of
https://chromium-review.googlesource.com/c/chromium/src/+/1095108 to
make the same change for the SetInvalidRefreshTokenForAccount() and
RemoveRefreshTokenForAccount() APIs as that CL made for the
SetRefreshTokenForAccount() API.

Bug: 798699
Change-Id: If3701f0d42fd0129ed324dc8b9f7ac7402c1cb3d
Reviewed-on: https://chromium-review.googlesource.com/1097087
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566765}
[modify] https://crrev.com/f363b1dd4851d89bb926c764661c3c26d897b030/components/browser_sync/sync_auth_manager_unittest.cc
[modify] https://crrev.com/f363b1dd4851d89bb926c764661c3c26d897b030/services/identity/public/cpp/identity_test_environment.cc
[modify] https://crrev.com/f363b1dd4851d89bb926c764661c3c26d897b030/services/identity/public/cpp/identity_test_environment.h
[modify] https://crrev.com/f363b1dd4851d89bb926c764661c3c26d897b030/services/identity/public/cpp/identity_test_utils.cc
[modify] https://crrev.com/f363b1dd4851d89bb926c764661c3c26d897b030/services/identity/public/cpp/identity_test_utils.h

Project Member

Comment 8 by bugdroid1@chromium.org, Jun 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0b846a220943bba32d505affa7c1a3fa18cab73a

commit 0b846a220943bba32d505affa7c1a3fa18cab73a
Author: Colin Blundell <blundell@chromium.org>
Date: Thu Jun 14 08:25:19 2018

Make identity_test_utils methods that deal with refresh tokens block

The identity_test_utils.h methods that deal with refresh tokens promise
that they block until the relevant operation is complete (as do their
wrapper IdentityTestEnvironment methods). However, they don't actually
do that. And that's really the most important part.

This CL makes reality match the contracts. As part of this change,
OneShotIdentityManagerObserver needs to be extended to have the
information of which event it's waiting on, as a single signin can
generate both a token updated and a primary account set event, and
the one that's received first might not be the one that the client of
the observer is actually intending to wait on.

Note: The current production impl doesn't actually require this
blocking as it is synchronous. However, it is a goal to change those
interactions back to being asynchronous, which this blocking in the
test code future-proofs for.

Bug: 798699
Change-Id: Ib8b098b65411ef22a8a3ceed4a4f58dfcad1e931
Reviewed-on: https://chromium-review.googlesource.com/1098966
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567190}
[modify] https://crrev.com/0b846a220943bba32d505affa7c1a3fa18cab73a/services/identity/public/cpp/identity_test_utils.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Jun 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/56bc73a946d9463ec3862c67a51b94bc54a96e1b

commit 56bc73a946d9463ec3862c67a51b94bc54a96e1b
Author: Colin Blundell <blundell@chromium.org>
Date: Thu Jun 14 11:15:01 2018

IdentityTestEnvironment: Add MakeAccountAvailable() API

This API allows test clients to populate an account from an email
address with IdentityManager and its backing classes. It is conceptually
similar to IdentityTestEnvironment::MakePrimaryAccountAvailable().

Bug: 798699
Change-Id: Iffaae9f989a37de3e406ccc10166ce44c7ed6bdb
Reviewed-on: https://chromium-review.googlesource.com/1099057
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567221}
[modify] https://crrev.com/56bc73a946d9463ec3862c67a51b94bc54a96e1b/services/identity/public/cpp/identity_test_environment.cc
[modify] https://crrev.com/56bc73a946d9463ec3862c67a51b94bc54a96e1b/services/identity/public/cpp/identity_test_environment.h
[modify] https://crrev.com/56bc73a946d9463ec3862c67a51b94bc54a96e1b/services/identity/public/cpp/identity_test_utils.cc
[modify] https://crrev.com/56bc73a946d9463ec3862c67a51b94bc54a96e1b/services/identity/public/cpp/identity_test_utils.h

Project Member

Comment 10 by bugdroid1@chromium.org, Jun 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/07306a99518690d280b86b201b7f633677d75a90

commit 07306a99518690d280b86b201b7f633677d75a90
Author: Colin Blundell <blundell@chromium.org>
Date: Thu Jun 21 09:54:32 2018

IdentityTestEnvironment: More APIs to interact with secondary accounts

This CL adds IdentityTestEnvironment APIs to interact with the refresh
token state of secondary accounts; these APIs will be needed in an
upcoming CL. In this CL we also change all such test APIs that return
the account ID of a newly-set account to instead return the AccountInfo;
for the upcoming use case, this will be convenient.

Bug: 798699
Change-Id: I9deceae9fb016147fce6206c263acf638d21ef6e
Reviewed-on: https://chromium-review.googlesource.com/1107984
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569201}
[modify] https://crrev.com/07306a99518690d280b86b201b7f633677d75a90/components/browser_sync/sync_auth_manager_unittest.cc
[modify] https://crrev.com/07306a99518690d280b86b201b7f633677d75a90/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/07306a99518690d280b86b201b7f633677d75a90/services/identity/public/cpp/identity_test_environment.cc
[modify] https://crrev.com/07306a99518690d280b86b201b7f633677d75a90/services/identity/public/cpp/identity_test_environment.h
[modify] https://crrev.com/07306a99518690d280b86b201b7f633677d75a90/services/identity/public/cpp/identity_test_utils.cc
[modify] https://crrev.com/07306a99518690d280b86b201b7f633677d75a90/services/identity/public/cpp/identity_test_utils.h

Project Member

Comment 11 by bugdroid1@chromium.org, Jul 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b5e688dcc4bffc28d177f53d4c2a17cafac3bc28

commit b5e688dcc4bffc28d177f53d4c2a17cafac3bc28
Author: Colin Blundell <blundell@chromium.org>
Date: Fri Jul 27 12:09:07 2018

IdentityManager test APIs: Add flexibility when clearing primary account

When clearing the primary account, three behaviors are possible:
1. Explicitly remove all accounts.
2. Explicitly keep all accounts.
3. Let the signin code internally decide between 1 and 2 based on its
own default policy.

The IdentityManager test APIS (IdentityTestEnvironment and
identity_test_utils.h) currently support only 3. However, an upcoming
conversion will require 1 and 2. This CL adds that support in to the
APIS for clearing the primary account.

Bug: 798699,  809923 
Change-Id: Idb717836c843a14a5f6d33aab5d60d7e14ff4d1c
Reviewed-on: https://chromium-review.googlesource.com/1149861
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578605}
[modify] https://crrev.com/b5e688dcc4bffc28d177f53d4c2a17cafac3bc28/services/identity/public/cpp/identity_test_environment.cc
[modify] https://crrev.com/b5e688dcc4bffc28d177f53d4c2a17cafac3bc28/services/identity/public/cpp/identity_test_environment.h
[modify] https://crrev.com/b5e688dcc4bffc28d177f53d4c2a17cafac3bc28/services/identity/public/cpp/identity_test_utils.cc
[modify] https://crrev.com/b5e688dcc4bffc28d177f53d4c2a17cafac3bc28/services/identity/public/cpp/identity_test_utils.h

Project Member

Comment 12 by bugdroid1@chromium.org, Jul 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/66ef2a877f14c2fa7b15b070c529e570eca617e6

commit 66ef2a877f14c2fa7b15b070c529e570eca617e6
Author: Colin Blundell <blundell@chromium.org>
Date: Fri Jul 27 13:00:48 2018

IdentityManager: Relax some constraints on test APIs

Various test APIs around IdentityManager currently DCHECK preconditions
when invoked (e.g., clearing the primary account DCHECKs that there is
a primary account). However, unittests for GCM that will shortly be
converted intentionally stress-test the system in various exceptional
ways, e.g., invoking the flow that the primary account was cleared when
there is no primary account. To accomodate porting those unittests
without having to reduce their coverage, this CL relaxes the constraints
on the relevant IdentityManager test APIs.

Bug: 798699,  809923 
Change-Id: Ibe2db519d0e231ea707ad09b329d6abc6ad7a9d2
Reviewed-on: https://chromium-review.googlesource.com/1149864
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578615}
[modify] https://crrev.com/66ef2a877f14c2fa7b15b070c529e570eca617e6/services/identity/public/cpp/identity_test_environment.h
[modify] https://crrev.com/66ef2a877f14c2fa7b15b070c529e570eca617e6/services/identity/public/cpp/identity_test_utils.cc
[modify] https://crrev.com/66ef2a877f14c2fa7b15b070c529e570eca617e6/services/identity/public/cpp/identity_test_utils.h

Project Member

Comment 13 by bugdroid1@chromium.org, Jul 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4bf47a559863ac429f4c61d31abe827670390d15

commit 4bf47a559863ac429f4c61d31abe827670390d15
Author: Colin Blundell <blundell@chromium.org>
Date: Fri Jul 27 15:25:11 2018

IdentityTestEnv: Add API to wait for token requests for given account

The current IdentityTestEnvironment APIs to wait for access token
requests and issue responses operate in a shotgun manner: when the next
access token request is received, the test API implementation issues a
response for *all* pending access token requests. However, unittests
that is targeted in an upcoming conversion requires more flexibility:
as the production code that it is testing interacts with multiple
accounts, the test issues access token requests for multiple accounts,
and then sequentially issues responses and checks that a specific flow
for each account in question was initiated in response to the specific
response. Moreover, one of these unittests issues responses in an
order different to the order in which the requests were made.

To accommodate porting of these tests, this CL adds the ability to wait
for an access token request for a given account and then issue a
response only for that account. If while waiting for a request from a
specific account a request for a *different* account is seen, the
waiting code forwards on the handling of the request for that other
account to the next iteration of the runloop; this allows for the case
where a test wants to handle requests for multiple accounts in an
order different to the order in which those requests were made.

We leave in a variant that does not take in the account ID and operates
as before, as this is convenient for the common case of tests that don't
care about this level of detail (usually because they are testing a
production flow that operates on the primary account).

Bug: 798699,  809923 
Change-Id: I6b1d93efd47b1eba128ac0b62c833736700e5196
Reviewed-on: https://chromium-review.googlesource.com/1149876
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578647}
[modify] https://crrev.com/4bf47a559863ac429f4c61d31abe827670390d15/services/identity/public/cpp/identity_test_environment.cc
[modify] https://crrev.com/4bf47a559863ac429f4c61d31abe827670390d15/services/identity/public/cpp/identity_test_environment.h

Project Member

Comment 14 by bugdroid1@chromium.org, Nov 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1c1528b77f4e357a8d048e9bad04779baa0e18a1

commit 1c1528b77f4e357a8d048e9bad04779baa0e18a1
Author: Colin Blundell <blundell@chromium.org>
Date: Thu Nov 08 11:51:08 2018

Document IdentityTestEnvironment's requirement of a task environment

After https://chromium-review.googlesource.com/c/chromium/src/+/1302175,
IdentityTestEnvironment now requires a properly set-up task environment.
The error that ensues at runtime if this is lacking is currently quite
cryptic. This CL adds documentation of the requirement and a transparent
runtime error for the case where the task environment is missing.

Bug: 798699
Change-Id: I7187b9d30f3072c2a0e697e7e325b01d238665bc
Reviewed-on: https://chromium-review.googlesource.com/c/1323073
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606417}
[modify] https://crrev.com/1c1528b77f4e357a8d048e9bad04779baa0e18a1/services/identity/public/cpp/identity_test_environment.cc
[modify] https://crrev.com/1c1528b77f4e357a8d048e9bad04779baa0e18a1/services/identity/public/cpp/identity_test_environment.h

Project Member

Comment 15 by bugdroid1@chromium.org, Nov 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a8c2d37b39b145d0db98727abdd50280c3c10813

commit a8c2d37b39b145d0db98727abdd50280c3c10813
Author: Colin Blundell <blundell@chromium.org>
Date: Mon Nov 12 13:45:40 2018

Add consistency to treatment of |id_token| in IdentityTestEnv APIs

I just noticed that the variant that issues an access token for all
accounts has an overload for passing |id_token|, while the variant that
specifies an account has it as an optional argument. Let's use the
optional argument approach for both, as we're shortly going to need
to add another optional argument for scopes.

Bug: 798699
Change-Id: I610cc5bf1ba596b92ccd7d18ad6aa3bcc32ac246
Reviewed-on: https://chromium-review.googlesource.com/c/1329794
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607212}
[modify] https://crrev.com/a8c2d37b39b145d0db98727abdd50280c3c10813/services/identity/public/cpp/identity_test_environment.cc
[modify] https://crrev.com/a8c2d37b39b145d0db98727abdd50280c3c10813/services/identity/public/cpp/identity_test_environment.h

Project Member

Comment 16 by bugdroid1@chromium.org, Nov 26

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b9b47487cb599d742c23128e26f1241be50586ae

commit b9b47487cb599d742c23128e26f1241be50586ae
Author: Colin Blundell <blundell@chromium.org>
Date: Mon Nov 26 13:06:01 2018

[IdentityManager] Add DCHECK to make error case less cryptic in tests

When tests set the refresh token for a given account, IdentityManager
must already know about the account (i.e., the account must be
seeded in AccountTrackerService). However, the error that occurs if
tests invoke SetRefreshTokenForAccount() rather than
MakeAccountAvailable() is cryptic and hard to debug. This CL adds a
DCHECK to make the error more transparent and give instructions on the
proper invocation.

Change-Id: I26e38bbccfd16e3e9b0e5c45931edb26ea0e4f0d
Bug: 798699
Reviewed-on: https://chromium-review.googlesource.com/c/1350610
Reviewed-by: Antonio Gomes <tonikitoo@igalia.com>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610832}
[modify] https://crrev.com/b9b47487cb599d742c23128e26f1241be50586ae/services/identity/public/cpp/identity_test_utils.cc

Sign in to add a comment