Signed settings refactoring: Proper caching and more tests.

chrome/browser/chromeos/login/signed_settings_helper_unittest.cc

Index: chrome/browser/chromeos/login/signed_settings_helper_unittest.cc
diff --git a/chrome/browser/chromeos/login/signed_settings_helper_unittest.cc b/chrome/browser/chromeos/login/signed_settings_helper_unittest.cc
index a808d770a30a967294ef057575050c6c0cc57ec1..5b04b55469952c886af7254b06e99279aceb5a97 100644
--- a/chrome/browser/chromeos/login/signed_settings_helper_unittest.cc
+++ b/chrome/browser/chromeos/login/signed_settings_helper_unittest.cc
@@ -4,9 +4,13 @@
 
 #include "chrome/browser/chromeos/login/signed_settings_helper.h"
 
+#include "base/bind.h"
 #include "chrome/browser/chromeos/cros/cros_library.h"
 #include "chrome/browser/chromeos/cros_settings_names.h"
 #include "chrome/browser/chromeos/dbus/dbus_thread_manager.h"
+#include "chrome/browser/chromeos/dbus/mock_dbus_thread_manager.h"
+#include "chrome/browser/chromeos/dbus/mock_session_manager_client.h"
+#include "chrome/browser/chromeos/login/mock_owner_key_utils.h"
 #include "chrome/browser/chromeos/login/mock_ownership_service.h"
 #include "chrome/browser/chromeos/login/owner_manager.h"
 #include "chrome/browser/chromeos/login/signed_settings.h"
@@ -16,51 +20,53 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using content::BrowserThread;
 using ::testing::_;
 using ::testing::A;
-using ::testing::AtLeast;
 using ::testing::InSequence;
 using ::testing::Invoke;
