wifi_sync: add WifiConfigDelegate

components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc

Index: components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc
diff --git a/components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc b/components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..13835ed8096c01dcfaf0987062d15430e70661d9
--- /dev/null
+++ b/components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc
@@ -0,0 +1,182 @@
+// 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/values.h"
+#include "chromeos/network/managed_network_configuration_handler.h"
+#include "components/wifi_sync/wifi_credential.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace wifi_sync {
+
+namespace {
+const char kSsid[] = "fake-ssid";
+const char kSsidNonUtf8[] = "\xc0";
+const char kUserHash[] = "fake-user-hash";
+}
+
+class FakeManagedNetworkConfigurationHandler

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

Taking a cue from the advice on FakeWifiConfigDelegate, I've placed the
FakeManagedNetworkConfigurationHandler in this file. (This file is the only
place that uses it.)

+    : public chromeos::ManagedNetworkConfigurationHandler {
+ public:
+  FakeManagedNetworkConfigurationHandler()
+      : create_configuration_count_(0) {
+  }
+
+  // ManagedNetworkConfigurationHandler implementation.
+  void AddObserver(chromeos::NetworkPolicyObserver* observer) override {
+    NOTIMPLEMENTED();
+  }
+  void RemoveObserver(chromeos::NetworkPolicyObserver* observer) override {
+    NOTIMPLEMENTED();
+  }
+  void GetProperties(
+      const std::string& service_path,
+      const chromeos::network_handler::DictionaryResultCallback& callback,
+      const chromeos::network_handler::ErrorCallback& error_callback) override {
+    NOTIMPLEMENTED();
+  }
+  void GetManagedProperties(
+      const std::string& userhash,
+      const std::string& service_path,
+      const chromeos::network_handler::DictionaryResultCallback& callback,
+      const chromeos::network_handler::ErrorCallback& error_callback) override {
+    NOTIMPLEMENTED();
+  }
+  void SetProperties(
+      const std::string& service_path,
+      const base::DictionaryValue& user_settings,
+      const base::Closure& callback,
+      const chromeos::network_handler::ErrorCallback& error_callback) const
+      override {
+    NOTIMPLEMENTED();
+  }
+  void CreateConfiguration(
+      const std::string& userhash,
+      const base::DictionaryValue& properties,
+      const chromeos::network_handler::StringResultCallback& callback,
+      const chromeos::network_handler::ErrorCallback& error_callback) const
+      override {
+    ++create_configuration_count_;

[erikwright (departed), 2015/01/15 15:12:51]

You don't really need a call count, since your tests each exercise a single
invocation. But you should be testing the success/failure callbacks of your code
under test.

The simplest way to do it, then, would be to declare members:

bool called_;  // init to false
StringResultCallback callback_;
ErrorCallback error_callback_;

And in this method:

EXPECT_FALSE(called_);
called_ = true;
callback_ = callback;
error_callback_ = error_callback;

And then in your test:

ASSERT_TRUE(create_configuration_called());
if (!callback_.is_null())
  callback_.Run()

And also have at least one test that invokes the error callback.

Note that this is actually an example of code that is much easier to test using
the mock-based strategy here, than it would have been with the more complex
initial test strategy. Consider this in the perspective of Bruce Dawson's talk
yesterday where he pointed out that error handling code is rife with bugs
because it is rarely tested.

[mukesh agrawal, 2015/01/15 19:05:55]

Done.

Thanks for the detailed advice here. I've updated the CL accordingly.

I definitely see your point about exercising the error callback. But I'd like to
ask a question about the need to exercise the success callback.

Namely: given that the the current implementation always passes a null success
callback, why do we want to exercise the success callback in the unit tests? Is
it, e.g., to a) more fully document how the ManagedNetworkConfigurationHandler
interacts with this class, b) to allow for future changes that use non-null
success callbacks, or c) something else?

[erikwright (departed), 2015/01/16 14:42:15]

The fact that the success callback is null is not a part of the public contract
of the class under test. It can change in the future. You could easily see
someone adding code to log something, or other purposes in the future.

