Fix WaitSet issue in tests

[saikishor]

This PR aims to fix the WaitSet issue with the command subscription in the tests

[bmagyar]

This approach looks very good! Thank you for taking the time!

[saikishor]

@bmagyar @destogl @christophfroehlich Apart from this failing test TestTrajectoryActions.test_tolerances_via_actions, the rest of the tests that use WaitSet pass with the new fixes. I think it's better to fix this test separately in a different PR after or before this. Thank you for your support.

5: [ RUN      ] TestTrajectoryActions.test_tolerances_via_actions
5: [INFO] [1721079954.094818503] [test_joint_trajectory_controller]: No specific joint names are used for command interfaces. Using 'joints' parameter.
5: [INFO] [1721079954.094849567] [test_joint_trajectory_controller]: Command interfaces are [position] and state interfaces are [position velocity].
5: [INFO] [1721079954.094887736] [test_joint_trajectory_controller]: Using 'splines' interpolation method.
5: [INFO] [1721079954.095324437] [test_joint_trajectory_controller]: Action status changes will be monitored at 20.00 Hz.
5: [INFO] [1721079954.397499342] [test_joint_trajectory_controller]: Received new action goal
5: [INFO] [1721079954.397541138] [test_joint_trajectory_controller]: Accepted new action goal
5: [INFO] [1721079954.897661639] [test_joint_trajectory_controller]: Goal reached, success!
5: /home/user/rolling_noble/ros2_control_ws/src/ros2_controllers/joint_trajectory_controller/test/test_trajectory_controller_utils.hpp:48: Failure
5: Expected equality of these values:
5:   active_tolerances.goal_time_tolerance
5:     Which is: 1
5:   default_goal_time
5:     Which is: 0.10000000000000001
5: Google Test trace:
5: /home/user/rolling_noble/ros2_control_ws/src/ros2_controllers/joint_trajectory_controller/test/test_trajectory_actions.cpp:517: Check default values
5: 
5: [INFO] [1721079956.397694646] [test_joint_trajectory_controller]: Received new action goal
5: [INFO] [1721079956.397755103] [test_joint_trajectory_controller]: Accepted new action goal
5: [INFO] [1721079956.897870466] [test_joint_trajectory_controller]: Goal reached, success!
5: [INFO] [1721079958.397807526] [test_joint_trajectory_controller]: Received new action goal
5: [INFO] [1721079958.397844261] [test_joint_trajectory_controller]: Accepted new action goal
5: [INFO] [1721079958.897976363] [test_joint_trajectory_controller]: Goal reached, success!
5: /home/user/rolling_noble/ros2_control_ws/src/ros2_controllers/joint_trajectory_controller/test/test_trajectory_controller_utils.hpp:48: Failure
5: Expected equality of these values:
5:   active_tolerances.goal_time_tolerance
5:     Which is: 1
5:   default_goal_time
5:     Which is: 0.10000000000000001
5: 
5: [  FAILED  ] TestTrajectoryActions.test_tolerances_via_actions (6014 ms)

[christophfroehlich]

@bmagyar @destogl @christophfroehlich Apart from this failing test TestTrajectoryActions.test_tolerances_via_actions, the rest of the tests that use WaitSet pass with the new fixes. I think it's better to fix this test separately in a different PR after or before this. Thank you for your support.

I reported this already in #1192 ;) I can have a look today to fix it 👀 @fmauch

[saikishor]

I reported this already in #1192 ;) I can have a look today to fix it 👀 @fmauch

Awesome. Thank you so much @christophfroehlich

[christophfroehlich]

The new code looks quite simple 👍 And if it works -> great! Thanks for your efforts!

[bmagyar]

Suggested change

	executor.add_node(test_subscription_node.get_node_base_interface());
	received_msg.reset();
	ASSERT_FALSE(received_msg);
	received_msg.reset();
	ASSERT_FALSE(received_msg);
	executor.add_node(test_subscription_node.get_node_base_interface());

I'm not sure I'm understanding this part completely but I think adding the node before resetting the local var is problematic. Why do we even need to reset it and then check that it's still reset?

[saikishor]

It shouldn't be a problem unless you spin in between. if you spin yes, then it would reset and be a problem. I was just resetting and added assert to be sure nothing much

[bmagyar]

Since it is a local variable... does it matter at all if we not reset it?

[saikishor]

Nope, it doesn't matter. I can remove it

[saikishor]

Thanks. Done

[codecov]

Codecov Report

Attention: Patch coverage is 83.09179% with 35 lines in your changes missing coverage. Please review.

Project coverage is 86.58%. Comparing base (0b43291) to head (1d2b3b6).
Report is 48 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #1206       +/-   ##
===========================================
+ Coverage   71.86%   86.58%   +14.71%     
===========================================
  Files          41       95       +54     
  Lines        3650     8585     +4935     
  Branches     1794      718     -1076     
===========================================
+ Hits         2623     7433     +4810     
- Misses        707      883      +176     
+ Partials      320      269       -51     

Flag	Coverage Δ
unittests	86.58% <83.09%> (+14.71%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...roller/test/test_ackermann_steering_controller.cpp	100.00% <100.00%> (ø)
...nce_controller/test/test_admittance_controller.cpp	100.00% <100.00%> (ø)
...ntroller/test/test_bicycle_steering_controller.cpp	100.00% <100.00%> (ø)
...ory_controller/test/test_trajectory_controller.cpp	99.74% <100.00%> (ø)
pid_controller/test/test_pid_controller.cpp	100.00% <100.00%> (ø)
...library/test/test_steering_controllers_library.hpp	96.87% <100.00%> (ø)
...troller/test/test_tricycle_steering_controller.cpp	100.00% <100.00%> (ø)
...ive_controller/test/test_diff_drive_controller.cpp	93.60% <80.00%> (ø)
...ollers/test/test_joint_group_effort_controller.cpp	98.43% <83.33%> (ø)
...ontroller/test/test_forward_command_controller.cpp	99.24% <83.33%> (ø)
... and 14 more

... and 56 files with indirect coverage changes

[bmagyar]

All good, thank you!