Ensure that real SigninManager is always used on non-ChromeOS |
||
Issue descriptionJD was converting some tests and ran into a cryptic crash, which turned out to be because the tests were using a homegrown fake of SigninManagerBase on all platforms (i.e., not just ChromeOS). This behavior violates IdentityManager (and its test infrastructure)'s assumed invariant that a SigninManagerBase object is always actually a SigninManager object when not on ChromeOS. This bug tracks fixing the violations and hardening this invariant to avoid future engineers running into this nasty problem. I put up https://chromium-review.googlesource.com/c/chromium/src/+/1213154 to find the violators, and there aren't many: components_unittests (retry summary) failures: CredentialsFilterTest.ShouldFilterOneForm CredentialsFilterTest.FilterResults_AllowAll PasswordSyncUtilEnterpriseTest.ShouldSavePasswordHash CredentialsFilterTest.ShouldSaveEnterprisePasswordHash PasswordSyncUtilTest.IsSyncAccountEmail CredentialsFilterTest.IsSyncAccountEmail CredentialsFilterTest.ReportFormLoginSuccess_ExistingSyncCredentials CredentialsFilterTest.ReportFormLoginSuccess_NewSyncCredentials CredentialsFilterTest.ShouldSave_SignIn_Form CredentialsFilterTest.ReportFormLoginSuccess_NotSyncing PasswordSyncUtilTest.IsSyncAccountCredential CredentialsFilterTest.ReportFormLoginSuccess_NotGAIACredentials CredentialsFilterTest.FilterResults_DisallowSyncOnReauth CredentialsFilterTest.ShouldSave_SyncCredential CredentialsFilterTest.ShouldSave_SyncCredential_NotSyncingPasswords CredentialsFilterTest.ShouldSaveGaiaPasswordHash CredentialsFilterTest.ShouldSave_NotSyncCredential CredentialsFilterTest.FilterResults_DisallowSync CredentialsFilterTest.ReportFormLoginSuccess_GAIANotSyncCredentials PasswordSyncUtilTest.GetSyncUsernameIfSyncingPasswords stdout ( 242 ms )unit_tests (retry summary) failures: SyncUIUtilTest.DistinctCasesReportUniqueMessageSets
,
Sep 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/05982d6ed890dc0add8ad6a42c45c5c8d9ce3825 commit 05982d6ed890dc0add8ad6a42c45c5c8d9ce3825 Author: Colin Blundell <blundell@chromium.org> Date: Wed Sep 12 11:42:04 2018 [Passwords] Avoid tests' clearing primary account from SigninManagerBase sync_credentials_filter_unittest.cc currently sets the primary account in SigninManagerBase multiple times within a single test. This cannot be done without clearing the primary account, so these tests use a homegrown fake that exposes clearing of the primary account. However, as part of crbug.com/882441 I want to get rid of this homegrown fake. This CL removes the need for these tests to clear the primary account from SigninManagerBase by regrouping the tests by account ID, so that only one signin needs to be done per test. There is no functional change of what is tested, as each test case done within a given test is independent from the other test cases. Bug: 882441 Change-Id: I1f4d5219c06a40b4c0f84b0c91d14312c5592354 Reviewed-on: https://chromium-review.googlesource.com/1218805 Reviewed-by: Vaclav Brozek <vabr@chromium.org> Commit-Queue: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#590646} [modify] https://crrev.com/05982d6ed890dc0add8ad6a42c45c5c8d9ce3825/components/password_manager/sync/browser/sync_credentials_filter_unittest.cc
,
Sep 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/98b0adc73f0be8f809844f3773c174a7076c742f commit 98b0adc73f0be8f809844f3773c174a7076c742f Author: Colin Blundell <blundell@chromium.org> Date: Wed Sep 12 14:35:24 2018 [Passwords] Remove unneeded custom fake of SigninManagerBase Some passwords unittests use a custom fake of SigninManagerBase. This fake isn't needed: it's used only to make ClearAuthenticatedAccountId() public, but none of the tests actually need that method to be called. This CL clears the way for changing these tests to use SigninManager when not on ChromeOS in the service of crbug.com/882441 . Bug: 882441 Change-Id: Iec486daa7d5c1141f543b4da6bb8b96f2f3d81b4 Reviewed-on: https://chromium-review.googlesource.com/1216482 Commit-Queue: Colin Blundell <blundell@chromium.org> Reviewed-by: Vaclav Brozek <vabr@chromium.org> Cr-Commit-Position: refs/heads/master@{#590673} [modify] https://crrev.com/98b0adc73f0be8f809844f3773c174a7076c742f/components/password_manager/sync/browser/sync_username_test_base.cc [modify] https://crrev.com/98b0adc73f0be8f809844f3773c174a7076c742f/components/password_manager/sync/browser/sync_username_test_base.h
,
Sep 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/184fb9da1d89c9b243bddc4364a8ac0e8b7bc2f7 commit 184fb9da1d89c9b243bddc4364a8ac0e8b7bc2f7 Author: Colin Blundell <blundell@chromium.org> Date: Wed Sep 12 17:52:37 2018 [passwords] Use real SigninManager in non-ChromeOS tests Some passwords unittests create a SigninManagerBase object on all platforms. We are moving to enforcing that SigninManagerBase is only instantiated via its derived class SigninManager on non-ChromeOS. This CL changes these tests to use SigninManager on non-ChromeOS. Bug: 882441 Change-Id: I6c2faf188f1c6647dbde2eb78f35f2812178daac Reviewed-on: https://chromium-review.googlesource.com/1216071 Commit-Queue: Colin Blundell <blundell@chromium.org> Reviewed-by: Vaclav Brozek <vabr@chromium.org> Cr-Commit-Position: refs/heads/master@{#590748} [modify] https://crrev.com/184fb9da1d89c9b243bddc4364a8ac0e8b7bc2f7/components/password_manager/sync/browser/sync_username_test_base.cc [modify] https://crrev.com/184fb9da1d89c9b243bddc4364a8ac0e8b7bc2f7/components/password_manager/sync/browser/sync_username_test_base.h
,
Sep 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4929182991b9dc92106ef66d4a4bfb12aea208dc commit 4929182991b9dc92106ef66d4a4bfb12aea208dc Author: Colin Blundell <blundell@chromium.org> Date: Mon Sep 17 09:26:10 2018 Enforce real SigninManager is always used on non-ChromeOS The codebase widely assumes that a SigninManagerBase object can always be downcast to a SigninManager object on non-ChromeOS. After some recent fixes of violations of this assumed invariant, this CL enforces the invariant by making SigninManagerBase's constructor protected when not on ChromeOS. Bug: 882441 Change-Id: Ie9e2131aaf6cf3b92bdf7d06b759c071df9908ef Reviewed-on: https://chromium-review.googlesource.com/1216345 Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Commit-Queue: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#591624} [modify] https://crrev.com/4929182991b9dc92106ef66d4a4bfb12aea208dc/components/signin/core/browser/signin_manager_base.h
,
Sep 17
|
||
►
Sign in to add a comment |
||
Comment 1 by blundell@chromium.org
, Sep 10