shill: Create Property Store Inspector
Refactor code instances in unit tests where we need to verify the
existence of a single property in a PropertyStore. We never need
to do this in "real life", so only include this functionality in
unit tests.
BUG=chromium-os:30046
TEST=Use in unit tests.
Change-Id: I2209e0dbbe8af123a7fe28e0970fa88fe66dcbf2
Reviewed-on: https://gerrit.chromium.org/gerrit/21447
Commit-Ready: Paul Stewart <pstew@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
diff --git a/Makefile b/Makefile
index d871669..48f9bbd 100644
--- a/Makefile
+++ b/Makefile
@@ -314,6 +314,7 @@
profile_dbus_property_exporter_unittest.o \
profile_unittest.o \
property_accessor_unittest.o \
+ property_store_inspector.o \
property_store_unittest.o \
resolver_unittest.o \
routing_table_unittest.o \
diff --git a/l2tp_ipsec_driver_unittest.cc b/l2tp_ipsec_driver_unittest.cc
index 05a51c6..62f7f37 100644
--- a/l2tp_ipsec_driver_unittest.cc
+++ b/l2tp_ipsec_driver_unittest.cc
@@ -19,6 +19,7 @@
#include "shill/mock_nss.h"
#include "shill/mock_vpn.h"
#include "shill/mock_vpn_service.h"
+#include "shill/property_store_inspector.h"
#include "shill/vpn.h"
using std::find;
@@ -76,11 +77,6 @@
return driver_->args();
}
- bool FindKeyValueStorePropertyInStore(const PropertyStore &store,
- const string &key,
- KeyValueStore *value,
- Error *error);
-
// Used to assert that a flag appears in the options.
void ExpectInFlags(const vector<string> &options, const string &flag,
const string &value);
@@ -112,23 +108,6 @@
void L2TPIPSecDriverTest::Notify(
const string &/*reason*/, const map<string, string> &/*dict*/) {}
-bool L2TPIPSecDriverTest::FindKeyValueStorePropertyInStore(
- const PropertyStore &store,
- const string &key,
- KeyValueStore *value,
- Error *error) {
- ReadablePropertyConstIterator<KeyValueStore> it =
- store.GetKeyValueStorePropertiesIter();
- for ( ; !it.AtEnd(); it.Advance()) {
- if (it.Key() == key) {
- *value = it.Value(error);
- return error->IsSuccess();
- }
- }
- error->Populate(Error::kNotFound);
- return false;
-}
-
void L2TPIPSecDriverTest::ExpectInFlags(
const vector<string> &options, const string &flag, const string &value) {
vector<string>::const_iterator it =
@@ -452,12 +431,12 @@
TEST_F(L2TPIPSecDriverTest, GetProvider) {
PropertyStore store;
driver_->InitPropertyStore(&store);
+ PropertyStoreInspector inspector(&store);
{
Error error;
KeyValueStore props;
- EXPECT_TRUE(
- FindKeyValueStorePropertyInStore(
- store, flimflam::kProviderProperty, &props, &error));
+ EXPECT_TRUE(inspector.GetKeyValueStoreProperty(
+ flimflam::kProviderProperty, &props, &error));
EXPECT_TRUE(props.LookupBool(flimflam::kPassphraseRequiredProperty, false));
EXPECT_TRUE(
props.LookupBool(flimflam::kL2tpIpsecPskRequiredProperty, false));
@@ -467,9 +446,8 @@
KeyValueStore props;
SetArg(flimflam::kL2tpIpsecPasswordProperty, "random-password");
SetArg(flimflam::kL2tpIpsecPskProperty, "random-psk");
- EXPECT_TRUE(
- FindKeyValueStorePropertyInStore(
- store, flimflam::kProviderProperty, &props, &error));
+ EXPECT_TRUE(inspector.GetKeyValueStoreProperty(
+ flimflam::kProviderProperty, &props, &error));
EXPECT_FALSE(props.LookupBool(flimflam::kPassphraseRequiredProperty, true));
EXPECT_FALSE(
props.LookupBool(flimflam::kL2tpIpsecPskRequiredProperty, true));
diff --git a/openvpn_driver_unittest.cc b/openvpn_driver_unittest.cc
index af974fc..c811ecb 100644
--- a/openvpn_driver_unittest.cc
+++ b/openvpn_driver_unittest.cc
@@ -26,6 +26,7 @@
#include "shill/mock_vpn.h"
#include "shill/mock_vpn_service.h"
#include "shill/nice_mock_control.h"
+#include "shill/property_store_inspector.h"
#include "shill/rpc_task.h"
#include "shill/scope_logger.h"
#include "shill/vpn.h"
@@ -96,11 +97,6 @@
return driver_->args();
}
- bool FindKeyValueStorePropertyInStore(const PropertyStore &store,
- const string &key,
- KeyValueStore *value,
- Error *error);
-
// Used to assert that a flag appears in the options.
void ExpectInFlags(const vector<string> &options, const string &flag,
const string &value);
@@ -144,23 +140,6 @@
void OpenVPNDriverTest::Notify(const string &/*reason*/,
const map<string, string> &/*dict*/) {}
-bool OpenVPNDriverTest::FindKeyValueStorePropertyInStore(
- const PropertyStore &store,
- const string &key,
- KeyValueStore *value,
- Error *error) {
- ReadablePropertyConstIterator<KeyValueStore> it =
- store.GetKeyValueStorePropertiesIter();
- for ( ; !it.AtEnd(); it.Advance()) {
- if (it.Key() == key) {
- *value = it.Value(error);
- return error->IsSuccess();
- }
- }
- error->Populate(Error::kNotFound);
- return false;
-}
-
void OpenVPNDriverTest::ExpectInFlags(const vector<string> &options,
const string &flag,
const string &value) {
@@ -669,12 +648,13 @@
TEST_F(OpenVPNDriverTest, GetProvider) {
PropertyStore store;
driver_->InitPropertyStore(&store);
+ PropertyStoreInspector inspector(&store);
{
Error error;
KeyValueStore props;
EXPECT_TRUE(
- FindKeyValueStorePropertyInStore(
- store, flimflam::kProviderProperty, &props, &error));
+ inspector.GetKeyValueStoreProperty(
+ flimflam::kProviderProperty, &props, &error));
EXPECT_TRUE(props.LookupBool(flimflam::kPassphraseRequiredProperty, false));
}
{
@@ -682,8 +662,8 @@
KeyValueStore props;
SetArg(flimflam::kOpenVPNPasswordProperty, "random-password");
EXPECT_TRUE(
- FindKeyValueStorePropertyInStore(
- store, flimflam::kProviderProperty, &props, &error));
+ inspector.GetKeyValueStoreProperty(
+ flimflam::kProviderProperty, &props, &error));
EXPECT_FALSE(props.LookupBool(flimflam::kPassphraseRequiredProperty, true));
}
}
diff --git a/property_store_inspector.cc b/property_store_inspector.cc
new file mode 100644
index 0000000..106749a
--- /dev/null
+++ b/property_store_inspector.cc
@@ -0,0 +1,169 @@
+// Copyright (c) 2012 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 "shill/property_store_inspector.h"
+
+#include "shill/error.h"
+
+using std::map;
+using std::string;
+using std::vector;
+
+namespace shill {
+
+PropertyStoreInspector::PropertyStoreInspector() {}
+
+PropertyStoreInspector::PropertyStoreInspector(const PropertyStore *store)
+ : store_(store) {}
+
+PropertyStoreInspector::~PropertyStoreInspector() {}
+
+bool PropertyStoreInspector::GetBoolProperty(const string &name,
+ bool *value,
+ Error *error) {
+ return GetProperty(name, value, error, &PropertyStore::GetBoolPropertiesIter);
+}
+
+bool PropertyStoreInspector::GetInt16Property(const string &name,
+ int16 *value,
+ Error *error) {
+ return GetProperty(name, value, error,
+ &PropertyStore::GetInt16PropertiesIter);
+}
+
+bool PropertyStoreInspector::GetInt32Property(const string &name,
+ int32 *value,
+ Error *error) {
+ return GetProperty(name, value, error,
+ &PropertyStore::GetInt32PropertiesIter);
+}
+
+bool PropertyStoreInspector::GetKeyValueStoreProperty(const string &name,
+ KeyValueStore *value,
+ Error *error) {
+ return GetProperty(name, value, error,
+ &PropertyStore::GetKeyValueStorePropertiesIter);
+}
+
+bool PropertyStoreInspector::GetStringProperty(const string &name,
+ string *value,
+ Error *error) {
+ return GetProperty(name, value, error,
+ &PropertyStore::GetStringPropertiesIter);
+}
+
+bool PropertyStoreInspector::GetStringmapProperty(const string &name,
+ map<string, string> *values,
+ Error *error) {
+ return GetProperty(name, values, error,
+ &PropertyStore::GetStringmapPropertiesIter);
+}
+
+bool PropertyStoreInspector::GetStringsProperty(const string &name,
+ vector<string> *values,
+ Error *error) {
+ return GetProperty(name, values, error,
+ &PropertyStore::GetStringsPropertiesIter);
+}
+
+bool PropertyStoreInspector::GetUint8Property(const string &name,
+ uint8 *value,
+ Error *error) {
+ return GetProperty(name, value, error,
+ &PropertyStore::GetUint8PropertiesIter);
+}
+
+bool PropertyStoreInspector::GetUint16Property(const string &name,
+ uint16 *value,
+ Error *error) {
+ return GetProperty(name, value, error,
+ &PropertyStore::GetUint16PropertiesIter);
+}
+
+bool PropertyStoreInspector::GetUint32Property(const string &name,
+ uint32 *value,
+ Error *error) {
+ return GetProperty(name, value, error,
+ &PropertyStore::GetUint32PropertiesIter);
+}
+
+bool PropertyStoreInspector::GetRpcIdentifierProperty(const string &name,
+ RpcIdentifier *value,
+ Error *error) {
+ return GetProperty(name, value, error,
+ &PropertyStore::GetRpcIdentifierPropertiesIter);
+}
+
+bool PropertyStoreInspector::ContainsBoolProperty(const string &name) {
+ return ContainsProperty(name, &PropertyStore::GetBoolPropertiesIter);
+}
+
+bool PropertyStoreInspector::ContainsInt16Property(const string &name) {
+ return ContainsProperty(name, &PropertyStore::GetInt16PropertiesIter);
+}
+
+bool PropertyStoreInspector::ContainsInt32Property(const string &name) {
+ return ContainsProperty(name, &PropertyStore::GetInt32PropertiesIter);
+}
+
+bool PropertyStoreInspector::ContainsKeyValueStoreProperty(const string &name) {
+ return ContainsProperty(name, &PropertyStore::GetKeyValueStorePropertiesIter);
+}
+
+bool PropertyStoreInspector::ContainsStringProperty(const string &name) {
+ return ContainsProperty(name, &PropertyStore::GetStringPropertiesIter);
+}
+
+bool PropertyStoreInspector::ContainsStringmapProperty(const string &name) {
+ return ContainsProperty(name, &PropertyStore::GetStringmapPropertiesIter);
+}
+
+bool PropertyStoreInspector::ContainsStringsProperty(const string &name) {
+ return ContainsProperty(name, &PropertyStore::GetStringsPropertiesIter);
+}
+
+bool PropertyStoreInspector::ContainsUint8Property(const string &name) {
+ return ContainsProperty(name, &PropertyStore::GetUint8PropertiesIter);
+}
+
+bool PropertyStoreInspector::ContainsUint16Property(const string &name) {
+ return ContainsProperty(name, &PropertyStore::GetUint16PropertiesIter);
+}
+
+bool PropertyStoreInspector::ContainsUint32Property(const string &name) {
+ return ContainsProperty(name, &PropertyStore::GetUint32PropertiesIter);
+}
+
+bool PropertyStoreInspector::ContainsRpcIdentifierProperty(const string &name) {
+ return ContainsProperty(name, &PropertyStore::GetRpcIdentifierPropertiesIter);
+}
+
+template <class V>
+bool PropertyStoreInspector::GetProperty(
+ const string &name,
+ V *value,
+ Error *error,
+ ReadablePropertyConstIterator<V>(PropertyStore::*getter)() const) {
+ ReadablePropertyConstIterator<V> it = ((*store_).*getter)();
+ for ( ; !it.AtEnd(); it.Advance()) {
+ if (it.Key() == name) {
+ *value = it.Value(error);
+ return error->IsSuccess();
+ }
+ }
+ error->Populate(Error::kNotFound);
+ return false;
+};
+
+template <class V>
+bool PropertyStoreInspector::ContainsProperty(
+ const std::string &name,
+ ReadablePropertyConstIterator<V>(PropertyStore::*getter)() const) {
+ V value;
+ Error error;
+ GetProperty(name, &value, &error, getter);
+ return error.type() != Error::kNotFound;
+}
+
+} // namespace shill
diff --git a/property_store_inspector.h b/property_store_inspector.h
new file mode 100644
index 0000000..12d2bcf
--- /dev/null
+++ b/property_store_inspector.h
@@ -0,0 +1,100 @@
+// Copyright (c) 2012 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_PROPERTY_STORE_INSPECTOR_
+#define SHILL_PROPERTY_STORE_INSPECTOR_
+
+#ifndef UNIT_TEST
+#error "Do not use PropertyStoreInspector in non-test code."
+#endif
+
+#include "shill/property_store.h"
+
+namespace shill {
+
+// This class supplies functions to unit tests for inspecting a PropertyStore
+// class to get single attributes. We never need to do this in the real
+// PropertyStore, since the only interface to it over the control API is
+// "GetProperties" which iterates over the entire store's properties.
+//
+// This code is done inefficiently -- if we were ever meant to retrieve
+// single properties in production code, we'd access the map directly.
+// For this reason, this class should only be used in unit tests.
+class PropertyStoreInspector {
+ public:
+ PropertyStoreInspector();
+ explicit PropertyStoreInspector(const PropertyStore *store);
+ virtual ~PropertyStoreInspector();
+
+ const PropertyStore *store() const { return store_; }
+ void set_store(const PropertyStore *store) { store_ = store; }
+
+ // Methods to allow the getting, of properties stored in the referenced
+ // |store_| by name. Upon success, these methods return true, return
+ // the property value in |value|, and leave |error| untouched. Upon
+ // failure, they return false and if non-NULL, |error| is set appropriately.
+ // The value for |error| may be returned by the property getter itself, or
+ // if the property is not found in the store, these functions return
+ // Error::kNotFound.
+ bool GetBoolProperty(const std::string &name, bool *value, Error *error);
+ bool GetInt16Property(const std::string &name, int16 *value, Error *error);
+ bool GetInt32Property(const std::string &name, int32 *value, Error *error);
+ bool GetKeyValueStoreProperty(const std::string &name,
+ KeyValueStore *value,
+ Error *error);
+ bool GetStringProperty(const std::string &name,
+ std::string *value,
+ Error *error);
+ bool GetStringmapProperty(const std::string &name,
+ std::map<std::string, std::string> *values,
+ Error *error);
+ bool GetStringsProperty(const std::string &name,
+ std::vector<std::string> *values,
+ Error *error);
+ bool GetUint8Property(const std::string &name, uint8 *value, Error *error);
+ bool GetUint16Property(const std::string &name, uint16 *value, Error *error);
+ bool GetUint32Property(const std::string &name, uint32 *value, Error *error);
+ bool GetRpcIdentifierProperty(const std::string &name,
+ RpcIdentifier *value,
+ Error *error);
+
+ // Methods to test the whether a property is enumerable for reading -- rather
+ // specifically that trying to do so would either succeed or return
+ // an error other than Error::kNotFound. Therefore, this function will
+ // return true for both successfully readable properties or properties that
+ // return a different error for their getter (e.g., Error:kPermissionDenied).
+ bool ContainsBoolProperty(const std::string &name);
+ bool ContainsInt16Property(const std::string &name);
+ bool ContainsInt32Property(const std::string &name);
+ bool ContainsKeyValueStoreProperty(const std::string &name);
+ bool ContainsStringProperty(const std::string &name);
+ bool ContainsStringmapProperty(const std::string &name);
+ bool ContainsStringsProperty(const std::string &name);
+ bool ContainsUint8Property(const std::string &name);
+ bool ContainsUint16Property(const std::string &name);
+ bool ContainsUint32Property(const std::string &name);
+ bool ContainsRpcIdentifierProperty(const std::string &name);
+
+ private:
+ template <class V>
+ bool GetProperty(
+ const std::string &name,
+ V *value,
+ Error *error,
+ ReadablePropertyConstIterator<V>(PropertyStore::*getter)() const);
+
+ template <class V>
+ bool ContainsProperty(
+ const std::string &name,
+ ReadablePropertyConstIterator<V>(PropertyStore::*getter)() const);
+
+ const PropertyStore *store_;
+
+ DISALLOW_COPY_AND_ASSIGN(PropertyStoreInspector);
+};
+
+} // namespace shill
+
+
+#endif // SHILL_PROPERTY_STORE_INSPECTOR_
diff --git a/service_unittest.cc b/service_unittest.cc
index 424c949..94f6346 100644
--- a/service_unittest.cc
+++ b/service_unittest.cc
@@ -25,6 +25,7 @@
#include "shill/mock_manager.h"
#include "shill/mock_profile.h"
#include "shill/mock_store.h"
+#include "shill/property_store_inspector.h"
#include "shill/property_store_unittest.h"
#include "shill/service_under_test.h"
@@ -520,11 +521,9 @@
// our supeclass (PropertyStoreTest) is declared with.
class ReadOnlyServicePropertyTest : public ServiceTest {};
TEST_P(ReadOnlyServicePropertyTest, PropertyWriteOnly) {
- ReadablePropertyConstIterator<string> it =
- (service_->store()).GetStringPropertiesIter();
string property(GetParam().reader().get_string());
- for( ; !it.AtEnd(); it.Advance())
- EXPECT_NE(it.Key(), property);
+ PropertyStoreInspector inspector(&service_->store());
+ EXPECT_FALSE(inspector.ContainsStringProperty(property));
}
INSTANTIATE_TEST_CASE_P(
diff --git a/vpn_driver_unittest.cc b/vpn_driver_unittest.cc
index 4b4e4c5..46a51a3 100644
--- a/vpn_driver_unittest.cc
+++ b/vpn_driver_unittest.cc
@@ -21,6 +21,7 @@
#include "shill/mock_store.h"
#include "shill/nice_mock_control.h"
#include "shill/property_store.h"
+#include "shill/property_store_inspector.h"
using std::string;
using testing::_;
@@ -94,14 +95,9 @@
KeyValueStore *GetArgs() { return driver_.args(); }
- bool FindStringPropertyInStore(const PropertyStore &store,
- const string &key,
- string *value,
- Error *error);
- bool FindKeyValueStorePropertyInStore(const PropertyStore &store,
- const string &key,
- KeyValueStore *value,
- Error *error);
+ bool GetProviderProperty(const PropertyStore &store,
+ const string &key,
+ string *value);
NiceMockControl control_;
NiceMock<MockDeviceInfo> device_info_;
@@ -112,36 +108,21 @@
VPNDriverUnderTest driver_;
};
-bool VPNDriverTest::FindStringPropertyInStore(const PropertyStore &store,
- const string &key,
- string *value,
- Error *error) {
- ReadablePropertyConstIterator<std::string> it =
- store.GetStringPropertiesIter();
- for ( ; !it.AtEnd(); it.Advance()) {
- if (it.Key() == key) {
- *value = it.Value(error);
- return error->IsSuccess();
- }
+bool VPNDriverTest::GetProviderProperty(const PropertyStore &store,
+ const string &key,
+ string *value) {
+ Error error;
+ PropertyStoreInspector inspector(&store);
+ KeyValueStore provider_properties;
+ EXPECT_TRUE(inspector.GetKeyValueStoreProperty(
+ flimflam::kProviderProperty, &provider_properties, &error));
+ if (!provider_properties.ContainsString(key)) {
+ return false;
}
- error->Populate(Error::kNotFound);
- return false;
-}
-
-bool VPNDriverTest::FindKeyValueStorePropertyInStore(const PropertyStore &store,
- const string &key,
- KeyValueStore *value,
- Error *error) {
- ReadablePropertyConstIterator<KeyValueStore> it =
- store.GetKeyValueStorePropertiesIter();
- for ( ; !it.AtEnd(); it.Advance()) {
- if (it.Key() == key) {
- *value = it.Value(error);
- return error->IsSuccess();
- }
+ if (value != NULL) {
+ *value = provider_properties.GetString(key);
}
- error->Populate(Error::kNotFound);
- return false;
+ return true;
}
TEST_F(VPNDriverTest, Load) {
@@ -207,16 +188,11 @@
// KeyValueStore.
PropertyStore store;
driver_.InitPropertyStore(&store);
+ PropertyStoreInspector inspector(&store);
// An un-set property should not be readable.
- {
- Error error;
- string string_property;
- EXPECT_FALSE(
- FindStringPropertyInStore(
- store, kPortProperty, &string_property, &error));
- EXPECT_EQ(Error::kNotFound, error.type());
- }
+ EXPECT_FALSE(inspector.ContainsStringProperty(kPortProperty));
+ EXPECT_FALSE(GetProviderProperty(store, kPortProperty, NULL));
const string kProviderName = "boo";
SetArg(kPortProperty, kPort);
@@ -225,47 +201,24 @@
// We should not be able to read a property out of the driver args using the
// key to the args directly.
- {
- Error error;
- string string_property;
- EXPECT_FALSE(
- FindStringPropertyInStore(
- store, kPortProperty, &string_property, &error));
- EXPECT_EQ(Error::kNotFound, error.type());
- }
+ EXPECT_FALSE(inspector.ContainsStringProperty(kPortProperty));
// We should instead be able to find it within the "Provider" stringmap.
- {
- Error error;
- KeyValueStore provider_properties;
- EXPECT_TRUE(
- FindKeyValueStorePropertyInStore(
- store, flimflam::kProviderProperty, &provider_properties, &error));
- EXPECT_EQ(kPort, provider_properties.LookupString(kPortProperty, ""));
- }
+ string value;
+ EXPECT_TRUE(GetProviderProperty(store, kPortProperty, &value));
+ EXPECT_EQ(kPort, value);
// Properties that start with the prefix "Provider." should be mapped to the
// name in the Properties dict with the prefix removed.
- {
- Error error;
- KeyValueStore provider_properties;
- EXPECT_TRUE(
- FindKeyValueStorePropertyInStore(
- store, flimflam::kProviderProperty, &provider_properties, &error));
- EXPECT_EQ(kProviderName,
- provider_properties.LookupString(flimflam::kNameProperty, ""));
- }
+ EXPECT_TRUE(GetProviderProperty(store, flimflam::kNameProperty, &value));
+ EXPECT_EQ(kProviderName, value);
- // If we clear this property, we should no longer be able to find it.
+ // If we clear a property, we should no longer be able to find it.
{
Error error;
EXPECT_TRUE(store.ClearProperty(kPortProperty, &error));
EXPECT_TRUE(error.IsSuccess());
- string string_property;
- EXPECT_FALSE(
- FindStringPropertyInStore(
- store, kPortProperty, &string_property, &error));
- EXPECT_EQ(Error::kNotFound, error.type());
+ EXPECT_FALSE(GetProviderProperty(store, kPortProperty, NULL));
}
// A second attempt to clear this property should return an error.
@@ -276,17 +229,7 @@
}
// Test write only properties.
- {
- Error error;
- string string_property;
- EXPECT_FALSE(
- FindStringPropertyInStore(
- store, kPINProperty, &string_property, &error));
- // We get NotFound here instead of PermissionDenied here due to the
- // implementation of ReadablePropertyConstIterator: it shields us from store
- // members for which Value() would have returned an error.
- EXPECT_EQ(Error::kNotFound, error.type());
- }
+ EXPECT_FALSE(GetProviderProperty(store, kPINProperty, NULL));
// Write properties to the driver args using the PropertyStore interface.
{