Support IdentityTestEnvironment taking in backing objects that it uses to construct IdentityManager |
||||
Issue descriptionProfileSyncServiceBundle (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.
,
Sep 19
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.
,
Sep 20
,
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
,
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
,
Oct 12
|
||||
►
Sign in to add a comment |
||||
Comment 1 by blundell@chromium.org
, Sep 19