Avoid Mimick argument matching for unconditional patches - #3221
Conversation
|
Tick the box to add this pull request to the merge queue (same as
|
| mmk_when(proxy_(any<ArgTs>()...), .then_call = type_erased_trampoline); | ||
| struct mmk_mock_options options {}; | ||
| options.sentinel_ = 1; | ||
| mock_ = mmk_mock_create_internal(target_.c_str(), type_erased_trampoline, options); |
There was a problem hiding this comment.
a broken patch could never go unnoticed before.
the raw internal call sidesteps that safety net, i believe.
it can return an invalid handle, mock_ stays useless, and replace_with returns as if everything worked.
the consequence is nasty specifically because this is a test utility...
| mmk_when(proxy_(any<ArgTs>()...), .then_call = type_erased_trampoline); | ||
| struct mmk_mock_options options {}; | ||
| options.sentinel_ = 1; | ||
| mock_ = mmk_mock_create_internal(target_.c_str(), type_erased_trampoline, options); |
There was a problem hiding this comment.
i do not think using internal methods and members are correct thing to do here. could it use public stub APIs instead of mmk_mock_create_internal especially with options.sentinel_?
can you disclose which AI tools and models are used to create this PR? |
|
according to the test, i think we need to do the test with source build instead of released pacakges. |
Replace the internal mock creation workaround with Mimick’s supported stub API. Install both scoped and self stubs to preserve existing call-site behavior, fail explicitly on installation errors, and avoid instantiating argument matchers for function-pointer arguments. Remove the Clang diagnostic suppression and add regression coverage for installation failure and the function-pointer callback. Signed-off-by: Ulaş <36420486+ukis666@users.noreply.github.com>
6c36aa0 to
9b75d07
Compare
|
Thanks for the review. I reworked the implementation to address the requested changes. The patch no longer uses mmk_mock_create_internal() or accesses internal Mimick members. It now uses the public stub API ( I also repeated the validation using a current ROS 2 Rolling source build, rather than released packages. On Ubuntu 24.04 Noble, all 372 source packages built successfully. The Full repository test results were 3,167 passed, 0 failed, 529 skipped with both GCC and Clang. Regarding the AI disclosure: GPT-5.6 Sol in Ultra mode was used to accelerate analysis, implementation iteration, and test/validation workflows. The PR description has been updated with the implementation and validation details. |
2 participants
Description
mocking_utils::Patchcurrently configures unconditional replacements throughmmk_when(..., mmk_any(...)). Although every argument is matched asANY,Mimick still instantiates its generic argument-comparison machinery.
For
rcl_logging_configure_with_output_handler, one of the arguments is afunction pointer. With Clang, this causes ordered comparisons of function
pointers to be instantiated and triggers
-Wordered-compare-function-pointers.This change removes the unconditional patch path from Mimick's generic argument
matcher and implements it using Mimick's public stub API:
mmk_stub_create()mmk_stub_destroy()MMK_STUB_INVALIDFor non-self scopes, the patch installs a stub for both the scoped symbol and
the bare symbol in the test executable. This is required for calls originating
through header/template code, which may resolve through the executable rather
than the scoped library.
The implementation also preserves the existing safety properties of
Patch:Since the generic argument-matching path is no longer instantiated, the
Clang-specific diagnostic suppression in
test_utilities.cppis no longerneeded and has been removed.
Regression coverage was also added for the failure path and the
function-pointer callback case.
Fixes #2488
Is this user-facing behavior change?
No. This only changes the internal test mocking utility.
Did you use Generative AI?
Yes. GPT-5.6 Sol in Ultra mode was used to accelerate analysis, implementation iteration, and test/validation workflows.
Additional Information
The revised implementation was validated against a current ROS 2 Rolling source
build on Ubuntu 24.04 Noble.
Validation included:
rclcpp,rclcpp_action,rclcpp_components, andrclcpp_lifecyclebuild successfully with both GCC 13.3.0 and Clang 18.1.3.
test_utilitiespasses with Clang and-Werror=ordered-compare-function-pointers.3,167 passed, 0 failed, 529 skipped.
3,167 passed, 0 failed, 529 skipped.
dual-public-stub implementation passes all 9, as does a matched clean
upstream control.
copyright,cpplint,lint_cmake,uncrustify,xmllint, andgit diff --checkpass.No Mimick source code or unrelated ROS 2 packages were modified.