version-handler: Refactor handler construction
Instead of requiring callers to build maps and info blobs, take the
minimal required amount of information about the blob configuration and
build required datastructures internally.
This reduces the amount of code, and nearly eliminates all of the
untested code in main.cpp.
Change-Id: Iaa398eb404814e9263e6707b71b38a9831d96697
Signed-off-by: William A. Kennington III <wak@google.com>
diff --git a/bmc/version-handler/test/Makefile.am b/bmc/version-handler/test/Makefile.am
index 0c02ff0..93daa37 100644
--- a/bmc/version-handler/test/Makefile.am
+++ b/bmc/version-handler/test/Makefile.am
@@ -9,16 +9,12 @@
$(GMOCK_CFLAGS) \
$(CODE_COVERAGE_CPPFLAGS)
AM_CXXFLAGS = \
- $(SDBUSPLUS_CFLAGS) \
- $(PHOSPHOR_LOGGING_CFLAGS) \
$(CODE_COVERAGE_CXXFLAGS)
AM_LDFLAGS = \
$(GTEST_LIBS) \
$(GMOCK_LIBS) \
-lgmock_main \
$(OESDK_TESTCASE_FLAGS) \
- $(SDBUSPLUS_LIBS) \
- $(PHOSPHOR_LOGGING_LIBS) \
$(CODE_COVERAGE_LIBS)
# Run all 'check' test programs
diff --git a/bmc/version-handler/test/version_canhandle_enumerate_unittest.cpp b/bmc/version-handler/test/version_canhandle_enumerate_unittest.cpp
index 3637c32..2816318 100644
--- a/bmc/version-handler/test/version_canhandle_enumerate_unittest.cpp
+++ b/bmc/version-handler/test/version_canhandle_enumerate_unittest.cpp
@@ -1,52 +1,29 @@
-#include "flags.hpp"
-#include "image_mock.hpp"
-#include "triggerable_mock.hpp"
-#include "util.hpp"
#include "version_handler.hpp"
+#include "version_mock.hpp"
#include <array>
-#include <string>
-#include <vector>
#include <gtest/gtest.h>
namespace ipmi_flash
{
+
TEST(VersionHandlerCanHandleTest, VerifyGoodInfoMap)
{
- VersionInfoMap test;
- std::array blobNames{"blob0", "blob1", "blob2", "blob3"};
+ constexpr std::array blobNames{"blob0", "blob1", "blob2", "blob3"};
+ VersionBlobHandler handler(createMockVersionConfigs(blobNames));
for (const auto& blobName : blobNames)
{
- test.try_emplace(blobName,
- VersionInfoPack(blobName,
- std::make_unique<VersionActionPack>(
- CreateTriggerMock()),
- CreateImageMock()));
- }
- auto handler = VersionBlobHandler::create(std::move(test));
- ASSERT_NE(handler, nullptr);
- for (const auto& blobName : blobNames)
- {
- EXPECT_TRUE(handler->canHandleBlob(blobName));
+ EXPECT_TRUE(handler.canHandleBlob(blobName));
}
}
TEST(VersionHandlerEnumerateTest, VerifyGoodInfoMap)
{
- VersionInfoMap test;
- std::vector<std::string> blobNames{"blob0", "blob1", "blob2", "blob3"};
- for (const auto& blobName : blobNames)
- {
- test.try_emplace(blobName,
- VersionInfoPack(blobName,
- std::make_unique<VersionActionPack>(
- CreateTriggerMock()),
- CreateImageMock()));
- }
- auto handler = VersionBlobHandler::create(std::move(test));
- ASSERT_NE(handler, nullptr);
- EXPECT_THAT(handler->getBlobIds(),
+ constexpr std::array blobNames{"blob0", "blob1", "blob2", "blob3"};
+ VersionBlobHandler handler(createMockVersionConfigs(blobNames));
+ EXPECT_THAT(handler.getBlobIds(),
::testing::UnorderedElementsAreArray(blobNames));
}
+
} // namespace ipmi_flash
diff --git a/bmc/version-handler/test/version_close_unittest.cpp b/bmc/version-handler/test/version_close_unittest.cpp
index 68e6add..83b4a43 100644
--- a/bmc/version-handler/test/version_close_unittest.cpp
+++ b/bmc/version-handler/test/version_close_unittest.cpp
@@ -1,16 +1,15 @@
-#include "flags.hpp"
-#include "image_mock.hpp"
-#include "triggerable_mock.hpp"
-#include "util.hpp"
#include "version_handler.hpp"
+#include "version_mock.hpp"
-#include <array>
+#include <memory>
#include <string>
#include <vector>
#include <gtest/gtest.h>
+
using ::testing::_;
using ::testing::Return;
+
namespace ipmi_flash
{
@@ -19,26 +18,10 @@
protected:
void SetUp() override
{
- VersionInfoMap vim;
- for (const auto& blobName : blobNames)
- {
- auto t = CreateTriggerMock();
- auto i = CreateImageMock();
- tm[blobName] = reinterpret_cast<TriggerMock*>(t.get());
- im[blobName] = reinterpret_cast<ImageHandlerMock*>(i.get());
- vim.try_emplace(
- blobName,
- VersionInfoPack(
- blobName, std::make_unique<VersionActionPack>(std::move(t)),
- std::move(i)));
- }
- h = VersionBlobHandler::create(std::move(vim));
- ASSERT_NE(h, nullptr);
- for (const auto& [key, val] : tm)
- {
- ON_CALL(*val, trigger()).WillByDefault(Return(true));
- }
+ h = std::make_unique<VersionBlobHandler>(
+ createMockVersionConfigs(blobNames, &im, &tm));
}
+
std::unique_ptr<blobs::GenericBlobInterface> h;
std::vector<std::string> blobNames{"blob0", "blob1", "blob2", "blob3"};
std::unordered_map<std::string, TriggerMock*> tm;
@@ -47,27 +30,23 @@
TEST_F(VersionCloseExpireBlobTest, VerifyOpenThenClose)
{
+ EXPECT_CALL(*tm.at("blob0"), trigger()).WillOnce(Return(true));
EXPECT_CALL(*tm.at("blob0"), status())
- .Times(1)
.WillOnce(Return(ActionStatus::success));
- EXPECT_CALL(*tm.at("blob0"), abort()).Times(0);
EXPECT_TRUE(h->open(0, blobs::read, "blob0"));
EXPECT_TRUE(h->close(0));
}
TEST_F(VersionCloseExpireBlobTest, VerifyUnopenedBlobCloseFails)
{
- EXPECT_CALL(*tm.at("blob0"), status()).Times(0);
- EXPECT_CALL(*tm.at("blob0"), abort()).Times(0);
EXPECT_FALSE(h->close(0));
}
TEST_F(VersionCloseExpireBlobTest, VerifyDoubleCloseFails)
{
+ EXPECT_CALL(*tm.at("blob0"), trigger()).WillOnce(Return(true));
EXPECT_CALL(*tm.at("blob0"), status())
- .Times(1)
.WillOnce(Return(ActionStatus::success));
- EXPECT_CALL(*tm.at("blob0"), abort()).Times(0);
EXPECT_TRUE(h->open(0, blobs::read, "blob0"));
EXPECT_TRUE(h->close(0));
EXPECT_FALSE(h->close(0));
@@ -75,10 +54,9 @@
TEST_F(VersionCloseExpireBlobTest, VerifyBadSessionNumberCloseFails)
{
+ EXPECT_CALL(*tm.at("blob0"), trigger()).WillOnce(Return(true));
EXPECT_CALL(*tm.at("blob0"), status())
- .Times(1)
.WillOnce(Return(ActionStatus::success));
- EXPECT_CALL(*tm.at("blob0"), abort()).Times(0);
EXPECT_TRUE(h->open(0, blobs::read, "blob0"));
EXPECT_FALSE(h->close(1));
EXPECT_TRUE(h->close(0));
@@ -86,8 +64,8 @@
TEST_F(VersionCloseExpireBlobTest, VerifyRunningActionIsAborted)
{
+ EXPECT_CALL(*tm.at("blob0"), trigger()).WillOnce(Return(true));
EXPECT_CALL(*tm.at("blob0"), status())
- .Times(1)
.WillOnce(Return(ActionStatus::running));
EXPECT_CALL(*tm.at("blob0"), abort()).Times(1);
EXPECT_TRUE(h->open(0, blobs::read, "blob0"));
diff --git a/bmc/version-handler/test/version_createhandler_unittest.cpp b/bmc/version-handler/test/version_createhandler_unittest.cpp
index c748f7f..849d1a4 100644
--- a/bmc/version-handler/test/version_createhandler_unittest.cpp
+++ b/bmc/version-handler/test/version_createhandler_unittest.cpp
@@ -1,12 +1,8 @@
-#include "flags.hpp"
-#include "image_mock.hpp"
-#include "triggerable_mock.hpp"
-#include "util.hpp"
#include "version_handler.hpp"
+#include "version_mock.hpp"
#include <array>
-#include <string>
-#include <vector>
+#include <utility>
#include <gtest/gtest.h>
@@ -15,18 +11,20 @@
TEST(VersionHandlerCanHandleTest, VerifyGoodInfoMapPasses)
{
- VersionInfoMap test;
- std::array blobNames{"blob0", "blob1", "blob2", "blob3"};
- for (const auto& blobName : blobNames)
- {
- test.try_emplace(blobName,
- VersionInfoPack(blobName,
- std::make_unique<VersionActionPack>(
- CreateTriggerMock()),
- CreateImageMock()));
- }
- auto handler = VersionBlobHandler::create(std::move(test));
- EXPECT_NE(handler, nullptr);
+ constexpr std::array blobs{"blob0", "blob1"};
+ VersionBlobHandler handler(createMockVersionConfigs(blobs));
+ EXPECT_THAT(handler.getBlobIds(),
+ testing::UnorderedElementsAreArray(blobs));
+}
+
+TEST(VersionHandlerCanHandleTest, VerifyDuplicatesIgnored)
+{
+ constexpr std::array blobs{"blob0"};
+ auto configs = createMockVersionConfigs(blobs);
+ configs.push_back(createMockVersionConfig(blobs[0]));
+ VersionBlobHandler handler(std::move(configs));
+ EXPECT_THAT(handler.getBlobIds(),
+ testing::UnorderedElementsAreArray(blobs));
}
} // namespace ipmi_flash
diff --git a/bmc/version-handler/test/version_mock.hpp b/bmc/version-handler/test/version_mock.hpp
new file mode 100644
index 0000000..80d3748
--- /dev/null
+++ b/bmc/version-handler/test/version_mock.hpp
@@ -0,0 +1,49 @@
+#include "buildjson.hpp"
+#include "image_mock.hpp"
+#include "triggerable_mock.hpp"
+#include "version_handler.hpp"
+
+#include <memory>
+#include <string>
+#include <utility>
+
+namespace ipmi_flash
+{
+
+auto createMockVersionConfig(const std::string& id,
+ ImageHandlerMock** im = nullptr,
+ TriggerMock** tm = nullptr)
+{
+ HandlerConfig<VersionBlobHandler::ActionPack> ret;
+ ret.blobId = id;
+ auto handler = std::make_unique<testing::StrictMock<ImageHandlerMock>>();
+ if (im != nullptr)
+ {
+ *im = handler.get();
+ }
+ ret.handler = std::move(handler);
+ ret.actions = std::make_unique<VersionBlobHandler::ActionPack>();
+ auto trigger = std::make_unique<testing::StrictMock<TriggerMock>>();
+ if (tm != nullptr)
+ {
+ *tm = trigger.get();
+ }
+ ret.actions->onOpen = std::move(trigger);
+ return ret;
+}
+
+template <typename C, typename Im = std::map<std::string, ImageHandlerMock*>,
+ typename Tm = std::map<std::string, TriggerMock*>>
+auto createMockVersionConfigs(const C& ids, Im* im = nullptr, Tm* tm = nullptr)
+{
+ std::vector<HandlerConfig<VersionBlobHandler::ActionPack>> ret;
+ for (const auto& id : ids)
+ {
+ ret.push_back(
+ createMockVersionConfig(id, im == nullptr ? nullptr : &(*im)[id],
+ tm == nullptr ? nullptr : &(*tm)[id]));
+ }
+ return ret;
+}
+
+} // namespace ipmi_flash
diff --git a/bmc/version-handler/test/version_open_unittest.cpp b/bmc/version-handler/test/version_open_unittest.cpp
index 7e287e5..7c7e5d2 100644
--- a/bmc/version-handler/test/version_open_unittest.cpp
+++ b/bmc/version-handler/test/version_open_unittest.cpp
@@ -1,16 +1,16 @@
-#include "flags.hpp"
-#include "image_mock.hpp"
-#include "triggerable_mock.hpp"
-#include "util.hpp"
#include "version_handler.hpp"
+#include "version_mock.hpp"
-#include <array>
+#include <memory>
#include <string>
+#include <unordered_map>
#include <vector>
#include <gtest/gtest.h>
+
using ::testing::_;
using ::testing::Return;
+
namespace ipmi_flash
{
@@ -19,32 +19,15 @@
protected:
void SetUp() override
{
- VersionInfoMap vim;
- for (const auto& blobName : blobNames)
+ h = std::make_unique<VersionBlobHandler>(
+ createMockVersionConfigs(blobNames, &im, &tm));
+ for (const auto& blob : blobNames)
{
- auto t = CreateTriggerMock();
- auto i = CreateImageMock();
- tm[blobName] = reinterpret_cast<TriggerMock*>(t.get());
- im[blobName] = reinterpret_cast<ImageHandlerMock*>(i.get());
- vim.try_emplace(
- blobName,
- VersionInfoPack(
- blobName, std::make_unique<VersionActionPack>(std::move(t)),
- std::move(i)));
- }
- h = VersionBlobHandler::create(std::move(vim));
- ASSERT_NE(h, nullptr);
- for (const auto& [key, val] : tm)
- {
- /* by default no action triggers expected to be called */
- EXPECT_CALL(*val, trigger()).Times(0);
- }
- for (const auto& [key, val] : im)
- {
- /* by default no image handler open is expected to be called */
- EXPECT_CALL(*val, open(_, _)).Times(0);
+ EXPECT_CALL(*tm.at(blob), status())
+ .WillRepeatedly(Return(ActionStatus::unknown));
}
}
+
std::unique_ptr<blobs::GenericBlobInterface> h;
std::vector<std::string> blobNames{"blob0", "blob1", "blob2", "blob3"};
std::unordered_map<std::string, TriggerMock*> tm;
diff --git a/bmc/version-handler/test/version_read_unittest.cpp b/bmc/version-handler/test/version_read_unittest.cpp
index 2f15e03..e1c4618 100644
--- a/bmc/version-handler/test/version_read_unittest.cpp
+++ b/bmc/version-handler/test/version_read_unittest.cpp
@@ -1,10 +1,7 @@
-#include "flags.hpp"
-#include "image_mock.hpp"
-#include "triggerable_mock.hpp"
-#include "util.hpp"
#include "version_handler.hpp"
+#include "version_mock.hpp"
-#include <array>
+#include <memory>
#include <string>
#include <vector>
@@ -20,25 +17,8 @@
protected:
void SetUp() override
{
- VersionInfoMap vim;
- for (const auto& blobName : blobNames)
- {
- auto t = CreateTriggerMock();
- auto i = CreateImageMock();
- tm[blobName] = reinterpret_cast<TriggerMock*>(t.get());
- im[blobName] = reinterpret_cast<ImageHandlerMock*>(i.get());
- vim.try_emplace(
- blobName,
- VersionInfoPack(
- blobName, std::make_unique<VersionActionPack>(std::move(t)),
- std::move(i)));
- }
- h = VersionBlobHandler::create(std::move(vim));
- ASSERT_NE(h, nullptr);
- for (const auto& [key, val] : tm)
- {
- ON_CALL(*val, trigger()).WillByDefault(Return(true));
- }
+ h = std::make_unique<VersionBlobHandler>(
+ createMockVersionConfigs(blobNames, &im, &tm));
}
std::unique_ptr<blobs::GenericBlobInterface> h;
std::vector<std::string> blobNames{"blob0", "blob1", "blob2", "blob3"};
@@ -51,6 +31,7 @@
TEST_F(VersionReadBlobTest, VerifyValidRead)
{
+ EXPECT_CALL(*tm.at("blob0"), trigger()).WillOnce(Return(true));
EXPECT_CALL(*tm.at("blob0"), status())
.Times(2)
.WillRepeatedly(Return(ActionStatus::success));
@@ -71,37 +52,28 @@
TEST_F(VersionReadBlobTest, VerifyUnopenedReadFails)
{
- EXPECT_CALL(*tm.at("blob0"), status()).Times(0);
- EXPECT_CALL(*im.at("blob0"), open(_, _)).Times(0);
- EXPECT_CALL(*im.at("blob0"), read(_, _)).Times(0);
-
EXPECT_THAT(h->read(defaultSessionNumber, 0, 10), IsEmpty());
}
TEST_F(VersionReadBlobTest, VerifyTriggerFailureReadFails)
{
+ EXPECT_CALL(*tm.at("blob0"), trigger()).WillOnce(Return(true));
EXPECT_CALL(*tm.at("blob0"), status())
- .Times(1)
.WillOnce(Return(ActionStatus::failed));
- EXPECT_CALL(*im.at("blob0"), open(_, _)).Times(0);
EXPECT_TRUE(h->open(defaultSessionNumber, blobs::read, "blob0"));
EXPECT_THAT(h->read(defaultSessionNumber, 0, 10), IsEmpty());
}
TEST_F(VersionReadBlobTest, VerifyReadFailsOnFileReadFailure)
{
+ EXPECT_CALL(*tm.at("blob0"), trigger()).WillOnce(Return(true));
EXPECT_CALL(*tm.at("blob0"), status())
- .Times(1)
.WillOnce(Return(ActionStatus::success));
/* file path gets bound to file_handler on creation so path parameter
* doesn't actually matter
*/
- EXPECT_CALL(*im.at("blob0"), open(_, std::ios::in))
- .Times(1)
- .WillOnce(Return(true));
- EXPECT_CALL(*im.at("blob0"), read(_, _))
- .Times(1)
- .WillOnce(Return(std::nullopt));
+ EXPECT_CALL(*im.at("blob0"), open(_, std::ios::in)).WillOnce(Return(true));
+ EXPECT_CALL(*im.at("blob0"), read(_, _)).WillOnce(Return(std::nullopt));
EXPECT_CALL(*im.at("blob0"), close()).Times(1);
EXPECT_TRUE(h->open(defaultSessionNumber, blobs::read, "blob0"));
@@ -110,16 +82,14 @@
TEST_F(VersionReadBlobTest, VerifyReadFailsOnFileOpenFailure)
{
+ EXPECT_CALL(*tm.at("blob0"), trigger()).WillOnce(Return(true));
/* first call to trigger status fails, second succeeds */
EXPECT_CALL(*tm.at("blob0"), status())
- .Times(1)
.WillOnce(Return(ActionStatus::success));
/* file path gets bound to file_handler on creation so path parameter
* doesn't actually matter
*/
- EXPECT_CALL(*im.at("blob0"), open(_, std::ios::in))
- .Times(1)
- .WillOnce(Return(false));
+ EXPECT_CALL(*im.at("blob0"), open(_, std::ios::in)).WillOnce(Return(false));
EXPECT_TRUE(h->open(defaultSessionNumber, blobs::read, "blob0"));
EXPECT_THAT(h->read(defaultSessionNumber, 0, 10), IsEmpty());