Added ConfigurationPolicyProvider::RefreshPolicies.

chrome/browser/policy/cloud_policy_provider_unittest.cc

Index: chrome/browser/policy/cloud_policy_provider_unittest.cc
diff --git a/chrome/browser/policy/cloud_policy_provider_unittest.cc b/chrome/browser/policy/cloud_policy_provider_unittest.cc
index 1af50371ae893c5430b038b9a6e8f9a7761bef5d..1d0f059e65783da4844f4da7787bb4808a213dc8 100644
--- a/chrome/browser/policy/cloud_policy_provider_unittest.cc
+++ b/chrome/browser/policy/cloud_policy_provider_unittest.cc
@@ -6,13 +6,19 @@
 
 #include "base/basictypes.h"
 #include "base/values.h"
+#include "chrome/browser/browser_process.h"

[Mattias Nissler (ping if slow), 2011/11/21 16:09:05]

needed?

[Joao da Silva, 2011/11/21 16:49:21]

Not after fixing the g_browser_process below.

+#include "chrome/browser/policy/browser_policy_connector.h"
 #include "chrome/browser/policy/cloud_policy_cache_base.h"
 #include "chrome/browser/policy/cloud_policy_provider_impl.h"
 #include "chrome/browser/policy/configuration_policy_pref_store.h"
+#include "chrome/browser/policy/configuration_policy_provider.h"
+#include "chrome/browser/policy/mock_configuration_policy_provider.h"
 #include "policy/policy_constants.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
 using testing::AnyNumber;