+  }
+  void RemoveConfiguration(
+      const std::string& service_path,
+      const base::Closure& callback,
+      const chromeos::network_handler::ErrorCallback& error_callback) const
+      override {
+    NOTIMPLEMENTED();
+  }
+  void SetPolicy(
+      ::onc::ONCSource onc_source,
+      const std::string& userhash,
+      const base::ListValue& network_configs_onc,
+      const base::DictionaryValue& global_network_config) override {
+    NOTIMPLEMENTED();
+  }
+  bool IsAnyPolicyApplicationRunning() const override {
+    NOTIMPLEMENTED();
+    return false;
+  }
+  const base::DictionaryValue* FindPolicyByGUID(
+      const std::string userhash,
+      const std::string& guid,
+      ::onc::ONCSource* onc_source) const override {
+    NOTIMPLEMENTED();
+    return nullptr;
+  }
+  const GuidToPolicyMap* GetNetworkConfigsFromPolicy(
+      const std::string& userhash) const override {
+    NOTIMPLEMENTED();
+    return nullptr;
+  }
+  const base::DictionaryValue* GetGlobalConfigFromPolicy(
+      const std::string& userhash) const override {
+    NOTIMPLEMENTED();
+    return nullptr;
+  }
+  const base::DictionaryValue* FindPolicyByGuidAndProfile(
+      const std::string& guid,
+      const std::string& profile_path) const override {
+    NOTIMPLEMENTED();
+    return nullptr;
+  }
+
+  // Returns the number of times CreateConfiguration has been called.
+  size_t GetCreateConfigurationCount() const {

[erikwright (departed), 2015/01/15 15:12:51]

nit: accessor style:

size_t create_configuration_call_count() const {}

[mukesh agrawal, 2015/01/15 19:05:55]

Done.

+    return create_configuration_count_;
+  }
+
+ private:
+  // The number of times CreateConfiguration has been called on this fake.
+  mutable size_t create_configuration_count_;
+};
+
+class WifiConfigDelegateChromeOsTest : public testing::Test {
+ protected:
+  WifiConfigDelegateChromeOsTest()
+      : fake_managed_network_configuration_handler_(
+          new FakeManagedNetworkConfigurationHandler()) {
+    config_delegate_.reset(
+        new WifiConfigDelegateChromeOs(
+            kUserHash,
+            fake_managed_network_configuration_handler_.get()));
+  }
+
+  // Wrapper for WifiConfigDelegateChromeOs::AddToLocalNetworks.
+  void AddToLocalNetworks(const WifiCredential& network_credential) {
+    config_delegate_->AddToLocalNetworks(network_credential);
+  }
+
+  // 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;
+  }
+
+  // Returns the number of times CreateConfiguration has been called
+  // on |fake_managed_network_configuration_handler_|.
+  size_t GetCreateConfigurationCount() const {

[erikwright (departed), 2015/01/15 15:12:50]

nit: accessor style: create_configuration_call_count

[mukesh agrawal, 2015/01/15 19:05:55]

Done.

+    return fake_managed_network_configuration_handler_
+        ->GetCreateConfigurationCount();
+  }
+
+ private:
+  scoped_ptr<WifiConfigDelegateChromeOs> config_delegate_;
+  scoped_ptr<FakeManagedNetworkConfigurationHandler>
+    fake_managed_network_configuration_handler_;
+
+  DISALLOW_COPY_AND_ASSIGN(WifiConfigDelegateChromeOsTest);
+};
+
+TEST_F(WifiConfigDelegateChromeOsTest, AddToLocalNetworksOpen) {
+  AddToLocalNetworks(MakeCredential(kSsid, SECURITY_CLASS_NONE, ""));
+  EXPECT_EQ(1U, GetCreateConfigurationCount());
+}
+
+TEST_F(WifiConfigDelegateChromeOsTest, AddToLocalNetworksWep) {
+  AddToLocalNetworks(MakeCredential(kSsid, SECURITY_CLASS_WEP, "abcde"));
+  EXPECT_EQ(1U, GetCreateConfigurationCount());
+}
+
+TEST_F(WifiConfigDelegateChromeOsTest, AddToLocalNetworksPsk) {
+  AddToLocalNetworks(
+      MakeCredential(kSsid, SECURITY_CLASS_PSK, "fake-psk-passphrase"));
+  EXPECT_EQ(1U, GetCreateConfigurationCount());
+}
+
+TEST_F(WifiConfigDelegateChromeOsTest, AddToLocalNetworksNonUtf8) {
+  AddToLocalNetworks(MakeCredential(kSsidNonUtf8, SECURITY_CLASS_PSK, ""));
+  // TODO(quiche): Change 0 to 1, once we support non-UTF-8 SSIDs.
+  EXPECT_EQ(0U, GetCreateConfigurationCount());
+}
+
+}  // namespace wifi_sync