Add new shill client for VPN

chromeos/dbus/shill_client_unittest_base.h

Index: chromeos/dbus/shill_client_unittest_base.h
diff --git a/chromeos/dbus/shill_client_unittest_base.h b/chromeos/dbus/shill_client_unittest_base.h
index 8eac510605ad0d59d7e57a4d93f4523795361b7a..c322cf3c89842695deafcd5178ab714b3695dbb5 100644
--- a/chromeos/dbus/shill_client_unittest_base.h
+++ b/chromeos/dbus/shill_client_unittest_base.h
@@ -13,6 +13,7 @@
 #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"
@@ -97,6 +98,14 @@ class ShillClientUnittestBase : public testing::Test {
                                          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();
@@ -114,6 +123,12 @@ class ShillClientUnittestBase : public testing::Test {
                             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);
 
@@ -126,6 +141,15 @@ class ShillClientUnittestBase : public testing::Test {
   // 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);
@@ -146,6 +170,7 @@ class ShillClientUnittestBase : public testing::Test {
   // 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
@@ -191,7 +216,23 @@ class ShillClientUnittestBase : public testing::Test {
  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,
@@ -218,6 +259,10 @@ class ShillClientUnittestBase : public testing::Test {
   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.