+using testing::Mock;
+using testing::StrictMock;
 using testing::_;
 
 namespace policy {
@@ -23,15 +29,16 @@ class MockCloudPolicyCache : public CloudPolicyCacheBase {
   virtual ~MockCloudPolicyCache() {}
 
   // CloudPolicyCacheBase implementation.
-  void Load() {}
-  void SetPolicy(const em::PolicyFetchResponse& policy) {}
+  void Load() OVERRIDE {}
+  void SetPolicy(const em::PolicyFetchResponse& policy) OVERRIDE {}
   bool DecodePolicyData(const em::PolicyData& policy_data,
                         PolicyMap* mandatory,
-                        PolicyMap* recommended) {
+                        PolicyMap* recommended) OVERRIDE {
     return true;
   }
-  bool IsReady() {
-    return true;
+
+  void SetUnmanaged() OVERRIDE {
+    is_unmanaged_ = true;
   }
 
   // Non-const accessors for underlying PolicyMaps.
@@ -43,14 +50,6 @@ class MockCloudPolicyCache : public CloudPolicyCacheBase {
     return &recommended_policy_;
   }
 
-  void SetUnmanaged() {
-    is_unmanaged_ = true;
-  }
-
-  void SetFetchingDone() {
-    // Implement pure virtual method.
-  }
-
   void set_initialized(bool initialized) {
     initialization_complete_ = initialized;
   }
@@ -62,8 +61,11 @@ class MockCloudPolicyCache : public CloudPolicyCacheBase {
 class CloudPolicyProviderTest : public testing::Test {
  protected:
   void CreateCloudPolicyProvider(CloudPolicyCacheBase::PolicyLevel level) {
-    cloud_policy_provider_.reset(new CloudPolicyProviderImpl(
-        GetChromePolicyDefinitionList(), level));
+    cloud_policy_provider_.reset(
+        new CloudPolicyProviderImpl(
+            &browser_policy_connector_,
+            GetChromePolicyDefinitionList(),
+            level));
   }
 
   // Appends the caches to a provider and then provides the policies to
@@ -71,6 +73,7 @@ class CloudPolicyProviderTest : public testing::Test {
   void RunCachesThroughProvider(MockCloudPolicyCache caches[], int n,
                                 CloudPolicyCacheBase::PolicyLevel level) {
     CloudPolicyProviderImpl provider(
+        g_browser_process->browser_policy_connector(),

[Mattias Nissler (ping if slow), 2011/11/21 16:09:05]

Wait, shouldn't you use &browser_policy_connector_ here as well?

[Joao da Silva, 2011/11/21 16:49:21]

Yep, sloppy me. Fixed.

         GetChromePolicyDefinitionList(),
         level);
     for (int i = 0; i < n; i++) {
@@ -110,13 +113,10 @@ class CloudPolicyProviderTest : public testing::Test {
     cloud_policy_provider_->CombineTwoPolicyMaps(base, overlay, out_map);
   }
 
- private:
-  // Some tests need a list of policies that doesn't contain any proxy
-  // policies. Note: these policies will be handled as if they had the
-  // type of Value::TYPE_INTEGER.
-  static const ConfigurationPolicyType simple_policies[];
-
   scoped_ptr<CloudPolicyProviderImpl> cloud_policy_provider_;
+
+ private:
+  BrowserPolicyConnector browser_policy_connector_;
   scoped_ptr<PolicyMap> policy_map_;
 };
 
@@ -240,4 +240,67 @@ TEST_F(CloudPolicyProviderTest, CombineTwoPolicyMapsProxies) {
   EXPECT_FALSE(B.Equals(C));
 }
 
+TEST_F(CloudPolicyProviderTest, RefreshPolicies) {
+  CreateCloudPolicyProvider(CloudPolicyCacheBase::POLICY_LEVEL_MANDATORY);
+  StrictMock<MockConfigurationPolicyObserver> observer;

[Mattias Nissler (ping if slow), 2011/11/21 16:09:05]

Using a StrictMock to ensure that no other events occur? The googlemock guide
advises against that in favor of putting explicit EXPECT_CALL().Times(0) on the
functions, and I tend to agree, since StrictMock tends to overly restrict
acceptable behavior. Why would we want to make sure other notifications don't
happen?

[Joao da Silva, 2011/11/21 16:49:21]

Removed.

This was introduced when the test complained about some "uninteresting" calls
and I wanted to check where they came from. It turned out to be the caches going
out of scope, invoking OnCacheGoingAway and triggering 3 updates when the
function terminates.

+  ConfigurationPolicyObserverRegistrar registrar;
+  registrar.Init(cloud_policy_provider_.get(), &observer);
+
+  // OnUpdatePolicy is called when the provider doesn't have any caches.
+  EXPECT_CALL(observer, OnUpdatePolicy(cloud_policy_provider_.get())).Times(1);
+  cloud_policy_provider_->RefreshPolicies();
+  Mock::VerifyAndClearExpectations(&observer);
+
+  // OnUpdatePolicy is called when all the caches have updated.
+  EXPECT_CALL(observer, OnUpdatePolicy(cloud_policy_provider_.get())).Times(2);
+  MockCloudPolicyCache cache0;
+  MockCloudPolicyCache cache1;
+  MockCloudPolicyCache cache2;

[Mattias Nissler (ping if slow), 2011/11/21 16:09:05]

It's a bit weird to have the cache2 declaration here. Move all declarations up
to the top?

[Joao da Silva, 2011/11/21 16:49:21]

Done.

+  cloud_policy_provider_->AppendCache(&cache0);
+  cloud_policy_provider_->AppendCache(&cache1);
+  Mock::VerifyAndClearExpectations(&observer);
+
+  EXPECT_CALL(observer, OnUpdatePolicy(cloud_policy_provider_.get())).Times(0);
+  cloud_policy_provider_->RefreshPolicies();
+  Mock::VerifyAndClearExpectations(&observer);
+
+  EXPECT_CALL(observer, OnUpdatePolicy(cloud_policy_provider_.get())).Times(0);
+  // Updating just one of the caches is not enough.
+  cloud_policy_provider_->OnCacheUpdate(&cache0);
+  Mock::VerifyAndClearExpectations(&observer);
+
+  EXPECT_CALL(observer, OnUpdatePolicy(cloud_policy_provider_.get())).Times(0);
+  // This cache wasn't available when RefreshPolicies was called, so it isn't
+  // required to fire the update.

[Mattias Nissler (ping if slow), 2011/11/21 16:09:05]

Is that intended? Either way, it probably doesn't matter for the problem we're
trying to solve.

[Joao da Silva, 2011/11/21 16:49:21]

It doesn't matter indeed, this comment is for clarification.

If a new cache comes along while we are waiting for an update, the update
shouldn't be fired until the pending caches are all updated too. Notice that
this only happens when there is a pending update, so it will be triggered
"soon".

If adding a new cache triggered an update it would lead to flakiness, since the
caller of RefreshPolicies expects all the existing caches to reload, but the
update fired by the addition might come before the reloading is done, and any
policy modifications are visible.

+  cloud_policy_provider_->AppendCache(&cache2);
+  Mock::VerifyAndClearExpectations(&observer);
+
+  EXPECT_CALL(observer, OnUpdatePolicy(cloud_policy_provider_.get())).Times(1);
+  cloud_policy_provider_->OnCacheUpdate(&cache1);
+  Mock::VerifyAndClearExpectations(&observer);
+
+  EXPECT_CALL(observer, OnUpdatePolicy(cloud_policy_provider_.get())).Times(0);
+  cloud_policy_provider_->RefreshPolicies();
+  Mock::VerifyAndClearExpectations(&observer);
+
+  EXPECT_CALL(observer, OnUpdatePolicy(cloud_policy_provider_.get())).Times(0);
+  cloud_policy_provider_->OnCacheUpdate(&cache0);
+  cloud_policy_provider_->OnCacheUpdate(&cache1);
+  Mock::VerifyAndClearExpectations(&observer);
+
+  // If a cache refreshes more than once, the provider should still waits for

[Mattias Nissler (ping if slow), 2011/11/21 16:09:05]

s/waits/wait/

[Joao da Silva, 2011/11/21 16:49:21]

Done.

+  // the others before firing the update.
+  EXPECT_CALL(observer, OnUpdatePolicy(cloud_policy_provider_.get())).Times(0);
+  cloud_policy_provider_->OnCacheUpdate(&cache0);
+  Mock::VerifyAndClearExpectations(&observer);
+
+  // Fire updates if one of the required caches goes away while waiting.
+  EXPECT_CALL(observer, OnUpdatePolicy(cloud_policy_provider_.get())).Times(1);
+  cloud_policy_provider_->OnCacheGoingAway(&cache2);
+  Mock::VerifyAndClearExpectations(&observer);
+
+  // The other 2 caches are removed (and trigger updates) at shutdown.
+  EXPECT_CALL(observer, OnUpdatePolicy(cloud_policy_provider_.get())).Times(2);
+}
+
 }  // namespace policy