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..57788cd6fb07e47c705a1dfb2ed3b1a4e9c25535 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 {
 
+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);
   }
@@ -112,74 +130,102 @@ class NetworkChangeNotifierChromeosUpdateTest : public testing::Test {
   NetworkChangeNotifierChromeos notifier_;
 };
 
-TEST_F(NetworkChangeNotifierChromeosUpdateTest, UpdateDefaultNetworkOffline) {
-  // Test that Online to Offline transitions are correctly handled.
-  SetNotifierState(NetworkChangeNotifier::CONNECTION_ETHERNET, "/service/1",
-                   "192.168.1.1");
-  SetDefaultNetworkState(false,  // offline.
-                        flimflam::kTypeEthernet, "", "/service/1", "");
-  bool type_changed = false, ip_changed = false, dns_changed = false;
-  ProcessDefaultNetworkUpdate(&type_changed, &ip_changed, &dns_changed);
-  VerifyNotifierState(NetworkChangeNotifier::CONNECTION_NONE, "", "");
-  EXPECT_TRUE(type_changed);
-  EXPECT_TRUE(ip_changed);
-  EXPECT_TRUE(dns_changed);
-}
+struct NotifierState {
+  NetworkChangeNotifier::ConnectionType type;
+  const char* service_path;
+  const char* ip_address;
+  const char* dns_servers;
+};
 
-TEST_F(NetworkChangeNotifierChromeosUpdateTest, UpdateDefaultNetworkOnline) {
-  // Test that Offline to Online transitions are correctly handled.
-  SetNotifierState(NetworkChangeNotifier::CONNECTION_NONE, "", "");
-
-  SetDefaultNetworkState(false,  // offline.
-                        flimflam::kTypeEthernet, "",
-                        "192.168.0.1", "/service/1");
-  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, "", "");
-  EXPECT_FALSE(type_changed);
-  EXPECT_FALSE(ip_changed);
-  EXPECT_FALSE(dns_changed);
-
-  SetDefaultNetworkState(true,  // online.
-                        flimflam::kTypeEthernet, "", "/service/1",
-                        "192.168.0.1");
-  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");
-  EXPECT_TRUE(type_changed);
-  EXPECT_TRUE(ip_changed);
-  EXPECT_TRUE(dns_changed);
-}
+struct DefaultNetworkState {
+  bool is_connected;
+  const char* type;
+  const char* technology;
+  const char* service_path;
+  const char* ip_address;
+  const char* dns_servers;
+};
+
+struct NotifierUpdateTestCase {
+  const char* test_description;
+  NotifierState initial_state;
+  DefaultNetworkState default_network_state;
+  NotifierState expected_state;
+  bool expected_type_changed;
+  bool expected_ip_changed;
+  bool expected_dns_changed;
+};
+
+NotifierUpdateTestCase test_cases[] = {

[stevenjb, 2013/03/28 20:30:58]

To make this more readable, please:
* Start each struct on a new line (e.g. line 160, 170)
* Don't follow a struct with non-struct values, e.g. line 163, put 'true, true,
true' on its own line
* Put the trailing } for each top leave entry on its own line, this will help
separate each test case

[gauravsh, 2013/03/29 00:02:11]

Done. (I used the clang chromium style formatter to do this earlier.)

[stevenjb, 2013/03/29 00:21:36]

Interesting. Well, this looks much nicer now, so thanks.

