wifi_sync: add WifiConfigDelegate

components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc

+// Copyright 2015 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 "components/wifi_sync/wifi_config_delegate_chromeos.h"
+
+#include "base/macros.h"
+#include "base/message_loop/message_loop.h"
+#include "base/values.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 "components/wifi_sync/network_state_helper_chromeos.h"
+#include "components/wifi_sync/wifi_credential.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace wifi_sync {
+
+namespace {
+const char kProfilePath[] = "/profile/1";
+const char kSsid[] = "fake-ssid";
+const char kSsidNonUtf8[] = "\xc0";
+const char kUserHash[] = "fake-user-hash";
+}
+
+class WifiConfigDelegateChromeOsTest : public testing::Test {
+ protected:
+  WifiConfigDelegateChromeOsTest() {
+    chromeos::DBusThreadManager::Initialize();

[erikwright (departed), 2015/01/13 19:03:14]

This test is visibly highly dependent on a lot of complex state, including
non-obvious things like having to RunUntilIdle, etc.

This will be prone to flakiness and will need to be updated over time as the
"magic incantation" needed to get everything right evolves.

Luckily, you don't need any of it. ManagedNetworkConfigurationHandler is a
pure-virtual interface and you only need to mock one method of it in order to
verify that your class calls it with correct parameters when appropriate. The
others, you can implement with NOTIMPLEMENTED().

[stevenjb, 2015/01/13 23:45:39]

While we could create a custom MNCH class and create stubs for the required
methods (which would make this a true unit test in that we would only be testing
the behavior of this class), I don't agree that we should.

I would suggest that the behavior of this class by itself is not very
interesting. Rather, the relationship between this class and MNCH is the more
important thing to test.

We created test interfaces for the fake Shill clients (as used below)
specifically to allow this sort of integration test. We maintain many of them
and they have not been historically flaky.

[erikwright (departed), 2015/01/14 14:51:26]

This test is not the place to verify that calling MNCH::CreateConfiguration does
what it's supposed to do.

This is a unit test. This unit is expected to call MNCH::CreateConfiguration in
response to calls to AddToLocalNetworks. That is what should be tested here.

You are correct that the class under test is very simple. That's why the unit
test will be very simple.

An integration test elsewhere can cover the operation of the entire system.

Overly coupled unit tests like this one impede development and refactoring
because small changes in one part of the system end up needing corresponding
changes in a multitude of unit tests that _should_ be unrelated.

[mukesh agrawal, 2015/01/15 03:01:48]

Acknowledged.

[mukesh agrawal, 2015/01/15 03:01:48]

Acknowledged.

[mukesh agrawal, 2015/01/15 03:01:48]

Done.

+    chromeos::NetworkHandler::Initialize();
+    network_handler_ = chromeos::NetworkHandler::Get();

[stevenjb, 2015/01/13 23:45:39]

I would avoid storing global singletons even in a test. Instead just use
NetworkHandler::Get().

[mukesh agrawal, 2015/01/15 03:01:48]

Done. (Offending code removed.)

+
+    const base::ListValue policy_network_configs;
+    const base::DictionaryValue policy_global_config;
+    network_handler_
+        ->managed_network_configuration_handler()
+        ->SetPolicy(onc::ONC_SOURCE_USER_POLICY,
+                    kUserHash,
+                    policy_network_configs,
+                    policy_global_config);
+
+    chromeos::DBusThreadManager::Get()
+        ->GetShillProfileClient()
+        ->GetTestInterface()
+        ->AddProfile(kProfilePath, kUserHash);

[stevenjb, 2015/01/13 23:45:39]

We don't really need to do this now since we don't yet handle the behavior for
an existing network, but when we do we'll want to add an existing service here
and test the behavior when setting properties for the existing service.

[mukesh agrawal, 2015/01/15 03:01:48]

Acknowledged.

+
+    config_delegate_.reset(
+        new WifiConfigDelegateChromeOs(
+            kUserHash,
+            network_handler_->managed_network_configuration_handler()));
+
+    message_loop_.RunUntilIdle();  // Process updates queued by Shill clients.
+  }
+
+  ~WifiConfigDelegateChromeOsTest() {
+    // Reset singletons.
+    chromeos::NetworkHandler::Shutdown();
+    chromeos::DBusThreadManager::Shutdown();
+  }
+
+  // Wrapper for WifiConfigDelegateChromeOs::AddToLocalNetworks.
+  void AddToLocalNetworks(const WifiCredential& network_credential) {
+    config_delegate_->AddToLocalNetworks(network_credential);
+    message_loop_.RunUntilIdle();  // Process updates queued by Shill clients.
+  }

[stevenjb, 2015/01/13 23:45:39]

nit: WS

[mukesh agrawal, 2015/01/15 03:01:48]

Done. (Offending code removed.)

+  // Returns true if |credential_to_find| exists in the set of
+  // credentials configured in Shill.
+  bool CredentialIsConfigured(const WifiCredential& credential_to_find) {
+    WifiCredential::CredentialSet credentials(
+        GetWifiCredentialsForShillProfile(
+            network_handler_->network_state_handler(), kProfilePath));
+    for (const auto &credential : credentials) {
+      if (credential.ssid() == credential_to_find.ssid() &&
+          credential.security_class() == credential_to_find.security_class() &&
+          credential.passphrase() == credential_to_find.passphrase()) {
+        return true;
+      }
+    }
+    return false;
+  }

[stevenjb, 2015/01/13 23:45:39]

nit: WS

[mukesh agrawal, 2015/01/15 03:01:48]

Done.

+  // Returns a new WifiCredential constructed from the given parameters.
+  WifiCredential MakeCredential(const std::string& ssid,
+                                WifiSecurityClass security_class,
+                                const std::string& passphrase) {
+    scoped_ptr<WifiCredential> credential =
+        WifiCredential::Create(
+            WifiCredential::MakeSsidBytesForTest(ssid),
+            security_class,
+            passphrase);
+    CHECK(credential);
+    return *credential;
+  }
+
+ private:
+  base::MessageLoop message_loop_;  // For ManagedNetworkConfigurationHandler.
+  scoped_ptr<WifiConfigDelegateChromeOs> config_delegate_;
+  chromeos::NetworkHandler* network_handler_;  // Unowned.
+
+  DISALLOW_COPY_AND_ASSIGN(WifiConfigDelegateChromeOsTest);
+};
+
+TEST_F(WifiConfigDelegateChromeOsTest, AddToLocalNetworksOpen) {
+  const WifiCredential credential(
+      MakeCredential(kSsid, SECURITY_CLASS_NONE, ""));
+  EXPECT_FALSE(CredentialIsConfigured(credential));
+  AddToLocalNetworks(credential);
+  EXPECT_TRUE(CredentialIsConfigured(credential));
+}
+
+// TODO(quiche): Use a non-empty passphrase, once
+// GetWifiCredentialsForShillProfile provides passphrases.
+TEST_F(WifiConfigDelegateChromeOsTest, AddToLocalNetworksWep) {
+  const WifiCredential credential(
+      MakeCredential(kSsid, SECURITY_CLASS_WEP, ""));
+  EXPECT_FALSE(CredentialIsConfigured(credential));
+  AddToLocalNetworks(credential);
+  EXPECT_TRUE(CredentialIsConfigured(credential));
+}
+
+// TODO(quiche): Use a non-empty passphrase, once
+// GetWifiCredentialsForShillProfile provides passphrases.
+TEST_F(WifiConfigDelegateChromeOsTest, AddToLocalNetworksPsk) {
+  const WifiCredential credential(
+      MakeCredential(kSsid, SECURITY_CLASS_PSK, ""));
+  EXPECT_FALSE(CredentialIsConfigured(credential));
+  AddToLocalNetworks(credential);
+  EXPECT_TRUE(CredentialIsConfigured(credential));
+}
+
+// TODO(quiche): Use a non-empty passphrase, once
+// GetWifiCredentialsForShillProfile provides passphrases.
+TEST_F(WifiConfigDelegateChromeOsTest, AddToLocalNetworksNonUtf8) {
+  const WifiCredential credential(
+      MakeCredential(kSsidNonUtf8, SECURITY_CLASS_PSK, ""));
+  EXPECT_FALSE(CredentialIsConfigured(credential));
+  AddToLocalNetworks(credential);
+  // TODO(quiche): Change to EXPECT_TRUE, once we support non-UTF-8 SSIDs.
+  EXPECT_FALSE(CredentialIsConfigured(credential));
+}
+
+}  // namespace wifi_sync