-using ::testing::Return;
-using ::testing::ReturnRef;
-using ::testing::SaveArg;
 using ::testing::WithArg;
 
 namespace em = enterprise_management;
 namespace chromeos {
 
-class MockSignedSettingsHelperCallback : public SignedSettingsHelper::Callback {
- public:
-  virtual ~MockSignedSettingsHelperCallback() {}
-
-  MOCK_METHOD3(OnStorePropertyCompleted, void(
-      SignedSettings::ReturnCode code,
-      const std::string& name,
-      const base::Value& value));
-  MOCK_METHOD3(OnRetrievePropertyCompleted, void(
-      SignedSettings::ReturnCode code,
-      const std::string& name,
-      const base::Value* value));
-};
+ACTION_P(Retrieve, policy_blob) { arg0.Run(policy_blob); }
+ACTION_P(Store, success) { arg1.Run(success); }
 
 class SignedSettingsHelperTest : public testing::Test,
                                  public SignedSettingsHelper::TestDelegate {
  public:
   SignedSettingsHelperTest()
-      : fake_email_("fakey@example.com"),
-        fake_prop_(kReleaseChannel),
-        fake_value_("false"),
-        message_loop_(MessageLoop::TYPE_UI),
-        ui_thread_(BrowserThread::UI, &message_loop_),
-        file_thread_(BrowserThread::FILE),
-        pending_ops_(0) {
+      : message_loop_(MessageLoop::TYPE_UI),
+        ui_thread_(content::BrowserThread::UI, &message_loop_),
+        file_thread_(content::BrowserThread::FILE, &message_loop_),
+        pending_ops_(0),
+        mock_dbus_thread_manager_(new MockDBusThreadManager) {
   }
 
   virtual void SetUp() {
-    file_thread_.Start();
     SignedSettingsHelper::Get()->set_test_delegate(this);
-    DBusThreadManager::Initialize();
+    DBusThreadManager::InitializeForTesting(mock_dbus_thread_manager_);
+
+    fake_policy_data_ = BuildPolicyData();
+    std::string data_serialized = fake_policy_data_.SerializeAsString();
+    std::string serialized_policy_;
+    fake_policy_ = BuildProto(data_serialized,
+                              std::string("false"),
+                              &serialized_policy_);
+
+    MockSessionManagerClient* client =
+        mock_dbus_thread_manager_->mock_session_manager_client();
+    EXPECT_CALL(*client, StorePolicy(_, _))
+        .WillRepeatedly(Store(true));

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

Can this be inlined with something along the lines of WithArgs<1,
2>(Invoke(base::Closure::Run))?

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

Couldn't get this running with some gmock voodoo but at least I commented what
the idea of this code is.

+    EXPECT_CALL(*client, RetrievePolicy(_))
+        .WillRepeatedly(Retrieve(serialized_policy_));
+
+    EXPECT_CALL(m_, StartSigningAttempt(_, A<OwnerManager::Delegate*>()))
+        .WillRepeatedly(WithArg<1>(
+            Invoke(&SignedSettingsHelperTest::OnKeyOpComplete)));
+    EXPECT_CALL(m_, StartVerifyAttempt(_, _, A<OwnerManager::Delegate*>()))
+        .WillRepeatedly(WithArg<2>(
+            Invoke(&SignedSettingsHelperTest::OnKeyOpComplete)));
   }
 
   virtual void TearDown() {
@@ -78,8 +84,6 @@ class SignedSettingsHelperTest : public testing::Test,
 
   virtual void OnOpCompleted(SignedSettings* op) {
     --pending_ops_;
-    if (!pending_ops_)
-      MessageLoop::current()->Quit();
   }
 
   static void OnKeyOpComplete(OwnerManager::Delegate* op) {
@@ -89,14 +93,26 @@ class SignedSettingsHelperTest : public testing::Test,
   em::PolicyData BuildPolicyData() {
     em::PolicyData to_return;
     em::ChromeDeviceSettingsProto pol;
-    to_return.set_policy_type(SignedSettings::kDevicePolicyType);
+    to_return.set_policy_type(chromeos::kDevicePolicyType);
     to_return.set_policy_value(pol.SerializeAsString());
     return to_return;
   }
 
-  const std::string fake_email_;
-  const std::string fake_prop_;
-  const base::StringValue fake_value_;
+  em::PolicyFetchResponse BuildProto(const std::string& data,
+                                     const std::string& sig,
+                                     std::string* out_serialized) {
+    em::PolicyFetchResponse fake_policy;
+    if (!data.empty())
+      fake_policy.set_policy_data(data);
+    if (!sig.empty())
+      fake_policy.set_policy_data_signature(sig);
+    EXPECT_TRUE(fake_policy.SerializeToString(out_serialized));
+    return fake_policy;
+  }
+
+  em::PolicyData fake_policy_data_;
+  em::PolicyFetchResponse fake_policy_;
+  std::string serialized_policy_;
   MockOwnershipService m_;
 
   MessageLoop message_loop_;
@@ -105,83 +121,86 @@ class SignedSettingsHelperTest : public testing::Test,
 
   int pending_ops_;
 
+  MockDBusThreadManager* mock_dbus_thread_manager_;
+
   ScopedStubCrosEnabler stub_cros_enabler_;
 };
 
+class SignedSettingsCallbacks {
+ public:
+  virtual ~SignedSettingsCallbacks() {}
+  // Callback of StorePolicyOp.
+  virtual void OnStorePolicyCompleted(SignedSettings::ReturnCode code) = 0;
+  // Callback of RetrievePolicyOp.
+  virtual void OnRetrievePolicyCompleted(
+      SignedSettings::ReturnCode code,
+      const em::PolicyFetchResponse& policy) = 0;
+};
+
+class MockSignedSettingsCallbacks
+    : public SignedSettingsCallbacks,
+      public base::SupportsWeakPtr<MockSignedSettingsCallbacks> {
+public:
+  virtual ~MockSignedSettingsCallbacks() {}
+
+  MOCK_METHOD1(OnStorePolicyCompleted, void(SignedSettings::ReturnCode));
+  MOCK_METHOD2(OnRetrievePolicyCompleted, void(SignedSettings::ReturnCode,
+                                               const em::PolicyFetchResponse&));
+};
+
 TEST_F(SignedSettingsHelperTest, SerializedOps) {
-  MockSignedSettingsHelperCallback cb;
-
-  EXPECT_CALL(m_, GetStatus(_))
-      .Times(2)
-      .WillRepeatedly(Return(OwnershipService::OWNERSHIP_TAKEN));
-  EXPECT_CALL(m_, has_cached_policy())
-      .Times(2)
-      .WillRepeatedly(Return(true));
-  em::PolicyData fake_pol = BuildPolicyData();
-  EXPECT_CALL(m_, cached_policy())
-      .Times(2)
-      .WillRepeatedly(ReturnRef(fake_pol));
-  EXPECT_CALL(m_, set_cached_policy(A<const em::PolicyData&>()))
-      .Times(1)
-      .WillRepeatedly(SaveArg<0>(&fake_pol));
+  MockSignedSettingsCallbacks cb;
 
   InSequence s;
-  EXPECT_CALL(m_, StartSigningAttempt(_, A<OwnerManager::Delegate*>()))
-      .WillOnce(WithArg<1>(Invoke(&SignedSettingsHelperTest::OnKeyOpComplete)));
-  EXPECT_CALL(cb, OnStorePropertyCompleted(SignedSettings::SUCCESS, _, _))
+  EXPECT_CALL(cb, OnStorePolicyCompleted(SignedSettings::SUCCESS))
       .Times(1);
-
-  EXPECT_CALL(cb, OnRetrievePropertyCompleted(SignedSettings::SUCCESS, _, _))
+  EXPECT_CALL(cb, OnRetrievePolicyCompleted(SignedSettings::SUCCESS, _))
       .Times(1);
 
 
   pending_ops_ = 2;
-  SignedSettingsHelper::Get()->StartStorePropertyOp(fake_prop_, fake_value_,
-      &cb);
-  SignedSettingsHelper::Get()->StartRetrieveProperty(fake_prop_, &cb);
-
-  message_loop_.Run();
+  SignedSettingsHelper::Get()->StartStorePolicyOp(
+      fake_policy_,
+      base::Bind(&MockSignedSettingsCallbacks::OnStorePolicyCompleted,
+                 base::Unretained(&cb)));
+  SignedSettingsHelper::Get()->StartRetrievePolicyOp(
+      base::Bind(&MockSignedSettingsCallbacks::OnRetrievePolicyCompleted,
+                 base::Unretained(&cb)));
+
+  message_loop_.RunAllPending();
+  ASSERT_EQ(0, pending_ops_);
 }
 
 TEST_F(SignedSettingsHelperTest, CanceledOps) {
-  MockSignedSettingsHelperCallback cb;
-
-  EXPECT_CALL(m_, GetStatus(_))
-      .Times(3)
-      .WillRepeatedly(Return(OwnershipService::OWNERSHIP_TAKEN));
-  EXPECT_CALL(m_, has_cached_policy())
-      .Times(3)
-      .WillRepeatedly(Return(true));
-  em::PolicyData fake_pol = BuildPolicyData();
-  EXPECT_CALL(m_, cached_policy())
-      .Times(3)
-      .WillRepeatedly(ReturnRef(fake_pol));
-  EXPECT_CALL(m_, set_cached_policy(A<const em::PolicyData&>()))
-      .Times(1)
-      .WillRepeatedly(SaveArg<0>(&fake_pol));
+  MockSignedSettingsCallbacks cb;
 
   InSequence s;
-
-  // RetrievePropertyOp for cb_to_be_canceled still gets executed but callback
-  // does not happen.
-  EXPECT_CALL(m_, StartSigningAttempt(_, A<OwnerManager::Delegate*>()))
-      .WillOnce(WithArg<1>(Invoke(&SignedSettingsHelperTest::OnKeyOpComplete)));
-  EXPECT_CALL(cb, OnStorePropertyCompleted(SignedSettings::SUCCESS, _, _))
+  EXPECT_CALL(cb, OnStorePolicyCompleted(SignedSettings::SUCCESS))
       .Times(1);
-  EXPECT_CALL(cb, OnRetrievePropertyCompleted(SignedSettings::SUCCESS, _, _))
+  EXPECT_CALL(cb, OnRetrievePolicyCompleted(SignedSettings::SUCCESS, _))
       .Times(1);
 
   pending_ops_ = 3;
-  MockSignedSettingsHelperCallback cb_to_be_canceled;
-  SignedSettingsHelper::Get()->StartRetrieveProperty(fake_prop_,
-                                                     &cb_to_be_canceled);
-  SignedSettingsHelper::Get()->CancelCallback(&cb_to_be_canceled);
 
-  SignedSettingsHelper::Get()->StartStorePropertyOp(fake_prop_, fake_value_,
-      &cb);
-  SignedSettingsHelper::Get()->StartRetrieveProperty(fake_prop_, &cb);
-
-  message_loop_.Run();
+  {
+    // This op will be deleted and never will be executed (expect only one call
+    // to OnRetrievePolicyCompleted above). However the OpComplete callback will
+    // be still called, therefore we expect three pending ops.
+    MockSignedSettingsCallbacks cb_to_be_deleted;
+    SignedSettingsHelper::Get()->StartRetrievePolicyOp(
+        base::Bind(&MockSignedSettingsCallbacks::OnRetrievePolicyCompleted,
+                   cb_to_be_deleted.AsWeakPtr()));
+  }
+  SignedSettingsHelper::Get()->StartStorePolicyOp(
+      fake_policy_,
+      base::Bind(&MockSignedSettingsCallbacks::OnStorePolicyCompleted,
+                 base::Unretained(&cb)));
+  SignedSettingsHelper::Get()->StartRetrievePolicyOp(
+      base::Bind(&MockSignedSettingsCallbacks::OnRetrievePolicyCompleted,
+                 base::Unretained(&cb)));
+
+  message_loop_.RunAllPending();
+  ASSERT_EQ(0, pending_ops_);
 }
 
 }  // namespace chromeos