New issue
Advanced search Search tips

Issue 886599 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 12
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 883318
issue 886600



Sign in to add a comment

Support IdentityTestEnvironment taking in backing objects that it uses to construct IdentityManager

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. We should develop a path for incrementally migrating this use case, which could then be reused if incremental test conversion is needed in the future.
 
Blocking: 886600
Summary: Support IdentityTestEnvironment taking in backing objects that it uses to construct IdentityManager (was: Determine way to incrementally migrate ProfileSyncServiceBundle to using IdentityTestEnvironment)
On reflection, the way that I would like us to enable such incremental conversion is to add one or more constructors of IdentityTestEnvironment that take in the necessary backing objects (e.g., FakeSigninManager(Base) and FakeProfileOAuth2TokenService). This would allow elegant incremental conversion of unittests:

- Create an IdentityTestEnvironment passing in all the backing objects being directly used by the test.
- Use IdentityTestEnvironment as desired to replace direct usage of the backing objects being directly used by the test.
- Once there is no more direct usage of the backing objects by the test(s) in question, eliminate the tests' knowledge of them and have it construct IdentityTestEnvironment via its no-args constructor. This should Just Work without any other changes being necessary.

Before adding the above constructor, we should fold IdentityTestEnvironmentInternal into IdentityTestEnvironment. Its purpose was to keep knowledge of the backing classes out of identity_test_environment.h, but with this change, that is no longer a goal at this time. The ivars will have to change from being objects to being std::unique_ptrs to support their either being passed in or internally constructed.

We should of course mention that the no-args constructor is the one that's preferred.
Owner: blundell@chromium.org
Status: Started (was: Available)
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 12

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

commit b64e54c9a221efad82a4a60956cd3b58ac371059
Author: Colin Blundell <blundell@chromium.org>
Date: Fri Oct 12 12:09:06 2018

Enable IdentityTestEnvironment to take in IdentityManager's dependencies

Currently, IdentityTestEnvironment internally constructs all the
dependencies of IdentityManager as well as the IdentityManager object
itself. This behavior is nice for hiding the fact that IdentityManager
has these dependencies from consumers, but doesn't allow for
usage of IdentityTestEnviroment in incremental conversion of tests that
have broad direct usage of these dependencies. Concretely,
profile_sync_test_util.h provides one such problematic case: it
constructs and directly exposes these dependencies, which are then
broadly used by a set of tests that consume this utility class.
Converting all these tests in one go is not practical.

This CL adds an alternate IdentityTestEnvironment constructor that
takes in the dependencies and constructs IdentityManager via those
passed-in dependencies. To enable this constructor, the CL also
encapsulates the fact that IdentityTestEnvironment otherwise obtains
these dependencies from an internal object within the current
IdentityTestEnvironment constructor.  It also moves ownership of
IdentityManager from that internal object to IdentityTestEnvironment
itself so that it can be created/owned independent of that internal
object.

A followup CL will use this new constructor.

Bug:  886599 
Change-Id: I4c6a503dcc11c327193f80bc7d7c48e467c6919b
Reviewed-on: https://chromium-review.googlesource.com/c/1273298
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599178}
[modify] https://crrev.com/b64e54c9a221efad82a4a60956cd3b58ac371059/services/identity/public/cpp/identity_test_environment.cc
[modify] https://crrev.com/b64e54c9a221efad82a4a60956cd3b58ac371059/services/identity/public/cpp/identity_test_environment.h

Project Member

Comment 5 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

Status: Fixed (was: Started)

Sign in to add a comment