Tests for CurrentStateMonitor using dependency injection

[tylerjw]

Description

This is mostly about demonstrating a pattern for using dependency injection to be able to unit test sections of the code with ros dependencies in a unit test.

Checklist

• Required by CI: Code is auto formatted using clang-format
• Extend the tutorials / documentation reference
• Create tests, which fail without this PR reference
• While waiting for someone to review your request, please help review another open pull request to support the maintainers

[codecov]

Codecov Report

Merging #562 (b4a1337) into main (958d0b0) will increase coverage by 0.08%.
The diff coverage is 31.25%.

@@            Coverage Diff             @@
##             main     #562      +/-   ##
==========================================
+ Coverage   46.64%   46.72%   +0.08%     
==========================================
  Files         183      184       +1     
  Lines       19667    19685      +18     
==========================================
+ Hits         9171     9195      +24     
+ Misses      10496    10490       -6     

Impacted Files	Coverage Δ
...eit/planning_scene_monitor/current_state_monitor.h	0.00% <0.00%> (ø)
...or/src/current_state_monitor_middleware_handle.cpp	0.00% <0.00%> (ø)
...anning_scene_monitor/src/current_state_monitor.cpp	12.83% <46.67%> (+12.83%)	⬆️
moveit_core/robot_state/src/robot_state.cpp	29.06% <100.00%> (+0.13%)	⬆️
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp	74.72% <0.00%> (-1.13%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 958d0b0...b4a1337. Read the comment docs.

[davetcoleman]

RCL is already a wrapper around DDS or other middlewares. Now we are creating a wrapper for the wrapper, in the name of testing? Does this kind of functionality belong in core ROS2, rather than a one-off implementation in this one MoveIt class? This type of new complexity in the name of testing does not seem like a good idea to me, in terms of long term maintenance. What's a solution we can use for all of MoveIt? Or for all ROS applications?

[davetcoleman]

Should there be a better name?

Suggested change

	class RclInterface
	class RclTestInterface

[davetcoleman]

This is just for testing right?

[davetcoleman]

This class seems like a generic tool that should not be specific to the CurrentStateMonitor. Maybe it should live in moveit_core/utils ?

[davetcoleman]

This is not very descriptive... the words you used here are all in the file name. Be more helpful to future developers, like:

Suggested change

	* @brief This class contains the rcl interfaces for CurrentStateMontior
	* @brief Dependency injection method for testing the CurrentStateMonitor

[davetcoleman]

should this also throw/print an error, if the string is undefined?

[tylerjw]

This is not a change in behavior (this is a straight copy-paste of code from current_state_monitor)

[tylerjw]

RCL is already a wrapper around DDS or other middlewares. Now we are creating a wrapper for the wrapper, in the name of testing? Does this kind of functionality belong in core ROS2, rather than a one-off implementation in this one MoveIt class?

My original idea was to use the abstract interfaces in rclcpp::node_interfaces. The problem with this became apparent quickly when trying to create a subscription. The underlying behavior in ros is done by a bunch of c functions. These functions depend on a concrete node class existing. There is no way to get dependency injection using types already in rclcpp.

This type of new complexity in the name of testing does not seem like a good idea to me, in terms of long term maintenance.

Increasing maintenance cost (more complex code) in exchange for better testing seems like a good tradeoff to me. With out dependency injection, we can't test these parts of the code without creating rostests. The downside to rostests is they are executing a bunch of code within ros and therefore our tests can go flaky when there are changes within ros. This is the situation with a the tests in moveit_servo and the ompl constraint planner on foxy. With dependency injection we can write tests that only test the code in moveit without testing ros itself.

What's a solution we can use for all of MoveIt? Or for all ROS applications?

This pattern is something we can use for all of moveit. It adds complexity to the implementation of various classes without changing the interfaces of those classes in exchange for tests of the behavior in those classes. We are currently debugging a concurrency issue inside PlanningSceneMonitor for a client that with this pattern we will be able to add tests to isolate (and fix) those issues.

This PR is very limited in its scope and does not include in depth testing of CurrentStateMonitor. That was intentional as we are using this PR as a pattern for how to add the infrastructure for dependency injection based unit tests.

Another benefit of this which I think @v4hn (and @henningkayser and @JafarAbdi who have been doing the syncs) will especially like is that it should make syncing improvements between ros1 and ros2 versions of moveit much easier (after these tests are backported, which should be fairly trivial). The reason for this is it isolates the ROS code to these MiddlewareHandle implementation classes from the actual logic in moveit itself.