Fix logic for default network notification

chromeos/network/network_state_handler_unittest.cc

Index: chromeos/network/network_state_handler_unittest.cc
diff --git a/chromeos/network/network_state_handler_unittest.cc b/chromeos/network/network_state_handler_unittest.cc
index ae3f5b20ed9c8a4f288922ee26b232d9b3be0518..f41a901c1f787e22fc22b0ec48cd1e028ca0fbf6 100644
--- a/chromeos/network/network_state_handler_unittest.cc
+++ b/chromeos/network/network_state_handler_unittest.cc
@@ -82,6 +82,7 @@ class TestObserver : public chromeos::NetworkStateHandlerObserver {
   }
 
   void DefaultNetworkChanged(const NetworkState* network) override {
+    EXPECT_TRUE(!network || network->IsConnectedState());
     ++default_network_change_count_;
     default_network_ = network ? network->path() : "";
     default_network_connection_state_ =
@@ -169,10 +170,10 @@ namespace chromeos {
 class NetworkStateHandlerTest : public testing::Test {
  public:
   NetworkStateHandlerTest()
-      : device_test_(NULL),
-        manager_test_(NULL),
-        profile_test_(NULL),
-        service_test_(NULL) {}
+      : device_test_(nullptr),

[pneubeck (no reviews), 2015/03/12 19:40:09]

nit: you could put all of these also to the declaration (since C++11) and drop
the constructor.

[stevenjb, 2015/03/12 19:57:07]

I'd rather do that consistently throughout these classes at some point, but
noted.

+        manager_test_(nullptr),
+        profile_test_(nullptr),
+        service_test_(nullptr) {}
   ~NetworkStateHandlerTest() override {}
 
   void SetUp() override {
@@ -254,6 +255,14 @@ class NetworkStateHandlerTest : public testing::Test {
     message_loop_.RunUntilIdle();
   }
 
+  void SetServiceProperty(const std::string& service_path,
+                          const std::string& key,
+                          const base::Value& value) {
+    DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
+        dbus::ObjectPath(service_path), key, value,
+        base::Bind(&base::DoNothing), base::Bind(&ErrorCallbackFunction));
+  }
+
   base::MessageLoopForUI message_loop_;
   scoped_ptr<NetworkStateHandler> network_state_handler_;
   scoped_ptr<TestObserver> test_observer_;
@@ -405,10 +414,8 @@ TEST_F(NetworkStateHandlerTest, GetVisibleNetworks) {
   EXPECT_EQ(kNumShillManagerClientStubImplServices, networks.size());
 
   // Change the visible state of a network.
-  DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
-      dbus::ObjectPath(kShillManagerClientStubWifi2),
-      shill::kVisibleProperty, base::FundamentalValue(false),
-      base::Bind(&base::DoNothing), base::Bind(&ErrorCallbackFunction));
+  SetServiceProperty(kShillManagerClientStubWifi2, shill::kVisibleProperty,
+                     base::FundamentalValue(false));
   message_loop_.RunUntilIdle();
   network_state_handler_->GetVisibleNetworkList(&networks);
   EXPECT_EQ(kNumShillManagerClientStubImplServices - 1, networks.size());
@@ -494,20 +501,14 @@ TEST_F(NetworkStateHandlerTest, ServicePropertyChanged) {
   EXPECT_EQ("", ethernet->security_class());
   EXPECT_EQ(1, test_observer_->PropertyUpdatesForService(eth1));
   base::StringValue security_class_value("TestSecurityClass");
-  DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
-      dbus::ObjectPath(eth1),
-      shill::kSecurityClassProperty, security_class_value,
-      base::Bind(&base::DoNothing), base::Bind(&ErrorCallbackFunction));
+  SetServiceProperty(eth1, shill::kSecurityClassProperty, security_class_value);
   message_loop_.RunUntilIdle();
   ethernet = network_state_handler_->GetNetworkState(eth1);
   EXPECT_EQ("TestSecurityClass", ethernet->security_class());
   EXPECT_EQ(2, test_observer_->PropertyUpdatesForService(eth1));
 
   // Changing a service to the existing value should not trigger an update.
-  DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
-      dbus::ObjectPath(eth1),
-      shill::kSecurityClassProperty, security_class_value,
-      base::Bind(&base::DoNothing), base::Bind(&ErrorCallbackFunction));
+  SetServiceProperty(eth1, shill::kSecurityClassProperty, security_class_value);
   message_loop_.RunUntilIdle();
   EXPECT_EQ(2, test_observer_->PropertyUpdatesForService(eth1));
 }
@@ -615,18 +616,18 @@ TEST_F(NetworkStateHandlerTest, DefaultServiceChanged) {
 
   // Change the default network by changing Manager.DefaultService.
   const std::string wifi1 = kShillManagerClientStubDefaultWifi;
-  base::StringValue wifi1_value(wifi1);
-  manager_test_->SetManagerProperty(
-      shill::kDefaultServiceProperty, wifi1_value);
+  SetServiceProperty(eth1, shill::kStateProperty,
+                     base::StringValue(shill::kStateIdle));

[pneubeck (no reviews), 2015/03/12 19:40:09]

since both of these calls send a notification to NSH, it's not obvious to the
reader how many updates are to expected and why.
Maybe add a comment to the EXPECT_EQ(1u, ...).

[stevenjb, 2015/03/12 19:57:07]

Done.

+  manager_test_->SetManagerProperty(shill::kDefaultServiceProperty,
+                                    base::StringValue(wifi1));
   message_loop_.RunUntilIdle();
   EXPECT_EQ(wifi1, test_observer_->default_network());
   EXPECT_EQ(1u, test_observer_->default_network_change_count());
 
   // Change the state of the default network.
   test_observer_->reset_change_counts();
-  base::StringValue connection_state_ready_value(shill::kStateReady);
   service_test_->SetServiceProperty(wifi1, shill::kStateProperty,
-                                   connection_state_ready_value);
+                                    base::StringValue(shill::kStateReady));
   message_loop_.RunUntilIdle();
   EXPECT_EQ(shill::kStateReady,
             test_observer_->default_network_connection_state());
@@ -635,21 +636,39 @@ TEST_F(NetworkStateHandlerTest, DefaultServiceChanged) {
   // Updating a property on the default network should trigger
   // a default network change.
   test_observer_->reset_change_counts();
-  DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
-      dbus::ObjectPath(wifi1),
-      shill::kSecurityClassProperty, base::StringValue("TestSecurityClass"),
-      base::Bind(&base::DoNothing), base::Bind(&ErrorCallbackFunction));
+  SetServiceProperty(wifi1, shill::kSecurityClassProperty,
+                     base::StringValue("TestSecurityClass"));
   message_loop_.RunUntilIdle();
   EXPECT_EQ(1u, test_observer_->default_network_change_count());
 
   // No default network updates for signal strength changes.
   test_observer_->reset_change_counts();
-  DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
-      dbus::ObjectPath(wifi1),
-      shill::kSignalStrengthProperty, base::FundamentalValue(32),
-      base::Bind(&base::DoNothing), base::Bind(&ErrorCallbackFunction));
+  SetServiceProperty(wifi1, shill::kSignalStrengthProperty,
+                     base::FundamentalValue(32));
   message_loop_.RunUntilIdle();
   EXPECT_EQ(0u, test_observer_->default_network_change_count());
