Added ConfigurationPolicyProvider::RefreshPolicies.

chrome/browser/policy/cloud_policy_provider_unittest.cc

 // 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/policy/cloud_policy_provider.h"
 
 #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 {
 
 class MockCloudPolicyCache : public CloudPolicyCacheBase {
  public:
   MockCloudPolicyCache() {}
   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) {
-    return true;
-  }
-  bool IsReady() {
+                        PolicyMap* recommended) OVERRIDE {
     return true;
   }
 
+  void SetUnmanaged() OVERRIDE {
+    is_unmanaged_ = true;
+  }
+
   // Non-const accessors for underlying PolicyMaps.
   PolicyMap* raw_mandatory_policy() {
     return &mandatory_policy_;
   }
 
   PolicyMap* raw_recommended_policy() {
     return &recommended_policy_;
   }
 
-  void SetUnmanaged() {
-    is_unmanaged_ = true;
-  }
-
-  void SetFetchingDone() {
-    // Implement pure virtual method.
-  }
-
   void set_initialized(bool initialized) {
     initialization_complete_ = initialized;
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockCloudPolicyCache);
 };
 
 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
   // |policy_map_|.
   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++) {
       provider.AppendCache(&caches[i]);
     }
     policy_map_.reset(new PolicyMap());
     provider.Provide(policy_map_.get());
   }
 
   // Checks a string policy in |policy_map_|.
@@ skipping 19 matching lines @@
     EXPECT_TRUE(NULL == policy_map_->Get(type));
   }
 
   void CombineTwoPolicyMaps(const PolicyMap& base,
                             const PolicyMap& overlay,
                             PolicyMap* out_map) {
     DCHECK(cloud_policy_provider_.get());
     cloud_policy_provider_->CombineTwoPolicyMaps(base, overlay, out_map);
   }
 
+  scoped_ptr<CloudPolicyProviderImpl> cloud_policy_provider_;
+
  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_;
+  BrowserPolicyConnector browser_policy_connector_;
   scoped_ptr<PolicyMap> policy_map_;
 };
 
 // Proxy setting distributed over multiple caches.
 TEST_F(CloudPolicyProviderTest,
        ProxySettingDistributedOverMultipleCaches) {
   // There are proxy_policy_count()+1 = 6 caches and they are mixed together by
   // one instance of CloudPolicyProvider. The first cache has some policies but
   // no proxy-related ones. The following caches have each one proxy-policy set.
   const int n = 6;
@@ skipping 103 matching lines @@
   B.Set(kPolicyProxyServer, Value::CreateIntegerValue(b_value));
   B.Set(kPolicyProxyPacUrl, Value::CreateIntegerValue(b_value));
   B.Set(kPolicyProxyBypassList, Value::CreateIntegerValue(b_value));
 
   CombineTwoPolicyMaps(A, B, &C);
 
   EXPECT_TRUE(A.Equals(C));
   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