Add NetworkHandler to own network handlers in src/chromeos/network

chromeos/network/managed_network_configuration_handler_unittest.cc

Index: chromeos/network/managed_network_configuration_handler_unittest.cc
diff --git a/chromeos/network/managed_network_configuration_handler_unittest.cc b/chromeos/network/managed_network_configuration_handler_unittest.cc
index 3c1780952831c2c44bb476e32aa6e12ccc6c3747..4a1aa9d076f0bf6970601ca6d101b49cd3b5747f 100644
--- a/chromeos/network/managed_network_configuration_handler_unittest.cc
+++ b/chromeos/network/managed_network_configuration_handler_unittest.cc
@@ -15,7 +15,7 @@
 #include "chromeos/dbus/mock_shill_service_client.h"
 #include "chromeos/dbus/shill_profile_client_stub.h"
 #include "chromeos/network/network_configuration_handler.h"
-#include "chromeos/network/network_profile_handler_stub.h"
+#include "chromeos/network/network_profile_handler.h"
 #include "chromeos/network/onc/onc_test_utils.h"
 #include "chromeos/network/onc/onc_utils.h"
 #include "dbus/object_path.h"
@@ -23,6 +23,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
+using ::testing::AnyNumber;
 using ::testing::Invoke;
 using ::testing::Mock;
 using ::testing::Pointee;
@@ -145,6 +146,8 @@ class ManagedNetworkConfigurationHandlerTest : public testing::Test {
         .WillRepeatedly(Return(static_cast<dbus::Bus*>(NULL)));
     DBusThreadManager::InitializeForTesting(dbus_thread_manager);
 
+    SetShillManagerClientExpectations();
+
     EXPECT_CALL(*dbus_thread_manager, GetShillManagerClient())
         .WillRepeatedly(Return(&mock_manager_client_));
     EXPECT_CALL(*dbus_thread_manager, GetShillServiceClient())
@@ -160,14 +163,12 @@ class ManagedNetworkConfigurationHandlerTest : public testing::Test {
         .WillByDefault(Invoke(&profiles_stub_,
                               &ShillProfileTestClient::GetEntry));
 
-    NetworkConfigurationHandler::Initialize();
-    ManagedNetworkConfigurationHandler::Initialize(&stub_profile_handler_);
+    NetworkHandler::Initialize();

[pneubeck (no reviews), 2013/05/13 09:17:06]

This is one of the implications of your change that I dislike a lot.

This test is supposed to test the configuration handler in isolation. With this
initialization, anything can happen behind the scenes. Then additional unrelated
expectations have to be set (see  SetShillManagerClientExpectations) which
unnecessarily weaken the test.

[gauravsh, 2013/05/13 23:30:12]

I agree with this concern. Having recently dealt with this in the context of
mock network library expectations in unit tests, having to set canned mock
expectations unrelated to what's being tested makes the test much more fragile.

I think we can achieve the goal of keeping ownership of various Network*Handlers
and initialization order be clearly defined, while keeping tests well isolated.
Just let tests keep the ability to initialize these pieces individually without
needing to go through NetworkHandler. And have the production code always access
the handlers through NetworkHandler.

[stevenjb, 2013/05/13 23:49:35]

See meta comment.

     message_loop_.RunUntilIdle();
   }
 
   virtual void TearDown() OVERRIDE {
-    ManagedNetworkConfigurationHandler::Shutdown();
-    NetworkConfigurationHandler::Shutdown();
+    NetworkHandler::Shutdown();
     DBusThreadManager::Shutdown();
   }
 
@@ -175,13 +176,15 @@ class ManagedNetworkConfigurationHandlerTest : public testing::Test {
     Mock::VerifyAndClearExpectations(&mock_manager_client_);
     Mock::VerifyAndClearExpectations(&mock_service_client_);
     Mock::VerifyAndClearExpectations(&mock_profile_client_);
+    SetShillManagerClientExpectations();
   }
 
   void InitializeStandardProfiles() {
     profiles_stub_.AddProfile(kUser1ProfilePath, kUser1);
-    stub_profile_handler_.AddProfile(NetworkProfile(kUser1ProfilePath, kUser1));
-    stub_profile_handler_.AddProfile(
-        NetworkProfile(kSharedProfilePath, std::string()));
+    NetworkHandler::Get()->network_profile_handler()->
+        AddProfileForTest(NetworkProfile(kUser1ProfilePath, kUser1));

[pneubeck (no reviews), 2013/05/13 09:17:06]

I don't see the improvement here.
Ok, for now it doesn't make a huge difference if there is one test method in the
normal profile handler or not.
But if the new initialization code doesn't allow using this stub anymore, it
indicates that the move is not in the right direction. What if we really want to
use a stub here?


Another problem I see: How can we cover the initialization itself of any of the
handlers in tests and in isolation?

[stevenjb, 2013/05/13 23:49:35]

See meta comment.

+    NetworkHandler::Get()->network_profile_handler()->
+        AddProfileForTest(NetworkProfile(kSharedProfilePath, std::string()));
   }
 
   void SetUpEntry(const std::string& path_to_shill_json,
@@ -209,15 +212,22 @@ class ManagedNetworkConfigurationHandlerTest : public testing::Test {
         onc::ONC_SOURCE_USER_POLICY, userhash, *network_configs);
   }
 
+  void SetShillManagerClientExpectations() {
+    EXPECT_CALL(mock_manager_client_, GetProperties(_)).Times(AnyNumber());

[pneubeck (no reviews), 2013/05/13 09:17:06]

Are these necessary because of calls from the other handlers?
That's not good.

We should move towards better isolation in tests not less.

[stevenjb, 2013/05/13 23:49:35]

Yes. See meta comment.

+    EXPECT_CALL(mock_manager_client_,
+                AddPropertyChangedObserver(_)).Times(AnyNumber());
+    EXPECT_CALL(mock_manager_client_,
+                RemovePropertyChangedObserver(_)).Times(AnyNumber());
+  }
+
   ManagedNetworkConfigurationHandler* managed_handler() {
-    return ManagedNetworkConfigurationHandler::Get();
+    return NetworkHandler::Get()->managed_network_configuration_handler();
   }
 
  protected:
   StrictMock<MockShillManagerClient> mock_manager_client_;
   StrictMock<MockShillServiceClient> mock_service_client_;
   StrictMock<MockShillProfileClient> mock_profile_client_;
-  NetworkProfileHandlerStub stub_profile_handler_;
   ShillProfileTestClient profiles_stub_;
   MessageLoop message_loop_;