Make CrosSettings and DeviceSettingsService non Lazy instances

chrome/browser/chromeos/settings/device_settings_test_helper.h

 // Copyright (c) 2012 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_DEVICE_SETTINGS_TEST_HELPER_H_
 #define CHROME_BROWSER_CHROMEOS_SETTINGS_DEVICE_SETTINGS_TEST_HELPER_H_
 
 #include <map>
 #include <string>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
 #include "base/message_loop.h"
 #include "base/string_util.h"
 #include "chrome/browser/chromeos/policy/device_policy_builder.h"
+#include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/chromeos/settings/device_settings_service.h"
 #include "chrome/browser/chromeos/settings/device_settings_test_helper.h"
 #include "chrome/browser/chromeos/settings/mock_owner_key_util.h"
 #include "chromeos/dbus/session_manager_client.h"
 #include "content/public/test/test_browser_thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace chromeos {
 
 // A helper class for tests mocking out session_manager's device settings
@@ skipping 90 matching lines @@
     PolicyState();
     ~PolicyState();
 
     bool HasPendingOperations() const {
       return !store_callbacks_.empty() || !retrieve_callbacks_.empty();
     }
   };
 
   PolicyState device_policy_;
   std::map<std::string, PolicyState> device_local_account_policy_;
+  scoped_ptr<ScopedTestCrosSettings> test_cros_settings_;

[Mattias Nissler (ping if slow), 2013/04/16 16:09:51]

This file shouldn't have to depend on CrosSettings really.

[stevenjb, 2013/04/16 16:49:43]

Enough tests that rely on this also seem to rely on CrosSettings, so this was a
convenient place to put this, rather than each individual test.
That said, if you really don't like this here, I can remove it and track down
each dependency.

[stevenjb, 2013/04/16 17:07:41]

I'll go ahead and move this, I agree it is awkward here.

[Mattias Nissler (ping if slow), 2013/04/16 18:04:13]

Thanks!

 
   DISALLOW_COPY_AND_ASSIGN(DeviceSettingsTestHelper);
 };
 
 // Wraps the singleton device settings and initializes it to the point where it
 // reports OWNERSHIP_NONE for the ownership status.
 class ScopedDeviceSettingsTestHelper : public DeviceSettingsTestHelper {
  public:
   ScopedDeviceSettingsTestHelper();
   virtual ~ScopedDeviceSettingsTestHelper();
@@ skipping 22 matching lines @@
   void ReloadDeviceSettings();
 
   MessageLoop loop_;
   content::TestBrowserThread ui_thread_;
   content::TestBrowserThread file_thread_;
 
   policy::DevicePolicyBuilder device_policy_;
 
   DeviceSettingsTestHelper device_settings_test_helper_;
   scoped_refptr<MockOwnerKeyUtil> owner_key_util_;
-  DeviceSettingsService device_settings_service_;
+
+  // Convenience pointer to DeviceSettingsService singleton.
+  DeviceSettingsService* device_settings_service_;

[Mattias Nissler (ping if slow), 2013/04/16 16:09:51]

The whole point of this class was to enable testing against a non-singleton
DeviceSettingsService instance. I'm not a fan of adding references to globals
all over the code.

[stevenjb, 2013/04/16 16:49:43]

The problem is, because it *is* a global singleton, we invariably end up
accessing it in test code whether we expect to or not, which means we have two
instances, causing all sorts of potential trouble. I prefer to control the
global singleton for the test.

[Mattias Nissler (ping if slow), 2013/04/16 18:04:13]

Per offline discussion, the plan is to not initialize the global pointer in
tests so access to hidden globals is flagged to people adding new tests.

 
  private:
   DISALLOW_COPY_AND_ASSIGN(DeviceSettingsTestBase);
 };
 
 }  // namespace chromeos
 
 #endif  // CHROME_BROWSER_CHROMEOS_SETTINGS_DEVICE_SETTINGS_TEST_HELPER_H_