New issue
Advanced search Search tips

Issue 890799 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 5
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Task

Blocking:
issue 883330



Sign in to add a comment

Convert chrome/browser/ui/ash/multi_user/multi_user_util_chromeos_unittest.cc to IdentityManager

Project Member Reported by sdefresne@chromium.org, Oct 1

Issue description

API used:
- SigninManagerBase::GetAuthenticatedAccountInfo()

 
Owner: ma...@igalia.com
Status: Started (was: Available)
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 5

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

commit cd05804c617938acd5c47da11f5a869eaf28d53a
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Mon Nov 05 17:01:02 2018

Migrate MultiUserUtilTest for ChromeOS to the IdentityManager

Remove dependencies on AccountTrackerService and SigninManagerBase
and rely instead on IdentityManager, IdentityTestEnvironment and
IdentityTestEnvironmentProfileAdaptor for the unit tests.

Note that this CL also changes the implementation of the tests
slightly, but this shouldn't change the logic that the test
is trying to check (that a valid account ID is always returned
for a valid profile even after revoking the refresh token).

The reasoning behind this modification (which has been bigger than
in other migrations) was based on several factors:

  - Since we're relying on IdentityTestEnvironment now, there's
    no need to keep using the fake SigninManagerBase nor the
    AccountTrackerService directly anymore.

  - With the AccountTrackerService (ATS) out of the way, we no
    longer simulate the token being revoked by directly removing
    the account from the ATS, meaning that further calls to the
    MultiUserTestingProfile::GetProfileUserName() method would
    never return an empty string. This is because that method would
    be now implemented in terms of IdentityManager, which would
    still be returning a valid email after the revoking since
    calling IdentityTestEnvironment::RemoveRefreshTokenForAccount()
    won't cause the account to be removed, just invalidated.

  - As a result, the way the ReturnValidAccountIdIfTokenRevoked test
    used to check whether the token is revoked, based on checking
    whether GetUserProfileName() returned an empty string or not,
    is no longer valid. Fortunately, we can simply use another API
    that is more accurate: IdentityManager::HasAccountWithRefreshToken().

  - Finally, since we're not using GetUserProfileName() at all anymore,
    there's also no need at all to subclass TestingProfile since overriding
    that method in terms of SigninManagerBase was its sole purpose for
    existing in the first place. Thus, we can drop that class as well.

In summary, quite some changes to the tests but the essence of it should
remain the same, relying in IdentityManager::HasAccountWithRefreshToken()
now instead of in the override for TestingProfile::GetUserProfileName(),
which is probably more precise and accurate.

Bug:  890799 
Change-Id: I8210678c4486c226abbe920c1945ae473d8e28cf
Reviewed-on: https://chromium-review.googlesource.com/c/1307442
Reviewed-by: Stefan Kuhne <skuhne@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Cr-Commit-Position: refs/heads/master@{#605364}
[modify] https://crrev.com/cd05804c617938acd5c47da11f5a869eaf28d53a/chrome/browser/ui/ash/multi_user/multi_user_util_chromeos_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment