Use proper dbus path when possible.
Following methods and properties were updated to use full dbus path,
instead of internal telemetry id:
- TriggerManager.AddTrigger() - 'reportIds' arg
- Trigger.ReportIds - renamed to 'Reports'
- Report.TriggerIds - renamed to 'Triggers'
Testing done:
- UTs were updated and are passing.
Signed-off-by: Szymon Dompke <szymon.dompke@intel.com>
Change-Id: I78d812d38289fac575d25b48503cc8b9c6f736fe
diff --git a/tests/meson.build b/tests/meson.build
index 70de004..25d5085 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -30,6 +30,7 @@
'../src/types/readings.cpp',
'../src/types/report_types.cpp',
'../src/utils/conversion_trigger.cpp',
+ '../src/utils/dbus_path_utils.cpp',
'../src/utils/generate_id.cpp',
'../src/utils/messanger_service.cpp',
'src/dbus_environment.cpp',
@@ -43,6 +44,7 @@
'src/test_metric.cpp',
'src/test_numeric_threshold.cpp',
'src/test_on_change_threshold.cpp',
+ 'src/test_path_append.cpp',
'src/test_persistent_json_storage.cpp',
'src/test_report.cpp',
'src/test_report_manager.cpp',
diff --git a/tests/src/dbus_environment.cpp b/tests/src/dbus_environment.cpp
index 07e0aff..a0f454e 100644
--- a/tests/src/dbus_environment.cpp
+++ b/tests/src/dbus_environment.cpp
@@ -1,5 +1,7 @@
#include "dbus_environment.hpp"
+#include "helpers.hpp"
+
#include <future>
#include <thread>
diff --git a/tests/src/helpers.hpp b/tests/src/helpers.hpp
index 8ae0b7f..1c4905c 100644
--- a/tests/src/helpers.hpp
+++ b/tests/src/helpers.hpp
@@ -5,3 +5,4 @@
#include "helpers/labeled_tuple_helpers.hpp"
#include "helpers/metric_value_helpers.hpp"
#include "helpers/types/duration_types_helpers.hpp"
+#include "helpers/types/object_path_helpers.hpp"
diff --git a/tests/src/helpers/types/object_path_helpers.hpp b/tests/src/helpers/types/object_path_helpers.hpp
new file mode 100644
index 0000000..799a402
--- /dev/null
+++ b/tests/src/helpers/types/object_path_helpers.hpp
@@ -0,0 +1,12 @@
+#pragma once
+#include <sdbusplus/message.hpp>
+
+#include <gmock/gmock.h>
+
+namespace sdbusplus::message::details
+{
+inline void PrintTo(const string_path_wrapper& path, std::ostream* os)
+{
+ *os << path.str;
+}
+} // namespace sdbusplus::message::details
diff --git a/tests/src/main.cpp b/tests/src/main.cpp
index 6e29319..39508c8 100644
--- a/tests/src/main.cpp
+++ b/tests/src/main.cpp
@@ -1,4 +1,5 @@
#include "dbus_environment.hpp"
+#include "helpers.hpp"
#include <gmock/gmock.h>
diff --git a/tests/src/params/trigger_params.hpp b/tests/src/params/trigger_params.hpp
index b17ea86..65f423e 100644
--- a/tests/src/params/trigger_params.hpp
+++ b/tests/src/params/trigger_params.hpp
@@ -1,16 +1,29 @@
#pragma once
+#include "report.hpp"
#include "types/duration_types.hpp"
#include "types/trigger_types.hpp"
+#include "utils/dbus_path_utils.hpp"
+#include "utils/transform.hpp"
#include <sdbusplus/message.hpp>
#include <chrono>
#include <utility>
+using sdbusplus::message::object_path;
+
class TriggerParams
{
public:
+ TriggerParams()
+ {
+ reportsProperty =
+ utils::transform(reportIdsProperty, [](const auto& id) {
+ return utils::pathAppend(utils::constants::reportDirPath, id);
+ });
+ }
+
TriggerParams& id(std::string val)
{
idProperty = std::move(val);
@@ -57,6 +70,21 @@
TriggerParams& reportIds(std::vector<std::string> val)
{
reportIdsProperty = std::move(val);
+ reportsProperty = utils::transform<std::vector>(
+ reportIdsProperty, [](const auto& id) {
+ return utils::pathAppend(utils::constants::reportDirPath, id);
+ });
+ return *this;
+ }
+
+ const std::vector<object_path>& reports() const
+ {
+ return reportsProperty;
+ }
+
+ TriggerParams& reports(std::vector<object_path> val)
+ {
+ reportsProperty = std::move(val);
return *this;
}
@@ -79,7 +107,9 @@
std::vector<LabeledSensorInfo> labeledSensorsProperty = {
{"service1", "/xyz/openbmc_project/sensors/temperature/BMC_Temp",
"metadata1"}};
- std::vector<std::string> reportIdsProperty = {"Report1"};
+ std::vector<std::string> reportIdsProperty = {"Report1",
+ "Prefixed/Report2"};
+ std::vector<object_path> reportsProperty;
LabeledTriggerThresholdParams labeledThresholdsProperty =
std::vector<numeric::LabeledThresholdParam>{
numeric::LabeledThresholdParam{numeric::Type::lowerCritical,
diff --git a/tests/src/stubs/dbus_sensor_object.cpp b/tests/src/stubs/dbus_sensor_object.cpp
index 9d6f185..b73478e 100644
--- a/tests/src/stubs/dbus_sensor_object.cpp
+++ b/tests/src/stubs/dbus_sensor_object.cpp
@@ -1,5 +1,7 @@
#include "dbus_sensor_object.hpp"
+#include "helpers.hpp"
+
#include <boost/asio.hpp>
#include <sdbusplus/asio/connection.hpp>
#include <sdbusplus/asio/object_server.hpp>
diff --git a/tests/src/test_conversion.cpp b/tests/src/test_conversion.cpp
index 2b7249c..6a69e39 100644
--- a/tests/src/test_conversion.cpp
+++ b/tests/src/test_conversion.cpp
@@ -1,3 +1,4 @@
+#include "helpers.hpp"
#include "utils/conversion.hpp"
#include <gmock/gmock.h>
diff --git a/tests/src/test_ensure.cpp b/tests/src/test_ensure.cpp
index b604538..d252bc3 100644
--- a/tests/src/test_ensure.cpp
+++ b/tests/src/test_ensure.cpp
@@ -1,3 +1,4 @@
+#include "helpers.hpp"
#include "utils/ensure.hpp"
#include <gmock/gmock.h>
diff --git a/tests/src/test_generate_id.cpp b/tests/src/test_generate_id.cpp
index 412fc9b..dc534dd 100644
--- a/tests/src/test_generate_id.cpp
+++ b/tests/src/test_generate_id.cpp
@@ -1,3 +1,4 @@
+#include "helpers.hpp"
#include "utils/generate_id.hpp"
#include <sdbusplus/exception.hpp>
diff --git a/tests/src/test_path_append.cpp b/tests/src/test_path_append.cpp
new file mode 100644
index 0000000..f667ef2
--- /dev/null
+++ b/tests/src/test_path_append.cpp
@@ -0,0 +1,59 @@
+#include "helpers.hpp"
+#include "utils/dbus_path_utils.hpp"
+
+#include <sdbusplus/exception.hpp>
+
+#include <gmock/gmock.h>
+
+using namespace testing;
+using sdbusplus::message::object_path;
+using utils::pathAppend;
+
+class TestPathAppend :
+ public Test,
+ public WithParamInterface<std::tuple<object_path, std::string, object_path>>
+{};
+
+INSTANTIATE_TEST_SUITE_P(
+ _, TestPathAppend,
+ Values(std::make_tuple(object_path("/Base/Path"), "one",
+ object_path("/Base/Path/one")),
+ std::make_tuple(object_path("/Base/Path"), "one/two",
+ object_path("/Base/Path/one/two")),
+ std::make_tuple(object_path("/Base/Path"), "one/two/foobar",
+ object_path("/Base/Path/one/two/foobar")),
+ std::make_tuple(object_path("/Base/Path/"), "one",
+ object_path("/Base/Path/one")),
+ std::make_tuple(object_path("/Base/Path/"), "one/two",
+ object_path("/Base/Path/one/two")),
+ std::make_tuple(object_path("/Base/Path/"), "one/two/foobar",
+ object_path("/Base/Path/one/two/foobar")),
+ std::make_tuple(object_path("/Base/Path"), "",
+ object_path("/Base/Path/"))));
+
+TEST_P(TestPathAppend, pathAppendsCorrectly)
+{
+ auto [basePath, extension, expectedPath] = GetParam();
+ EXPECT_EQ(pathAppend(basePath, extension), expectedPath);
+}
+
+class TestPathAppendFail :
+ public Test,
+ public WithParamInterface<std::tuple<object_path, std::string>>
+{};
+
+INSTANTIATE_TEST_SUITE_P(
+ _, TestPathAppendFail,
+ Values(std::make_tuple(object_path("/Base/Path"), "/one"),
+ std::make_tuple(object_path("/Base/Path"), "one/"),
+ std::make_tuple(object_path("/Base/Path"), "one/two/"),
+ std::make_tuple(object_path("/Base/Path"), "one//two"),
+ std::make_tuple(object_path("/Base/Path"), "/"),
+ std::make_tuple(object_path("/Base/Path"), "//")));
+
+TEST_P(TestPathAppendFail, pathAppendsCorrectly)
+{
+ auto [basePath, extension] = GetParam();
+ EXPECT_THROW(pathAppend(basePath, extension),
+ sdbusplus::exception::SdBusError);
+}
diff --git a/tests/src/test_report.cpp b/tests/src/test_report.cpp
index 8226f66..ac2419d 100644
--- a/tests/src/test_report.cpp
+++ b/tests/src/test_report.cpp
@@ -14,6 +14,7 @@
#include "utils/clock.hpp"
#include "utils/contains.hpp"
#include "utils/conv_container.hpp"
+#include "utils/dbus_path_utils.hpp"
#include "utils/messanger.hpp"
#include "utils/transform.hpp"
#include "utils/tstring.hpp"
@@ -23,6 +24,7 @@
using namespace testing;
using namespace std::literals::string_literals;
using namespace std::chrono_literals;
+using sdbusplus::message::object_path;
namespace tstring = utils::tstring;
using ErrorMessageDbusType = std::tuple<std::string, std::string>;
@@ -238,7 +240,7 @@
EXPECT_THAT(getProperty<std::string>(sut->getPath(), "Name"),
Eq(defaultParams().reportName()));
EXPECT_THAT(
- getProperty<std::vector<std::string>>(sut->getPath(), "TriggerIds"),
+ getProperty<std::vector<object_path>>(sut->getPath(), "Triggers"),
IsEmpty());
EXPECT_THAT(
getProperty<ErrorMessagesDbusType>(sut->getPath(), "ErrorMessages"),
@@ -551,7 +553,8 @@
TEST_F(TestReport, deletingNonExistingReportReturnInvalidRequestDescriptor)
{
- auto ec = deleteReport(Report::reportDir + "NonExisting"s);
+ auto ec =
+ deleteReport(utils::constants::reportDirPath.str + "NonExisting"s);
EXPECT_THAT(ec.value(), Eq(EBADR));
}
@@ -581,8 +584,9 @@
{"someOtherReport", defaultParams().reportId()}});
EXPECT_THAT(
- getProperty<std::vector<std::string>>(sut->getPath(), "TriggerIds"),
- UnorderedElementsAre("trigger1", "trigger3"));
+ getProperty<std::vector<object_path>>(sut->getPath(), "Triggers"),
+ UnorderedElementsAre(utils::constants::triggerDirPath / "trigger1",
+ utils::constants::triggerDirPath / "trigger3"));
}
TEST_F(TestReport, updatesTriggerIdWhenTriggerIsRemoved)
@@ -604,8 +608,8 @@
messages::Presence::Removed, "trigger1", {defaultParams().reportId()}});
EXPECT_THAT(
- getProperty<std::vector<std::string>>(sut->getPath(), "TriggerIds"),
- UnorderedElementsAre("trigger3"));
+ getProperty<std::vector<object_path>>(sut->getPath(), "Triggers"),
+ UnorderedElementsAre(utils::constants::triggerDirPath / "trigger3"));
}
TEST_F(TestReport, updatesTriggerIdWhenTriggerIsModified)
@@ -627,8 +631,9 @@
messages::Presence::Exist, "trigger3", {defaultParams().reportId()}});
EXPECT_THAT(
- getProperty<std::vector<std::string>>(sut->getPath(), "TriggerIds"),
- UnorderedElementsAre("trigger1", "trigger3"));
+ getProperty<std::vector<object_path>>(sut->getPath(), "Triggers"),
+ UnorderedElementsAre(utils::constants::triggerDirPath / "trigger1",
+ utils::constants::triggerDirPath / "trigger3"));
}
class TestReportStore :
@@ -1217,8 +1222,9 @@
sut = makeReport(ReportParams());
EXPECT_THAT(
- getProperty<std::vector<std::string>>(sut->getPath(), "TriggerIds"),
- UnorderedElementsAre("trigger1", "trigger2"));
+ getProperty<std::vector<object_path>>(sut->getPath(), "Triggers"),
+ UnorderedElementsAre(utils::constants::triggerDirPath / "trigger1",
+ utils::constants::triggerDirPath / "trigger2"));
}
TEST_F(TestReportInitialization,
diff --git a/tests/src/test_trigger.cpp b/tests/src/test_trigger.cpp
index 9032d05..5c41d97 100644
--- a/tests/src/test_trigger.cpp
+++ b/tests/src/test_trigger.cpp
@@ -20,6 +20,7 @@
using namespace testing;
using namespace std::literals::string_literals;
+using sdbusplus::message::object_path;
static constexpr size_t expectedTriggerVersion = 2;
@@ -138,8 +139,8 @@
EXPECT_THAT((getProperty<SensorsInfo>(sut->getPath(), "Sensors")),
Eq(utils::fromLabeledSensorsInfo(triggerParams.sensors())));
EXPECT_THAT(
- getProperty<std::vector<std::string>>(sut->getPath(), "ReportNames"),
- Eq(triggerParams.reportIds()));
+ getProperty<std::vector<object_path>>(sut->getPath(), "Reports"),
+ Eq(triggerParams.reports()));
EXPECT_THAT(
getProperty<bool>(sut->getPath(), "Discrete"),
Eq(isTriggerThresholdDiscrete(triggerParams.thresholdParams())));
@@ -152,7 +153,8 @@
TEST_F(TestTrigger, checkBasicGetters)
{
EXPECT_THAT(sut->getId(), Eq(triggerParams.id()));
- EXPECT_THAT(sut->getPath(), Eq(Trigger::triggerDir + triggerParams.id()));
+ EXPECT_THAT(sut->getPath(),
+ Eq(utils::constants::triggerDirPath.str + triggerParams.id()));
}
TEST_F(TestTrigger, setPropertyNameToCorrectValue)
@@ -165,47 +167,81 @@
TEST_F(TestTrigger, setPropertyReportNames)
{
- std::vector<std::string> newNames = {"abc", "one", "two"};
- EXPECT_THAT(setProperty(sut->getPath(), "ReportNames", newNames),
+ std::vector<object_path> newNames = {
+ utils::constants::reportDirPath / "abc",
+ utils::constants::reportDirPath / "one",
+ utils::constants::reportDirPath / "prefix" / "two"};
+ EXPECT_THAT(setProperty(sut->getPath(), "Reports", newNames),
Eq(boost::system::errc::success));
EXPECT_THAT(
- getProperty<std::vector<std::string>>(sut->getPath(), "ReportNames"),
+ getProperty<std::vector<object_path>>(sut->getPath(), "Reports"),
Eq(newNames));
}
TEST_F(TestTrigger, sendsUpdateWhenReportNamesChanges)
{
- const std::vector<std::string> newPropertyVal = {"abc", "one", "two"};
+ std::vector<object_path> newPropertyVal = {
+ utils::constants::reportDirPath / "abc",
+ utils::constants::reportDirPath / "one",
+ utils::constants::reportDirPath / "two"};
EXPECT_CALL(triggerPresenceChanged,
Call(FieldsAre(messages::Presence::Exist, triggerParams.id(),
- UnorderedElementsAreArray(newPropertyVal))));
+ UnorderedElementsAre("abc", "one", "two"))));
- EXPECT_THAT(setProperty(sut->getPath(), "ReportNames", newPropertyVal),
+ EXPECT_THAT(setProperty(sut->getPath(), "Reports", newPropertyVal),
Eq(boost::system::errc::success));
}
TEST_F(TestTrigger, sendsUpdateWhenReportNamesChangesToSameValue)
{
- const std::vector<std::string> newPropertyVal = triggerParams.reportIds();
+ const std::vector<object_path> newPropertyVal = triggerParams.reports();
EXPECT_CALL(
triggerPresenceChanged,
Call(FieldsAre(messages::Presence::Exist, triggerParams.id(),
UnorderedElementsAreArray(triggerParams.reportIds()))));
- EXPECT_THAT(setProperty(sut->getPath(), "ReportNames", newPropertyVal),
+ EXPECT_THAT(setProperty(sut->getPath(), "Reports", newPropertyVal),
Eq(boost::system::errc::success));
}
TEST_F(TestTrigger,
DISABLED_settingPropertyReportNamesThrowsExceptionWhenDuplicateReportIds)
{
- std::vector<std::string> newPropertyVal{"report1", "report2", "report1"};
+ std::vector<object_path> newPropertyVal{
+ utils::constants::reportDirPath / "report1",
+ utils::constants::reportDirPath / "report2",
+ utils::constants::reportDirPath / "report1"};
EXPECT_CALL(triggerPresenceChanged, Call(_)).Times(0);
- EXPECT_THAT(setProperty(sut->getPath(), "ReportNames", newPropertyVal),
+ EXPECT_THAT(setProperty(sut->getPath(), "Reports", newPropertyVal),
+ Eq(boost::system::errc::invalid_argument));
+}
+
+TEST_F(
+ TestTrigger,
+ DISABLED_settingPropertyReportNamesThrowsExceptionWhenReportWithTooManyPrefixes)
+{
+ std::vector<object_path> newPropertyVal{
+ object_path("/xyz/openbmc_project/Telemetry/Reports/P1/P2/MyReport")};
+
+ EXPECT_CALL(triggerPresenceChanged, Call(_)).Times(0);
+
+ EXPECT_THAT(setProperty(sut->getPath(), "Reports", newPropertyVal),
+ Eq(boost::system::errc::invalid_argument));
+}
+
+TEST_F(TestTrigger,
+ DISABLED_settingPropertyReportNamesThrowsExceptionWhenReportWithBadPath)
+{
+ std::vector<object_path> newPropertyVal{
+ object_path("/xyz/openbmc_project/Telemetry/NotReports/MyReport")};
+
+ EXPECT_CALL(triggerPresenceChanged, Call(_)).Times(0);
+
+ EXPECT_THAT(setProperty(sut->getPath(), "Reports", newPropertyVal),
Eq(boost::system::errc::invalid_argument));
}
@@ -218,8 +254,8 @@
std::dynamic_pointer_cast<NiceMock<ThresholdMock>>(threshold);
EXPECT_CALL(*thresholdMockPtr, updateSensors(_));
}
- SensorsInfo newSensors({std::make_pair(
- sdbusplus::message::object_path("/abc/def"), "metadata")});
+ SensorsInfo newSensors(
+ {std::make_pair(object_path("/abc/def"), "metadata")});
EXPECT_THAT(setProperty(sut->getPath(), "Sensors", newSensors),
Eq(boost::system::errc::success));
}
@@ -307,7 +343,8 @@
TEST_F(TestTrigger, deletingNonExistingTriggerReturnInvalidRequestDescriptor)
{
- auto ec = deleteTrigger(Trigger::triggerDir + "NonExisting"s);
+ auto ec =
+ deleteTrigger(utils::constants::triggerDirPath.str + "NonExisting"s);
EXPECT_THAT(ec.value(), Eq(EBADR));
}
diff --git a/tests/src/test_trigger_actions.cpp b/tests/src/test_trigger_actions.cpp
index e8c48d4..42488aa 100644
--- a/tests/src/test_trigger_actions.cpp
+++ b/tests/src/test_trigger_actions.cpp
@@ -1,4 +1,5 @@
#include "dbus_environment.hpp"
+#include "helpers.hpp"
#include "messages/update_report_ind.hpp"
#include "trigger_actions.hpp"
#include "utils/messanger.hpp"
diff --git a/tests/src/test_trigger_manager.cpp b/tests/src/test_trigger_manager.cpp
index efdac65..58d0df1 100644
--- a/tests/src/test_trigger_manager.cpp
+++ b/tests/src/test_trigger_manager.cpp
@@ -10,6 +10,7 @@
#include "utils/transform.hpp"
using namespace testing;
+using sdbusplus::message::object_path;
class TestTriggerManager : public Test
{
@@ -33,7 +34,7 @@
utils::transform(
params.triggerActions(),
[](const auto& action) { return actionToString(action); }),
- sensorInfos, params.reportIds(),
+ sensorInfos, params.reports(),
std::visit(utils::FromLabeledThresholdParamConversion(),
params.thresholdParams()));
return DbusEnvironment::waitForFuture(addTriggerPromise.get_future());
@@ -134,6 +135,38 @@
EXPECT_THAT(path, Eq(std::string()));
}
+TEST_F(TestTriggerManager, addTriggerWithProperReportPaths)
+{
+ auto [ec, path] = addTrigger(TriggerParams().reports(
+ {object_path("/xyz/openbmc_project/Telemetry/Reports/MyReport"),
+ object_path(
+ "/xyz/openbmc_project/Telemetry/Reports/MyPrefix/MyReport")}));
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::success));
+ EXPECT_THAT(path, Eq(triggerMock.getPath()));
+}
+
+TEST_F(TestTriggerManager, DISABLED_failToAddTriggerWithBadReportsPath)
+{
+ triggerFactoryMock.expectMake(std::nullopt, Ref(*sut), Ref(storageMock))
+ .Times(0);
+
+ auto [ec, path] = addTrigger(TriggerParams().reports(
+ {object_path("/xyz/openbmc_project/Telemetry/NotReports/MyReport")}));
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::invalid_argument));
+ EXPECT_THAT(path, Eq(std::string()));
+}
+
+TEST_F(TestTriggerManager, DISABLED_failToAddTriggerWithTooManyReportPrefixes)
+{
+ triggerFactoryMock.expectMake(std::nullopt, Ref(*sut), Ref(storageMock))
+ .Times(0);
+
+ auto [ec, path] = addTrigger(TriggerParams().reports({object_path(
+ "/xyz/openbmc_project/Telemetry/Reports/P1/P2/MyReport")}));
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::invalid_argument));
+ EXPECT_THAT(path, Eq(std::string()));
+}
+
TEST_F(TestTriggerManager, addTriggerWithoutIdAndName)
{
triggerFactoryMock
diff --git a/tests/src/utils/generate_unique_mock_id.cpp b/tests/src/utils/generate_unique_mock_id.cpp
index f3e3f01..7ce4040 100644
--- a/tests/src/utils/generate_unique_mock_id.cpp
+++ b/tests/src/utils/generate_unique_mock_id.cpp
@@ -1,5 +1,7 @@
#include "generate_unique_mock_id.hpp"
+#include "helpers.hpp"
+
uint64_t generateUniqueMockId()
{
static uint64_t id = 0u;