Signed settings refactoring: Proper caching and more tests.

chrome/browser/chromeos/cros_settings_unittest.cc

Index: chrome/browser/chromeos/cros_settings_unittest.cc
diff --git a/chrome/browser/chromeos/cros_settings_unittest.cc b/chrome/browser/chromeos/cros_settings_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..4cc22e59003a69c00febeceab474e1c7bab48b45
--- /dev/null
+++ b/chrome/browser/chromeos/cros_settings_unittest.cc
@@ -0,0 +1,251 @@
+// Copyright (c) 2011 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/login/signed_settings.h"
+
+#include <vector>
+
+#include "base/bind.h"
+#include "base/memory/weak_ptr.h"
+#include "base/message_loop.h"
+#include "base/values.h"
+#include "chrome/browser/chromeos/cros_settings.h"
+#include "chrome/browser/chromeos/cros_settings_names.h"
+#include "chrome/browser/chromeos/cros/cros_library.h"
+#include "chrome/browser/chromeos/login/signed_settings_cache.h"
+#include "chrome/browser/policy/proto/chrome_device_policy.pb.h"
+#include "chrome/browser/policy/proto/device_management_backend.pb.h"
+#include "chrome/test/base/testing_browser_process.h"
+#include "chrome/test/base/testing_pref_service.h"
+#include "content/test/test_browser_thread.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace em = enterprise_management;
+namespace chromeos {
+
+class CrosSettingsTest : public testing::Test {
+ protected:
+  CrosSettingsTest()
+      : message_loop_(MessageLoop::TYPE_UI),
+        ui_thread_(content::BrowserThread::UI, &message_loop_),
+        file_thread_(content::BrowserThread::FILE, &message_loop_),
+        pointer_factory_(this),
+        local_state_(static_cast<TestingBrowserProcess*>(g_browser_process)) {
+  }
+
+  virtual ~CrosSettingsTest() {
+  }
+
+  virtual void TearDown() {
+    // Reset the cache between tests.

[Mattias Nissler (ping if slow), 2011/12/02 12:13:37]

You might also want to do that on SetUp(), since otherwise tests that leave
behind state might kill you.

[pastarmovj, 2011/12/02 14:43:38]

Done.

+    em::PolicyData fake_pol;
+    PrepareEmptyPolicy(&fake_pol);
+    signed_settings_cache::Store(fake_pol, local_state_.Get());
+    CrosSettings::Get()->ReloadProviders();
+  }
+
+  void FetchPref(const std::string& pref) {
+    content::BrowserThread::PostTask(
+        content::BrowserThread::UI, FROM_HERE,
+        base::Bind(&CrosSettingsTest::FetchPrefOnUIThread,
+                   pointer_factory_.GetWeakPtr(), pref));

[Mattias Nissler (ping if slow), 2011/12/02 12:13:37]

Do we need the task posting here? I guess not, so please remove.

[pastarmovj, 2011/12/02 14:43:38]

Nope removed.

+  }
+
+  void SetPref(const std::string& pref_name, const base::Value* value) {
+    // TODO(pastarmovj): Use the new base::Owned here to wrap |value|.
+    content::BrowserThread::PostTask(
+        content::BrowserThread::UI, FROM_HERE,
+        base::Bind(&CrosSettingsTest::SetPrefOnUIThread,
+                   pointer_factory_.GetWeakPtr(), pref_name, value));
+  }
+
+  void FetchPrefOnUIThread(const std::string& pref) {
+    DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+    if (expected_props_.find(pref) == expected_props_.end())
+      return;
+
+    if (CrosSettings::Get()->GetTrusted(pref,
+            base::Bind(&CrosSettingsTest::FetchPrefOnUIThread,
+                       pointer_factory_.GetWeakPtr(), pref))) {
+      scoped_ptr<base::Value> expected_value(
+          expected_props_.find(pref)->second);
+      const base::Value* pref_value = CrosSettings::Get()->GetPref(pref);
+      if (expected_value.get()) {
+        ASSERT_TRUE(pref_value);
+        ASSERT_TRUE(expected_value->Equals(pref_value));
+      } else {
+        ASSERT_FALSE(pref_value);
+      }
+      expected_props_.erase(pref);
+    }
+  }
+
+  void SetPrefOnUIThread(const std::string& pref_name,
+                         const base::Value* value) {
+    DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+    CrosSettings::Get()->Set(pref_name, *value);
+  }
+
+  void AddExpectation(const std::string& pref_name, base::Value* value) {
+    expected_props_[pref_name] = value;

[Mattias Nissler (ping if slow), 2011/12/02 12:13:37]

base::Value*& entry = expected_props_[pref_name];
delete entry;
entry = value;

[pastarmovj, 2011/12/02 14:43:38]

Done.

+  }
+
+  void PrepareEmptyPolicy(em::PolicyData* policy) {
+    // Prepare some policy blob.
+    em::PolicyFetchResponse response;
+    em::ChromeDeviceSettingsProto pol;
+    policy->set_policy_type(chromeos::kDevicePolicyType);
+    policy->set_username("me@owner");
+    policy->set_policy_value(pol.SerializeAsString());
+    // Wipe the signed settings store.
+    response.set_policy_data(policy->SerializeAsString());
+    response.set_policy_data_signature("false");
+  }
+
+  std::map<std::string, base::Value*> expected_props_;
+
+  MessageLoop message_loop_;
+  content::TestBrowserThread ui_thread_;
+  content::TestBrowserThread file_thread_;
+
+  base::WeakPtrFactory<CrosSettingsTest> pointer_factory_;
+
+  ScopedTestingLocalState local_state_;
+
+  ScopedStubCrosEnabler stub_cros_enabler_;
+};
+
+TEST_F(CrosSettingsTest, SetPref) {
+  // Change to something that is not the default.
+  AddExpectation(kAccountsPrefAllowGuest,
+                 base::Value::CreateBooleanValue(false));
+  SetPref(kAccountsPrefAllowGuest, expected_props_[kAccountsPrefAllowGuest]);
+  FetchPref(kAccountsPrefAllowGuest);
+  message_loop_.RunAllPending();
+  ASSERT_TRUE(expected_props_.empty());
+}
+
+TEST_F(CrosSettingsTest, GetPref) {
+  // We didn't change the default so look for it.
+  AddExpectation(kAccountsPrefAllowGuest,
+                 base::Value::CreateBooleanValue(true));
+  FetchPref(kAccountsPrefAllowGuest);
+  message_loop_.RunAllPending();
+  ASSERT_TRUE(expected_props_.empty());

[Mattias Nissler (ping if slow), 2011/12/02 12:13:37]

Since you have the last two lines in every test, you may just want them to
TearDown().

[pastarmovj, 2011/12/02 14:43:38]

Done.

+}
+
+TEST_F(CrosSettingsTest, SetWhitelist) {
+  // Setting the whitelist should also switch the value of
+  // kAccountsPrefAllowNewUser to false.
+  scoped_ptr<base::ListValue> whitelist(new base::ListValue);
+  whitelist->Append(base::Value::CreateStringValue("me@owner"));
+  AddExpectation(kAccountsPrefAllowNewUser,
+                 base::Value::CreateBooleanValue(false));
+  AddExpectation(kAccountsPrefUsers, whitelist->DeepCopy());
+  SetPref(kAccountsPrefUsers, whitelist.get());
+  FetchPref(kAccountsPrefAllowNewUser);
+  FetchPref(kAccountsPrefUsers);
+  message_loop_.RunAllPending();
+  ASSERT_TRUE(expected_props_.empty());
+}
+
+TEST_F(CrosSettingsTest, SetWhitelistWithListOps) {
+  base::ListValue* whitelist = new base::ListValue();
+  scoped_ptr<base::Value> hacky_user(base::Value::CreateStringValue("h@xxor"));

[Mattias Nissler (ping if slow), 2011/12/02 12:13:37]

No need to wrap in a scoped_ptr (here and all over the place).

[pastarmovj, 2011/12/02 14:43:38]

Done.

+  whitelist->Append(hacky_user->DeepCopy());
+  AddExpectation(kAccountsPrefAllowNewUser,
+                 base::Value::CreateBooleanValue(false));
+  AddExpectation(kAccountsPrefUsers, whitelist);
+  // Add some user to the whitelist.
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&CrosSettings::AppendToList,
+                 base::Unretained(CrosSettings::Get()),
+                 kAccountsPrefUsers, hacky_user.get()));

