Use GUID instead of ServicePath in networkingPrivate API

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 ca167d4189ef5e6d6f5c2f718e03accb49baa5de..b493189a01e3537a74fdf8df2ef14748a36f335d 100644
--- a/chromeos/network/network_state_handler_unittest.cc
+++ b/chromeos/network/network_state_handler_unittest.cc
@@ -17,6 +17,7 @@
 #include "chromeos/dbus/shill_manager_client.h"
 #include "chromeos/dbus/shill_profile_client.h"
 #include "chromeos/dbus/shill_service_client.h"
+#include "chromeos/network/favorite_state.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler_observer.h"
 #include "chromeos/network/shill_property_util.h"
@@ -360,11 +361,43 @@ TEST_F(NetworkStateHandlerTest, ServicePropertyChanged) {
 TEST_F(NetworkStateHandlerTest, FavoriteState) {
   // Set the profile entry of a service
   const std::string profile = "/profile/profile1";
-  const std::string wifi1 = kShillManagerClientStubDefaultWifi;
+  const std::string wifi_path = kShillManagerClientStubDefaultWifi;
   profile_test_->AddProfile(profile, "" /* userhash */);
-  EXPECT_TRUE(profile_test_->AddService(profile, wifi1));
+  EXPECT_TRUE(profile_test_->AddService(profile, wifi_path));
   UpdateManagerProperties();
   EXPECT_EQ(1u, test_observer_->favorite_count());
+  const FavoriteState* favorite =
+      network_state_handler_->GetFavoriteStateFromServicePath(
+          wifi_path, true /* is_configured */);
+  ASSERT_TRUE(favorite);
+
+  // Remove the service, verify that there is no longer a FavoriteState for it.
+  service_test_->RemoveService(wifi_path);
+  UpdateManagerProperties();
+  EXPECT_FALSE(network_state_handler_->GetFavoriteStateFromServicePath(
+      wifi_path, false /* configured_only */));
+}
+
+TEST_F(NetworkStateHandlerTest, GetState) {

[pneubeck (no reviews), 2014/05/14 08:46:07]

Looking at the names 'GetState' vs. 'FavoriteState' and also the tests
themselves, it's unclear to me what the difference is / what is actually tested.

Can these be merged or alternatively made more distinguished (e.g. GetState ->
NetworkState and remove *Favorite* from it)?

[stevenjb, 2014/05/14 17:31:56]

I combined these and cleaned them up a little.
I decided with the previous CL not to explicitly test that a FavoriteState no
longer exists after "RemoveService" because it's not really clear what that
should do. At some point we should make that "RemoveVisibleService" and keep it
in ServiceCompleteList, and add a "RemoveServiceFromProfile". But not today.

+  const std::string profile = "/profile/profile1";
+  profile_test_->AddProfile(profile, "" /* userhash */);
+  base::StringValue profile_value(profile);
+  EXPECT_TRUE(service_test_->SetServiceProperty(
+      kShillManagerClientStubWifi2, shill::kProfileProperty, profile_value));
+  UpdateManagerProperties();
+
+  const NetworkState* wifi_network =
+      network_state_handler_->GetNetworkState(kShillManagerClientStubWifi2);
+  ASSERT_TRUE(wifi_network);
+  const FavoriteState* wifi_favorite =
+      network_state_handler_->GetFavoriteStateFromServicePath(
+          kShillManagerClientStubWifi2, true /* configured_only */);
+  ASSERT_TRUE(wifi_favorite);
+  EXPECT_EQ(wifi_network->path(), wifi_favorite->path());
+  ASSERT_FALSE(wifi_favorite->guid().empty());
+  const FavoriteState* wifi_favorite_guid =
+      network_state_handler_->GetFavoriteStateFromGuid(wifi_favorite->guid());
+  EXPECT_EQ(wifi_favorite, wifi_favorite_guid);
 }
 
 TEST_F(NetworkStateHandlerTest, NetworkConnectionStateChanged) {
@@ -489,4 +522,88 @@ TEST_F(NetworkStateHandlerTest, RequestUpdate) {
   EXPECT_EQ(2, test_observer_->PropertyUpdatesForService(
       kShillManagerClientStubDefaultWifi));
 }
+
+TEST_F(NetworkStateHandlerTest, NetworkGuidInProfile) {
+  const std::string profile = "/profile/profile1";
+  const std::string wifi_path = "wifi_with_guid";
+  const std::string wifi_guid = "WIFI_GUID";
+  const bool is_service_configured = true;
+
+  // And a network to the default Profile with a specified GUID.

[pneubeck (no reviews), 2014/05/14 08:46:07]

typo: And -> Add

[stevenjb, 2014/05/14 17:31:56]

Done.

+  service_test_->AddServiceWithIPConfig(
+      wifi_path,
+      wifi_guid,
+      wifi_path  /* name */,
+      shill::kTypeWifi,
+      shill::kStateOnline,
+      "" /* ipconfig_path */,
+      true /* add_to_visible */,
+      true /* add_to_watchlist */);
+  profile_test_->AddProfile(profile, "" /* userhash */);
+  EXPECT_TRUE(profile_test_->AddService(profile, wifi_path));
+  UpdateManagerProperties();
+
+  // Verify that a FavoriteState exists with a matching GUID.
+  const FavoriteState* favorite =
+      network_state_handler_->GetFavoriteStateFromServicePath(
+          wifi_path, is_service_configured);
+  ASSERT_TRUE(favorite);
+  EXPECT_EQ(wifi_guid, favorite->guid());
+
+  // Verify that a NetworkState exists with the same GUID.
+  const NetworkState* network =
+      network_state_handler_->GetNetworkState(wifi_path);
+  ASSERT_TRUE(network);
+  EXPECT_EQ(wifi_guid, network->guid());
+
+  // Remove the service (simulating a network going out of range).
+  service_test_->RemoveService(wifi_path);
+  UpdateManagerProperties();
+  EXPECT_FALSE(network_state_handler_->GetNetworkState(wifi_path));

[pneubeck (no reviews), 2014/05/14 08:46:07]

this EXPECT should also be moved to test 'FavoriteState' or 'GetState'.

[stevenjb, 2014/05/14 17:31:56]

See above.

+
+  // Add the service (simulating a network coming back in range) and verify that
+  // the NetworkState was created with the same GUID.
+  AddService(wifi_path, wifi_path, shill::kTypeWifi, shill::kStateOnline);
+  UpdateManagerProperties();
+  network = network_state_handler_->GetNetworkState(wifi_path);
+  ASSERT_TRUE(network);
+  EXPECT_EQ(wifi_guid, network->guid());
+}

[pneubeck (no reviews), 2014/05/14 08:46:07]

optional: at the end of this test and the next one, you could again verify that
the FavoriteState also has the correct GUID.

[stevenjb, 2014/05/14 17:31:56]

Done.

+
+TEST_F(NetworkStateHandlerTest, NetworkGuidNotInProfile) {
+  const std::string wifi_path = "wifi_with_guid";
+  const bool is_service_configured = false;
+
+  // And a network without adding it to a profile.

[pneubeck (no reviews), 2014/05/14 08:46:07]

typo: And -> Add

[stevenjb, 2014/05/14 17:31:56]

Done.

+  AddService(wifi_path, wifi_path, shill::kTypeWifi, shill::kStateOnline);
+  UpdateManagerProperties();
+
+  // Verify that a FavoriteState exists with an assigned GUID.
+  const FavoriteState* favorite =
+      network_state_handler_->GetFavoriteStateFromServicePath(
+          wifi_path, is_service_configured);
+  ASSERT_TRUE(favorite);
+  std::string wifi_guid = favorite->guid();
+  EXPECT_FALSE(wifi_guid.empty());
+
+  // Verify that a NetworkState exists with the same GUID.
+  const NetworkState* network =
+      network_state_handler_->GetNetworkState(wifi_path);
+  ASSERT_TRUE(network);
+  EXPECT_EQ(wifi_guid, network->guid());
+
+  // Remove the service (simulating a network going out of range).
+  service_test_->RemoveService(wifi_path);
+  UpdateManagerProperties();
+  EXPECT_FALSE(network_state_handler_->GetNetworkState(wifi_path));

[pneubeck (no reviews), 2014/05/14 08:46:07]

remove

[stevenjb, 2014/05/14 17:31:56]

It may be redundant, but I think it makes the test more clear.

+
+  // Add the service (simulating a network coming back in range) and verify that
+  // the NetworkState was created with the same GUID.
+  AddService(wifi_path, wifi_path, shill::kTypeWifi, shill::kStateOnline);
+  UpdateManagerProperties();
+  network = network_state_handler_->GetNetworkState(wifi_path);
+  ASSERT_TRUE(network);
+  EXPECT_EQ(wifi_guid, network->guid());
+}
+
 }  // namespace chromeos