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 323 matching lines @@
       dbus::ObjectPath(eth1),
       shill::kSecurityProperty, security_value,
       base::Bind(&base::DoNothing), base::Bind(&ErrorCallbackFunction));
   message_loop_.RunUntilIdle();
   EXPECT_EQ(2, test_observer_->PropertyUpdatesForService(eth1));
 }
 
 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) {
   // 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();
   EXPECT_EQ(shill::kStateIdle,
@@ skipping 104 matching lines @@
 TEST_F(NetworkStateHandlerTest, RequestUpdate) {
   // Request an update for kShillManagerClientStubDefaultWifi.
   EXPECT_EQ(1, test_observer_->PropertyUpdatesForService(
       kShillManagerClientStubDefaultWifi));
   network_state_handler_->RequestUpdateForNetwork(
       kShillManagerClientStubDefaultWifi);
   message_loop_.RunUntilIdle();
   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