Add new shill client for VPN

chromeos/dbus/shill_client_unittest_base.h

 // 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.
 
 #ifndef CHROMEOS_DBUS_SHILL_CLIENT_UNITTEST_BASE_H_
 #define CHROMEOS_DBUS_SHILL_CLIENT_UNITTEST_BASE_H_
 
 #include <string>
 
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "chromeos/dbus/dbus_method_call_status.h"
 #include "chromeos/dbus/shill_client_helper.h"
 #include "chromeos/dbus/shill_property_changed_observer.h"
+#include "chromeos/dbus/shill_third_party_vpn_observer.h"
 #include "dbus/mock_bus.h"
 #include "dbus/mock_object_proxy.h"
 #include "dbus/object_proxy.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::MatcherInterface;
 using ::testing::MatchResultListener;
 using ::testing::Matcher;
 using ::testing::MakeMatcher;
 
@@ skipping 64 matching lines @@
   // A mock PropertyChangedObserver that can be used to check expected values.
   class MockPropertyChangeObserver
       : public ShillPropertyChangedObserver {
    public:
     MockPropertyChangeObserver();
     ~MockPropertyChangeObserver();
     MOCK_METHOD2(OnPropertyChanged, void(const std::string& name,
                                          const base::Value& value));
   };
 
+  class MockShillThirdPartyVpnObserver : public ShillThirdPartyVpnObserver {

[pneubeck (no reviews), 2014/11/11 13:09:35]

can be moved to shill_third_party_vpn_driver_client_unittest.cc

[kaliamoorthi, 2014/11/11 14:58:33]

This is inline with the existing client. I understand this is arguable, but
there is no compelling reason to not have it here.

[pneubeck (no reviews), 2014/11/12 14:21:06]

IMO, you need a compelling reason to break locality and not the other way round.

Handing off to Steven, who is the primary maintainer of this class, to decide.

[stevenjb, 2014/11/12 18:05:13]

This observer doesn't belong here. MockPropertyChangeObserver is shared by
multiple tests, this is not. You should be able to put it in an anonymous
namespace in the unittest.

+   public:
+    MockShillThirdPartyVpnObserver();
+    ~MockShillThirdPartyVpnObserver();
+    MOCK_METHOD2(OnPacketReceived, void(const uint8* data, size_t length));
+    MOCK_METHOD1(OnPlatformMessage, void(uint32 message));
+  };
+
   explicit ShillClientUnittestBase(const std::string& interface_name,
                                       const dbus::ObjectPath& object_path);
   virtual ~ShillClientUnittestBase();
 
   virtual void SetUp() override;
   virtual void TearDown() override;
 
  protected:
   // A callback to intercept and check the method call arguments.
   typedef base::Callback<void(
       dbus::MessageReader* reader)> ArgumentCheckCallback;
 
   // Sets expectations for called method name and arguments, and sets response.
   void PrepareForMethodCall(const std::string& method_name,
                             const ArgumentCheckCallback& argument_checker,
                             dbus::Response* response);
 
+  // Sends platform message signal to the tested client.
+  void SendPlatformMessageSignal(dbus::Signal* signal);

[pneubeck (no reviews), 2014/11/11 13:09:35]

Why are these new functions/members part of the base class if it's only used by 
shill_third_party_vpn_driver_client_unittest.cc ?

this class is already very unstructured and crowded. i think it would be better
to move this and all VPNDriver specific parts to the test fixture of
shill_third_party_vpn_driver_client_unittest.cc.

[kaliamoorthi, 2014/11/11 14:58:33]

This is inline with the existing client. I understand this is arguable, but
there is no compelling reason to not have it here.

[pneubeck (no reviews), 2014/11/12 14:21:06]

Again, IMO, there is no reason why locality should be broken in this case.

Up to Steven to decide what he prefers.

[stevenjb, 2014/11/12 18:05:13]

This (and the other changes) are fine since they are not vpn specific and could
be used by (and improved for) other tests.

+
+  // Sends packet received signal to the tested client.
+  void SendPacketReceievedSignal(dbus::Signal* signal);
+
   // Sends property changed signal to the tested client.
   void SendPropertyChangedSignal(dbus::Signal* signal);
 
   // Checks the name and the value which are sent by PropertyChanged signal.
   static void ExpectPropertyChanged(const std::string& expected_name,
                                     const base::Value* expected_value,
                                     const std::string& name,
                                     const base::Value& value);
 
   // Expects the reader to be empty.
   static void ExpectNoArgument(dbus::MessageReader* reader);
 
