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

chromeos/network/managed_network_configuration_handler_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 "chromeos/network/managed_network_configuration_handler.h"
 
 #include <iostream>
 #include <sstream>
 
 #include "base/message_loop.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/mock_dbus_thread_manager.h"
 #include "chromeos/dbus/mock_shill_manager_client.h"
 #include "chromeos/dbus/mock_shill_profile_client.h"
 #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"
 #include "testing/gmock/include/gmock/gmock.h"
 #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;
 using ::testing::Return;
 using ::testing::SaveArg;
 using ::testing::StrEq;
 using ::testing::StrictMock;
 using ::testing::_;
 
 namespace test_utils = ::chromeos::onc::test_utils;
@@ skipping 102 matching lines @@
 
   virtual ~ManagedNetworkConfigurationHandlerTest() {
   }
 
   virtual void SetUp() OVERRIDE {
     MockDBusThreadManager* dbus_thread_manager = new MockDBusThreadManager;
     EXPECT_CALL(*dbus_thread_manager, GetSystemBus())
         .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())
         .WillRepeatedly(Return(&mock_service_client_));
     EXPECT_CALL(*dbus_thread_manager, GetShillProfileClient())
         .WillRepeatedly(Return(&mock_profile_client_));
 
     ON_CALL(mock_profile_client_, GetProperties(_,_,_))
         .WillByDefault(Invoke(&profiles_stub_,
                               &ShillProfileTestClient::GetProperties));
 
     ON_CALL(mock_profile_client_, GetEntry(_,_,_,_))
         .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();
   }
 
   void VerifyAndClearExpectations() {
     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,
                   const std::string& profile_path,
                   const std::string& entry_path) {
     scoped_ptr<base::DictionaryValue> entry =
         test_utils::ReadTestDictionary(path_to_shill_json);
     profiles_stub_.AddEntry(profile_path, entry_path, *entry);
   }
 
   void SetPolicy(onc::ONCSource onc_source,
                  const std::string& userhash,
                  const std::string& path_to_onc) {
     scoped_ptr<base::DictionaryValue> policy;
     if (path_to_onc.empty())
       policy = onc::ReadDictionaryFromJson(onc::kEmptyUnencryptedConfiguration);
     else
       policy = test_utils::ReadTestDictionary(path_to_onc);
 
     base::ListValue* network_configs = NULL;
     policy->GetListWithoutPathExpansion(
         onc::toplevel_config::kNetworkConfigurations, &network_configs);
 
     managed_handler()->SetPolicy(
         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_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ManagedNetworkConfigurationHandlerTest);
 };
 
 TEST_F(ManagedNetworkConfigurationHandlerTest, ProfileInitialization) {
   InitializeStandardProfiles();
   message_loop_.RunUntilIdle();
@@ skipping 195 matching lines @@
               ConfigureServiceForProfile(
                   dbus::ObjectPath(kUser1ProfilePath),
                   IsEqualTo(expected_shill_properties.get()),
                   _, _));
 
   InitializeStandardProfiles();
   message_loop_.RunUntilIdle();
 }
 
 }  // namespace chromeos