+  { "Online -> Offline", { NetworkChangeNotifier::CONNECTION_ETHERNET,
+                           kService1, kIpAddress1, kDnsServers1 },
+    { false, flimflam::kTypeEthernet, "", kService1, "", "" },
+    { NetworkChangeNotifier::CONNECTION_NONE, "", "", "" }, true, true, true },
+  { "Offline -> Offline",
+    { NetworkChangeNotifier::CONNECTION_NONE, "", "", "" },
+    { false, flimflam::kTypeEthernet, "", kService1, kIpAddress1,
+      kDnsServers1 },
+    { NetworkChangeNotifier::CONNECTION_NONE, "", "", "" }, false, false,
+    false },
+  { "Offline -> Online", { NetworkChangeNotifier::CONNECTION_NONE, "", "", "" },
+    { true, flimflam::kTypeEthernet, "", kService1, kIpAddress1, kDnsServers1 },
+    { NetworkChangeNotifier::CONNECTION_ETHERNET, kService1, kIpAddress1,
+      kDnsServers1 },
+    true, true, true },
+  { "Online -> Online (new default service, different connection type)",
+    { NetworkChangeNotifier::CONNECTION_ETHERNET, kService1, kIpAddress1,
+      kDnsServers1 },
+    { true, flimflam::kTypeWifi, "", kService2, kIpAddress1, kDnsServers1 },
+    { NetworkChangeNotifier::CONNECTION_WIFI, kService2, kIpAddress1,
+      kDnsServers1 },
+    true, true, true },
+  { "Online -> Online (new default service, same connection type)",
+    { NetworkChangeNotifier::CONNECTION_WIFI, kService2, kIpAddress1,
+      kDnsServers1 },
+    { true, flimflam::kTypeWifi, "", kService3, kIpAddress1, kDnsServers1 },
+    { NetworkChangeNotifier::CONNECTION_WIFI, kService3, kIpAddress1,
+      kDnsServers1 },
+    false, true, true },
+  { "Online -> Online (same default service, new IP address, same DNS)",
+    { NetworkChangeNotifier::CONNECTION_WIFI, kService3, kIpAddress1,
+      kDnsServers1 },
+    { true, flimflam::kTypeWifi, "", kService3, kIpAddress2, kDnsServers1 },
+    { NetworkChangeNotifier::CONNECTION_WIFI, kService3, kIpAddress2,
+      kDnsServers1 },
+    false, true, false },
+  { "Online -> Online (same default service, same IP address, new DNS)",
+    { NetworkChangeNotifier::CONNECTION_WIFI, kService3, kIpAddress2,
+      kDnsServers1 },
+    { true, flimflam::kTypeWifi, "", kService3, kIpAddress2, kDnsServers2 },
+    { NetworkChangeNotifier::CONNECTION_WIFI, kService3, kIpAddress2,
+      kDnsServers2 },
+    false, false, true }
+};
 
-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");
-
-  SetDefaultNetworkState(true,  // online.
-                        flimflam::kTypeWifi, "", "/service/2", "192.168.1.2");
-  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" );
-  EXPECT_TRUE(type_changed);
-  EXPECT_TRUE(ip_changed);
-  EXPECT_TRUE(dns_changed);
-
-  SetDefaultNetworkState(true,  // online.
-                        flimflam::kTypeWifi, "", "/service/3", "192.168.1.2");
-  ProcessDefaultNetworkUpdate(&type_changed, &ip_changed, &dns_changed);
-  VerifyNotifierState(NetworkChangeNotifier::CONNECTION_WIFI, "/service/3",
-                      "192.168.1.2" );
-  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_TRUE(dns_changed);
+TEST_F(NetworkChangeNotifierChromeosUpdateTest, UpdateDefaultNetworkOffline) {
+  for (size_t i = 0; i < arraysize(test_cases); ++i) {
+    SCOPED_TRACE(test_cases[i].test_description);
+    SetNotifierState(test_cases[i].initial_state.type,
+                     test_cases[i].initial_state.service_path,
+                     test_cases[i].initial_state.ip_address,
+                     test_cases[i].initial_state.dns_servers);

[stevenjb, 2013/03/28 20:30:58]

This could be further simplified by having SetNotifierState take a
NotifierState&

[gauravsh, 2013/03/29 00:02:11]

Good idea. Done.

+    SetDefaultNetworkState(
+        test_cases[i].default_network_state.is_connected,
+        test_cases[i].default_network_state.type,
+        test_cases[i].default_network_state.technology,
+        test_cases[i].default_network_state.service_path,
+        test_cases[i].default_network_state.ip_address,
+        test_cases[i].default_network_state.dns_servers);

[stevenjb, 2013/03/28 20:30:58]

Same here for DefaultNotifierState

[gauravsh, 2013/03/29 00:02:11]

Done.

+    bool type_changed = false, ip_changed = false, dns_changed = false;
+    ProcessDefaultNetworkUpdate(&type_changed, &ip_changed, &dns_changed);
+    VerifyNotifierState(test_cases[i].expected_state.type,
+                        test_cases[i].expected_state.service_path,
+                        test_cases[i].expected_state.ip_address,
+                        test_cases[i].expected_state.dns_servers);

[stevenjb, 2013/03/28 20:30:58]

And here with NotifierState&

[gauravsh, 2013/03/29 00:02:11]

Done.

+    EXPECT_TRUE(type_changed == test_cases[i].expected_type_changed);
+    EXPECT_TRUE(ip_changed == test_cases[i].expected_ip_changed);
+    EXPECT_TRUE(dns_changed == test_cases[i].expected_dns_changed);
+  }
 }
 
 }  // namespace chromeos