Remove CrosSettings::Get()->Set from tests

Get rid of CrosSettings::Get()->Set...() usage in tests.
This CL changes all such calls to ScopedCrosSettingsTestHelper::Set.
This call does the same thing as before, but it means there is only
one call to CrosSettings->Set that will need to be changed in order
to separate the read and write APIs.

This is part of an ancient bug to make CrosSettings read-only -
the equivalent API for writing is OwnerSettingsService.
It will also be useful for moving CrosSettings out of chrome/browser
without having to move all of OwnerSettingsService.

Another minor clean up:
There are two different ways to have a test setup CrosSettings -
one way is to use ScopedCrosSettingsTestHelper, which sets up
InstallAttributes, DeviceSettings, and CrosSettings.
The other way to set them all up using individual helper classes,
including ScopedDeviceSettingsTestHelper. What is not obvious, is that
the first way sets up a real session manager, but the second way
sets up a fake session manager. This CL also changes
existing_user_controller_auto_login_unittest.cc to use the first way
for consistency, and adds the call "SetFakeSessionManager()" which
is more explicit, instead of having different helper classes set
things up in subtly different ways.


BUG=433840

Change-Id: I0c84af93d446ce8f82d1aabbf070195abea24dd4
Reviewed-on: https://chromium-review.googlesource.com/c/1256966
Commit-Queue: A Olsen <olsen@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600331}

chrome/browser/chromeos/extensions/echo_private_apitest.cc

@@ -9,6 +9,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
+#include "chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h"
 #include "chrome/browser/chromeos/ui/echo_dialog_view.h"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/extensions/extension_function_test_utils.h"
