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

Delete Python integration tests #2186

Merged
merged 1 commit into from
Jul 3, 2020
Merged

Delete Python integration tests #2186

merged 1 commit into from
Jul 3, 2020

Conversation

AndyZe
Copy link
Member

@AndyZe AndyZe commented Jul 3, 2020

Previously there was a mix of C++ tests and Python tests. The Python tests require a ros-pytest dependency (this is the only package in MoveIt that uses it). That's a little bit of a maintenance burden.

This just deletes the Python tests. Some of them are redundant with C++ anyway. The test coverage that we'll be losing is:

  • drift dimensions service
  • halt message
  • vel/accel limits

But we can add those to the C++ tests relatively easily now that we have a good framework for it.

Discussion about ros-pytest here: #2066 (comment)

@AndyZe AndyZe requested review from rhaschke and v4hn as code owners July 3, 2020 14:51
@AndyZe AndyZe mentioned this pull request Jul 3, 2020
20 tasks
Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

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

Deleting python tests because they are the only ones (using the framework) is a bad excuse. We have too few python tests as-is.
But as these are not at all related to our python interfaces, but only test ROS interfaces, they can be dropped indeed.

@codecov
Copy link

codecov bot commented Jul 3, 2020

Codecov Report

Merging #2186 into master will increase coverage by 0.15%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2186      +/-   ##
==========================================
+ Coverage   57.61%   57.77%   +0.15%     
==========================================
  Files         327      326       -1     
  Lines       24923    25661     +738     
==========================================
+ Hits        14359    14825     +466     
- Misses      10564    10836     +272     
Impacted Files Coverage Δ
...eit/planning_scene_monitor/current_state_monitor.h 50.00% <0.00%> (-50.00%) ⬇️
...nning_scene_monitor/src/planning_scene_monitor.cpp 70.10% <0.00%> (-29.90%) ⬇️
moveit_ros/moveit_servo/src/servo_calcs.cpp 61.26% <0.00%> (-8.93%) ⬇️
...g_scene_interface/src/planning_scene_interface.cpp 51.51% <0.00%> (-3.28%) ⬇️
...meterization/work_space/pose_model_state_space.cpp 82.31% <0.00%> (-0.69%) ⬇️
moveit_ros/moveit_servo/src/servo_server.cpp
...it/planning_scene_monitor/planning_scene_monitor.h 91.30% <0.00%> (+0.39%) ⬆️
...ipulation/pick_place/src/manipulation_pipeline.cpp 77.35% <0.00%> (+0.94%) ⬆️
moveit_core/robot_model/src/joint_model_group.cpp 61.81% <0.00%> (+2.42%) ⬆️
...e/src/parameterization/model_based_state_space.cpp 73.10% <0.00%> (+2.75%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a15551f...077f404. Read the comment docs.

@v4hn v4hn added the awaits 2nd review one maintainer approved this request label Jul 3, 2020
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Andy, you are the maintainer of this. So, I trust your decision.

@rhaschke rhaschke merged commit 0274ea9 into moveit:master Jul 3, 2020
rhaschke pushed a commit to ubi-agni/moveit that referenced this pull request Jul 4, 2020
which (mostly) are redundant to the corresponding C++ tests.
@AndyZe AndyZe deleted the andyz/delete_python_tests branch September 18, 2020 13:33
sjahr pushed a commit to sjahr/moveit that referenced this pull request Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaits 2nd review one maintainer approved this request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants