NetworkChangeNotifierChromeos: Handle IPConfig property changes on the default network

chromeos/network/network_change_notifier_chromeos_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/network_change_notifier_chromeos.h"
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "chromeos/network/network_change_notifier_factory_chromeos.h"
 #include "chromeos/network/network_state.h"
 #include "net/base/network_change_notifier.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 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) {
   struct TypeMapping {
     const char* shill_type;
     const char* technology;
     NetworkChangeNotifier::ConnectionType connection_type;
   };
   TypeMapping type_mappings[] = {
     { flimflam::kTypeEthernet, "", NetworkChangeNotifier::CONNECTION_ETHERNET },
@@ skipping 34 matching lines @@
 }
 
 class NetworkChangeNotifierChromeosUpdateTest : public testing::Test {
  protected:
   NetworkChangeNotifierChromeosUpdateTest() : default_network_("") {
   }
   virtual ~NetworkChangeNotifierChromeosUpdateTest() {}
 
   void SetNotifierState(NetworkChangeNotifier::ConnectionType type,
                         std::string service_path,
-                        std::string ip_address) {
+                        std::string ip_address,
+                        std::string dns_servers) {
+    notifier_.connection_type_ = type;
+    notifier_.service_path_ = service_path;
     notifier_.ip_address_ = ip_address;
-    notifier_.service_path_ = service_path;
-    notifier_.connection_type_ = type;
+    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_service_path, notifier_.service_path_);
     EXPECT_EQ(expected_ip_address, notifier_.ip_address_);
-    EXPECT_EQ(expected_service_path, notifier_.service_path_);
+    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
       default_network_.connection_state_ = flimflam::kStateConfiguration;
     default_network_.type_ = type;
     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);
   }
 
  private:
   NetworkState default_network_;
   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");
+  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);
 }
 
 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);
 }
 
 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, "", "/service/3", "192.168.1.2");
+                         flimflam::kTypeWifi, "", kService3,
+                         kIpAddress1, kDnsServers1);
   ProcessDefaultNetworkUpdate(&type_changed, &ip_changed, &dns_changed);
-  VerifyNotifierState(NetworkChangeNotifier::CONNECTION_WIFI, "/service/3",
-                      "192.168.1.2" );
+  VerifyNotifierState(NetworkChangeNotifier::CONNECTION_WIFI, kService3,
+                      kIpAddress1, 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_TRUE(dns_changed);
+
+  // Same default service, new IP address, same DNS.
+  SetDefaultNetworkState(true,  // online.
+                         flimflam::kTypeWifi, "", kService3,
+                         kIpAddress2, kDnsServers1);
+  ProcessDefaultNetworkUpdate(&type_changed, &ip_changed, &dns_changed);
+  VerifyNotifierState(NetworkChangeNotifier::CONNECTION_WIFI, kService3,
+                      kIpAddress2, kDnsServers1);
+  EXPECT_FALSE(type_changed);
+  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);
 }
 
 }  // namespace chromeos