Cleanup of kStubDeviceSettings switch |
|
Issue descriptionCould do some or all of the following: 1. Rename FakeOwnerSettingsService to StubOwnerSettingsService, consistent with StubSettingsProvider and kStubDeviceSettings 2. Remove kStubDeviceSettings switch, instead have a function which does the same in tests eg StubDeviceSettingsForTest() 3. Remove Fakes / Stubs from prod build and only use them in tests. 4. Clean up ScopedCrosSettingsTestHelper (which has the closest thing we currently have to StubDeviceSettingsForTest): - it has two main responsibilities, sometimes only one is needed, each must be called at most once, and one happens automatically during construction. This makes it a bit messy to use - it has the ability to stub cros settings partway through a test, which very few tests make use of, and it has extra code for copying settings between the real and the stubbed stores. This is probably unnecessary complexity (Some background in the comments on CL 1329973)
,
Dec 31
I have started a change: https://chromium-review.googlesource.com/c/chromium/src/+/1392186 that begins step one of removing the kStubDeviceSettings switch. This involves several smaller changes: 1. OwnerSettingsServiceChromeOsFactory currently has functionality useful for testing, such that it creates OwnerSettingsService instances that are backed by the StubCrosSettingsProvider. However, this functionality is only available by setting a switch. So, I have changed OwnerSettingsServiceChromeOsFactory so that this behavior can be set programmatically, so that we can move away from switches. 2. ScopedCrosSettingsTestHelper currently has functionality so that an initialized CrosSettings instance can have the device settings provider swapped out, and replaced with a fake one. However, doing this in a test which runs the main Chrome initialization code is tricky to get right. Doing it before the initialization code is run does not work, since there is no initialized CrosSettings yet to modify. Doing it afterwards can be too late, since initialization code that reads a particular value may already have been run. That is why there are currently some switches control the initial values of certain Cros Settings, so that they can have the right values during test initialization - for instance, the switch kLoginUser can be used to set the initial value of the kDeviceOwner setting, which is necessary for certain tests. So, I have changed CrosSettings so that a CrosSettings instance can be set for testing before any other initialization is done, and without worrying about it being reinitialized, with a new SetForTesting function that prevents reinitialization by production code. 3. ScopedCrosSettingsTestHelper is overly complex - it allows for any or all of the following: - Initializing CrosSettings - this it does by default, but it can be disabled. - Swapping in a StubCrosSettingsProvider - this it does not do by default, but it can be done with a method call. - Copying settings from the fake to the real device settings provider or vice versa. This is hardly ever used. Most (all?) tests which currently use it, could be made simpler and/or switch-free if they just used CrosSettings::SetForTesting. So, I have made a different class, ScopedTestingCrosSettings, that is simpler and calls CrosSettings::SetForTesting. It does not allow the device settings provider to be swapped mid test, and so it had no need for copying settings between different device settings providers. 4. users_private_apitest.cc is migrated away from using ScopedCrosSettingsTestHelper + switches + swapping providers mid-test, and migrated to ScopedTestingCrosSettings. This is a proof of concept, and I hope to do other tests soon. ---- Future CLs along the same lines: I hope to migrate more tests away from ScopedCrosSettingsTestHelper + switches + swapping providers mid test, and migrate them to ScopedTestingCrosSettings, which calls CrosSettings::SetForTesting once at the start of the test, and doesn't allow providers to be swapped. If all tests are able to be migrated, I will delete ScopedCrosSettingsTestHelper. If a few tests cannot be migrated, I will look at my options for simplifying those tests. I hope to migrate all tests away from suing the kStubDeviceSettings, and then I will delete the switch, completing step 1 of this cleanup.
,
Jan 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bac20b94585bb8cf23a5f6d93e110b30ce10057d commit bac20b94585bb8cf23a5f6d93e110b30ce10057d Author: A Olsen <olsen@chromium.org> Date: Thu Jan 03 11:18:08 2019 Migrate away from using switches for testing - Changes OwnerSettingsServiceChromeOsFactory so that its stubbed behavior can be used without setting a switch - Changes CrosSettings so that it can be stubbed during a test without worrying about it being reinitialized later - Add a new class ScopedTestingCrosSettings that makes use of this new functionality, is much simpler than ScopedCrosSettingsTestHelper. - Changes one test - users_private_apitest.cc - that previously used switches, so that it no longer uses switches, as a proof-of-concept. For more details, see the bug. Bug: 909635 Change-Id: I57b053038592512fb7cfb7c09061d5d03475e979 Reviewed-on: https://chromium-review.googlesource.com/c/1392186 Commit-Queue: A Olsen <olsen@chromium.org> Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Cr-Commit-Position: refs/heads/master@{#619620} [modify] https://crrev.com/bac20b94585bb8cf23a5f6d93e110b30ce10057d/chrome/browser/chromeos/BUILD.gn [modify] https://crrev.com/bac20b94585bb8cf23a5f6d93e110b30ce10057d/chrome/browser/chromeos/extensions/users_private/users_private_apitest.cc [modify] https://crrev.com/bac20b94585bb8cf23a5f6d93e110b30ce10057d/chrome/browser/chromeos/ownership/owner_settings_service_chromeos_factory.cc [modify] https://crrev.com/bac20b94585bb8cf23a5f6d93e110b30ce10057d/chrome/browser/chromeos/ownership/owner_settings_service_chromeos_factory.h [modify] https://crrev.com/bac20b94585bb8cf23a5f6d93e110b30ce10057d/chrome/browser/chromeos/settings/cros_settings.cc [modify] https://crrev.com/bac20b94585bb8cf23a5f6d93e110b30ce10057d/chrome/browser/chromeos/settings/cros_settings.h [modify] https://crrev.com/bac20b94585bb8cf23a5f6d93e110b30ce10057d/chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h [add] https://crrev.com/bac20b94585bb8cf23a5f6d93e110b30ce10057d/chrome/browser/chromeos/settings/scoped_testing_cros_settings.cc [add] https://crrev.com/bac20b94585bb8cf23a5f6d93e110b30ce10057d/chrome/browser/chromeos/settings/scoped_testing_cros_settings.h [modify] https://crrev.com/bac20b94585bb8cf23a5f6d93e110b30ce10057d/chromeos/settings/system_settings_provider.cc [modify] https://crrev.com/bac20b94585bb8cf23a5f6d93e110b30ce10057d/chromeos/settings/system_settings_provider.h
,
Jan 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ecb6eb41941fa6a00d368cdeeb26381facf62c82 commit ecb6eb41941fa6a00d368cdeeb26381facf62c82 Author: A Olsen <olsen@chromium.org> Date: Fri Jan 04 18:15:46 2019 Remove all instances of kStubCrosSettings switch Use ScopedTestingCrosSettings instead. See bug for more details. Bug: 909635 Change-Id: I37daf492df8fe0be9f2b67ac634a2e68ed886b6f Reviewed-on: https://chromium-review.googlesource.com/c/1394308 Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Commit-Queue: A Olsen <olsen@chromium.org> Cr-Commit-Position: refs/heads/master@{#620000} [modify] https://crrev.com/ecb6eb41941fa6a00d368cdeeb26381facf62c82/chrome/browser/chromeos/extensions/echo_private_apitest.cc [modify] https://crrev.com/ecb6eb41941fa6a00d368cdeeb26381facf62c82/chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc [modify] https://crrev.com/ecb6eb41941fa6a00d368cdeeb26381facf62c82/chrome/browser/chromeos/login/users/user_manager_hide_supervised_users_browsertest.cc [modify] https://crrev.com/ecb6eb41941fa6a00d368cdeeb26381facf62c82/chrome/browser/chromeos/login/webview_login_browsertest.cc [modify] https://crrev.com/ecb6eb41941fa6a00d368cdeeb26381facf62c82/chrome/browser/chromeos/preferences_chromeos_browsertest.cc [modify] https://crrev.com/ecb6eb41941fa6a00d368cdeeb26381facf62c82/chrome/browser/extensions/api/settings_private/settings_private_apitest.cc
,
Jan 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d75c43dc99e26dd2c0901446a2414cb64c43c85f commit d75c43dc99e26dd2c0901446a2414cb64c43c85f Author: A Olsen <olsen@chromium.org> Date: Thu Jan 10 18:44:36 2019 Remove stub-cros-settings switch This removes some testing code from production that was active only when this switch was set. Bug: 909635 Change-Id: I55652a2edd1a1c16d0601d877a954ce60d89e6c0 Reviewed-on: https://chromium-review.googlesource.com/c/1402874 Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Reviewed-by: Achuith Bhandarkar <achuith@chromium.org> Commit-Queue: A Olsen <olsen@chromium.org> Cr-Commit-Position: refs/heads/master@{#621667} [modify] https://crrev.com/d75c43dc99e26dd2c0901446a2414cb64c43c85f/chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc [modify] https://crrev.com/d75c43dc99e26dd2c0901446a2414cb64c43c85f/chrome/browser/chromeos/ownership/owner_settings_service_chromeos_factory.cc [modify] https://crrev.com/d75c43dc99e26dd2c0901446a2414cb64c43c85f/chrome/browser/chromeos/profiles/profile_helper.cc [modify] https://crrev.com/d75c43dc99e26dd2c0901446a2414cb64c43c85f/chrome/browser/chromeos/settings/cros_settings.cc [modify] https://crrev.com/d75c43dc99e26dd2c0901446a2414cb64c43c85f/chrome/browser/chromeos/settings/cros_settings.h [modify] https://crrev.com/d75c43dc99e26dd2c0901446a2414cb64c43c85f/chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.cc [modify] https://crrev.com/d75c43dc99e26dd2c0901446a2414cb64c43c85f/chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h [modify] https://crrev.com/d75c43dc99e26dd2c0901446a2414cb64c43c85f/chromeos/constants/chromeos_switches.cc [modify] https://crrev.com/d75c43dc99e26dd2c0901446a2414cb64c43c85f/chromeos/constants/chromeos_switches.h |
|
►
Sign in to add a comment |
|
Comment 1 by olsen@chromium.org
, Nov 30