-
Notifications
You must be signed in to change notification settings - Fork 324
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
Fixes tests to work with use_global_arguments NodeOptions parameter #1256
Fixes tests to work with use_global_arguments NodeOptions parameter #1256
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't we need the changes for admittance controller and range_sensor_broadcaster?
the tests succeed, which means that they load successfully without parameters?
Shouldn't we check if they initialized properly?
1: [ERROR] [1724184278.166220317] [load_admittance_controller]: Exception thrown during init stage with message: parameter 'joints' is not initialized
1:
1: [ERROR] [1724184278.166266164] [test_controller_manager]: Could not initialize the controller named 'load_admittance_controller'
1: [ OK ] TestLoadAdmittanceController.load_controller (40 ms)
range_sensor_broadcaster initializes fine, I guess it doesn't need non-default parameters.
@christophfroehlich it's probable that they are returning successful even without parameters. That's the only explanation. I could try fixing them tomorrow. |
but it throws the exception, and CM::add_controller_impl should return nullptr.. Furthermore, I'm not sure why load_JTC succeeds. it should throw an error without paramaters, but I only see
This is not really linked to this PR here. I suggest applying the same changes to admittance_ctrl and range_sensor_broadcaster (the only controllers using a yaml file which are not already addressed) and create a follow-up issue for those false-positive tests. |
Ok. I'm outside right now. I'll try to do that sooner |
@christophfroehlich applied the fix to both the controllers as well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1256 +/- ##
==========================================
+ Coverage 80.40% 80.43% +0.02%
==========================================
Files 105 105
Lines 9331 9353 +22
Branches 816 818 +2
==========================================
+ Hits 7503 7523 +20
Misses 1556 1556
- Partials 272 274 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Needs: ros-controls/ros2_control#1701
Fixes: #1252
Fixes: #1253
Fixes: #1254
Fixes: #1255