shill: Replace TestProxyFactory variants with MockProxyFactory.
This CL reduces the number of TestProxyFactory variants in unit tests as
follows:
- Add a ReturnAndReleasePointee Google Mock action, which can be used
with EXPECT_CALL on MockProxyFactory, to mimic how TestProxyFactory
handles the scoped_ptr of mock proxies.
- Use MockProxyFactor with EXPECT_CALL to mimic
TestProxyFactory::Create*Proxy methods.
- Check arguments passed to MockProxyFactory::Create*Proxy() via
EXPECT_CALL.
BUG=chromium:338623
TEST=`FEATURES=test emerge-{x86,amd64,arm}-generic shill`
Change-Id: Ieb0689f76feb105a2d1efaf1e7c52b4bc15fb8d7
Reviewed-on: https://chromium-review.googlesource.com/184033
Reviewed-by: Paul Stewart <pstew@chromium.org>
Tested-by: Ben Chan <benchan@chromium.org>
Commit-Queue: Ben Chan <benchan@chromium.org>
diff --git a/dbus_manager_unittest.cc b/dbus_manager_unittest.cc
index 908a21b..10cad6e 100644
--- a/dbus_manager_unittest.cc
+++ b/dbus_manager_unittest.cc
@@ -6,12 +6,11 @@
#include <base/bind.h>
#include <base/memory/weak_ptr.h>
-#include <gmock/gmock.h>
-#include <gtest/gtest.h>
#include "shill/error.h"
#include "shill/mock_dbus_service_proxy.h"
-#include "shill/proxy_factory.h"
+#include "shill/mock_proxy_factory.h"
+#include "shill/testing.h"
using base::Bind;
using base::Unretained;
@@ -40,7 +39,6 @@
public:
DBusManagerTest()
: proxy_(new MockDBusServiceProxy()),
- proxy_factory_(this),
manager_(new DBusManager()) {}
virtual void SetUp() {
@@ -49,20 +47,6 @@
}
protected:
- class TestProxyFactory : public ProxyFactory {
- public:
- explicit TestProxyFactory(DBusManagerTest *test) : test_(test) {}
-
- virtual DBusServiceProxyInterface *CreateDBusServiceProxy() {
- return test_->proxy_.release();
- }
-
- private:
- DBusManagerTest *test_;
-
- DISALLOW_COPY_AND_ASSIGN(TestProxyFactory);
- };
-
class DBusNameWatcherCallbackObserver {
public:
DBusNameWatcherCallbackObserver()
@@ -96,13 +80,19 @@
DISALLOW_COPY_AND_ASSIGN(DBusNameWatcherCallbackObserver);
};
+ MockDBusServiceProxy *ExpectCreateDBusServiceProxy() {
+ EXPECT_CALL(proxy_factory_, CreateDBusServiceProxy())
+ .WillOnce(ReturnAndReleasePointee(&proxy_));
+ return proxy_.get();
+ }
+
scoped_ptr<MockDBusServiceProxy> proxy_;
- TestProxyFactory proxy_factory_;
+ MockProxyFactory proxy_factory_;
scoped_ptr<DBusManager> manager_;
};
TEST_F(DBusManagerTest, GetNameOwnerFails) {
- MockDBusServiceProxy *proxy = proxy_.get();
+ MockDBusServiceProxy *proxy = ExpectCreateDBusServiceProxy();
EXPECT_CALL(*proxy, set_name_owner_changed_callback(_));
manager_->Start();
@@ -123,7 +113,7 @@
TEST_F(DBusManagerTest,
GetNameOwnerReturnsAfterDBusManagerAndNameWatcherDestroyed)
{
- MockDBusServiceProxy *proxy = proxy_.get();
+ MockDBusServiceProxy *proxy = ExpectCreateDBusServiceProxy();
EXPECT_CALL(*proxy, set_name_owner_changed_callback(_));
manager_->Start();
@@ -148,7 +138,7 @@
}
TEST_F(DBusManagerTest, NameWatchers) {
- MockDBusServiceProxy *proxy = proxy_.get();
+ MockDBusServiceProxy *proxy = ExpectCreateDBusServiceProxy();
// Start the DBus service manager.
EXPECT_CALL(*proxy, set_name_owner_changed_callback(_));
diff --git a/dhcp_config_unittest.cc b/dhcp_config_unittest.cc
index f094531..cd37627 100644
--- a/dhcp_config_unittest.cc
+++ b/dhcp_config_unittest.cc
@@ -9,7 +9,6 @@
#include <base/files/scoped_temp_dir.h>
#include <base/stringprintf.h>
#include <chromeos/dbus/service_constants.h>
-#include <gtest/gtest.h>
#include "shill/dbus_adaptor.h"
#include "shill/dhcp_provider.h"
@@ -19,8 +18,9 @@
#include "shill/mock_glib.h"
#include "shill/mock_log.h"
#include "shill/mock_minijail.h"
+#include "shill/mock_proxy_factory.h"
#include "shill/property_store_unittest.h"
-#include "shill/proxy_factory.h"
+#include "shill/testing.h"
using base::Bind;
using base::FilePath;
@@ -52,7 +52,6 @@
public:
DHCPConfigTest()
: proxy_(new MockDHCPProxy()),
- proxy_factory_(this),
minijail_(new MockMinijail()),
config_(new DHCPConfig(&control_,
dispatcher(),
@@ -86,18 +85,6 @@
bool lease_file_exists);
protected:
- class TestProxyFactory : public ProxyFactory {
- public:
- explicit TestProxyFactory(DHCPConfigTest *test) : test_(test) {}
-
- virtual DHCPProxyInterface *CreateDHCPProxy(const string &/*service*/) {
- return test_->proxy_.release();
- }
-
- private:
- DHCPConfigTest *test_;
- };
-
static const int kPID;
static const unsigned int kTag;
@@ -105,7 +92,7 @@
FilePath pid_file_;
ScopedTempDir temp_dir_;
scoped_ptr<MockDHCPProxy> proxy_;
- TestProxyFactory proxy_factory_;
+ MockProxyFactory proxy_factory_;
MockControl control_;
scoped_ptr<MockMinijail> minijail_;
DHCPConfigRefPtr config_;
@@ -195,6 +182,8 @@
static const char kService[] = ":1.200";
EXPECT_TRUE(proxy_.get());
EXPECT_FALSE(config_->proxy_.get());
+ EXPECT_CALL(proxy_factory_, CreateDHCPProxy(kService))
+ .WillOnce(ReturnAndReleasePointee(&proxy_));
config_->InitProxy(kService);
EXPECT_FALSE(proxy_.get());
EXPECT_TRUE(config_->proxy_.get());
diff --git a/ethernet_unittest.cc b/ethernet_unittest.cc
index 533382c..56a3588 100644
--- a/ethernet_unittest.cc
+++ b/ethernet_unittest.cc
@@ -9,8 +9,6 @@
#include <base/memory/ref_counted.h>
#include <base/memory/scoped_ptr.h>
-#include <gmock/gmock.h>
-#include <gtest/gtest.h>
#include "shill/mock_device_info.h"
#include "shill/mock_dhcp_config.h"
@@ -24,12 +22,14 @@
#include "shill/mock_log.h"
#include "shill/mock_manager.h"
#include "shill/mock_metrics.h"
+#include "shill/mock_proxy_factory.h"
#include "shill/mock_rtnl_handler.h"
#include "shill/mock_service.h"
#include "shill/mock_supplicant_interface_proxy.h"
#include "shill/mock_supplicant_process_proxy.h"
#include "shill/nice_mock_control.h"
-#include "shill/proxy_factory.h"
+#include "shill/testing.h"
+#include "shill/wpa_supplicant.h"
using std::string;
using testing::_;
@@ -67,7 +67,6 @@
&dispatcher_,
&metrics_,
&manager_)),
- proxy_factory_(this),
supplicant_interface_proxy_(
new NiceMock<MockSupplicantInterfaceProxy>()),
supplicant_process_proxy_(new NiceMock<MockSupplicantProcessProxy>()) {}
@@ -93,31 +92,6 @@
}
protected:
- class TestProxyFactory : public ProxyFactory {
- public:
- explicit TestProxyFactory(EthernetTest *test) : test_(test) {}
-
- virtual SupplicantProcessProxyInterface *CreateSupplicantProcessProxy(
- const char */*dbus_path*/, const char */*dbus_addr*/) {
- return test_->supplicant_process_proxy_.release();
- }
-
- virtual SupplicantInterfaceProxyInterface *CreateSupplicantInterfaceProxy(
- SupplicantEventDelegateInterface */*delegate*/,
- const DBus::Path &/*object_path*/,
- const char */*dbus_addr*/) {
- return test_->supplicant_interface_proxy_.release();
- }
-
- MOCK_METHOD2(CreateSupplicantNetworkProxy,
- SupplicantNetworkProxyInterface *(
- const DBus::Path &object_path,
- const char *dbus_addr));
-
- private:
- EthernetTest *test_;
- };
-
static const char kDeviceName[];
static const char kDeviceAddress[];
static const char kInterfacePath[];
@@ -170,14 +144,15 @@
return ethernet_->StartEapAuthentication();
}
void StartSupplicant() {
- SupplicantInterfaceProxyInterface *interface =
- supplicant_interface_proxy_.get();
- SupplicantProcessProxyInterface *process = supplicant_process_proxy_.get();
- EXPECT_CALL(*supplicant_process_proxy_.get(), CreateInterface(_))
+ MockSupplicantInterfaceProxy *interface_proxy =
+ ExpectCreateSupplicantInterfaceProxy();
+ MockSupplicantProcessProxy *process_proxy =
+ ExpectCreateSupplicantProcessProxy();
+ EXPECT_CALL(*process_proxy, CreateInterface(_))
.WillOnce(Return(kInterfacePath));
EXPECT_TRUE(InvokeStartSupplicant());
- EXPECT_EQ(interface, GetSupplicantInterfaceProxy());
- EXPECT_EQ(process, GetSupplicantProcessProxy());
+ EXPECT_EQ(interface_proxy, GetSupplicantInterfaceProxy());
+ EXPECT_EQ(process_proxy, GetSupplicantProcessProxy());
EXPECT_EQ(kInterfacePath, GetSupplicantInterfacePath());
}
void TriggerOnEapDetected() { ethernet_->OnEapDetected(); }
@@ -188,6 +163,21 @@
ethernet_->TryEapAuthenticationTask();
}
+ MockSupplicantInterfaceProxy *ExpectCreateSupplicantInterfaceProxy() {
+ EXPECT_CALL(proxy_factory_,
+ CreateSupplicantInterfaceProxy(_, DBus::Path(kInterfacePath),
+ StrEq(WPASupplicant::kDBusAddr)))
+ .WillOnce(ReturnAndReleasePointee(&supplicant_interface_proxy_));
+ return supplicant_interface_proxy_.get();
+ }
+ MockSupplicantProcessProxy *ExpectCreateSupplicantProcessProxy() {
+ EXPECT_CALL(proxy_factory_,
+ CreateSupplicantProcessProxy(StrEq(WPASupplicant::kDBusPath),
+ StrEq(WPASupplicant::kDBusAddr)))
+ .WillOnce(ReturnAndReleasePointee(&supplicant_process_proxy_));
+ return supplicant_process_proxy_.get();
+ }
+
StrictMock<MockEventDispatcher> dispatcher_;
MockGLib glib_;
NiceMockControl control_interface_;
@@ -205,7 +195,7 @@
MockRTNLHandler rtnl_handler_;
scoped_refptr<MockEthernetService> mock_service_;
scoped_refptr<MockService> mock_eap_service_;
- NiceMock<TestProxyFactory> proxy_factory_;
+ NiceMock<MockProxyFactory> proxy_factory_;
scoped_ptr<MockSupplicantInterfaceProxy> supplicant_interface_proxy_;
scoped_ptr<MockSupplicantProcessProxy> supplicant_process_proxy_;
};
@@ -378,7 +368,7 @@
EXPECT_CALL(*process_proxy, CreateInterface(_)).Times(0);
EXPECT_TRUE(InvokeStartSupplicant());
- // Also, the mock pointers should remain; if the TestProxyFactory was
+ // Also, the mock pointers should remain; if the MockProxyFactory was
// invoked again, they would be NULL.
EXPECT_EQ(interface_proxy, GetSupplicantInterfaceProxy());
EXPECT_EQ(process_proxy, GetSupplicantProcessProxy());
@@ -386,9 +376,10 @@
}
TEST_F(EthernetTest, StartSupplicantWithInterfaceExistsException) {
+ MockSupplicantProcessProxy *process_proxy =
+ ExpectCreateSupplicantProcessProxy();
MockSupplicantInterfaceProxy *interface_proxy =
- supplicant_interface_proxy_.get();
- MockSupplicantProcessProxy *process_proxy = supplicant_process_proxy_.get();
+ ExpectCreateSupplicantInterfaceProxy();
EXPECT_CALL(*process_proxy, CreateInterface(_))
.WillOnce(Throw(DBus::Error(
"fi.w1.wpa_supplicant1.InterfaceExists",
@@ -402,7 +393,8 @@
}
TEST_F(EthernetTest, StartSupplicantWithUnknownException) {
- MockSupplicantProcessProxy *process_proxy = supplicant_process_proxy_.get();
+ MockSupplicantProcessProxy *process_proxy =
+ ExpectCreateSupplicantProcessProxy();
EXPECT_CALL(*process_proxy, CreateInterface(_))
.WillOnce(Throw(DBus::Error(
"fi.w1.wpa_supplicant1.UnknownError",
diff --git a/manager_unittest.cc b/manager_unittest.cc
index 1ba75f7..4a2008f 100644
--- a/manager_unittest.cc
+++ b/manager_unittest.cc
@@ -38,6 +38,7 @@
#include "shill/mock_metrics.h"
#include "shill/mock_power_manager.h"
#include "shill/mock_profile.h"
+#include "shill/mock_proxy_factory.h"
#include "shill/mock_resolver.h"
#include "shill/mock_service.h"
#include "shill/mock_store.h"
@@ -45,7 +46,6 @@
#include "shill/mock_wifi_service.h"
#include "shill/portal_detector.h"
#include "shill/property_store_unittest.h"
-#include "shill/proxy_factory.h"
#include "shill/resolver.h"
#include "shill/service_under_test.h"
#include "shill/wifi_service.h"
@@ -73,6 +73,7 @@
using ::testing::NiceMock;
using ::testing::Ref;
using ::testing::Return;
+using ::testing::ReturnNull;
using ::testing::ReturnRef;
using ::testing::SaveArg;
using ::testing::SetArgumentPointee;
@@ -95,6 +96,9 @@
wifi_provider_(new NiceMock<MockWiFiProvider>()),
crypto_util_proxy_(new NiceMock<MockCryptoUtilProxy>(dispatcher(),
glib())) {
+ ON_CALL(proxy_factory_, CreatePowerManagerProxy(_))
+ .WillByDefault(ReturnNull());
+
mock_devices_.push_back(new NiceMock<MockDevice>(control_interface(),
dispatcher(),
metrics(),
@@ -310,19 +314,6 @@
DISALLOW_COPY_AND_ASSIGN(ServiceWatcher);
};
- class TestProxyFactory : public ProxyFactory {
- public:
- TestProxyFactory() {}
-
- virtual PowerManagerProxyInterface *CreatePowerManagerProxy(
- PowerManagerProxyDelegate */*delegate*/) {
- return NULL;
- }
-
- private:
- DISALLOW_COPY_AND_ASSIGN(TestProxyFactory);
- };
-
class TerminationActionTest :
public base::SupportsWeakPtr<TerminationActionTest> {
public:
@@ -415,7 +406,7 @@
ethernet_eap_provider_->set_service(service);
}
- TestProxyFactory proxy_factory_;
+ NiceMock<MockProxyFactory> proxy_factory_;
scoped_ptr<MockPowerManager> power_manager_;
vector<scoped_refptr<MockDevice> > mock_devices_;
scoped_ptr<MockDeviceInfo> device_info_;
diff --git a/service_unittest.cc b/service_unittest.cc
index a35e593..cca0f8b 100644
--- a/service_unittest.cc
+++ b/service_unittest.cc
@@ -51,6 +51,7 @@
using testing::Mock;
using testing::NiceMock;
using testing::Return;
+using testing::ReturnNull;
using testing::ReturnRef;
using testing::StrictMock;
using testing::StrNe;
@@ -75,6 +76,9 @@
storage_id_(ServiceUnderTest::kStorageId),
power_manager_(new MockPowerManager(NULL, &proxy_factory_)),
eap_(new MockEapCredentials()) {
+ ON_CALL(proxy_factory_, CreatePowerManagerProxy(_))
+ .WillByDefault(ReturnNull());
+
service_->time_ = &time_;
DefaultValue<Timestamp>::Set(Timestamp());
service_->diagnostics_reporter_ = &diagnostics_reporter_;
@@ -90,19 +94,6 @@
protected:
typedef scoped_refptr<MockProfile> MockProfileRefPtr;
- class TestProxyFactory : public ProxyFactory {
- public:
- TestProxyFactory() {}
-
- virtual PowerManagerProxyInterface *CreatePowerManagerProxy(
- PowerManagerProxyDelegate *delegate) {
- return NULL;
- }
-
- private:
- DISALLOW_COPY_AND_ASSIGN(TestProxyFactory);
- };
-
ServiceMockAdaptor *GetAdaptor() {
return dynamic_cast<ServiceMockAdaptor *>(service_->adaptor());
}
@@ -222,7 +213,7 @@
scoped_refptr<ServiceUnderTest> service_;
scoped_refptr<ServiceUnderTest> service2_;
string storage_id_;
- TestProxyFactory proxy_factory_;
+ NiceMock<MockProxyFactory> proxy_factory_;
MockPowerManager *power_manager_; // Owned by |mock_manager_|.
MockEapCredentials *eap_; // Owned by |service_|.
};
diff --git a/testing.h b/testing.h
new file mode 100644
index 0000000..a452d68
--- /dev/null
+++ b/testing.h
@@ -0,0 +1,36 @@
+// Copyright (c) 2014 The Chromium OS 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 SHILL_TESTING_H_
+#define SHILL_TESTING_H_
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+namespace shill {
+
+// A Google Mock action (similar to testing::ReturnPointee) that takes a pointer
+// to a scoped_ptr object, releases and returns the raw pointer managed by the
+// scoped_ptr object when the action is invoked.
+//
+// Example usage:
+//
+// TEST(FactoryTest, CreateStuff) {
+// MockFactory factory;
+// scoped_ptr<Stuff> stuff(new Stuff());
+// EXPECT_CALL(factory, CreateStuff())
+// .WillOnce(ReturnAndReleasePointee(&stuff));
+// }
+//
+// If |factory.CreateStuff()| is called, the ownership of the Stuff object
+// managed by |stuff| is transferred to the caller of |factory.CreateStuff()|.
+// Otherwise, the Stuff object will be destroyed once |stuff| goes out of
+// scope when the test completes.
+ACTION_P(ReturnAndReleasePointee, scoped_pointer) {
+ return scoped_pointer->release();
+}
+
+} // namespace shill
+
+#endif // SHILL_TESTING_H_