Introduced a ForwardingPolicyProvider.

chrome/browser/policy/forwarding_policy_provider_unittest.cc

+// Copyright (c) 2013 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/forwarding_policy_provider.h"
+
+#include "base/basictypes.h"

[bartfab (slow), 2013/11/11 14:40:08]

Nit: Already included by header file.

[Joao da Silva, 2013/11/12 15:26:33]

Done.

+#include "base/values.h"
+#include "chrome/browser/policy/mock_configuration_policy_provider.h"
+#include "chrome/browser/policy/policy_bundle.h"
+#include "chrome/browser/policy/policy_map.h"
+#include "chrome/browser/policy/policy_types.h"
+#include "chrome/browser/policy/schema_registry.h"
+#include "components/policy/core/common/policy_namespace.h"
+#include "components/policy/core/common/schema.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::_;
+using testing::Mock;
+using testing::Return;
+
+namespace policy {
+
+class ForwardingPolicyProviderTest : public testing::Test {

[bartfab (slow), 2013/11/11 14:40:08]

Nit: The style guide forbids inline method definitions, except for "trivial or
performance-critical, and very short, methods." It is a common misconception
that some kind of exception to this rule applies in tests.

[Joao da Silva, 2013/11/12 15:26:33]

There is an exception for tests. I dug the latest thread where this topic was
discussed:

https://groups.google.com/a/chromium.org/d/topic/chromium-dev/X8D176Uzp6w/dis...

The conclusion was added to a companion doc of the style guide:

http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-What-ab...

"For testing code, most of these factors are reduced or eliminated.  Test
framework classes don't tend to be instantiated separately and passed around as
objects; they're effectively just bundles of file-scope functionality coupled
with a mechanism to reset state between tests.  In these cases, defining the
test functions inline at their declaration sites has little negative effect and
can reduce the amount of "boilerplate" in the test file.  Ultimately, test file
inlining should be a readability-focused judgment call on the part of the test
author and reviewer."

I'm on the side that finds inlining in tests more readable, so I'd prefer to
keep this. I don't feel strongly though, if you prefer then I'll change it.

[bartfab (slow), 2013/11/13 11:56:35]

Interesting. I had followed a different thread that concluded there was no valid
exception :). But I do not feel strongly either way.

+ protected:
+  ForwardingPolicyProviderTest() : forwarding_provider_(&mock_provider_) {
+    mock_provider_.Init();
+    forwarding_provider_.Init(&schema_registry_);
+    forwarding_provider_.AddObserver(&observer_);
+  }
+
+  virtual ~ForwardingPolicyProviderTest() {
+    forwarding_provider_.RemoveObserver(&observer_);
+    forwarding_provider_.Shutdown();
+    mock_provider_.Shutdown();
+  }
+
+  SchemaRegistry schema_registry_;
+  MockConfigurationPolicyObserver observer_;
+  MockConfigurationPolicyProvider mock_provider_;
+  ForwardingPolicyProvider forwarding_provider_;
+};
+
+TEST_F(ForwardingPolicyProviderTest, Empty) {
+  EXPECT_FALSE(schema_registry_.IsReady());
+  EXPECT_FALSE(
+      forwarding_provider_.IsInitializationComplete(POLICY_DOMAIN_EXTENSIONS));
+
+  EXPECT_CALL(mock_provider_, IsInitializationComplete(POLICY_DOMAIN_CHROME))
+      .WillOnce(Return(false));
+  EXPECT_FALSE(
+      forwarding_provider_.IsInitializationComplete(POLICY_DOMAIN_CHROME));
+  Mock::VerifyAndClearExpectations(&mock_provider_);
+
+  const PolicyBundle empty_bundle;
+  EXPECT_TRUE(forwarding_provider_.policies().Equals(empty_bundle));
+}
+
+TEST_F(ForwardingPolicyProviderTest, ForwardsChromePolicy) {
+  PolicyBundle bundle;
+  PolicyNamespace chrome_ns(POLICY_DOMAIN_CHROME, "");

[bartfab (slow), 2013/11/11 14:40:08]

Nit: const - or simply construct it inline.

[Joao da Silva, 2013/11/12 15:26:33]

Done.

+  bundle.Get(chrome_ns).Set("policy", POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
+                            base::Value::CreateStringValue("value"), NULL);
+
+  EXPECT_CALL(observer_, OnUpdatePolicy(&forwarding_provider_));
+  scoped_ptr<PolicyBundle> copy(new PolicyBundle);

[bartfab (slow), 2013/11/11 14:40:08]

Nit 1: I would not call this |copy| because its main function is not to be an
identical copy but a policy bundle that gets sent to the provider. It is
specifically *not* an identical copy of |bundle| once you add the component
policy.

Nit 2: #include "base/memory/scoped_ptr.h"

[Joao da Silva, 2013/11/12 15:26:33]

Done.

+  copy->CopyFrom(bundle);
+  copy->Get(PolicyNamespace(POLICY_DOMAIN_EXTENSIONS, "xyz"))
+      .Set("component policy", POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
+           base::Value::CreateStringValue("not forwarded"), NULL);
+  mock_provider_.UpdatePolicy(copy.Pass());
+  Mock::VerifyAndClearExpectations(&observer_);
+
+  EXPECT_FALSE(
+      forwarding_provider_.IsInitializationComplete(POLICY_DOMAIN_EXTENSIONS));
+  EXPECT_TRUE(forwarding_provider_.policies().Equals(bundle));
+}
+
+TEST_F(ForwardingPolicyProviderTest, RefreshPolicies) {
+  EXPECT_CALL(mock_provider_, RefreshPolicies());
+  forwarding_provider_.RefreshPolicies();
+  Mock::VerifyAndClearExpectations(&mock_provider_);
+}
+
+TEST_F(ForwardingPolicyProviderTest, SchemaReady) {
+  EXPECT_CALL(observer_, OnUpdatePolicy(&forwarding_provider_));
+  schema_registry_.SetReady(POLICY_DOMAIN_CHROME);
+  schema_registry_.SetReady(POLICY_DOMAIN_EXTENSIONS);
+  Mock::VerifyAndClearExpectations(&observer_);
+
+  EXPECT_TRUE(forwarding_provider_.IsInitializationComplete(
+      policy::POLICY_DOMAIN_EXTENSIONS));
+}
+
+TEST_F(ForwardingPolicyProviderTest, SchemaReadyWithComponents) {
+  PolicyMap policy_map;
+  policy_map.Set("foo", POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
+                 base::Value::CreateStringValue("omg"), NULL);
+  scoped_ptr<PolicyBundle> bundle(new PolicyBundle);
+  bundle->Get(PolicyNamespace(POLICY_DOMAIN_CHROME, "")).CopyFrom(policy_map);
+  bundle->Get(PolicyNamespace(POLICY_DOMAIN_EXTENSIONS, "xyz"))
+      .CopyFrom(policy_map);
+  EXPECT_CALL(observer_, OnUpdatePolicy(&forwarding_provider_));
+  mock_provider_.UpdatePolicy(bundle.Pass());
+  Mock::VerifyAndClearExpectations(&observer_);
+
+  EXPECT_CALL(mock_provider_, RefreshPolicies());
+  schema_registry_.SetReady(POLICY_DOMAIN_CHROME);

[bartfab (slow), 2013/11/11 14:40:08]

Nit: You could additionally verify that this SetReady() alone does not cause a
RefreshPolicies().

[Joao da Silva, 2013/11/12 15:26:33]

Done.

+  schema_registry_.RegisterComponent(
+      PolicyNamespace(POLICY_DOMAIN_EXTENSIONS, "xyz"), Schema());
+  schema_registry_.SetReady(POLICY_DOMAIN_EXTENSIONS);
+  Mock::VerifyAndClearExpectations(&mock_provider_);
+
+  EXPECT_FALSE(forwarding_provider_.IsInitializationComplete(
+      policy::POLICY_DOMAIN_EXTENSIONS));
+  PolicyBundle expected_bundle;
+  expected_bundle.Get(PolicyNamespace(POLICY_DOMAIN_CHROME, ""))
+      .CopyFrom(policy_map);
+  EXPECT_TRUE(forwarding_provider_.policies().Equals(expected_bundle));
+
+  EXPECT_CALL(observer_, OnUpdatePolicy(&forwarding_provider_));
+  forwarding_provider_.OnUpdatePolicy(&mock_provider_);
+  Mock::VerifyAndClearExpectations(&observer_);
+
+  EXPECT_TRUE(forwarding_provider_.IsInitializationComplete(
+      policy::POLICY_DOMAIN_EXTENSIONS));
+  expected_bundle.Get(PolicyNamespace(POLICY_DOMAIN_EXTENSIONS, "xyz"))
+      .CopyFrom(policy_map);
+  EXPECT_TRUE(forwarding_provider_.policies().Equals(expected_bundle));
+}
+
+TEST_F(ForwardingPolicyProviderTest, DelegateUpdates) {
+  schema_registry_.RegisterComponent(
+      PolicyNamespace(POLICY_DOMAIN_EXTENSIONS, "xyz"), Schema());
+  EXPECT_FALSE(schema_registry_.IsReady());
+  EXPECT_FALSE(forwarding_provider_.IsInitializationComplete(
+      policy::POLICY_DOMAIN_EXTENSIONS));
+
+  PolicyMap policy_map;
+  policy_map.Set("foo", POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
+                 base::Value::CreateStringValue("omg"), NULL);
+  // Chrome policy updates are forwarded even if the components aren't ready.
+  EXPECT_CALL(observer_, OnUpdatePolicy(&forwarding_provider_));
+  mock_provider_.UpdateChromePolicy(policy_map);
+  Mock::VerifyAndClearExpectations(&observer_);
+
+  EXPECT_CALL(mock_provider_, RefreshPolicies());
+  schema_registry_.SetReady(POLICY_DOMAIN_CHROME);
+  schema_registry_.SetReady(POLICY_DOMAIN_EXTENSIONS);
+  EXPECT_TRUE(schema_registry_.IsReady());
+  Mock::VerifyAndClearExpectations(&mock_provider_);
+  EXPECT_FALSE(forwarding_provider_.IsInitializationComplete(
+      policy::POLICY_DOMAIN_EXTENSIONS));
+
+  // The forwarding provider becomes ready after this refresh completes, and
+  // starts forwarding policy updates after that.
+  EXPECT_CALL(observer_, OnUpdatePolicy(_));
+  mock_provider_.UpdateChromePolicy(policy_map);
+  Mock::VerifyAndClearExpectations(&observer_);
+
+  EXPECT_TRUE(forwarding_provider_.IsInitializationComplete(
+      policy::POLICY_DOMAIN_EXTENSIONS));
+
+  // Keeps forwarding from now on.

[bartfab (slow), 2013/11/11 14:40:08]

Nit: No need for "from now on" - the forwarding was happening before already.

[Joao da Silva, 2013/11/12 15:26:33]

Done.

+  EXPECT_CALL(observer_, OnUpdatePolicy(_));
+  mock_provider_.UpdateChromePolicy(policy_map);
+  Mock::VerifyAndClearExpectations(&observer_);
+}
+
+}  // namespace policy