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}

chrome/browser/chromeos/BUILD.gn

@@ -2042,6 +2042,8 @@ static_library("test_support") {
     "login/test/test_condition_waiter.h",
     "scoped_set_running_on_chromeos_for_testing.cc",
     "scoped_set_running_on_chromeos_for_testing.h",
+    "settings/scoped_testing_cros_settings.cc",
+    "settings/scoped_testing_cros_settings.h",
   ]
 
   deps = [

chrome/browser/chromeos/extensions/users_private/users_private_apitest.cc

@@ -16,11 +16,14 @@
 #include "chrome/browser/chromeos/login/lock/screen_locker_tester.h"
 #include "chrome/browser/chromeos/login/test/oobe_base_test.h"
 #include "chrome/browser/chromeos/ownership/owner_settings_service_chromeos_factory.h"
+#include "chrome/browser/chromeos/settings/scoped_testing_cros_settings.h"
+#include "chrome/browser/chromeos/settings/stub_cros_settings_provider.h"
+#include "chrome/browser/chromeos/settings/stub_install_attributes.h"
 #include "chrome/browser/extensions/api/settings_private/prefs_util.h"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/api/users_private.h"
-#include "chromeos/chromeos_switches.h"
+#include "chromeos/settings/cros_settings_names.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/ownership/mock_owner_key_util.h"
 #include "components/prefs/pref_service.h"
@@ -120,6 +123,9 @@ class UsersPrivateApiTest : public ExtensionApiTest {
 
     chromeos::OwnerSettingsServiceChromeOSFactory::GetInstance()
         ->SetOwnerKeyUtilForTesting(owner_key_util);
+
+    scoped_testing_cros_settings_.device_settings()->Set(
+        chromeos::kDeviceOwner, base::Value("testuser@gmail.com"));
   }
   ~UsersPrivateApiTest() override = default;
 
@@ -129,14 +135,6 @@ class UsersPrivateApiTest : public ExtensionApiTest {
     return base::WrapUnique(s_test_delegate_);
   }
 
-  void SetUpCommandLine(base::CommandLine* command_line) override {
-    ExtensionApiTest::SetUpCommandLine(command_line);
-    command_line->AppendSwitch(chromeos::switches::kStubCrosSettings);
-    command_line->AppendSwitchASCII(chromeos::switches::kLoginUser,
-                                    "testuser@gmail.com");
-    command_line->AppendSwitchASCII(chromeos::switches::kLoginProfile, "user");
-  }
-
   void SetUpOnMainThread() override {
     ExtensionApiTest::SetUpOnMainThread();
     if (!s_test_delegate_)
@@ -159,6 +157,9 @@ class UsersPrivateApiTest : public ExtensionApiTest {
   static TestDelegate* s_test_delegate_;
 
  private:
+  chromeos::ScopedStubInstallAttributes scoped_stub_install_attributes_;
+  chromeos::ScopedTestingCrosSettings scoped_testing_cros_settings_;
+
   DISALLOW_COPY_AND_ASSIGN(UsersPrivateApiTest);
 };
 

chrome/browser/chromeos/ownership/owner_settings_service_chromeos_factory.cc

@@ -10,6 +10,7 @@
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/chromeos/settings/device_settings_service.h"
+#include "chrome/browser/chromeos/settings/stub_cros_settings_provider.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chromeos/chromeos_paths.h"
 #include "chromeos/chromeos_switches.h"
@@ -23,6 +24,8 @@ namespace {
 
 DeviceSettingsService* g_device_settings_service_for_testing_ = nullptr;
 
+StubCrosSettingsProvider* g_stub_cros_settings_provider_for_testing_ = nullptr;
+
 DeviceSettingsService* GetDeviceSettingsService() {
   if (g_device_settings_service_for_testing_)
     return g_device_settings_service_for_testing_;
@@ -61,6 +64,12 @@ void OwnerSettingsServiceChromeOSFactory::SetDeviceSettingsServiceForTesting(
   g_device_settings_service_for_testing_ = device_settings_service;
 }
 
+// static
+void OwnerSettingsServiceChromeOSFactory::SetStubCrosSettingsProviderForTesting(
+    StubCrosSettingsProvider* stub_cros_settings_provider) {
+  g_stub_cros_settings_provider_for_testing_ = stub_cros_settings_provider;
+}
+
 scoped_refptr<ownership::OwnerKeyUtil>
 OwnerSettingsServiceChromeOSFactory::GetOwnerKeyUtil() {
   if (owner_key_util_.get())
@@ -86,14 +95,22 @@ KeyedService* OwnerSettingsServiceChromeOSFactory::BuildInstanceFor(
     return nullptr;
   }
 
-  // If kStubCrosSettings is set, we treat the current user as the owner, and
-  // write settings directly to the stubbed provider in CrosSettings.
-  // This is done using the FakeOwnerSettingsService.
+  // TODO(olsen): Delete this code once no tests use kStubCrosSettings switch.
+  // See http://crbug.com/909635
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kStubCrosSettings)) {
+          switches::kStubCrosSettings) &&
+      g_stub_cros_settings_provider_for_testing_ == nullptr) {
+    g_stub_cros_settings_provider_for_testing_ =
+        CrosSettings::Get()->stubbed_provider_for_test();
+  }
+
+  // If g_stub_cros_settings_provider_for_testing_ is set, we treat the current
+  // user as the owner, and write settings directly to the stubbed provider.
+  // This is done using the FakeOwnerSettingsService.
+  if (g_stub_cros_settings_provider_for_testing_ != nullptr) {
     return new FakeOwnerSettingsService(
         profile, GetInstance()->GetOwnerKeyUtil(),
-        CrosSettings::Get()->stubbed_provider_for_test());
+        g_stub_cros_settings_provider_for_testing_);
   }
 
   return new OwnerSettingsServiceChromeOS(

chrome/browser/chromeos/ownership/owner_settings_service_chromeos_factory.h

@@ -25,6 +25,7 @@ namespace chromeos {
 
 class DeviceSettingsService;
 class OwnerSettingsServiceChromeOS;
+class StubCrosSettingsProvider;
 
 class OwnerSettingsServiceChromeOSFactory
     : public BrowserContextKeyedServiceFactory {
@@ -37,6 +38,9 @@ class OwnerSettingsServiceChromeOSFactory
   static void SetDeviceSettingsServiceForTesting(
       DeviceSettingsService* device_settings_service);
 
+  static void SetStubCrosSettingsProviderForTesting(
+      StubCrosSettingsProvider* stub_cros_settings_provider);
+
   scoped_refptr<ownership::OwnerKeyUtil> GetOwnerKeyUtil();
 
   void SetOwnerKeyUtilForTesting(

chrome/browser/chromeos/settings/cros_settings.cc

@@ -23,8 +23,17 @@ namespace chromeos {
 
 static CrosSettings* g_cros_settings = nullptr;
 
+// Calling SetForTesting sets this flag. This flag means that the production
+// code which calls Initialize and Shutdown will have no effect - the test
+// install attributes will remain in place until ShutdownForTesting is called.
+bool g_using_cros_settings_for_testing = false;
+
 // static
 void CrosSettings::Initialize(PrefService* local_state) {
+  // Don't reinitialize if a specific instance has already been set for test.
+  if (g_using_cros_settings_for_testing)
+    return;
+
   CHECK(!g_cros_settings);
   g_cros_settings = new CrosSettings(DeviceSettingsService::Get(), local_state);
 }
@@ -36,6 +45,9 @@ bool CrosSettings::IsInitialized() {
 
 // static
 void CrosSettings::Shutdown() {
+  if (g_using_cros_settings_for_testing)
+    return;
+
   DCHECK(g_cros_settings);
   delete g_cros_settings;
   g_cros_settings = nullptr;
@@ -47,6 +59,22 @@ CrosSettings* CrosSettings::Get() {
   return g_cros_settings;
 }
 
+// static
+void CrosSettings::SetForTesting(CrosSettings* test_instance) {
+  DCHECK(!g_cros_settings);
+  DCHECK(!g_using_cros_settings_for_testing);
+  g_cros_settings = test_instance;
+  g_using_cros_settings_for_testing = true;
+}
+
+// static
+void CrosSettings::ShutdownForTesting() {
+  DCHECK(g_using_cros_settings_for_testing);
+  // Don't delete the test instance, we are not the owner.
+  g_cros_settings = nullptr;
+  g_using_cros_settings_for_testing = false;
+}
+
 bool CrosSettings::IsUserWhitelisted(const std::string& username,
                                      bool* wildcard_match) const {
   // Skip whitelist check for tests.
@@ -62,6 +90,8 @@ bool CrosSettings::IsUserWhitelisted(const std::string& username,
   return FindEmailInList(kAccountsPrefUsers, username, wildcard_match);
 }
 
+CrosSettings::CrosSettings() = default;
+
 CrosSettings::CrosSettings(DeviceSettingsService* device_settings_service,
                            PrefService* local_state) {
   CrosSettingsProvider::NotifyObserversCallback notify_cb(

chrome/browser/chromeos/settings/cros_settings.h

@@ -39,12 +39,22 @@ class CrosSettings {
   static void Shutdown();
   static CrosSettings* Get();
 
+  // Sets the singleton to |test_instance|. Does not take ownership of the
+  // instance. Should be matched with a call to |ShutdownForTesting| once the
+  // test is finished and before the instance is deleted.
+  static void SetForTesting(CrosSettings* test_instance);
+  static void ShutdownForTesting();
+
   // Checks if the given username is whitelisted and allowed to sign-in to
   // this device. |wildcard_match| may be NULL. If it's present, it'll be set to
   // true if the whitelist check was satisfied via a wildcard.
   bool IsUserWhitelisted(const std::string& username,
                          bool* wildcard_match) const;
 
+  // Creates an instance with no providers as yet. This is meant for unit tests,
+  // production code uses the singleton returned by Get() above.
+  CrosSettings();
+
   // Creates a device settings service instance. This is meant for unit tests,
   // production code uses the singleton returned by Get() above.
   CrosSettings(DeviceSettingsService* device_settings_service,

chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h

@@ -26,6 +26,9 @@ class FakeOwnerSettingsService;
 class ScopedTestCrosSettings;
 class ScopedTestDeviceSettingsService;
 
+// Helps in a variety of ways with setting up CrosSettings for testing.
+// This class is overly complex for most use-cases - if possible, prefer to
+// use ScopedTestingCrosSettings for new tests.
 class ScopedCrosSettingsTestHelper {
  public:
   // In some cases it is required to pass |create_settings_service| as false:

chrome/browser/chromeos/settings/scoped_testing_cros_settings.cc

+// Copyright (c) 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/chromeos/settings/scoped_testing_cros_settings.h"
+
+#include "chrome/browser/chromeos/settings/cros_settings.h"
+#include "chrome/browser/chromeos/settings/stub_cros_settings_provider.h"
+#include "chromeos/settings/system_settings_provider.h"
+
+#include "chrome/browser/chromeos/ownership/owner_settings_service_chromeos_factory.h"
+
+namespace chromeos {
+
+ScopedTestingCrosSettings::ScopedTestingCrosSettings() {
+  test_instance_ = std::make_unique<CrosSettings>();
+
+  std::unique_ptr<StubCrosSettingsProvider> device_settings =
+      std::make_unique<StubCrosSettingsProvider>();
+  OwnerSettingsServiceChromeOSFactory::SetStubCrosSettingsProviderForTesting(
+      device_settings.get());
+  device_settings_ptr_ = device_settings.get();
+  test_instance_->AddSettingsProvider(std::move(device_settings));
+
+  std::unique_ptr<SystemSettingsProvider> system_settings =
+      std::make_unique<SystemSettingsProvider>();
+  system_settings_ptr_ = system_settings.get();
+  test_instance_->AddSettingsProvider(std::move(system_settings));
+
+  CrosSettings::SetForTesting(test_instance_.get());
+}
+
+ScopedTestingCrosSettings::~ScopedTestingCrosSettings() {
+  OwnerSettingsServiceChromeOSFactory::SetStubCrosSettingsProviderForTesting(
+      nullptr);
+  device_settings_ptr_ = nullptr;
+  system_settings_ptr_ = nullptr;
+  CrosSettings::ShutdownForTesting();
+}
+
+}  // namespace chromeos

chrome/browser/chromeos/settings/scoped_testing_cros_settings.h

+// Copyright (c) 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_CHROMEOS_SETTINGS_SCOPED_TESTING_CROS_SETTINGS_H_
+#define CHROME_BROWSER_CHROMEOS_SETTINGS_SCOPED_TESTING_CROS_SETTINGS_H_
+
+#include <memory>
+
+#include "base/macros.h"
+
+namespace chromeos {
+
+class CrosSettings;
+class StubCrosSettingsProvider;
+class SystemSettingsProvider;
+
+// Helper class which calls CrosSettings::SetForTesting when it is constructed,
+// and calls CrosSettings::ShutdownForTesting when it goes out of scope,
+// with a CrosSettings instance that it creates and owns.
+// This CrosSettings instance has two settings providers:
+// a StubCrosSettingsProvider for device settings,
+// and a regular SystemSettingsProvider for the system settings.
+//
+// OwnerSettingsServiceChromeOSFactory::SetStubCrosSettingsProviderForTesting is
+// caled too, with the StubCrosSettingsProvider, so that any
+// OwnerSettingsService created will write to the StubCrosSettingsProvider.
+//
+// Prefer to use this class instead of ScopedCrosSettingsTestHelper - that class
+// has even more responsibilities and is overly complex for most use-cases.
+class ScopedTestingCrosSettings {
+ public:
+  // Creates a CrosSettings test instance that has two settings providers -
+  // a StubCrosSettingsProvider, and a SystemsSettingsProvider - and sets it
+  // for testing.
+  ScopedTestingCrosSettings();
+
+  ~ScopedTestingCrosSettings();
+
+  StubCrosSettingsProvider* device_settings() { return device_settings_ptr_; }
+
+  SystemSettingsProvider* system_settings() { return system_settings_ptr_; }
+
+ private:
+  std::unique_ptr<CrosSettings> test_instance_;
+
+  // These are raw pointers since these objects are owned by |test_instance_|.
+  StubCrosSettingsProvider* device_settings_ptr_;
+  SystemSettingsProvider* system_settings_ptr_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedTestingCrosSettings);
+};
+
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_SETTINGS_SCOPED_TESTING_CROS_SETTINGS_H_

chromeos/settings/system_settings_provider.cc

@@ -31,9 +31,22 @@ bool FineGrainedTimeZoneDetectionEnabled() {
 
 }  // namespace
 
+SystemSettingsProvider::SystemSettingsProvider()
+    : CrosSettingsProvider(CrosSettingsProvider::NotifyObserversCallback()) {
+  Init();
+}
+
 SystemSettingsProvider::SystemSettingsProvider(
     const NotifyObserversCallback& notify_cb)
     : CrosSettingsProvider(notify_cb) {
+  Init();
+}
+
+SystemSettingsProvider::~SystemSettingsProvider() {
+  system::TimezoneSettings::GetInstance()->RemoveObserver(this);
+}
+
+void SystemSettingsProvider::Init() {
   system::TimezoneSettings* timezone_settings =
       system::TimezoneSettings::GetInstance();
   timezone_settings->AddObserver(this);
@@ -45,10 +58,6 @@ SystemSettingsProvider::SystemSettingsProvider(
       new base::Value(FineGrainedTimeZoneDetectionEnabled()));
 }
 
-SystemSettingsProvider::~SystemSettingsProvider() {
-  system::TimezoneSettings::GetInstance()->RemoveObserver(this);
-}
-
 void SystemSettingsProvider::DoSet(const std::string& path,
                                    const base::Value& in_value) {
   // TODO(olsen): crbug.com/433840 - separate read path and write path.

chromeos/settings/system_settings_provider.h

@@ -24,6 +24,7 @@ class CHROMEOS_EXPORT SystemSettingsProvider
     : public CrosSettingsProvider,
       public system::TimezoneSettings::Observer {
  public:
+  SystemSettingsProvider();
   explicit SystemSettingsProvider(const NotifyObserversCallback& notify_cb);
   ~SystemSettingsProvider() override;
 
@@ -36,6 +37,9 @@ class CHROMEOS_EXPORT SystemSettingsProvider
   void TimezoneChanged(const icu::TimeZone& timezone) override;
 
  private:
+  // Code common to both constructors.
+  void Init();
+
   // CrosSettingsProvider implementation.
   void DoSet(const std::string& path, const base::Value& in_value) override;