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 c0d63259d1a56f2861cf6b9dfb097ff1d516fb6d..849baef1bf360c31bd8d0f29c9491ee0993b98df 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"
@@ -213,6 +214,12 @@ class NetworkStateHandlerTest : public testing::Test {
                               add_to_watchlist);
   }
 
+  void UpdateManagerProperties() {
+    message_loop_.RunUntilIdle();
+    network_state_handler_->UpdateManagerProperties();
+    message_loop_.RunUntilIdle();
+  }
+
   base::MessageLoopForUI message_loop_;
   scoped_ptr<NetworkStateHandler> network_state_handler_;
   scoped_ptr<TestObserver> test_observer_;
@@ -349,9 +356,7 @@ TEST_F(NetworkStateHandlerTest, FavoriteState) {
   ShillProfileClient::TestInterface* profile_test =
       DBusThreadManager::Get()->GetShillProfileClient()->GetTestInterface();
   EXPECT_TRUE(profile_test->AddService("/profile/default", wifi1));
-  message_loop_.RunUntilIdle();
-  network_state_handler_->UpdateManagerProperties();
-  message_loop_.RunUntilIdle();
+  UpdateManagerProperties();
   EXPECT_EQ(1u, test_observer_->favorite_count());
 }
 
@@ -478,4 +483,39 @@ TEST_F(NetworkStateHandlerTest, RequestUpdate) {
       kShillManagerClientStubDefaultWireless));
 }
 
+TEST_F(NetworkStateHandlerTest, NetworkGuid) {

[pneubeck (no reviews), 2014/05/12 13:37:07]

you need a second test that does more or less the same but with
profile_path()=="" (and where the network goes out-of-range and back)

a third test could be: a visible wifi with InProfile()==false which is then
stored; the GUID should then be present in the profile.

[stevenjb, 2014/05/13 01:19:00]

Good point. I added the second test. The "InProfile" test effectively includes
the "third test" because it has to retrieve the saved guid from the Profile. (In
fact, my changes to NSH that update the map caused the InProfile test to fail,
correctly; I had to make a bunch of changes to FakeShillProfileClient to make it
work. I may try to separate those out).

+  // And a Favorite, ensure it has a GUID.
+  const std::string wifi_path = "wifi_with_guid";
+  service_test_->AddService(
+      wifi_path, wifi_path, shill::kTypeWifi, shill::kStateOnline,
+      true /* add_to_visible */, true /* add_to_watchlist */);
+  ShillProfileClient::TestInterface* profile_test =

[pneubeck (no reviews), 2014/05/12 13:37:07]

nit: better move this to the test class to make the test more readable.

[stevenjb, 2014/05/13 01:19:00]

Done.

+      DBusThreadManager::Get()->GetShillProfileClient()->GetTestInterface();
+  EXPECT_TRUE(profile_test->AddService("/profile/default", wifi_path));
+  UpdateManagerProperties();
+  // Verify that a FavoriteState existis with a valid GUID.

[pneubeck (no reviews), 2014/05/12 13:37:07]

existis -> exists

[stevenjb, 2014/05/13 01:19:00]

Done.

+  const FavoriteState* favorite =
+      network_state_handler_->GetFavoriteState(wifi_path);

[pneubeck (no reviews), 2014/05/12 13:37:07]

you could also test GetFavoriteStateFromGuid() here.

To not make test longer, you could at first do GetNetworkState and then
GetFavoriteSTateFromGuid.

[stevenjb, 2014/05/13 01:19:00]

Added a GetState test

+  ASSERT_TRUE(favorite);
+  std::string wifi_guid = favorite->guid();
+  EXPECT_FALSE(wifi_guid.empty());
+  // Verify that a NetworkState existis with the same GUID.

[pneubeck (no reviews), 2014/05/12 13:37:07]

existis -> exists

[stevenjb, 2014/05/13 01:19:00]

Done.

+  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 for it.
+  service_test_->RemoveService(wifi_path);

[pneubeck (no reviews), 2014/05/12 13:37:07]

I think you're not testing the right thing anymore from this point.

service_test_->RemoveService would mean, that the network goes out-of-range
_and_ the user is removing its configuration.
See my toplevel comment what the behavior in that case could be.

[stevenjb, 2014/05/13 01:19:00]

ServiceClientTest::RemoveService does *NOT* remove the service from the Profile,
so this is testing "going out of range".

That said, the two separate tests should be a little more clear now. Hopefully.

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

In that case and if the profile isn't removed, shouldn't the FavoriteState
continue to exist?
'out of range' means that the service is only removed from the Manager's
Services property but stays in the Manager's ServiceCompleteList.

So what you're testing is more like a ProfilePop and ProfilePush of Shill (as it
happens on a logout/login).

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

Well, the Fake clients are just an approximation. Shill would never have a
Profile entry but no Service. I can add a comment, but the intention here is to
test NSH correctly handling a FavortieState disappearing and re-appearing, e.g.
after a Shill restart, or profile pop/push, or a re-configure, e.g. by policy.

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

so why the misleading 'out-of-range' comment that is wrong?
Just call it 'profile pop/push' or 'Shill restart'.

+  UpdateManagerProperties();
+  EXPECT_FALSE(network_state_handler_->GetNetworkState(wifi_path));
+  // Add the service and verify that a NetworkState exists with the same guid.
+  service_test_->AddService(
+      wifi_path, wifi_path, shill::kTypeWifi, shill::kStateOnline,
+      true /* add_to_visible */, true /* add_to_watchlist */);
+  UpdateManagerProperties();
+  network = network_state_handler_->GetNetworkState(wifi_path);
+  ASSERT_TRUE(network);
+  EXPECT_EQ(wifi_guid, network->guid());
+}
+
 }  // namespace chromeos