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..48eafed4b0536c07d937000bd36108e05c5e59aa 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"
@@ -367,6 +368,28 @@ TEST_F(NetworkStateHandlerTest, FavoriteState) {
   EXPECT_EQ(1u, test_observer_->favorite_count());
 }
 
+TEST_F(NetworkStateHandlerTest, GetState) {
+  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) {
   // Change a network state.
   const std::string eth1 = kShillManagerClientStubDefaultService;
@@ -489,4 +512,95 @@ TEST_F(NetworkStateHandlerTest, RequestUpdate) {
   EXPECT_EQ(2, test_observer_->PropertyUpdatesForService(
       kShillManagerClientStubDefaultWifi));
 }
+
+TEST_F(NetworkStateHandlerTest, NetworkGuidInProfile) {
+  // And a network to the default Profile.
+  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;
+
+  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();

[pneubeck (no reviews), 2014/05/13 08:43:54]

Just to restate what's going on here (for my own clarification):


Here the network handlers see a profile push and a configured wifi (with an
existing GUID) appearing.

[stevenjb, 2014/05/13 18:25:07]

I'll add comments and break these each into two tests, although the second test
effectively has to duplicate the setup from the first test to be useful.

+
+  // Verify that a FavoriteState exists with a valid 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, verify that there is no longer a NetworkState or
+  // FavoriteState for it.
+  service_test_->RemoveService(wifi_path);

[pneubeck (no reviews), 2014/05/13 08:43:54]

the service is completely disappearing as if the profile were popped

+  UpdateManagerProperties();
+  EXPECT_FALSE(network_state_handler_->GetNetworkState(wifi_path));
+  EXPECT_FALSE(network_state_handler_->GetFavoriteStateFromServicePath(

[pneubeck (no reviews), 2014/05/13 08:43:54]

this verifies unrelated to GUID that the network/favoriteState is removed. I
think this should be in separate test.

+      wifi_path, false /* configured_only */));
+
+  // Add the service and verify that a FavorteState exists with the same guid.

[pneubeck (no reviews), 2014/05/13 08:43:54]

instead of relying on the FakeShillProfileClient to correctly be updated during
the test, you could just manually set the clients to a fixed state here (maybe
keeping the complexity of the FakeClients lower).

[stevenjb, 2014/05/13 18:25:07]

I have intentionally avoided adding fake behavior to the NSH classes. It is much
cleaner to me to make the FakeShill*Client behavior more closely match Shill
behavior than to maintain two layers of fake behavior.

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

Sure, I didn't want to imply that.

What I meant is: The Handlers don't know how the FakeClients were modified until
they are notified. Thus, behind the scenes you can manually put the FakeClients
into whatever state you want, without relying on the FakeClients doing that
themselves.

Effectively, that would mean that the FakeClient doesn't need your latest
modifications, and you would replace line 566 by a copy of 533-541,543.

+  AddService(wifi_path, wifi_path, shill::kTypeWifi, shill::kStateOnline);

[pneubeck (no reviews), 2014/05/13 08:43:54]

this makes the service appear again (like a ProfilePush).

This part is required as NetworkStateHandler keeps state in the SpecifierGuidMap
and might mix stuff up.

Good. Maybe drop a rational, why this second part is here.

+  UpdateManagerProperties();
+  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.
+  network = network_state_handler_->GetNetworkState(wifi_path);
+  ASSERT_TRUE(network);
+  EXPECT_EQ(wifi_guid, network->guid());
+}
+
+TEST_F(NetworkStateHandlerTest, NetworkGuidNotInProfile) {
+  // And a network but do not save it to a profile.
+  const std::string wifi_path = "wifi_with_guid";
+  const bool is_service_configured = false;
+
+  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, verify that there is no longer a NetworkState or
+  // FavoriteState for it.
+  service_test_->RemoveService(wifi_path);
+  UpdateManagerProperties();
+  EXPECT_FALSE(network_state_handler_->GetNetworkState(wifi_path));

[pneubeck (no reviews), 2014/05/13 08:43:54]

as in the previous test, this is unrelated an should be tested separately; you
could remove these two EXPECTs.

+  EXPECT_FALSE(network_state_handler_->GetFavoriteStateFromServicePath(
+      wifi_path, false /* configured_only */));
+
+  // Add the service and verify that a FavorteState exists with the same guid.
+  AddService(wifi_path, wifi_path, shill::kTypeWifi, shill::kStateOnline);
+  UpdateManagerProperties();
+  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.
+  network = network_state_handler_->GetNetworkState(wifi_path);
+  ASSERT_TRUE(network);
+  EXPECT_EQ(wifi_guid, network->guid());
+}
+
 }  // namespace chromeos