Add parameter api integration test #2662
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2662 +/- ##
==========================================
- Coverage 50.74% 43.03% -7.71%
==========================================
Files 392 692 +300
Lines 32553 56297 +23744
Branches 0 7273 +7273
==========================================
+ Hits 16517 24219 +7702
- Misses 16036 31913 +15877
- Partials 0 165 +165 ☔ View full report in Codecov by Sentry. |
sjahr
force-pushed
the
pr-add_param_api_unittest
branch
from
5ab2966 to
c3d3c8b
Compare
moveit_ros/planning_interface/test/launch/move_group_api.test.py
Outdated
Show resolved
Hide resolved
| @@ -43,6 +43,9 @@ | |||
| <test_depend>moveit_resources_panda_moveit_config</test_depend> | |||
| <test_depend>moveit_simple_controller_manager</test_depend> | |||
| <test_depend>moveit_planners_ompl</test_depend> | |||
| <test_depend>moveit_planners_chomp</test_depend> | |||
| <test_depend>moveit_planners_stomp</test_depend> | |||
| <test_depend>pilz_industrial_motion_planner</test_depend> | |||
There was a problem hiding this comment.
so this adds a circular test dependency, even though the planning interface is not listed here. I think we should have a higher-level package for running integration tests for that purpose.
There was a problem hiding this comment.
I moved the test into its own package. At the moment it is part of moveit_ros but we can move it somewhere else too
moveit_ros/planning_interface/test/launch/move_group_api.test.py
Outdated
Show resolved
Hide resolved
| for (const auto& param_name : move_group_test::PARAMETER_NAME_LIST) | ||
| { | ||
| bool param_exists = false; | ||
| if (params_client->wait_for_service(std::chrono::seconds(1))) |
There was a problem hiding this comment.
Is this going to cause it to wait 1 second for each parameter in the case where this fails? That will make this test take forever. You already have this outside the for loop so maybe consider dropping it here and assume that if we've connected before to the parameter client we are connected here too.
There was a problem hiding this comment.
I've experienced issues when I did not use this. My guess is that the other node couldn't handle getting so many service requests in little time. This fixes this as we're waiting for the service node to be ready to handle a request once it has answered another one.
There was a problem hiding this comment.
Let's try if the test still works if we drastically decrease the time
There was a problem hiding this comment.
Looking at the API it of the parameter thing it looks like we could ask for multiple parameters at once. Maybe it makes sense to just ask for all of them in one shot?
| for (const auto& param_name : move_group_test::PARAMETER_NAME_LIST) | ||
| { | ||
| bool param_exists = false; | ||
| if (params_client->wait_for_service(std::chrono::seconds(0.1))) |
There was a problem hiding this comment.
It seems that if this were to fail each parameter would cause a 1 second wait making this test take a long time. You already checked that the service is connected above.
|
This pull request is in conflict. Could you fix it @sjahr? |
Co-authored-by: Henning Kayser <henningkayser@picknik.ai>
sjahr
force-pushed
the
pr-add_param_api_unittest
branch
from
2e3f19c to
b197f9d
Compare
Description
Unittest to check if all parameters are correctly loaded when a move_group is launched
Checklist