NetworkChangeNotifierChromeos: Handle IPConfig property changes on the default network

chromeos/network/network_change_notifier_chromeos_unittest.cc

Index: chromeos/network/network_change_notifier_chromeos_unittest.cc
diff --git a/chromeos/network/network_change_notifier_chromeos_unittest.cc b/chromeos/network/network_change_notifier_chromeos_unittest.cc
index 1771322ef3559d6a4a8a807ed836b7efe75af5c5..9b3da4e6fd18c4421872c3831136dd4eb503acc6 100644
--- a/chromeos/network/network_change_notifier_chromeos_unittest.cc
+++ b/chromeos/network/network_change_notifier_chromeos_unittest.cc
@@ -15,6 +15,18 @@
 
 namespace chromeos {

[pneubeck (no reviews), 2013/03/28 10:37:54]

Can we please take one step back and think about what we're actually testing
here?

The Notifier does mostly two things:
  1) Translate the connection state (ConnectionTypeFromShill, ~30 lines)
  2) Diff the old network state with the new state (UpdateState, ~40 lines)


This test, as it is, tests for 2) and for
  3) Caching of the last network state (~10 lines)

Why does that need 230 lines of code?
The test could be simplified by e.g. having one test with the parameters
(initial state, new state, expected changes)

[gauravsh, 2013/03/28 18:48:54]

The # of lines for the test compared to the lines of code being tested is
irrelevant. These are regression test cases which are meant to verify some
common state transitions we see in practice, and to ensure they are correctly
being handled. Not every one of these cases will be explicitly handled in the
code, which can make the tests easily larger than the code being tested.

If you feel there are specific test cases which are not useful, please point
them out.

I do agree with you that we should de-duplicate the testing code. Which is what
I did by making the test table driven in the next patchset. However, as you'll
see, we are now at 231 lines. :) 

[stevenjb, 2013/03/28 19:27:13]

