sync: add more integration tests for wifi_sync

chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc

Index: chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc
diff --git a/chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc b/chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc
index 8e5400247c21808abee5edcbbdf3b236208d1440..0a9b3189e68b5ce70b55c6f9076efd491ac20db4 100644
--- a/chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc
+++ b/chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc
@@ -6,14 +6,23 @@
 
 #include <string>
 
+#include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/strings/stringprintf.h"
+#include "base/values.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/shill_profile_client.h"
+#include "chromeos/network/managed_network_configuration_handler.h"
 #include "chromeos/network/network_handler.h"
-#include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
+#include "components/onc/onc_constants.h"
 #include "components/wifi_sync/network_state_helper_chromeos.h"
-#include "components/wifi_sync/wifi_security_class.h"
+#include "components/wifi_sync/wifi_credential_syncable_service_factory.h"
+
+using wifi_sync::WifiCredential;
 
 using WifiCredentialSet = wifi_sync::WifiCredential::CredentialSet;
 
@@ -23,6 +32,13 @@ namespace {
 
 const char kProfilePrefix[] = "/profile/";
 
+void LogCreateConfigurationFailure(
+    const std::string& debug_hint,
+    const std::string& /* network_config_error_message */,
+    scoped_ptr<base::DictionaryValue> /* network_config_error_data */) {
+  LOG(FATAL) << debug_hint;
+}
+
 std::string ChromeOsUserHashForBrowserContext(
     const content::BrowserContext& context) {
   return context.GetPath().BaseName().value();
@@ -33,17 +49,79 @@ std::string ShillProfilePathForBrowserContext(
   return kProfilePrefix + ChromeOsUserHashForBrowserContext(context);

[pneubeck (no reviews), 2015/01/30 14:38:45]

if this is for testing only, it should be irrelevant what path you're using. you
could clarify that in a comment.

otherwise this would copy the logic how to construct the profile path and would
have to be kept in sync. which would be problematic.
please consider using NetworkProfileHandler::GetProfileForUserhash instead to
get the profile path in production code.

[mukesh agrawal, 2015/01/30 22:28:14]

Are you referring to the use of kProfilePrefix (which is primarily stylistic),
or the fact that we derive the profile path from the browser context? The latter
has a functional purpose. (We need distinct profiles for each SyncableService.
Otherwise, we'll mistakenly believe that the clients are synchronized, because
they're sharing network profile data.)

I'm happy to add comments to explain either, or both points. I just want to make
sure that I don't over-comment.
Acknowledged.

The production code currently doesn't deal with profile paths, since MNCH
operates on either a user hash, or a service path. But I'll keep this in mind in
case I add code that does use profile paths.

(There is some code in wifi_sync/network_state_helper_chromeos.cc, but right now
it's only called from sync integration tests. I have an open bug to move that
code into this directory: crbug.com/449020.)

[pneubeck (no reviews), 2015/01/31 10:26:40]

makes sense.
I'd just add a short comment in front of the return, like "Return arbitrary, but
distinct profile paths per context."

[mukesh agrawal, 2015/02/02 20:36:40]

Done.

 }
 
-chromeos::NetworkStateHandler* GetNetworkStateHandler() {
-  DCHECK(chromeos::NetworkHandler::Get()->network_state_handler());
-  return chromeos::NetworkHandler::Get()->network_state_handler();
+::chromeos::ShillProfileClient::TestInterface*
+GetShillProfileClientTestInterface() {
+  DCHECK(::chromeos::DBusThreadManager::Get()->GetShillProfileClient());

[pneubeck (no reviews), 2015/01/30 14:38:45]

the DCHECKs (here and in the other Get*()) are not really required as stuff will
crash when dereferencing the returned pointer anyways and these crashes are
nearby the Get*() call, thus easy to trace back.

[mukesh agrawal, 2015/01/30 22:28:14]

Agreed that a nullptr would cause a crash.

The reason I've placed DCHECKs is so that I can get a lead on the error without
having to pull up a debugger, and deciphering a stack trace that may be
complicated by optimizations. And since these are DCHECKs, they won't bloat/slow
release code.

For those reasons, I'd like to keep the DCHECKs in place. Let me know if you
still believe they should be removed.

[pneubeck (no reviews), 2015/01/31 10:26:40]

no, keep them if you find them helpful.
just keep them at a reasonable number / don't bloat the code with them. e.g.
this function is at the limit, IMO (more DCHECK than actual code).

(this function might be more readable by assign the DBusThreadManager to a
variable)

[mukesh agrawal, 2015/02/02 20:36:40]

Acknowledged.

+  DCHECK(::chromeos::DBusThreadManager::Get()->GetShillProfileClient()
+         ->GetTestInterface());
+  return ::chromeos::DBusThreadManager::Get()->GetShillProfileClient()
+      ->GetTestInterface();
+}
+
+::chromeos::ManagedNetworkConfigurationHandler*
+GetManagedNetworkConfigurationHandler() {
+  DCHECK(::chromeos::NetworkHandler::Get()
+         ->managed_network_configuration_handler());
+  return ::chromeos::NetworkHandler::Get()
+      ->managed_network_configuration_handler();
+}
+
+::chromeos::NetworkStateHandler* GetNetworkStateHandler() {
+  DCHECK(::chromeos::NetworkHandler::Get()->network_state_handler());
+  return ::chromeos::NetworkHandler::Get()->network_state_handler();
 }
 
 }  // namespace
 
+namespace chromeos {
+
+void SetUpChromeOs() {
+  wifi_sync::WifiCredentialSyncableServiceFactory::GetInstance()
+      ->IgnoreLoginStateForTest();
+}
+
+bool SetupClientForProfileChromeOs(const Profile* profile) {
+  DCHECK(profile);
+  GetShillProfileClientTestInterface()
+      ->AddProfile(ShillProfilePathForBrowserContext(*profile),
+                   ChromeOsUserHashForBrowserContext(*profile));
+
+  const base::ListValue policy_network_configs;
+  const base::DictionaryValue policy_global_config;
+  GetManagedNetworkConfigurationHandler()
+      ->SetPolicy(onc::ONC_SOURCE_UNKNOWN,
+                  ChromeOsUserHashForBrowserContext(*profile),
+                  policy_network_configs,
+                  policy_global_config);
+
+  return true;

[pneubeck (no reviews), 2015/01/30 14:38:45]

return value not required?

[mukesh agrawal, 2015/01/30 22:28:14]

I think your point is that, since the function always returns true, the return
value could be omitted.

That's true. But then I'd need to add a "return true" to the ChromeOS specific
code in wifi_credentials_helper.cc:SetupClientForProfile.

I believe the code is cleaner as-is. But I can change it if you disagree.

[pneubeck (no reviews), 2015/01/31 10:26:40]

If you want to keep it, I'd suggest to at least document what the return value
means.
Exposing to the caller that this function can never fail, is however preferable
IMO.

[mukesh agrawal, 2015/02/02 20:36:40]

Done.

+}
+
+void AddWifiCredentialToProfileChromeOs(
+    Profile* profile, const WifiCredential& credential) {
+  DCHECK(profile);
+  scoped_ptr<base::DictionaryValue> onc_properties =
+      credential.ToOncProperties();
+  if (!onc_properties) {

[pneubeck (no reviews), 2015/01/30 14:38:45]

nit: equivalent to
  CHECK(onc_properties) << ...

[mukesh agrawal, 2015/01/30 22:28:14]

Done.

+    LOG(FATAL) << "Failed to generate ONC properties for "
+               << credential.ToString();
+  }
+  GetManagedNetworkConfigurationHandler()
+      ->CreateConfiguration(
+          ChromeOsUserHashForBrowserContext(*profile),
+          *onc_properties,
+          ::chromeos::network_handler::StringResultCallback(),
+          base::Bind(LogCreateConfigurationFailure,
+                     base::StringPrintf("Failed to add credential %s",
+                                        credential.ToString().c_str())));
+}
+
 WifiCredentialSet GetWifiCredentialsForProfileChromeOs(const Profile* profile) {
   DCHECK(profile);
   return wifi_sync::GetWifiCredentialsForShillProfile(
       GetNetworkStateHandler(), ShillProfilePathForBrowserContext(*profile));
 }
 
+}  // namespace chromeos
+
 }  // namespace wifi_credentials_helper