diff --git a/rclcpp/test/mocking_utils/patch.hpp b/rclcpp/test/mocking_utils/patch.hpp index 8f23d543b9..69550c2130 100644 --- a/rclcpp/test/mocking_utils/patch.hpp +++ b/rclcpp/test/mocking_utils/patch.hpp @@ -31,6 +31,7 @@ #endif #include +#include #include #include #include @@ -386,8 +387,6 @@ template class Patch { public: - using mock_type = typename PatchTraits::mock_type; - /// Construct a patch. /** * \param[in] target Symbol target string, using Mimick syntax @@ -395,40 +394,42 @@ class Patch * binary, "lib:library_name" to target a given library, "file:path/to/library" * to target a given file, or "sym:other_symbol" to target the first library * that defines said symbol. - * \param[in] proxy An indirection to call the target function. - * This indirection must ensure this call goes through the function's - * trampoline, as setup by the dynamic linker. + * \param[in] proxy An indirection that keeps a reference to the target symbol + * in the main executable. * \return a mocking_utils::Patch instance. */ explicit Patch(const std::string & target, std::function proxy) - : target_(target), proxy_(proxy) - { - } + : target_(target), proxy_(std::move(proxy)) + {} // Copy construction and assignment are disabled. Patch(const Patch &) = delete; Patch & operator=(const Patch &) = delete; - Patch(Patch && other) - { - mock_ = other.mock_; - other.mock_ = nullptr; - } + Patch(Patch && other) noexcept + : target_stub_(std::exchange(other.target_stub_, MMK_STUB_INVALID)), + self_stub_(std::exchange(other.self_stub_, MMK_STUB_INVALID)), + target_(std::move(other.target_)), + proxy_(std::move(other.proxy_)), + configured_(other.configured_) + {} - Patch & operator=(Patch && other) + Patch & operator=(Patch && other) noexcept { - if (mock_) { - mmk_reset(mock_); + if (this != &other) { + reset(); + target_stub_ = std::exchange(other.target_stub_, MMK_STUB_INVALID); + self_stub_ = std::exchange(other.self_stub_, MMK_STUB_INVALID); + target_ = std::move(other.target_); + proxy_ = std::move(other.proxy_); + configured_ = other.configured_; } - mock_ = other.mock_; - other.mock_ = nullptr; + return *this; } ~Patch() { - if (mock_) { - mmk_reset(mock_); - } + reset(); } /// Inject a @p replacement for the patched function. @@ -446,27 +447,49 @@ class Patch } private: - // Helper for template parameter pack expansion using `mmk_any` - // macro as pattern. - template - T any() {return mmk_any(T);} - void replace_with(std::function replacement) { - if (mock_) { + if (configured_) { throw std::logic_error("Cannot configure patch more than once"); } auto type_erased_trampoline = reinterpret_cast(prepare_trampoline(replacement)); - auto MMK_MANGLE(mock_type, create) = - PatchTraits::MMK_MANGLE(mock_type, create); - mock_ = mmk_mock(target_.c_str(), mock_type); - mmk_when(proxy_(any()...), .then_call = type_erased_trampoline); + const auto scope_separator = target_.find('@'); + const bool patch_self = + scope_separator != std::string::npos && target_.substr(scope_separator + 1) != "self"; + const auto self_target = patch_self ? target_.substr(0, scope_separator) : std::string{}; + + configured_ = true; + target_stub_ = mmk_stub_create(target_.c_str(), type_erased_trampoline, nullptr); + if (target_stub_ == MMK_STUB_INVALID) { + throw std::runtime_error("Failed to create patch for '" + target_ + "'"); + } + if (patch_self) { + self_stub_ = mmk_stub_create(self_target.c_str(), type_erased_trampoline, nullptr); + if (self_stub_ == MMK_STUB_INVALID) { + reset(); + throw std::runtime_error("Failed to create patch for '" + target_ + "'"); + } + } + } + + void reset() noexcept + { + if (self_stub_ != MMK_STUB_INVALID) { + mmk_stub_destroy(self_stub_); + self_stub_ = MMK_STUB_INVALID; + } + if (target_stub_ != MMK_STUB_INVALID) { + mmk_stub_destroy(target_stub_); + target_stub_ = MMK_STUB_INVALID; + } } - mock_type mock_{nullptr}; + struct mmk_stub * target_stub_{MMK_STUB_INVALID}; + struct mmk_stub * self_stub_{MMK_STUB_INVALID}; std::string target_; std::function proxy_; + bool configured_{false}; }; /// Make a patch for a `target` function. @@ -474,7 +497,8 @@ class Patch * Useful for type deduction during \ref mocking_utils::Patch construction. * * \param[in] target Symbol target string, using Mimick syntax. - * \param[in] proxy An indirection to call the target function. + * \param[in] proxy An indirection that keeps a reference to the target symbol + * in the main executable. * \return a mocking_utils::Patch instance. * * \tparam ID Numerical identifier for this patch. Ought to be unique. @@ -490,8 +514,8 @@ auto make_patch(const std::string & target, std::function proxy) /// Define a dummy operator `op` for a given `type`. /** - * Useful to enable patching functions that take arguments whose types - * do not define basic comparison operators, as required by Mimick. + * Retained for compatibility with tests that define comparison operators + * for types passed through the mocking utility. */ #define MOCKING_UTILS_BOOL_OPERATOR_RETURNS_FALSE(type_, op) \ template \ @@ -509,7 +533,7 @@ auto make_patch(const std::string & target, std::function proxy) /// A transparent forwarding proxy to a given `function`. /** - * Useful to ensure a call to `function` goes through its trampoline. + * Useful to keep the target symbol reachable in the main executable. */ #define MOCKING_UTILS_PATCH_PROXY(function) \ [] (auto && ... args)->decltype(auto) { \ diff --git a/rclcpp/test/rclcpp/test_utilities.cpp b/rclcpp/test/rclcpp/test_utilities.cpp index b64b0a31c6..8a2786b474 100644 --- a/rclcpp/test/rclcpp/test_utilities.cpp +++ b/rclcpp/test/rclcpp/test_utilities.cpp @@ -25,20 +25,8 @@ #include "rclcpp/exceptions.hpp" #include "rclcpp/utilities.hpp" -#ifdef __clang__ -#pragma clang diagnostic push -#pragma clang diagnostic ignored "-Wordered-compare-function-pointers" -#endif -// TODO(ahcorde): the function mocking_utils::patch_and_return called with -// rcl_logging_configure_with_output_handler is returning: "Comparison between pointer and integer" -// Disabling this warning is fine for now. -// Related issue https://github.com/ros2/rclcpp/issues/2488 #include "../mocking_utils/patch.hpp" -#ifdef __clang__ -#pragma clang diagnostic pop -#endif - #include "../utils/rclcpp_gtest_macros.hpp" TEST(TestUtilities, remove_ros_arguments) { @@ -171,6 +159,17 @@ TEST(TestUtilities, test_context_basic_access_const_methods) { EXPECT_EQ(0u, context1->get_pre_shutdown_callbacks().size()); } +TEST(TestUtilities, patch_install_failure_is_reported) { + auto mock = mocking_utils::make_patch<__COUNTER__, rcl_ret_t(void)>( + "mocking_utils_symbol_that_does_not_exist@self", nullptr); + EXPECT_THROW( + mock.then_call([]() {return RCL_RET_OK;}), + std::runtime_error); + EXPECT_THROW( + mock.then_call([]() {return RCL_RET_OK;}), + std::logic_error); +} + MOCKING_UTILS_BOOL_OPERATOR_RETURNS_FALSE(rcl_guard_condition_options_t, ==) MOCKING_UTILS_BOOL_OPERATOR_RETURNS_FALSE(rcl_guard_condition_options_t, !=) MOCKING_UTILS_BOOL_OPERATOR_RETURNS_FALSE(rcl_guard_condition_options_t, >) @@ -187,10 +186,21 @@ TEST(TestUtilities, test_context_init_shutdown_fails) { { auto context_fail_init = std::make_shared(); - auto mock = mocking_utils::patch_and_return( - "lib:rclcpp", rcl_logging_configure_with_output_handler, RCL_RET_ERROR); + bool replacement_called = false; + rcl_logging_output_handler_t received_output_handler = nullptr; + auto mock = mocking_utils::patch( + "lib:rclcpp", rcl_logging_configure_with_output_handler, + ([&replacement_called, &received_output_handler]( + const rcl_arguments_t *, const rcl_allocator_t *, + rcl_logging_output_handler_t output_handler) { + replacement_called = true; + received_output_handler = output_handler; + return RCL_RET_ERROR; + })); EXPECT_THROW(context_fail_init->init(0, nullptr), rclcpp::exceptions::RCLError); EXPECT_FALSE(context_fail_init->is_valid()); + EXPECT_TRUE(replacement_called); + EXPECT_NE(nullptr, received_output_handler); } {