New issue
Advanced search Search tips

Issue 886600 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 15
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocked on:
issue 886599

Blocking:
issue 883318



Sign in to add a comment

Port ProfileSyncServiceBundle and its consumers to using IdentityTestEnvironment

Project Member Reported by blundell@chromium.org, Sep 19

Issue description

ProfileSyncServiceBundle (in //components/browser_sync/profile_sync_test_util.h) constructs and exposes SigninManager, ProfileOAuth2TokenService, and IdentityManager objects. It should instead just construct an IdentityManager object via IdentityTestEnvironment and expose the IdentityTestEnvironment object for its consumers to interact with.

Getting there from here is a bit tricky as there's quite a bit of direct usage; converting it all in one go (required to use IdentityTestEnvironment) would be a lot to bite off, but //services/identity/public/cpp/identity_test_utils.h doesn't currently expose all the necessary functionality. The blocking bug tracks determining a migration path.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 12

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

commit 863a437d0fdbbba19ad5446420b72d2f91748a72
Author: Colin Blundell <blundell@chromium.org>
Date: Fri Oct 12 13:10:51 2018

Start using IdentityTestEnvironment in ProfileSyncServiceBundle

ProfileSyncServiceBundle owns and exposes signin-related objects for
use by sync tests. These signin-related objects include ones that are
deprecated by IdentityManager and no longer used directly in the
production code being tested; however, up until now it has not been
feasible to convert this test utility to using IdentityTestEnvironment,
as doing so would have required converting the widespread usage of this
utility in one go.

We recently introduced an IdentityTestEnvironment usage mode wherein
it can be supplied with IdentityManager's dependencies from the
external client, precisely to enable incremental conversion in use cases
like this. This CL exploits this usage mode to introduce
IdentityTestEnvironment in ProfileSyncServiceBundle.h and provide an
example of its usage to eliminate a direct usage of the deprecated
classes. Once the conversion is complete, ProfileSyncServiceBundle can
then be changed to *only* hold an IdentityTestEnvironment object.

Bug:  886600 ,  886599 
Change-Id: I205c3567fc0a9bcd6f7b613c00a247c302d71be5
Reviewed-on: https://chromium-review.googlesource.com/c/1273065
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599189}
[modify] https://crrev.com/863a437d0fdbbba19ad5446420b72d2f91748a72/components/browser_sync/BUILD.gn
[modify] https://crrev.com/863a437d0fdbbba19ad5446420b72d2f91748a72/components/browser_sync/profile_sync_service_autofill_unittest.cc
[modify] https://crrev.com/863a437d0fdbbba19ad5446420b72d2f91748a72/components/browser_sync/profile_sync_test_util.cc
[modify] https://crrev.com/863a437d0fdbbba19ad5446420b72d2f91748a72/components/browser_sync/profile_sync_test_util.h

Work on this bug can start now, following the example of the CL from c#1.
Owner: ma...@igalia.com
Status: Started (was: Available)
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 31

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

commit 48141d2b252e5470cda294466e6d7e4d47192b48
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Wed Oct 31 09:37:10 2018

Port ProfileSyncServiceBundle's consumers to using IdentityTestEnvironment

Bug:  886600 
Change-Id: Idd339ae91bffeeceaeee929accde53fedc9f804d
Reviewed-on: https://chromium-review.googlesource.com/c/1299162
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Cr-Commit-Position: refs/heads/master@{#604189}
[modify] https://crrev.com/48141d2b252e5470cda294466e6d7e4d47192b48/components/browser_sync/profile_sync_service_startup_unittest.cc
[modify] https://crrev.com/48141d2b252e5470cda294466e6d7e4d47192b48/components/browser_sync/profile_sync_service_unittest.cc

Status: Fixed (was: Started)
I believe this was the last bit needed here (but please reopen if that's not the case) => Fixed
Status: Started (was: Fixed)
Hi Mario,

Apologies for the greatly-delayed reply here!


I think that there's more work required here: We want to completely eliminate ProfileSyncServiceBundle's knowledge of SigninManager/ProfileOAuth2TokenService. It looks like that hasn't happened on ToT, e.g. https://cs.chromium.org/chromium/src/components/browser_sync/profile_sync_test_util.h?q=ProfileSyncServiceBu&sq=package:chromium&g=0&l=132

Thanks!
Oops... right! Working on this now then, sorry for overlooking those bits.
Project Member

Comment 9 by bugdroid1@chromium.org, Nov 15

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

commit c893a340e9c7ddfae4878b6e8cedbd4a86abd45e
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Thu Nov 15 18:05:29 2018

Move ProfileSyncServiceBundle away from low-level signin related objects

Remove any knowledge of AccountTrackerService, FakeSiginManager[Base],
FakeProfileOAuth2TokenService and FakeGaiaCookieManagerService from
ProfileSyncServiceBundle, and depend solely on IdentityTestEnvironment.

Bug:  886600 
Change-Id: I268bc85d70c164efa3b9650565b9de13c98f4b4b
Reviewed-on: https://chromium-review.googlesource.com/c/1337618
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Cr-Commit-Position: refs/heads/master@{#608433}
[modify] https://crrev.com/c893a340e9c7ddfae4878b6e8cedbd4a86abd45e/components/browser_sync/profile_sync_service_unittest.cc
[modify] https://crrev.com/c893a340e9c7ddfae4878b6e8cedbd4a86abd45e/components/browser_sync/profile_sync_test_util.cc
[modify] https://crrev.com/c893a340e9c7ddfae4878b6e8cedbd4a86abd45e/components/browser_sync/profile_sync_test_util.h

Status: Fixed (was: Started)
CL landed, moving back to fixed then...

Sign in to add a comment