Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JTC] Improve update methods for tests #858

Merged

Conversation

christophfroehlich
Copy link
Contributor

@christophfroehlich christophfroehlich commented Nov 26, 2023

I made an attempt to improve the JTC tests once more.

The ideas come from #697 and #802, and partially fix them.

  • state_topic_consistency is now the only test subscribing to the state message publisher.
  • All other tests now use the class variables used for building the state message -> no problem with the trylock() of the realtime publisher, which causes problems with tests not running synchronously with the clock, like with [CI] joint_state_broadcaster tests are flaky with coverage build #821
  • This enables a new updateControllerAsync test method, which calls the update() of JTC with feasible time/period arguments but asynchronous to the current clock.
  • The "old" updateController test method has a sleep_for now, avoiding calling the update() of JTC in the while loop with almost zero duration.
  • Both methods run until the end time is reached (<= end_time instead of < end_time). This allows for reducing the thresholds, because the points should be reached exactly, independent of any timing issue.
  • Cherry-picking small udpates from [JTC] Accept larger number of joints than command_joints #809 allowing to reactivate the jump-tests.

Overall: The total test time of JTC is decreased by more than 1min. And I hope they are less flaky now.

Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent!

Copy link

codecov bot commented Nov 27, 2023

Codecov Report

Merging #858 (6b99228) into master (2854a0b) will increase coverage by 0.08%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #858      +/-   ##
==========================================
+ Coverage   45.51%   45.59%   +0.08%     
==========================================
  Files          40       40              
  Lines        3636     3636              
  Branches     1716     1716              
==========================================
+ Hits         1655     1658       +3     
+ Misses        808      806       -2     
+ Partials     1173     1172       -1     
Flag Coverage Δ
unittests 45.59% <ø> (+0.08%) ⬆️

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

see 1 file with indirect coverage changes

@bmagyar bmagyar merged commit b853228 into ros-controls:master Nov 27, 2023
13 of 14 checks passed
@christophfroehlich christophfroehlich deleted the jtc/trajectory_error_tests branch November 27, 2023 11:31
mergify bot pushed a commit that referenced this pull request Nov 27, 2023
(cherry picked from commit b853228)

# Conflicts:
#	joint_trajectory_controller/test/test_trajectory_controller.cpp
#	joint_trajectory_controller/test/test_trajectory_controller_utils.hpp
christophfroehlich added a commit to christophfroehlich/ros2_controllers that referenced this pull request Nov 27, 2023
christophfroehlich added a commit to christophfroehlich/ros2_controllers that referenced this pull request Nov 29, 2023
christophfroehlich added a commit to christophfroehlich/ros2_controllers that referenced this pull request Nov 29, 2023
christophfroehlich added a commit to christophfroehlich/ros2_controllers that referenced this pull request Nov 30, 2023
christophfroehlich added a commit to christophfroehlich/ros2_controllers that referenced this pull request Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants