Use GUID instead of ServicePath in networkingPrivate API

chromeos/network/network_state_handler_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_state_handler.h"
 
 #include <map>
 #include <set>
 #include <string>
 
 #include "base/bind.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/values.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/shill_device_client.h"
 #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"
 #include "dbus/object_path.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 namespace {
 
 void ErrorCallbackFunction(const std::string& error_name,
@@ skipping 176 matching lines @@
                               add_to_visible,
                               add_to_watchlist);
     service_test_->AddService(kShillManagerClientStubCellular,
                               kShillManagerClientStubCellular,
                               shill::kTypeCellular,
                               shill::kStateIdle,
                               add_to_visible,
                               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_;
   ShillDeviceClient::TestInterface* device_test_;
   ShillManagerClient::TestInterface* manager_test_;
   ShillServiceClient::TestInterface* service_test_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(NetworkStateHandlerTest);
 };
@@ skipping 116 matching lines @@
   message_loop_.RunUntilIdle();
   EXPECT_EQ(2, test_observer_->PropertyUpdatesForService(eth1));
 }
 
 TEST_F(NetworkStateHandlerTest, FavoriteState) {
   // Set the profile entry of a service
   const std::string wifi1 = kShillManagerClientStubDefaultWireless;
   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());
 }
 
 TEST_F(NetworkStateHandlerTest, NetworkConnectionStateChanged) {
   // Change a network state.
   const std::string eth1 = kShillManagerClientStubDefaultService;
   base::StringValue connection_state_idle_value(shill::kStateIdle);
   service_test_->SetServiceProperty(eth1, shill::kStateProperty,
                                    connection_state_idle_value);
   message_loop_.RunUntilIdle();
@@ skipping 106 matching lines @@
   // Request an update for kShillManagerClientStubDefaultWireless.
   EXPECT_EQ(1, test_observer_->PropertyUpdatesForService(
       kShillManagerClientStubDefaultWireless));
   network_state_handler_->RequestUpdateForNetwork(
       kShillManagerClientStubDefaultWireless);
   message_loop_.RunUntilIdle();
   EXPECT_EQ(2, test_observer_->PropertyUpdatesForService(
       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