@@ -129,6 +130,8 @@ class ExtensionEchoPrivateApiTest : public extensions::ExtensionApiTest {
  protected:
   int expected_dialog_buttons_;
   DialogTestAction dialog_action_;
+  chromeos::ScopedCrosSettingsTestHelper settings_helper_{
+      /* create_settings_service= */ false};
 
  private:
   int dialog_invocation_count_;
@@ -239,8 +242,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionEchoPrivateApiTest,
 IN_PROC_BROWSER_TEST_F(ExtensionEchoPrivateApiTest,
                        GetUserConsent_AllowRedeemPrefTrue) {
   const int tab_id = OpenAndActivateTab();
-  chromeos::CrosSettings::Get()->SetBoolean(
-            chromeos::kAllowRedeemChromeOsRegistrationOffers, true);
+  settings_helper_.SetBoolean(chromeos::kAllowRedeemChromeOsRegistrationOffers,
+                              true);
 
   expected_dialog_buttons_ = ui::DIALOG_BUTTON_CANCEL | ui::DIALOG_BUTTON_OK;
   dialog_action_ = DIALOG_TEST_ACTION_ACCEPT;
@@ -253,8 +256,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionEchoPrivateApiTest,
 IN_PROC_BROWSER_TEST_F(ExtensionEchoPrivateApiTest,
                        GetUserConsent_ConsentDenied) {
   const int tab_id = OpenAndActivateTab();
-  chromeos::CrosSettings::Get()->SetBoolean(
-            chromeos::kAllowRedeemChromeOsRegistrationOffers, true);
+  settings_helper_.SetBoolean(chromeos::kAllowRedeemChromeOsRegistrationOffers,
+                              true);
 
   expected_dialog_buttons_ = ui::DIALOG_BUTTON_CANCEL | ui::DIALOG_BUTTON_OK;
   dialog_action_ = DIALOG_TEST_ACTION_CANCEL;
@@ -267,8 +270,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionEchoPrivateApiTest,
 IN_PROC_BROWSER_TEST_F(ExtensionEchoPrivateApiTest,
                        GetUserConsent_AllowRedeemPrefFalse) {
   const int tab_id = OpenAndActivateTab();
-  chromeos::CrosSettings::Get()->SetBoolean(
-            chromeos::kAllowRedeemChromeOsRegistrationOffers, false);
+  settings_helper_.SetBoolean(chromeos::kAllowRedeemChromeOsRegistrationOffers,
+                              false);
 
   expected_dialog_buttons_ = ui::DIALOG_BUTTON_CANCEL;
   dialog_action_ = DIALOG_TEST_ACTION_CANCEL;

chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc

@@ -14,8 +14,7 @@
 #include "chrome/browser/chromeos/login/users/mock_user_manager.h"
 #include "chrome/browser/chromeos/policy/device_local_account.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
-#include "chrome/browser/chromeos/settings/device_settings_test_helper.h"
-#include "chrome/browser/chromeos/settings/stub_install_attributes.h"
+#include "chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h"
 #include "chrome/test/base/scoped_testing_local_state.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chromeos/settings/cros_settings_names.h"
@@ -64,6 +63,8 @@ class ExistingUserControllerAutoLoginTest : public ::testing::Test {
         .WillRepeatedly(Return(mock_user_manager_->CreatePublicAccountUser(
             auto_login_account_id_)));
 
+    settings_helper_.SetFakeSessionManager();
+
     std::unique_ptr<base::DictionaryValue> account(new base::DictionaryValue);
     account->SetKey(kAccountsPrefDeviceLocalAccountsKeyId,
                     base::Value(auto_login_user_id_));
@@ -72,7 +73,7 @@ class ExistingUserControllerAutoLoginTest : public ::testing::Test {
         base::Value(policy::DeviceLocalAccount::TYPE_PUBLIC_SESSION));
     base::ListValue accounts;
     accounts.Append(std::move(account));
-    CrosSettings::Get()->Set(kAccountsPrefDeviceLocalAccounts, accounts);
+    settings_helper_.Set(kAccountsPrefDeviceLocalAccounts, accounts);
 
     // Prevent settings changes from auto-starting the timer.
     existing_user_controller_->local_account_auto_login_id_subscription_
@@ -86,10 +87,10 @@ class ExistingUserControllerAutoLoginTest : public ::testing::Test {
   }
 
   void SetAutoLoginSettings(const std::string& user_id, int delay) {
-    CrosSettings::Get()->SetString(kAccountsPrefDeviceLocalAccountAutoLoginId,
-                                   user_id);
-    CrosSettings::Get()->SetInteger(
-        kAccountsPrefDeviceLocalAccountAutoLoginDelay, delay);
+    settings_helper_.SetString(kAccountsPrefDeviceLocalAccountAutoLoginId,
+                               user_id);
+    settings_helper_.SetInteger(kAccountsPrefDeviceLocalAccountAutoLoginDelay,
+                                delay);
   }
 
   // ExistingUserController private member accessors.
@@ -138,9 +139,7 @@ class ExistingUserControllerAutoLoginTest : public ::testing::Test {
   ScopedTestingLocalState local_state_;
 
   // Required by ExistingUserController:
-  ScopedStubInstallAttributes test_install_attributes_;
-  ScopedDeviceSettingsTestHelper device_settings_test_helper_;
-  ScopedTestCrosSettings test_cros_settings_{local_state_.Get()};
+  ScopedCrosSettingsTestHelper settings_helper_;
   MockUserManager* mock_user_manager_;
   user_manager::ScopedUserManager scoped_user_manager_;
   std::unique_ptr<ArcKioskAppManager> arc_kiosk_app_manager_;

chrome/browser/chromeos/login/login_browsertest.cc

@@ -25,6 +25,7 @@
 #include "chrome/browser/chromeos/login/ui/login_display_host_webui.h"
 #include "chrome/browser/chromeos/login/wizard_controller.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
+#include "chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h"
 #include "chrome/browser/chromeos/settings/stub_install_attributes.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/profiles/profiles_state.h"
@@ -181,6 +182,10 @@ class LoginTest : public LoginManagerTest {
     user_context.SetKey(Key(kPassword));
     SetExpectedCredentials(user_context);
   }
+
+ protected:
+  ScopedCrosSettingsTestHelper settings_helper_{
+      /* create_settings_service= */ false};
 };
 
 // Used to make sure that the system tray is visible and within the screen
@@ -259,7 +264,7 @@ IN_PROC_BROWSER_TEST_F(LoginSigninTest, WebUIVisible) {
 IN_PROC_BROWSER_TEST_F(LoginTest, PRE_GaiaAuthOffline) {
   RegisterUser(AccountId::FromUserEmailGaiaId(kTestUser, kGaiaId));
   StartupUtils::MarkOobeCompleted();
-  CrosSettings::Get()->SetBoolean(kAccountsPrefShowUserNamesOnSignIn, false);
+  settings_helper_.SetBoolean(kAccountsPrefShowUserNamesOnSignIn, false);
 }
 
 // Flaky, see http://crbug/692364.

chrome/browser/chromeos/login/login_ui_keyboard_browsertest.cc

@@ -14,6 +14,7 @@
 #include "chrome/browser/chromeos/login/startup_utils.h"
 #include "chrome/browser/chromeos/login/test/oobe_screen_waiter.h"
 #include "chrome/browser/chromeos/login/ui/login_display_host.h"
+#include "chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h"
 #include "chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/chromeos_switches.h"
@@ -198,7 +199,7 @@ class LoginUIKeyboardTestWithUsersAndOwner : public chromeos::LoginManagerTest {
     chromeos::input_method::InputMethodManager::Get()->MigrateInputMethods(
         &user_input_methods);
 
-    CrosSettings::Get()->SetString(kDeviceOwner, kTestUser3);
+    settings_helper_.SetString(kDeviceOwner, kTestUser3);
 
     LoginManagerTest::SetUpOnMainThread();
   }
@@ -223,6 +224,8 @@ class LoginUIKeyboardTestWithUsersAndOwner : public chromeos::LoginManagerTest {
 
  protected:
   std::vector<std::string> user_input_methods;
+  ScopedCrosSettingsTestHelper settings_helper_{
+      /* create_settings_service= */ false};
 };
 
 void LoginUIKeyboardTestWithUsersAndOwner::CheckGaiaKeyboard() {

chrome/browser/chromeos/login/webview_login_browsertest.cc

@@ -22,6 +22,7 @@
 #include "chrome/browser/chromeos/policy/device_policy_builder.h"
 #include "chrome/browser/chromeos/policy/device_policy_cros_browser_test.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h"
 #include "chrome/browser/policy/test/local_policy_test_server.h"
 #include "chrome/browser/ui/login/login_handler.h"
 #include "chrome/browser/ui/webui/signin/signin_utils.h"
@@ -224,6 +225,10 @@ class WebviewLoginTest : public OobeBaseTest {
     return web_view_found;
   }
 
+ protected:
+  ScopedCrosSettingsTestHelper settings_helper_{
+      /* create_settings_service= */ false};
+
  private:
   DISALLOW_COPY_AND_ASSIGN(WebviewLoginTest);
 };
@@ -288,7 +293,7 @@ IN_PROC_BROWSER_TEST_F(WebviewLoginTest, DISABLED_BackButton) {
 IN_PROC_BROWSER_TEST_F(WebviewLoginTest, AllowGuest) {
   WaitForGaiaPageLoad();
   JsExpect("!$('guest-user-header-bar-item').hidden");
-  CrosSettings::Get()->SetBoolean(kAccountsPrefAllowGuest, false);
+  settings_helper_.SetBoolean(kAccountsPrefAllowGuest, false);
   JsExpect("$('guest-user-header-bar-item').hidden");
 }
 
@@ -301,8 +306,8 @@ IN_PROC_BROWSER_TEST_F(WebviewLoginTest, AllowNewUser) {
   JsExpect(frame_url + ".search('flow=nosignup') == -1");
 
   // Disallow new users - we also need to set a whitelist due to weird logic.
-  CrosSettings::Get()->Set(kAccountsPrefUsers, base::ListValue());
-  CrosSettings::Get()->SetBoolean(kAccountsPrefAllowNewUser, false);
+  settings_helper_.Set(kAccountsPrefUsers, base::ListValue());
+  settings_helper_.SetBoolean(kAccountsPrefAllowNewUser, false);
   WaitForGaiaPageReload();
 
   // flow=nosignup indicates that user creation is not allowed.

chrome/browser/chromeos/preferences_chromeos_browsertest.cc

@@ -15,6 +15,7 @@
 #include "chrome/browser/chromeos/login/ui/user_adding_screen.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
+#include "chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h"
 #include "chrome/browser/chromeos/settings/stub_cros_settings_provider.h"
 #include "chrome/browser/chromeos/system/fake_input_device_settings.h"
 #include "chrome/common/pref_names.h"
@@ -59,7 +60,7 @@ class PreferencesTest : public LoginManagerTest {
     static_cast<input_method::InputMethodManagerImpl*>(
         input_method::InputMethodManager::Get())
         ->SetImeKeyboardForTesting(keyboard_);
-    CrosSettings::Get()->SetString(kDeviceOwner, test_users_[0].GetUserEmail());
+    settings_helper_.SetString(kDeviceOwner, test_users_[0].GetUserEmail());
   }
 
   // Sets set of preferences in given |prefs|. Value of prefernece depends of
@@ -119,6 +120,8 @@ class PreferencesTest : public LoginManagerTest {
   }
 
   std::vector<AccountId> test_users_;
+  ScopedCrosSettingsTestHelper settings_helper_{
+      /* create_settings_service= */ false};
 
  private:
   system::InputDeviceSettings::FakeInterface* input_settings_;

chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.cc

@@ -11,11 +11,13 @@
 #include "chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/chromeos/settings/device_settings_cache.h"
+#include "chrome/browser/chromeos/settings/device_settings_provider.h"
 #include "chrome/browser/chromeos/settings/device_settings_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/ownership/mock_owner_key_util.h"
 #include "components/policy/proto/chrome_device_policy.pb.h"
 #include "components/policy/proto/device_management_backend.pb.h"
+#include "content/public/test/test_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace chromeos {
@@ -81,8 +83,16 @@ void ScopedCrosSettingsTestHelper::SetCurrentUserIsOwner(bool owner) {
 
 void ScopedCrosSettingsTestHelper::Set(const std::string& path,
                                        const base::Value& in_value) {
-  CHECK(IsDeviceSettingsProviderStubbed());
-  stub_settings_provider_ptr_->Set(path, in_value);
+  CHECK(DeviceSettingsProvider::IsDeviceSetting(path));
+  if (IsDeviceSettingsProviderStubbed()) {
+    // Set in the stub settings provider.
+    stub_settings_provider_ptr_->Set(path, in_value);
+  } else {
+    // Set in the real settings provider.
+    // TODO(olsen): Separate the write path (OwnerSettingsService) away from the
+    // read path (CrosSettings) - http://crbug.com/433840
+    CrosSettings::Get()->Set(path, in_value);
+  }
 }
 
 void ScopedCrosSettingsTestHelper::SetBoolean(const std::string& path,
@@ -131,6 +141,13 @@ void ScopedCrosSettingsTestHelper::CopyStoredValue(const std::string& path) {
   }
 }
 
+void ScopedCrosSettingsTestHelper::SetFakeSessionManager() {
+  DeviceSettingsService::Get()->SetSessionManager(
+      &fake_session_manager_client_, new ownership::MockOwnerKeyUtil());
+  DeviceSettingsService::Get()->Load();
+  content::RunAllTasksUntilIdle();
+}
+
 StubInstallAttributes* ScopedCrosSettingsTestHelper::InstallAttributes() {
   return test_install_attributes_->Get();
 }

chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h

@@ -11,6 +11,7 @@
 #include "base/macros.h"
 #include "chrome/browser/chromeos/settings/stub_cros_settings_provider.h"
 #include "chrome/browser/chromeos/settings/stub_install_attributes.h"
+#include "chromeos/dbus/fake_session_manager_client.h"
 #include "chromeos/settings/cros_settings_provider.h"
 
 class Profile;
@@ -67,12 +68,17 @@ class ScopedCrosSettingsTestHelper {
   // device settings service.
   void StoreCachedDeviceSetting(const std::string& path);
 
+  // Sets the underlying DeviceSettingsService session manager to a
+  // FakeSessionManagerClient.
+  void SetFakeSessionManager();
+
   // Get the scoped install attributes to change them as needed for the
   // current test.
   StubInstallAttributes* InstallAttributes();
 
  private:
   // Helpers used to mock out cros settings.
+  FakeSessionManagerClient fake_session_manager_client_;
   std::unique_ptr<ScopedStubInstallAttributes> test_install_attributes_;
   std::unique_ptr<ScopedTestDeviceSettingsService>
       test_device_settings_service_;