I tend to agree that lines of test code is not a good metric here.
De-duplication of common code is good, but I would much rather have too many
tests than too few.

 
+namespace {
+
+const char kDnsServers1[] = "192.168.0.1,192.168.0.2";
+const char kDnsServers2[] = "192.168.3.1,192.168.3.2";
+const char kIpAddress1[] = "192.168.1.1";
+const char kIpAddress2[] = "192.168.1.2";
+const char kService1[] = "/service/1";
+const char kService2[] = "/service/2";
+const char kService3[] = "/service/3";
+
+}
+
 using net::NetworkChangeNotifier;
 
 TEST(NetworkChangeNotifierChromeosTest, ConnectionTypeFromShill) {
@@ -69,26 +81,31 @@ class NetworkChangeNotifierChromeosUpdateTest : public testing::Test {
 
   void SetNotifierState(NetworkChangeNotifier::ConnectionType type,
                         std::string service_path,
-                        std::string ip_address) {
-    notifier_.ip_address_ = ip_address;
-    notifier_.service_path_ = service_path;
+                        std::string ip_address,
+                        std::string dns_servers) {
     notifier_.connection_type_ = type;
+    notifier_.service_path_ = service_path;
+    notifier_.ip_address_ = ip_address;
+    notifier_.dns_servers_ = dns_servers;
   }
 
   void VerifyNotifierState(NetworkChangeNotifier::ConnectionType expected_type,
                            std::string expected_service_path,
-                           std::string expected_ip_address) {
+                           std::string expected_ip_address,
+                           std::string expected_dns_servers) {
     EXPECT_EQ(expected_type, notifier_.connection_type_);
-    EXPECT_EQ(expected_ip_address, notifier_.ip_address_);
     EXPECT_EQ(expected_service_path, notifier_.service_path_);
+    EXPECT_EQ(expected_ip_address, notifier_.ip_address_);
+    EXPECT_EQ(expected_dns_servers, notifier_.dns_servers_);
   }
 
   // Sets the default network state used for notifier updates.
   void SetDefaultNetworkState(bool is_connected,
-                             std::string type,
-                             std::string technology,
-                             std::string service_path,
-                             std::string ip_address) {
+                              std::string type,
+                              std::string technology,
+                              std::string service_path,
+                              std::string ip_address,
+                              std::string dns_servers) {
     if (is_connected)
       default_network_.connection_state_ = flimflam::kStateOnline;
     else
@@ -97,12 +114,13 @@ class NetworkChangeNotifierChromeosUpdateTest : public testing::Test {
     default_network_.technology_ = technology;
     default_network_.path_ = service_path;
     default_network_.ip_address_ = ip_address;
+    default_network_.dns_servers_ = dns_servers;
   }
 
   // Process an default network update based on the state of |default_network_|.
   void ProcessDefaultNetworkUpdate(bool* type_changed,
-                                  bool* ip_changed,
-                                  bool* dns_changed) {
+                                   bool* ip_changed,
+                                   bool* dns_changed) {
     notifier_.UpdateState(&default_network_, type_changed, ip_changed,
                           dns_changed);
   }
@@ -114,13 +132,13 @@ class NetworkChangeNotifierChromeosUpdateTest : public testing::Test {
 
 TEST_F(NetworkChangeNotifierChromeosUpdateTest, UpdateDefaultNetworkOffline) {
   // Test that Online to Offline transitions are correctly handled.
-  SetNotifierState(NetworkChangeNotifier::CONNECTION_ETHERNET, "/service/1",
-                   "192.168.1.1");
+  SetNotifierState(NetworkChangeNotifier::CONNECTION_ETHERNET, kService1,
+                   kIpAddress1, kDnsServers1);
   SetDefaultNetworkState(false,  // offline.
-                        flimflam::kTypeEthernet, "", "/service/1", "");
+                         flimflam::kTypeEthernet, "", kService1, "", "");
   bool type_changed = false, ip_changed = false, dns_changed = false;
   ProcessDefaultNetworkUpdate(&type_changed, &ip_changed, &dns_changed);
-  VerifyNotifierState(NetworkChangeNotifier::CONNECTION_NONE, "", "");
+  VerifyNotifierState(NetworkChangeNotifier::CONNECTION_NONE, "", "", "");
   EXPECT_TRUE(type_changed);
   EXPECT_TRUE(ip_changed);
   EXPECT_TRUE(dns_changed);
@@ -128,27 +146,28 @@ TEST_F(NetworkChangeNotifierChromeosUpdateTest, UpdateDefaultNetworkOffline) {
 
 TEST_F(NetworkChangeNotifierChromeosUpdateTest, UpdateDefaultNetworkOnline) {
   // Test that Offline to Online transitions are correctly handled.
-  SetNotifierState(NetworkChangeNotifier::CONNECTION_NONE, "", "");
+  SetNotifierState(NetworkChangeNotifier::CONNECTION_NONE, "", "", "");
 
   SetDefaultNetworkState(false,  // offline.
-                        flimflam::kTypeEthernet, "",
-                        "192.168.0.1", "/service/1");
+                         flimflam::kTypeEthernet, "",
+                         kService1, kIpAddress1,
+                         kDnsServers1);
   bool type_changed = false, ip_changed = false, dns_changed = false;
   ProcessDefaultNetworkUpdate(&type_changed, &ip_changed, &dns_changed);
   // If the new default network is still offline, nothing should have changed.
-  VerifyNotifierState(NetworkChangeNotifier::CONNECTION_NONE, "", "");
+  VerifyNotifierState(NetworkChangeNotifier::CONNECTION_NONE, "", "", "");
   EXPECT_FALSE(type_changed);
   EXPECT_FALSE(ip_changed);
   EXPECT_FALSE(dns_changed);
 
   SetDefaultNetworkState(true,  // online.
-                        flimflam::kTypeEthernet, "", "/service/1",
-                        "192.168.0.1");
+                         flimflam::kTypeEthernet, "",
+                         kService1, kIpAddress1, kDnsServers1);
   ProcessDefaultNetworkUpdate(&type_changed, &ip_changed, &dns_changed);
   // Now the new default network is online, so this should trigger a notifier
   // state change.
-  VerifyNotifierState(NetworkChangeNotifier::CONNECTION_ETHERNET, "/service/1",
-                      "192.168.0.1");
+  VerifyNotifierState(NetworkChangeNotifier::CONNECTION_ETHERNET, kService1,
+                      kIpAddress1, kDnsServers1);
   EXPECT_TRUE(type_changed);
   EXPECT_TRUE(ip_changed);
   EXPECT_TRUE(dns_changed);
@@ -157,28 +176,54 @@ TEST_F(NetworkChangeNotifierChromeosUpdateTest, UpdateDefaultNetworkOnline) {
 TEST_F(NetworkChangeNotifierChromeosUpdateTest, UpdateDefaultNetworkChanged) {
   // Test that Online to Online transitions (default network changes) are
   // correctly handled.
-  SetNotifierState(NetworkChangeNotifier::CONNECTION_ETHERNET, "/service/1",
-                   "192.168.1.1");
+  SetNotifierState(NetworkChangeNotifier::CONNECTION_ETHERNET,
+                   kService1,
+                   kIpAddress1, kDnsServers1);
 
+  // New default service, different connection type.
   SetDefaultNetworkState(true,  // online.
-                        flimflam::kTypeWifi, "", "/service/2", "192.168.1.2");
+                         flimflam::kTypeWifi, "", kService2, kIpAddress1,
+                         kDnsServers1);
   bool type_changed = false, ip_changed = false, dns_changed = false;
   ProcessDefaultNetworkUpdate(&type_changed, &ip_changed, &dns_changed);
-  VerifyNotifierState(NetworkChangeNotifier::CONNECTION_WIFI, "/service/2",
-                      "192.168.1.2" );
+  VerifyNotifierState(NetworkChangeNotifier::CONNECTION_WIFI,
+                      kService2,
+                      kIpAddress1, kDnsServers1);
   EXPECT_TRUE(type_changed);
   EXPECT_TRUE(ip_changed);
   EXPECT_TRUE(dns_changed);
 
+  // New default service, same connection type.
+  SetDefaultNetworkState(true,  // online.
+                         flimflam::kTypeWifi, "", kService3,
+                         kIpAddress1, kDnsServers1);
+  ProcessDefaultNetworkUpdate(&type_changed, &ip_changed, &dns_changed);
+  VerifyNotifierState(NetworkChangeNotifier::CONNECTION_WIFI, kService3,
+                      kIpAddress1, kDnsServers1);
+  EXPECT_FALSE(type_changed);
+  EXPECT_TRUE(ip_changed);
+  EXPECT_TRUE(dns_changed);
+
+  // Same default service, new IP address, same DNS.
   SetDefaultNetworkState(true,  // online.
-                        flimflam::kTypeWifi, "", "/service/3", "192.168.1.2");
+                         flimflam::kTypeWifi, "", kService3,
+                         kIpAddress2, kDnsServers1);
   ProcessDefaultNetworkUpdate(&type_changed, &ip_changed, &dns_changed);
-  VerifyNotifierState(NetworkChangeNotifier::CONNECTION_WIFI, "/service/3",
-                      "192.168.1.2" );
+  VerifyNotifierState(NetworkChangeNotifier::CONNECTION_WIFI, kService3,
+                      kIpAddress2, kDnsServers1);
   EXPECT_FALSE(type_changed);
-  // A service path change (even with a corresponding IP change) should still
-  // trigger an IP address update to observers.
   EXPECT_TRUE(ip_changed);
+  EXPECT_FALSE(dns_changed);
+
+  // Same default service, same IP address, new DNS.
+  SetDefaultNetworkState(true,  // online.
+                         flimflam::kTypeWifi, "", kService3, kIpAddress2,
+                         kDnsServers2);
+  ProcessDefaultNetworkUpdate(&type_changed, &ip_changed, &dns_changed);
+  VerifyNotifierState(NetworkChangeNotifier::CONNECTION_WIFI, kService3,
+                      kIpAddress2, kDnsServers2);
+  EXPECT_FALSE(type_changed);
+  EXPECT_FALSE(ip_changed);
   EXPECT_TRUE(dns_changed);
 }