[Mattias Nissler (ping if slow), 2011/12/02 12:13:37]

task posting not needed here I guess.

[pastarmovj, 2011/12/02 14:43:38]

Done.

+  FetchPref(kAccountsPrefAllowNewUser);
+  FetchPref(kAccountsPrefUsers);
+  message_loop_.RunAllPending();
+  ASSERT_TRUE(expected_props_.empty());
+}
+
+TEST_F(CrosSettingsTest, SetWhitelistWithListOps2) {
+  scoped_ptr<base::ListValue> whitelist(new base::ListValue);
+  scoped_ptr<base::Value> hacky_user(base::Value::CreateStringValue("h@xxor"));
+  scoped_ptr<base::Value> lamy_user(base::Value::CreateStringValue("l@mer"));
+  whitelist->Append(hacky_user->DeepCopy());
+  base::ListValue* expected_list = whitelist->DeepCopy();
+  whitelist->Append(lamy_user->DeepCopy());
+  AddExpectation(kAccountsPrefAllowNewUser,
+                 base::Value::CreateBooleanValue(false));
+  AddExpectation(kAccountsPrefUsers, whitelist->DeepCopy());
+  SetPref(kAccountsPrefUsers, whitelist.get());
+  FetchPref(kAccountsPrefAllowNewUser);
+  FetchPref(kAccountsPrefUsers);
+  message_loop_.RunAllPending();

[Mattias Nissler (ping if slow), 2011/12/02 12:13:37]

I think at this point we should also verify that all expectations are met. You
probably want to write a VerifyAndClearExpections() helper function.

[pastarmovj, 2011/12/02 14:43:38]

Put the proper assert here.

+  // Now try to remove one element from that list.
+  AddExpectation(kAccountsPrefUsers, expected_list);
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&CrosSettings::RemoveFromList,
+                 base::Unretained(CrosSettings::Get()),
+                 kAccountsPrefUsers, lamy_user.get()));
+  FetchPref(kAccountsPrefAllowNewUser);
+  FetchPref(kAccountsPrefUsers);
+  message_loop_.RunAllPending();
+  ASSERT_TRUE(expected_props_.empty());
+}
+
+TEST_F(CrosSettingsTest, SetEmptyWhitelist) {
+  // Setting the whitelist empty should switch the value of
+  // kAccountsPrefAllowNewUser to true.
+  scoped_ptr<base::ListValue> whitelist(new base::ListValue);
+  scoped_ptr<base::Value> disallow_new(base::Value::CreateBooleanValue(false));
+  AddExpectation(kAccountsPrefAllowNewUser,
+                 base::Value::CreateBooleanValue(true));
+  SetPref(kAccountsPrefUsers, whitelist.get());
+  SetPref(kAccountsPrefAllowNewUser, disallow_new.get());
+  FetchPref(kAccountsPrefAllowNewUser);
+  FetchPref(kAccountsPrefUsers);
+  message_loop_.RunAllPending();
+  ASSERT_TRUE(expected_props_.empty());
+}
+
+TEST_F(CrosSettingsTest, SetWhitelistAndNoNewUsers) {
+  // Setting the whitelist should allow us to set kAccountsPrefAllowNewUser to
+  // false (which is the implicit value too).
+  scoped_ptr<base::ListValue> whitelist(new base::ListValue);
+  whitelist->Append(base::Value::CreateStringValue("me@owner"));
+  AddExpectation(kAccountsPrefUsers, whitelist->DeepCopy());
+  AddExpectation(kAccountsPrefAllowNewUser,
+                 base::Value::CreateBooleanValue(false));
+  SetPref(kAccountsPrefUsers, whitelist.get());
+  SetPref(kAccountsPrefAllowNewUser,
+          expected_props_[kAccountsPrefAllowNewUser]);
+  FetchPref(kAccountsPrefAllowNewUser);
+  FetchPref(kAccountsPrefUsers);
+  message_loop_.RunAllPending();
+  ASSERT_TRUE(expected_props_.empty());
+}
+
+TEST_F(CrosSettingsTest, SetAllowNewUsers) {
+  // Setting kAccountsPrefAllowNewUser to true with no whitelist should be ok.
+  AddExpectation(kAccountsPrefAllowNewUser,
+                 base::Value::CreateBooleanValue(true));
+  SetPref(kAccountsPrefAllowNewUser,
+          expected_props_[kAccountsPrefAllowNewUser]);
+  FetchPref(kAccountsPrefAllowNewUser);
+  message_loop_.RunAllPending();
+  ASSERT_TRUE(expected_props_.empty());
+}
+
+TEST_F(CrosSettingsTest, SetOwner) {
+  scoped_ptr<base::Value> hacky_owner(base::Value::CreateStringValue("h@xxor"));
+  AddExpectation(kDeviceOwner, base::Value::CreateStringValue("h@xxor"));
+  SetPref(kDeviceOwner, hacky_owner.get());
+  FetchPref(kDeviceOwner);
+  message_loop_.RunAllPending();
+  ASSERT_TRUE(expected_props_.empty());
+}
+
+}  // namespace chromeos