+  // Expects the reader to have a uint32
+  static void ExpectUint32Argument(uint32 expected_value,
+                                   dbus::MessageReader* reader);
+
+  // Expects the reader to have an array of bytes
+  static void ExpectArrayOfBytesArgument(
+      const std::string& expected_bytes,
+      dbus::MessageReader* reader);
+
   // Expects the reader to have a string.
   static void ExpectStringArgument(const std::string& expected_string,
                                    dbus::MessageReader* reader);
 
   static void ExpectArrayOfStringsArgument(
       const std::vector<std::string>& expected_strings,
       dbus::MessageReader* reader);
 
   // Expects the reader to have a Value.
   static void ExpectValueArgument(const base::Value* expected_value,
                                   dbus::MessageReader* reader);
 
   // Expects the reader to have a string and a Value.
   static void ExpectStringAndValueArguments(const std::string& expected_string,
                                             const base::Value* expected_value,
                                             dbus::MessageReader* reader);
 
   // Expects the reader to have a string-to-variant dictionary.
   static void ExpectDictionaryValueArgument(
       const base::DictionaryValue* expected_dictionary,
+      bool string_valued,
       dbus::MessageReader* reader);
 
   // Creates a DictionaryValue with example Service properties. The caller owns
   // the result.
   static base::DictionaryValue* CreateExampleServiceProperties();
 
   // Expects the call status to be SUCCESS.
   static void ExpectNoResultValue(DBusMethodCallStatus call_status);
 
   // Checks the result and expects the call status to be SUCCESS.
@@ skipping 25 matching lines @@
       const base::DictionaryValue& result);
 
   // A message loop to emulate asynchronous behavior.
   base::MessageLoop message_loop_;
   // The mock bus.
   scoped_refptr<dbus::MockBus> mock_bus_;
 
  private:
   // Checks the requested interface name and signal name.
   // Used to implement the mock proxy.
-  void OnConnectToSignal(
+  void OnConnectToPlatformMessage(
       const std::string& interface_name,
       const std::string& signal_name,
       const dbus::ObjectProxy::SignalCallback& signal_callback,
+      const dbus::ObjectProxy::OnConnectedCallback& on_connected_callback);
+
+  // Checks the requested interface name and signal name.
+  // Used to implement the mock proxy.
+  void OnConnectToPacketReceieved(

[pneubeck (no reviews), 2014/11/11 13:09:35]

typo: Receieved -> Received

[kaliamoorthi, 2014/11/11 14:58:32]

Done.

+      const std::string& interface_name,
+      const std::string& signal_name,
+      const dbus::ObjectProxy::SignalCallback& signal_callback,
+      const dbus::ObjectProxy::OnConnectedCallback& on_connected_callback);
+
+  // Checks the requested interface name and signal name.
+  // Used to implement the mock proxy.
+  void OnConnectToPropertyChanged(
+      const std::string& interface_name,
+      const std::string& signal_name,
+      const dbus::ObjectProxy::SignalCallback& signal_callback,
       const dbus::ObjectProxy::OnConnectedCallback& on_connected_callback);
 
   // Checks the content of the method call and returns the response.
   // Used to implement the mock proxy.
   void OnCallMethod(
       dbus::MethodCall* method_call,
       int timeout_ms,
       const dbus::ObjectProxy::ResponseCallback& response_callback);
 
   // Checks the content of the method call and returns the response.
   // Used to implement the mock proxy.
   void OnCallMethodWithErrorCallback(
       dbus::MethodCall* method_call,
       int timeout_ms,
       const dbus::ObjectProxy::ResponseCallback& response_callback,
       const dbus::ObjectProxy::ErrorCallback& error_callback);
 
   // The interface name.
   const std::string interface_name_;
   // The object path.
   const dbus::ObjectPath object_path_;
   // The mock object proxy.
   scoped_refptr<dbus::MockObjectProxy> mock_proxy_;
+  // The PlatformMessage signal handler given by the tested client.
+  dbus::ObjectProxy::SignalCallback platform_message_handler_;
+  // The PacketReceived signal handler given by the tested client.
+  dbus::ObjectProxy::SignalCallback packet_receieved__handler_;
   // The PropertyChanged signal handler given by the tested client.
   dbus::ObjectProxy::SignalCallback property_changed_handler_;
   // The name of the method which is expected to be called.
   std::string expected_method_name_;
   // The response which the mock object proxy returns.
   dbus::Response* response_;
   // A callback to intercept and check the method call arguments.
   ArgumentCheckCallback argument_checker_;
 };
 
 }  // namespace chromeos
 
 #endif  // CHROMEOS_DBUS_SHILL_CLIENT_UNITTEST_BASE_H_