test: ✅ improve speed of joint_trajectory_controller tests

[jaron-l]

Summary:

This PR mainly focuses on changing joint_trajectory_controller/test/test_trajectory_controller_utils.hpp:updateController to be synchronous for reliability which has the added benefit of making them run faster. On my system, this change brings the joint_trajectory_controller tests from 26.3s to 5.24s.

Specific points of interest and requests for feedback:

• joint_trajectory_controller/src/joint_trajectory_controller.cpp::publish_state
  • I made the state_update_rate parameter not rate limit when set to 0. This is a breaking change for that parameter if someone uses it to disable the controller. My argument for this change is that other controllers behave that way and that is a user wants to disable the controller, they can do that through the lifecycle functionality and not through the update rate limiter. This was done because it makes the tests easier to work with if you don't have to arbitrarily wait for the filter to allow updates.
• joint_trajectory_controller/test/test_trajectory_controller.cpp::TrajectoryControllertestParameterized.zero_state_publish_rate
  • I removed this test as it checks for the above point.
• joint_trajectory_controller/test/test_trajectory_controller.cpp::TrajectoryControllertestParameterized.test_state_publish_rate
  • I changed this test to not be parameterized as it doesn't change behavior in response to the parameters. I suspect that it was originally made that way because almost all of the tests are parameterized and the author was just following the other test examples; however, since this test requires a sleep for it to work, it is inefficient to run it several times when each time is identical.

Additional notes:

• This PR is intentionally held back from the tip of master at this point in time because master is unstable and not building nor passing tests.

[mergify]

This pull request is in conflict. Could you fix it @jaron-l?

[codecov-commenter]

Codecov Report

Merging #464 (b069b53) into master (e7f9962) will decrease coverage by 6.03%.
The diff coverage is 20.21%.

@@            Coverage Diff             @@
##           master     #464      +/-   ##
==========================================
- Coverage   35.78%   29.74%   -6.04%     
==========================================
  Files         189        7     -182     
  Lines       17570      743   -16827     
  Branches    11592      428   -11164     
==========================================
- Hits         6287      221    -6066     
+ Misses        994      162     -832     
+ Partials    10289      360    -9929     

Flag	Coverage Δ
unittests	29.74% <20.21%> (-6.04%)	⬇️

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

Impacted Files	Coverage Δ
...de/diff_drive_controller/diff_drive_controller.hpp	100.00% <ø> (ø)
...ontroller/test/test_load_diff_drive_controller.cpp	12.50% <0.00%> (ø)
diff_drive_controller/src/odometry.cpp	42.16% <11.11%> (ø)
...ive_controller/test/test_diff_drive_controller.cpp	17.62% <12.08%> (ø)
diff_drive_controller/src/speed_limiter.cpp	46.55% <13.33%> (ø)
...troller/include/diff_drive_controller/odometry.hpp	20.00% <20.00%> (ø)
...iff_drive_controller/src/diff_drive_controller.cpp	32.13% <24.11%> (ø)
...jectory_controller/joint_trajectory_controller.hpp
...s/test/test_load_joint_group_effort_controller.cpp
...test/test_load_joint_group_velocity_controller.cpp
... and 192 more

[destogl]

Lookg good! Thanks!

[jaron-l]

This will likely need something like ros-controls/realtime_tools#73 as I believe the tests are failing now because the realtime loop fails at trylock. Will move to draft for now.

[mergify]

This pull request is in conflict. Could you fix it @jaron-l?

[christophfroehlich]

I'd say the update method was changed similarly with #858: But I made it updating with a fixed update rate instead of calling update() only twice as proposed here.
I avoided the problem with trylock by still using the correct clock for the message-tests, and don't rely on the publisher for the other tests.

The part with the wait_set might be an improvement, but currently there isn't any problem with the subscriber. I'll keep that in mind if any problem arises.