+
+  // Change the default network to a Connecting network, then set the
+  // state to Connected. The DefaultNetworkChange dnotification should not

[pneubeck (no reviews), 2015/03/12 19:40:09]

dnotification -> notification

+  // fire until the network is connected.
+  test_observer_->reset_change_counts();
+  SetServiceProperty(wifi1, shill::kStateProperty,
+                     base::StringValue(shill::kStateIdle));
+  message_loop_.RunUntilIdle();
+  EXPECT_EQ(1u, test_observer_->default_network_change_count());
+  EXPECT_EQ(std::string(), test_observer_->default_network());
+
+  const std::string wifi2 = kShillManagerClientStubWifi2;
+  manager_test_->SetManagerProperty(shill::kDefaultServiceProperty,
+                                    base::StringValue(wifi2));
+  message_loop_.RunUntilIdle();
+  EXPECT_EQ(1u, test_observer_->default_network_change_count());
+  // Change the connection state of the default network, observer should fire.
+  SetServiceProperty(wifi2, shill::kStateProperty,
+                     base::StringValue(shill::kStateReady));
+  message_loop_.RunUntilIdle();
+  EXPECT_EQ(wifi2, test_observer_->default_network());
+  EXPECT_EQ(2u, test_observer_->default_network_change_count());
 }
 
 TEST_F(NetworkStateHandlerTest, RequestUpdate) {