Eliminate NetworkManagerChanged

chromeos/network/shill_property_handler_unittest.cc

 // Copyright (c) 2012 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/shill_property_handler.h"
 
 #include <map>
 #include <set>
 #include <string>
 
@@ skipping 18 matching lines @@
 void DoNothingWithCallStatus(DBusMethodCallStatus call_status) {
 }
 
 void ErrorCallbackFunction(const std::string& error_name,
                            const std::string& error_message) {
   LOG(ERROR) << "Shill Error: " << error_name << " : " << error_message;
 }
 
 class TestListener : public internal::ShillPropertyHandler::Listener {
  public:
-  TestListener() : manager_updates_(0), errors_(0) {
+  TestListener() : technology_list_updates_(0),
+                   errors_(0) {
   }
 
   virtual void UpdateManagedList(ManagedState::ManagedType type,
                                  const base::ListValue& entries) OVERRIDE {
     UpdateEntries(GetTypeString(type), entries);
   }
 
   virtual void UpdateManagedStateProperties(
       ManagedState::ManagedType type,
       const std::string& path,
@@ skipping 11 matching lines @@
     AddPropertyUpdate(flimflam::kServicesProperty, service_path);
   }
 
   virtual void UpdateDeviceProperty(
       const std::string& device_path,
       const std::string& key,
       const base::Value& value) OVERRIDE {
     AddPropertyUpdate(flimflam::kDevicesProperty, device_path);
   }
 
-  virtual void NotifyManagerPropertyChanged() OVERRIDE {
-    ++manager_updates_;
+  virtual void TechnologyListChanged() OVERRIDE {
+    ++technology_list_updates_;
   }
 
   virtual void CheckPortalListChanged(
       const std::string& check_portal_list) OVERRIDE {
   }
 
   virtual void ManagedStateListChanged(
       ManagedState::ManagedType type) OVERRIDE {
     AddStateListUpdate(GetTypeString(type));
   }
 
   std::vector<std::string>& entries(const std::string& type) {
     return entries_[type];
   }
   std::map<std::string, int>& property_updates(const std::string& type) {
     return property_updates_[type];
   }
   std::map<std::string, int>& initial_property_updates(
       const std::string& type) {
     return initial_property_updates_[type];
   }
   int list_updates(const std::string& type) { return list_updates_[type]; }
-  int manager_updates() { return manager_updates_; }
+  int technology_list_updates() { return technology_list_updates_; }
   int errors() { return errors_; }
 
  private:
   std::string GetTypeString(ManagedState::ManagedType type) {
     if (type == ManagedState::MANAGED_TYPE_NETWORK) {
       return flimflam::kServicesProperty;
     } else if (type == ManagedState::MANAGED_TYPE_FAVORITE) {
       return shill::kServiceCompleteListProperty;
     } else if (type == ManagedState::MANAGED_TYPE_DEVICE) {
       return flimflam::kDevicesProperty;
@@ skipping 34 matching lines @@
     list_updates_[type] += 1;
   }
 
   // Map of list-type -> paths
   std::map<std::string, std::vector<std::string> > entries_;
   // Map of list-type -> map of paths -> update counts
   std::map<std::string, std::map<std::string, int> > property_updates_;
   std::map<std::string, std::map<std::string, int> > initial_property_updates_;
   // Map of list-type -> list update counts
   std::map<std::string, int > list_updates_;
-  int manager_updates_;
+  int technology_list_updates_;
   int errors_;
 };
 
 }  // namespace
 
 class ShillPropertyHandlerTest : public testing::Test {
  public:
   ShillPropertyHandlerTest()
       : manager_test_(NULL),
         device_test_(NULL),
@@ skipping 118 matching lines @@
   ShillManagerClient::TestInterface* manager_test_;
   ShillDeviceClient::TestInterface* device_test_;
   ShillServiceClient::TestInterface* service_test_;
   ShillProfileClient::TestInterface* profile_test_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ShillPropertyHandlerTest);
 };
 
 TEST_F(ShillPropertyHandlerTest, ShillPropertyHandlerStub) {
-  EXPECT_EQ(1, listener_->manager_updates());
   EXPECT_TRUE(shill_property_handler_->IsTechnologyAvailable(
       flimflam::kTypeWifi));
   EXPECT_TRUE(shill_property_handler_->IsTechnologyEnabled(
       flimflam::kTypeWifi));
   const size_t kNumShillManagerClientStubImplDevices = 2;
   EXPECT_EQ(kNumShillManagerClientStubImplDevices,
             listener_->entries(flimflam::kDevicesProperty).size());
   const size_t kNumShillManagerClientStubImplServices = 4;
   EXPECT_EQ(kNumShillManagerClientStubImplServices,
             listener_->entries(flimflam::kServicesProperty).size());
 
   EXPECT_EQ(0, listener_->errors());
 }
 
 TEST_F(ShillPropertyHandlerTest, ShillPropertyHandlerTechnologyChanged) {
-  EXPECT_EQ(1, listener_->manager_updates());
+  const int initial_technology_updates = 2;  // Available and Enabled lists

[pneubeck (no reviews), 2013/09/11 20:13:35]

did the number of notifications increase for UpdateManagerProperties() calls as
well?

[stevenjb, 2013/09/11 21:23:46]

UpdateManagerProperties() will trigger 2 notifications, one for Available and
one for Enabled technologies. This is a temporary sub-optimal situation that
will go away once we switch to Device.State. 

+  EXPECT_EQ(initial_technology_updates, listener_->technology_list_updates());
 
   // Remove a technology. Updates both the Available and Enabled lists.
   manager_test_->RemoveTechnology(flimflam::kTypeWimax);
   message_loop_.RunUntilIdle();
-  EXPECT_EQ(3, listener_->manager_updates());
+  EXPECT_EQ(initial_technology_updates + 2,
+            listener_->technology_list_updates());
 
   // Add a disabled technology.
   manager_test_->AddTechnology(flimflam::kTypeWimax, false);
   message_loop_.RunUntilIdle();
-  EXPECT_EQ(4, listener_->manager_updates());
+  EXPECT_EQ(initial_technology_updates + 3,
+            listener_->technology_list_updates());
   EXPECT_TRUE(shill_property_handler_->IsTechnologyAvailable(
       flimflam::kTypeWimax));
   EXPECT_FALSE(shill_property_handler_->IsTechnologyEnabled(
       flimflam::kTypeWimax));
 
   // Enable the technology.
   DBusThreadManager::Get()->GetShillManagerClient()->EnableTechnology(
       flimflam::kTypeWimax,
       base::Bind(&base::DoNothing), base::Bind(&ErrorCallbackFunction));
   message_loop_.RunUntilIdle();
-  EXPECT_EQ(5, listener_->manager_updates());
+  EXPECT_EQ(initial_technology_updates + 4,
+            listener_->technology_list_updates());
   EXPECT_TRUE(shill_property_handler_->IsTechnologyEnabled(
       flimflam::kTypeWimax));
 
   EXPECT_EQ(0, listener_->errors());
 }
 
 TEST_F(ShillPropertyHandlerTest, ShillPropertyHandlerDevicePropertyChanged) {
-  EXPECT_EQ(1, listener_->manager_updates());

[pneubeck (no reviews), 2013/09/11 20:13:35]

could you modify the listener_ to through an error if any of the update
functions were unexpectedly called?

(that's IMO one of the useful features of gmock's StrictMock)

[stevenjb, 2013/09/11 21:23:46]

We test the NetworkStateHandler notification counts pretty thoroughly there,
which I think is more important / interesting. I'm not sure that any possible
spurrious Listener update is especially valuable. (I'm not suggesting that it is
without value, but there is a practical limit to the number of assumptions that
can and should be tested; at this point TechnologyListChanged is not in any way
related to Device properties changing).

   EXPECT_EQ(1, listener_->list_updates(flimflam::kDevicesProperty));
   const size_t kNumShillManagerClientStubImplDevices = 2;
   EXPECT_EQ(kNumShillManagerClientStubImplDevices,
             listener_->entries(flimflam::kDevicesProperty).size());
   // Add a device.
   const std::string kTestDevicePath("test_wifi_device1");
   AddDevice(flimflam::kTypeWifi, kTestDevicePath);
   message_loop_.RunUntilIdle();
-  EXPECT_EQ(1, listener_->manager_updates());  // No new manager updates.
   EXPECT_EQ(2, listener_->list_updates(flimflam::kDevicesProperty));
   EXPECT_EQ(kNumShillManagerClientStubImplDevices + 1,
             listener_->entries(flimflam::kDevicesProperty).size());
   // Device changes are not observed.
   // Remove a device
   RemoveDevice(kTestDevicePath);
   message_loop_.RunUntilIdle();
   EXPECT_EQ(3, listener_->list_updates(flimflam::kDevicesProperty));
   EXPECT_EQ(kNumShillManagerClientStubImplDevices,
             listener_->entries(flimflam::kDevicesProperty).size());
 
   EXPECT_EQ(0, listener_->errors());
 }
 
 TEST_F(ShillPropertyHandlerTest, ShillPropertyHandlerServicePropertyChanged) {
-  EXPECT_EQ(1, listener_->manager_updates());
   EXPECT_EQ(1, listener_->list_updates(flimflam::kServicesProperty));
   const size_t kNumShillManagerClientStubImplServices = 4;
   EXPECT_EQ(kNumShillManagerClientStubImplServices,
             listener_->entries(flimflam::kServicesProperty).size());
 
   // Add an unwatched service.
   const std::string kTestServicePath("test_wifi_service1");
   AddService(flimflam::kTypeWifi, kTestServicePath,
              flimflam::kStateIdle, false);
   message_loop_.RunUntilIdle();
-  EXPECT_EQ(1, listener_->manager_updates());  // No new manager updates.
   // Watched and unwatched services trigger a service list update.
   EXPECT_EQ(2, listener_->list_updates(flimflam::kServicesProperty));
   EXPECT_EQ(kNumShillManagerClientStubImplServices + 1,
             listener_->entries(flimflam::kServicesProperty).size());
   // Service receives an initial property update.
   EXPECT_EQ(1, listener_->initial_property_updates(
       flimflam::kServicesProperty)[kTestServicePath]);
   // Change a property.
   base::FundamentalValue scan_interval(3);
   DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
@@ skipping 150 matching lines @@
   message_loop_.RunUntilIdle();
   // Property change should trigger an update for the Network only; no
   // property updates pushed by Shill affect Favorites.
   EXPECT_EQ(2, listener_->property_updates(
       flimflam::kServicesProperty)[kTestServicePath2]);
   EXPECT_EQ(0, listener_->property_updates(
       shill::kServiceCompleteListProperty)[kTestServicePath2]);
 }
 
 }  // namespace chromeos