Using PolicyServiceWatcher instead of PolicyWatcherLinux/Win/Mac.

remoting/host/policy_hack/policy_watcher_unittest.cc

Index: remoting/host/policy_hack/policy_watcher_unittest.cc
diff --git a/remoting/host/policy_hack/policy_watcher_unittest.cc b/remoting/host/policy_hack/policy_watcher_unittest.cc
index cb9415485e8bc961f1d9678cfa0f2d9cbaa65a09..6c7d385d95ae8228020541168260ca0af3dfa69f 100644
--- a/remoting/host/policy_hack/policy_watcher_unittest.cc
+++ b/remoting/host/policy_hack/policy_watcher_unittest.cc
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <iostream>
+
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
@@ -19,8 +21,7 @@ namespace policy_hack {
 
 class PolicyWatcherTest : public testing::Test {
  public:
-  PolicyWatcherTest() {
-  }
+  PolicyWatcherTest() : message_loop_(base::MessageLoop::TYPE_IO) {}
 
   void SetUp() override {
     message_loop_proxy_ = base::MessageLoopProxy::current();
@@ -448,5 +449,78 @@ TEST_F(PolicyWatcherTest, PolicyUpdateResetsTransientErrorsCounter) {
   StopWatching();
 }
 
+// Unit tests cannot instantiate PolicyWatcher on ChromeOS
+// (as this requires running inside a browser process).

[Mattias Nissler (ping if slow), 2015/01/06 09:06:12]

You could create a fake PolicyService and test against that perhaps?

[Łukasz Anforowicz, 2015/01/07 17:54:15]

If PolicyServiceWatcher is here for the long term, then I definitely need/want
to add unit tests (mostly for its logic that deals with translating a PolicyMap
into a DictionaryValue, but maybe for the update/initialized callbacks logic as
well).  For now - I am not sure if PolicyServiceWatcher will stay (i.e. maybe we
should replace DictionaryValue with passing a thread-safe PolicyMap instead). 
Also - the code for translating PolicyMap into DictionaryValue has already
existed in PolicyWatcherChromeOS before my changes (and it didn't have unit
tests), so I am not making things any worse in this changelist.

[Mattias Nissler (ping if slow), 2015/01/08 09:58:36]

OK, fair enough.

+#ifndef OS_CHROMEOS
+
+namespace {
+
+void OnPolicyUpdatedDumpPolicy(scoped_ptr<base::DictionaryValue> policies) {
+  if (!VLOG_IS_ON(1)) {
+    return;
+  }
+
+  std::cout << "OnPolicyUpdated callback received the following policies:"
+            << std::endl;
+
+  for (base::DictionaryValue::Iterator iter(*policies); !iter.IsAtEnd();
+       iter.Advance()) {
+    std::cout << iter.key() << " = ";
+    switch (iter.value().GetType()) {
+      case base::Value::Type::TYPE_STRING: {
+        std::string value;
+        CHECK(iter.value().GetAsString(&value));
+        std::cout << "string: " << '"' << value << '"';
+        break;
+      }
+      case base::Value::Type::TYPE_BOOLEAN: {
+        bool value;
+        CHECK(iter.value().GetAsBoolean(&value));
+        std::cout << "boolean: " << (value ? "True" : "False");
+        break;
+      }
+      default: {
+        std::cout << "unrecognized type";
+        break;
+      }
+    }
+    std::cout << std::endl;
+  }
+}
+
+}  // anonymous namespace
+
+// To dump policy contents, run unit tests with the following flags:
+// --gtest_filter=*TestRealChromotingPolicy* -v=1
+TEST_F(PolicyWatcherTest, TestRealChromotingPolicy) {
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner =
+      base::MessageLoop::current()->task_runner();
+  scoped_ptr<PolicyWatcher> policy_watcher(
+      PolicyWatcher::Create(nullptr, task_runner));
+
+  {
+    base::RunLoop run_loop;
+    policy_watcher->StartWatching(base::Bind(OnPolicyUpdatedDumpPolicy),
+                                  base::Bind(base::DoNothing));
+    run_loop.RunUntilIdle();
+  }
+
+  {
+    base::RunLoop run_loop;
+    PolicyWatcher* raw_policy_watcher = policy_watcher.release();
+    raw_policy_watcher->StopWatching(
+        base::Bind(base::IgnoreResult(
+                       &base::SequencedTaskRunner::DeleteSoon<PolicyWatcher>),
+                   task_runner, FROM_HERE, raw_policy_watcher));
+    run_loop.RunUntilIdle();
+  }
+
+  // Today, the only verification offered by this test is:
+  // - Manual verification of policy values dumped by OnPolicyUpdatedDumpPolicy
+  // - Automated verification that nothing crashed

[Mattias Nissler (ping if slow), 2015/01/06 09:06:12]

It may make sense to add a test that tests PolicyServiceWatcher against a fake
ConfigurationPolicyProvider?

[Łukasz Anforowicz, 2015/01/07 17:54:15]

Yes, but see my other comment for explanation why I didn't do it in the current
changelist.

Also - the new test is still useful - it let's me do ad-hoc/manual validation of
the values present on my Linux/Mac/Windows + it at least proves that the
actual/concrete readers + PolicyServiceWatcher do not crash when reading an
actual policy.

[Mattias Nissler (ping if slow), 2015/01/08 09:58:36]

OK, might still want to document proposed improvements in the comment.

[Łukasz Anforowicz, 2015/01/08 23:09:26]

I thought about it and the test against a MockConfigurationPolicyProvider will
be useful in the long-term even if PolicyServiceWatcher goes away (that might
mean the test has to be refactored, but this is true for the other already
existing tests as well).   OTOH, I think I'll keep the current changelist as-is
(it just preserves the test coverage we have today) and try to add the test in
the future.  I've actually tried adding this test today but backed out because
1) it forces changes to the interface of PolicyServiceWatcher (to expose
CreateFromPolicyLoader + GetChromotingPolicyNamespace) and 2) I am not sure if I
want to test via the existing MockConfigurationPolicyProvider VS new
FakeAsyncPolicyLoader (the latter one will give me slightly more coverage,
especially around which schema gets used by PolicyServiceWatcher) and 3) the
test got quite big after I had to debug some silly mistakes and added full-blown
dictionary matcher for GMock.

+}
+
+#endif
+
 }  // namespace policy_hack
 }  // namespace remoting