apmanager: add ProcessFactory for better unittesting
Use ProcessMock instead of real process when starting hostapd and
dnsmasq during unittest.
BUG=chromium:431759
TEST=USE="asan clang" FEATURES=test emerge-$BOARD apmanager
Start AP service using apmanager and verify client can connect
to it.
Change-Id: Ifea32a2dd08247db9ea4306ecef5947b305a3c8d
Reviewed-on: https://chromium-review.googlesource.com/239975
Trybot-Ready: Zeping Qiu <zqiu@chromium.org>
Tested-by: Zeping Qiu <zqiu@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Commit-Queue: Zeping Qiu <zqiu@chromium.org>
diff --git a/apmanager.gyp b/apmanager.gyp
index da70d22..87255a5 100644
--- a/apmanager.gyp
+++ b/apmanager.gyp
@@ -57,6 +57,7 @@
'dhcp_server_factory.cc',
'file_writer.cc',
'manager.cc',
+ 'process_factory.cc',
'service.cc',
'shill_proxy.cc',
],
@@ -107,6 +108,7 @@
'mock_dhcp_server_factory.cc',
'mock_file_writer.cc',
'mock_manager.cc',
+ 'mock_process_factory.cc',
'mock_service.cc',
'service_unittest.cc',
'testrunner.cc',
diff --git a/dhcp_server.cc b/dhcp_server.cc
index 99c74d3..1b3bcf2 100644
--- a/dhcp_server.cc
+++ b/dhcp_server.cc
@@ -33,7 +33,8 @@
interface_name_(interface_name),
server_address_(shill::IPAddress::kFamilyIPv4),
rtnl_handler_(shill::RTNLHandler::GetInstance()),
- file_writer_(FileWriter::GetInstance()) {}
+ file_writer_(FileWriter::GetInstance()),
+ process_factory_(ProcessFactory::GetInstance()) {}
DHCPServer::~DHCPServer() {
if (dnsmasq_process_) {
@@ -73,9 +74,8 @@
shill::IPAddress(shill::IPAddress::kFamilyIPv4));
rtnl_handler_->SetInterfaceFlags(interface_index, IFF_UP, IFF_UP);
- // TODO(zqiu): use ProcessFactory for better unit testing.
// Start a dnsmasq process.
- dnsmasq_process_.reset(new chromeos::ProcessImpl());
+ dnsmasq_process_.reset(process_factory_->CreateProcess());
dnsmasq_process_->AddArg(kDnsmasqPath);
dnsmasq_process_->AddArg(base::StringPrintf("--conf-file=%s",
file_name.c_str()));
diff --git a/dhcp_server.h b/dhcp_server.h
index 4b26714..f50567a 100644
--- a/dhcp_server.h
+++ b/dhcp_server.h
@@ -8,11 +8,11 @@
#include <string>
#include <base/macros.h>
-#include <chromeos/process.h>
#include <shill/net/ip_address.h>
#include <shill/net/rtnl_handler.h>
#include "apmanager/file_writer.h"
+#include "apmanager/process_factory.h"
namespace apmanager {
@@ -45,6 +45,7 @@
std::unique_ptr<chromeos::Process> dnsmasq_process_;
shill::RTNLHandler* rtnl_handler_;
FileWriter* file_writer_;
+ ProcessFactory* process_factory_;
DISALLOW_COPY_AND_ASSIGN(DHCPServer);
};
diff --git a/dhcp_server_unittest.cc b/dhcp_server_unittest.cc
index 76ab959..dd90546 100644
--- a/dhcp_server_unittest.cc
+++ b/dhcp_server_unittest.cc
@@ -16,6 +16,7 @@
#include <shill/net/mock_rtnl_handler.h>
#include "apmanager/mock_file_writer.h"
+#include "apmanager/mock_process_factory.h"
using chromeos::ProcessMock;
using ::testing::_;
@@ -46,11 +47,15 @@
public:
DHCPServerTest()
: dhcp_server_(new DHCPServer(kServerAddressIndex, kTestInterfaceName)),
- rtnl_handler_(new shill::MockRTNLHandler()) {}
+ rtnl_handler_(new shill::MockRTNLHandler()),
+ file_writer_(MockFileWriter::GetInstance()),
+ process_factory_(MockProcessFactory::GetInstance()) {}
virtual ~DHCPServerTest() {}
virtual void SetUp() {
dhcp_server_->rtnl_handler_ = rtnl_handler_.get();
+ dhcp_server_->file_writer_ = file_writer_;
+ dhcp_server_->process_factory_ = process_factory_;
}
virtual void TearDown() {
@@ -69,13 +74,11 @@
return dhcp_server_->GenerateConfigFile();
}
- void SetFileWriter(FileWriter* file_writer) {
- dhcp_server_->file_writer_ = file_writer;
- }
-
protected:
std::unique_ptr<DHCPServer> dhcp_server_;
std::unique_ptr<shill::MockRTNLHandler> rtnl_handler_;
+ MockFileWriter* file_writer_;
+ MockProcessFactory* process_factory_;
};
@@ -94,12 +97,10 @@
}
TEST_F(DHCPServerTest, StartSuccess) {
- // Setup mock file writer.
- MockFileWriter* file_writer = MockFileWriter::GetInstance();
- SetFileWriter(file_writer);
+ ProcessMock* process = new ProcessMock();
const int kInterfaceIndex = 1;
- EXPECT_CALL(*file_writer,
+ EXPECT_CALL(*file_writer_,
Write(kDnsmasqConfigFilePath, kExpectedDnsmasqConfigFile))
.WillOnce(Return(true));
EXPECT_CALL(*rtnl_handler_.get(), GetInterfaceIndex(kTestInterfaceName))
@@ -108,10 +109,8 @@
AddInterfaceAddress(kInterfaceIndex, _, _, _)).Times(1);
EXPECT_CALL(*rtnl_handler_.get(),
SetInterfaceFlags(kInterfaceIndex, IFF_UP, IFF_UP)).Times(1);
- // This will attempt to start a real dnsmasq process, and won't cause any
- // trouble since the interface name specified is invalid. This can be avoid
- // once we move to use ProcessFactory for process creation, which allows us
- // to use the MockProcess instead of the real Process.
+ EXPECT_CALL(*process_factory_, CreateProcess()).WillOnce(Return(process));
+ EXPECT_CALL(*process, Start()).WillOnce(Return(true));
EXPECT_TRUE(dhcp_server_->Start());
Mock::VerifyAndClearExpectations(rtnl_handler_.get());
}
diff --git a/mock_process_factory.cc b/mock_process_factory.cc
new file mode 100644
index 0000000..587c7cd
--- /dev/null
+++ b/mock_process_factory.cc
@@ -0,0 +1,21 @@
+// Copyright 2015 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.
+
+#include "apmanager/mock_process_factory.h"
+
+namespace apmanager {
+
+namespace {
+base::LazyInstance<MockProcessFactory> g_mock_process_factory
+ = LAZY_INSTANCE_INITIALIZER;
+} // namespace
+
+MockProcessFactory::MockProcessFactory() {}
+MockProcessFactory::~MockProcessFactory() {}
+
+MockProcessFactory* MockProcessFactory::GetInstance() {
+ return g_mock_process_factory.Pointer();
+}
+
+} // namespace apmanager
diff --git a/mock_process_factory.h b/mock_process_factory.h
new file mode 100644
index 0000000..4a598ce
--- /dev/null
+++ b/mock_process_factory.h
@@ -0,0 +1,35 @@
+// Copyright 2015 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 APMANAGER_MOCK_PROCESS_FACTORY_H_
+#define APMANAGER_MOCK_PROCESS_FACTORY_H_
+
+#include <base/lazy_instance.h>
+#include <gmock/gmock.h>
+
+#include "apmanager/process_factory.h"
+
+namespace apmanager {
+
+class MockProcessFactory : public ProcessFactory {
+ public:
+ ~MockProcessFactory() override;
+
+ // This is a singleton. Use MockDHCPServerFactory::GetInstance()->Foo().
+ static MockProcessFactory* GetInstance();
+
+ MOCK_METHOD0(CreateProcess, chromeos::Process*());
+
+ protected:
+ MockProcessFactory();
+
+ private:
+ friend struct base::DefaultLazyInstanceTraits<MockProcessFactory>;
+
+ DISALLOW_COPY_AND_ASSIGN(MockProcessFactory);
+};
+
+} // namespace apmanager
+
+#endif // APMANAGER_MOCK_PROCESS_FACTORY_H_
diff --git a/process_factory.cc b/process_factory.cc
new file mode 100644
index 0000000..f7456a3
--- /dev/null
+++ b/process_factory.cc
@@ -0,0 +1,27 @@
+// Copyright 2015 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.
+
+#include "apmanager/process_factory.h"
+
+namespace apmanager {
+
+namespace {
+
+base::LazyInstance<ProcessFactory> g_process_factory
+ = LAZY_INSTANCE_INITIALIZER;
+
+} // namespace
+
+ProcessFactory::ProcessFactory() {}
+ProcessFactory::~ProcessFactory() {}
+
+ProcessFactory* ProcessFactory::GetInstance() {
+ return g_process_factory.Pointer();
+}
+
+chromeos::Process* ProcessFactory::CreateProcess() {
+ return new chromeos::ProcessImpl();
+}
+
+} // namespace apmanager
diff --git a/process_factory.h b/process_factory.h
new file mode 100644
index 0000000..967dc67
--- /dev/null
+++ b/process_factory.h
@@ -0,0 +1,36 @@
+// Copyright 2015 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 APMANAGER_PROCESS_FACTORY_H_
+#define APMANAGER_PROCESS_FACTORY_H_
+
+#include <string>
+
+#include <base/lazy_instance.h>
+
+#include <chromeos/process.h>
+
+namespace apmanager {
+
+class ProcessFactory {
+ public:
+ virtual ~ProcessFactory();
+
+ // This is a singleton. Use ProcessFactory::GetInstance()->Foo().
+ static ProcessFactory* GetInstance();
+
+ virtual chromeos::Process* CreateProcess();
+
+ protected:
+ ProcessFactory();
+
+ private:
+ friend struct base::DefaultLazyInstanceTraits<ProcessFactory>;
+
+ DISALLOW_COPY_AND_ASSIGN(ProcessFactory);
+};
+
+} // namespace apmanager
+
+#endif // APMANAGER_PROCESS_FACTORY_H_
diff --git a/service.cc b/service.cc
index 5caa449..dc6d26e 100644
--- a/service.cc
+++ b/service.cc
@@ -23,6 +23,8 @@
const char Service::kHostapdPath[] = "/usr/sbin/hostapd";
const char Service::kHostapdConfigPathFormat[] =
"/var/run/apmanager/hostapd/hostapd-%d.conf";
+const char Service::kHostapdControlInterfacePath[] =
+ "/var/run/apmanager/hostapd/ctrl_iface";
const int Service::kTerminationTimeoutSeconds = 2;
Service::Service(Manager* manager, int service_identifier)
@@ -36,7 +38,8 @@
dbus_path_(dbus::ObjectPath(service_path_)),
config_(new Config(manager, service_path_)),
dhcp_server_factory_(DHCPServerFactory::GetInstance()),
- file_writer_(FileWriter::GetInstance()) {
+ file_writer_(FileWriter::GetInstance()),
+ process_factory_(ProcessFactory::GetInstance()) {
SetConfig(config_->dbus_path());
// TODO(zqiu): come up with better server address management. This is good
// enough for now.
@@ -147,7 +150,7 @@
}
bool Service::StartHostapdProcess(const string& config_file_path) {
- hostapd_process_.reset(new chromeos::ProcessImpl());
+ hostapd_process_.reset(process_factory_->CreateProcess());
hostapd_process_->AddArg(kHostapdPath);
hostapd_process_->AddArg(config_file_path);
if (!hostapd_process_->Start()) {
diff --git a/service.h b/service.h
index 9caf65b..a7cecac 100644
--- a/service.h
+++ b/service.h
@@ -14,6 +14,7 @@
#include "apmanager/dbus_adaptors/org.chromium.apmanager.Service.h"
#include "apmanager/dhcp_server_factory.h"
#include "apmanager/file_writer.h"
+#include "apmanager/process_factory.h"
namespace apmanager {
@@ -42,7 +43,7 @@
static const char kHostapdPath[];
static const char kHostapdConfigPathFormat[];
- static const char kHostapdControlInterfacePathFormat[];
+ static const char kHostapdControlInterfacePath[];
static const int kTerminationTimeoutSeconds;
// Return true if hostapd process is currently running.
@@ -69,6 +70,7 @@
std::unique_ptr<DHCPServer> dhcp_server_;
DHCPServerFactory* dhcp_server_factory_;
FileWriter* file_writer_;
+ ProcessFactory* process_factory_;
DISALLOW_COPY_AND_ASSIGN(Service);
};
diff --git a/service_unittest.cc b/service_unittest.cc
index f73d561..970023c 100644
--- a/service_unittest.cc
+++ b/service_unittest.cc
@@ -18,6 +18,7 @@
#include "apmanager/mock_dhcp_server_factory.h"
#include "apmanager/mock_file_writer.h"
#include "apmanager/mock_manager.h"
+#include "apmanager/mock_process_factory.h"
using chromeos::ProcessMock;
using ::testing::_;
@@ -37,7 +38,17 @@
class ServiceTest : public testing::Test {
public:
- ServiceTest() : service_(&manager_, kServiceIdentifier) {}
+ ServiceTest()
+ : service_(&manager_, kServiceIdentifier),
+ dhcp_server_factory_(MockDHCPServerFactory::GetInstance()),
+ file_writer_(MockFileWriter::GetInstance()),
+ process_factory_(MockProcessFactory::GetInstance()) {}
+
+ virtual void SetUp() {
+ service_.dhcp_server_factory_ = dhcp_server_factory_;
+ service_.file_writer_ = file_writer_;
+ service_.process_factory_ = process_factory_;
+ }
void StartDummyProcess() {
service_.hostapd_process_.reset(new chromeos::ProcessImpl);
@@ -51,17 +62,12 @@
service_.config_.reset(config);
}
- void SetDHCPServerFactory(DHCPServerFactory* factory) {
- service_.dhcp_server_factory_ = factory;
- }
-
- void SetFileWriter(FileWriter* file_writer) {
- service_.file_writer_ = file_writer;
- }
-
protected:
Service service_;
MockManager manager_;
+ MockDHCPServerFactory* dhcp_server_factory_;
+ MockFileWriter* file_writer_;
+ MockProcessFactory* process_factory_;
};
MATCHER_P(IsServiceErrorStartingWith, message, "") {
@@ -95,23 +101,20 @@
SetConfig(config);
// Setup mock DHCP server.
- MockDHCPServerFactory* dhcp_server_factory =
- MockDHCPServerFactory::GetInstance();
MockDHCPServer* dhcp_server = new MockDHCPServer();
- SetDHCPServerFactory(dhcp_server_factory);
-
- // Setup mock file writer.
- MockFileWriter* file_writer = MockFileWriter::GetInstance();
- SetFileWriter(file_writer);
+ // Setup mock process.
+ ProcessMock* process = new ProcessMock();
std::string config_str(kHostapdConfig);
chromeos::ErrorPtr error;
EXPECT_CALL(*config, GenerateConfigFile(_, _)).WillOnce(
DoAll(SetArgPointee<1>(config_str), Return(true)));
- EXPECT_CALL(*file_writer, Write(kHostapdConfigFilePath, kHostapdConfig))
+ EXPECT_CALL(*file_writer_, Write(kHostapdConfigFilePath, kHostapdConfig))
.WillOnce(Return(true));
EXPECT_CALL(*config, ClaimDevice()).WillOnce(Return(true));
- EXPECT_CALL(*dhcp_server_factory, CreateDHCPServer(_, _))
+ EXPECT_CALL(*process_factory_, CreateProcess()).WillOnce(Return(process));
+ EXPECT_CALL(*process, Start()).WillOnce(Return(true));
+ EXPECT_CALL(*dhcp_server_factory_, CreateDHCPServer(_, _))
.WillOnce(Return(dhcp_server));
EXPECT_CALL(*dhcp_server, Start()).WillOnce(Return(true));
EXPECT_TRUE(service_.Start(&error));