PM: Inject providers to RealState and PolicyManager.
Since RealState allocated a set of hard-coded real providers, it was
necessary for us to pass in underlying interfaces in order to be able to
mock dependencies like DBus or a system clock. This worked but scaled
poorly as we needed to test higher level classes like PolicyManager.
Instead, we now allow to inject the provider objects themselves into
RealState, and a State object into PolicyManager, which allows us to
easily fake them at each level for testing purposes.
BUG=None
TEST=Unit tests.
Change-Id: I85b38990e42039cb6cf37e97234fd18e2c3fea2e
Reviewed-on: https://chromium-review.googlesource.com/189776
Reviewed-by: Gilad Arnold <garnold@chromium.org>
Commit-Queue: Gilad Arnold <garnold@chromium.org>
Tested-by: Gilad Arnold <garnold@chromium.org>
diff --git a/policy_manager/policy_manager.cc b/policy_manager/policy_manager.cc
index 027a4fe..1357707 100644
--- a/policy_manager/policy_manager.cc
+++ b/policy_manager/policy_manager.cc
@@ -17,15 +17,14 @@
return handle_ptr->get() && (*handle_ptr)->Init();
}
-bool PolicyManager::Init(chromeos_update_engine::DBusWrapperInterface* dbus,
- chromeos_update_engine::ClockInterface* clock) {
+bool PolicyManager::Init(State* state) {
// TODO(deymo): Make it possible to replace this policy with a different
// implementation with a build-time flag.
policy_.reset(new ChromeOSPolicy());
- state_.reset(new RealState(dbus, clock));
+ state_.reset(state);
- return state_->Init();
+ return true;
}
void PolicyManager::RunFromMainLoop(const Closure& callback) {
diff --git a/policy_manager/policy_manager.h b/policy_manager/policy_manager.h
index 51cc24a..33bb89c 100644
--- a/policy_manager/policy_manager.h
+++ b/policy_manager/policy_manager.h
@@ -11,8 +11,6 @@
#include <base/memory/ref_counted.h>
#include <base/memory/scoped_ptr.h>
-#include "update_engine/clock_interface.h"
-#include "update_engine/dbus_wrapper_interface.h"
#include "update_engine/policy_manager/default_policy.h"
#include "update_engine/policy_manager/policy.h"
#include "update_engine/policy_manager/state.h"
@@ -24,10 +22,10 @@
public:
PolicyManager() {}
- // Initializes the PolicyManager instance. Returns whether the initialization
- // succeeded.
- bool Init(chromeos_update_engine::DBusWrapperInterface* dbus,
- chromeos_update_engine::ClockInterface* clock);
+ // Initializes the PolicyManager instance, assuming ownership on the provided
+ // |state|, which is assumed to be pre-initialized. Returns whether the
+ // initialization succeeded.
+ bool Init(State* state);
// PolicyRequest() evaluates the given policy with the provided arguments and
// returns the result. The |policy_method| is the pointer-to-method of the
diff --git a/policy_manager/policy_manager_unittest.cc b/policy_manager/policy_manager_unittest.cc
index e2f3218..00a83b4 100644
--- a/policy_manager/policy_manager_unittest.cc
+++ b/policy_manager/policy_manager_unittest.cc
@@ -8,77 +8,35 @@
#include <base/bind.h>
#include <base/memory/scoped_ptr.h>
-// TODO(garnold) Remove once shill DBus constants not needed.
-#include <chromeos/dbus/service_constants.h>
#include <gtest/gtest.h>
#include <gmock/gmock.h>
-#include "update_engine/fake_clock.h"
-#include "update_engine/mock_dbus_wrapper.h"
#include "update_engine/policy_manager/default_policy.h"
+#include "update_engine/policy_manager/fake_state.h"
#include "update_engine/policy_manager/mock_policy.h"
#include "update_engine/policy_manager/pmtest_utils.h"
#include "update_engine/policy_manager/policy_manager.h"
-// TODO(garnold) Remove once we stop mocking DBus.
#include "update_engine/test_utils.h"
using base::Bind;
using base::Callback;
-using chromeos_update_engine::FakeClock;
-using chromeos_update_engine::GValueNewString;
-using chromeos_update_engine::GValueFree;
-using chromeos_update_engine::MockDBusWrapper;
using std::pair;
using std::string;
using std::vector;
-using testing::NiceMock;
using testing::Return;
-using testing::SetArgPointee;
using testing::StrictMock;
using testing::_;
-namespace {
-
-// TODO(garnold) This whole section gets removed once we mock the shill provider
-// itself in tests.
-
-// Fake dbus-glib objects.
-DBusGConnection* const kFakeConnection = reinterpret_cast<DBusGConnection*>(1);
-DBusGProxy* const kFakeManagerProxy = reinterpret_cast<DBusGProxy*>(2);
-
-} // namespace
-
namespace chromeos_policy_manager {
class PmPolicyManagerTest : public ::testing::Test {
protected:
virtual void SetUp() {
- // TODO(garnold) Replace this low-level DBus injection with a high-level
- // mock shill provider.
- EXPECT_CALL(mock_dbus_, BusGet(_, _))
- .WillOnce(Return(kFakeConnection));
- EXPECT_CALL(mock_dbus_, ProxyNewForName(_, _, _, _))
- .WillOnce(Return(kFakeManagerProxy));
- EXPECT_CALL(mock_dbus_, ProxyAddSignal_2(_, _, _, _))
- .WillOnce(Return());
- EXPECT_CALL(mock_dbus_, ProxyConnectSignal(_, _, _, _, _))
- .WillOnce(Return());
- auto properties = g_hash_table_new_full(g_str_hash, g_str_equal, free,
- GValueFree);
- g_hash_table_insert(properties, strdup(shill::kDefaultServiceProperty),
- GValueNewString("/"));
- EXPECT_CALL(mock_dbus_, ProxyCall_0_1(_, _, _, _))
- .WillOnce(DoAll(SetArgPointee<3>(g_hash_table_ref(properties)),
- Return(true)));
-
- EXPECT_TRUE(pmut_.Init(&mock_dbus_, &fake_clock_));
-
- // TODO(garnold) Remove this, too.
- g_hash_table_unref(properties);
+ FakeState* fake_state = new FakeState();
+ ASSERT_TRUE(fake_state->Init());
+ EXPECT_TRUE(pmut_.Init(fake_state));
}
- NiceMock<MockDBusWrapper> mock_dbus_;
- FakeClock fake_clock_;
PolicyManager pmut_;
};
diff --git a/policy_manager/real_state.cc b/policy_manager/real_state.cc
index 4e140d6..8f99869 100644
--- a/policy_manager/real_state.cc
+++ b/policy_manager/real_state.cc
@@ -9,11 +9,10 @@
namespace chromeos_policy_manager {
-// TODO(garnold) We should be injecting actual provider objects here.
-RealState::RealState(chromeos_update_engine::DBusWrapperInterface* dbus,
- chromeos_update_engine::ClockInterface* clock) {
- set_random_provider(new RealRandomProvider());
- set_shill_provider(new RealShillProvider(dbus, clock));
+RealState::RealState(RandomProvider* random_provider,
+ ShillProvider* shill_provider) {
+ set_random_provider(random_provider);
+ set_shill_provider(shill_provider);
}
} // namespace chromeos_policy_manager
diff --git a/policy_manager/real_state.h b/policy_manager/real_state.h
index 48ab56c..e2eddd7 100644
--- a/policy_manager/real_state.h
+++ b/policy_manager/real_state.h
@@ -5,17 +5,18 @@
#ifndef CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_REAL_STATE_H_
#define CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_REAL_STATE_H_
-#include "update_engine/clock_interface.h"
-#include "update_engine/dbus_wrapper_interface.h"
+#include "update_engine/clock.h"
#include "update_engine/policy_manager/state.h"
+#include "update_engine/real_dbus_wrapper.h"
namespace chromeos_policy_manager {
// State implementation class.
class RealState : public State {
public:
- RealState(chromeos_update_engine::DBusWrapperInterface* dbus,
- chromeos_update_engine::ClockInterface* clock);
+ // Instantiate with given providers, assuming ownership of them.
+ RealState(RandomProvider* random_provider, ShillProvider* shill_provider);
+
~RealState() {}
private:
diff --git a/policy_manager/real_state_unittest.cc b/policy_manager/real_state_unittest.cc
index 13f8db2..6078223 100644
--- a/policy_manager/real_state_unittest.cc
+++ b/policy_manager/real_state_unittest.cc
@@ -2,70 +2,26 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
-// TODO(garnold) Remove once shill DBus constants not needed.
-#include <chromeos/dbus/service_constants.h>
#include <gtest/gtest.h>
-#include "update_engine/fake_clock.h"
-#include "update_engine/mock_dbus_wrapper.h"
+#include "update_engine/policy_manager/fake_random_provider.h"
+#include "update_engine/policy_manager/fake_shill_provider.h"
#include "update_engine/policy_manager/real_state.h"
#include "update_engine/policy_manager/pmtest_utils.h"
-// TODO(garnold) Remove once we stop mocking DBus.
-#include "update_engine/test_utils.h"
-
-using chromeos_update_engine::FakeClock;
-using chromeos_update_engine::GValueNewString;
-using chromeos_update_engine::GValueFree;
-using chromeos_update_engine::MockDBusWrapper;
-using testing::_;
-using testing::NiceMock;
-using testing::Return;
-using testing::SetArgPointee;
-
-namespace {
-
-// TODO(garnold) This whole section gets removed once we mock the shill provider
-// itself in tests.
-
-// Fake dbus-glib objects.
-DBusGConnection* const kFakeConnection = reinterpret_cast<DBusGConnection*>(1);
-DBusGProxy* const kFakeManagerProxy = reinterpret_cast<DBusGProxy*>(2);
-
-} // namespace
namespace chromeos_policy_manager {
TEST(PmRealStateTest, InitTest) {
- NiceMock<MockDBusWrapper> mock_dbus;
- FakeClock fake_clock;
-
- // TODO(garnold) Replace this low-level DBus injection with a high-level
- // mock shill provider.
- EXPECT_CALL(mock_dbus, BusGet(_, _))
- .WillOnce(Return(kFakeConnection));
- EXPECT_CALL(mock_dbus, ProxyNewForName(_, _, _, _))
- .WillOnce(Return(kFakeManagerProxy));
- EXPECT_CALL(mock_dbus, ProxyAddSignal_2(_, _, _, _))
- .WillOnce(Return());
- EXPECT_CALL(mock_dbus, ProxyConnectSignal(_, _, _, _, _))
- .WillOnce(Return());
- auto properties = g_hash_table_new_full(g_str_hash, g_str_equal, free,
- GValueFree);
- g_hash_table_insert(properties, strdup(shill::kDefaultServiceProperty),
- GValueNewString("/"));
- EXPECT_CALL(mock_dbus, ProxyCall_0_1(_, _, _, _))
- .WillOnce(DoAll(SetArgPointee<3>(g_hash_table_ref(properties)),
- Return(true)));
-
- RealState state(&mock_dbus, &fake_clock);
+ RealState state(new FakeRandomProvider(), new FakeShillProvider());
EXPECT_TRUE(state.Init());
- // TODO(garnold) Remove this, too.
- g_hash_table_unref(properties);
-
- // Check that the providers are being initialized.
+ // Check that the providers are being initialized. Beyond ensuring that we get
+ // non-null provider handles, verifying that we can get a single variable from
+ // each provider is enough of an indication that it has initialized.
PMTEST_ASSERT_NOT_NULL(state.random_provider());
PMTEST_EXPECT_NOT_NULL(state.random_provider()->var_seed());
+ PMTEST_ASSERT_NOT_NULL(state.shill_provider());
+ PMTEST_EXPECT_NOT_NULL(state.shill_provider()->var_is_connected());
}
} // namespace chromeos_policy_manager