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

[MSA] Testing Framework for MoveItSetupAssistant #1383

Merged
merged 6 commits into from
Jul 12, 2022

Conversation

DLu
Copy link
Contributor

@DLu DLu commented Jun 20, 2022

Description

Adds a nice testing harness for easy checking of generated files.

New Features:

  • Updated sensor configuration format
  • Keep trajectory execution parameters around

Tests

  • Ported the ROS 1 tests for perception and controllers
  • Added some SRDF Planning Groups tests

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@mergify
Copy link

mergify bot commented Jun 20, 2022

Please target the main branch for development, we will backport the changes to feature/msa for you if approved and if they don't break API.

@codecov
Copy link

codecov bot commented Jun 20, 2022

Codecov Report

Merging #1383 (83171c6) into main (54b1a98) will decrease coverage by 10.70%.
The diff coverage is 79.42%.

@@             Coverage Diff             @@
##             main    #1383       +/-   ##
===========================================
- Coverage   61.56%   50.86%   -10.69%     
===========================================
  Files         274      381      +107     
  Lines       24982    31735     +6753     
===========================================
+ Hits        15377    16139      +762     
- Misses       9605    15596     +5991     
Impacted Files Coverage Δ
...it_setup_controllers/moveit_controllers_config.hpp 66.67% <ø> (ø)
.../include/moveit_setup_framework/data_warehouse.hpp 100.00% <ø> (ø)
...tant/moveit_setup_framework/src/data_warehouse.cpp 70.46% <56.25%> (ø)
...moveit_setup_app_plugins/src/perception_config.cpp 71.22% <100.00%> (ø)
...etup_controllers/src/moveit_controllers_config.cpp 85.15% <100.00%> (ø)
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 75.36% <0.00%> (-1.07%) ⬇️
...veit/robot_state_rviz_plugin/robot_state_display.h 0.00% <0.00%> (ø)
...clude/moveit_setup_app_plugins/launches_config.hpp 0.00% <0.00%> (ø)
...e/moveit_setup_srdf_plugins/robot_poses_widget.hpp 0.00% <0.00%> (ø)
... and 106 more

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 54b1a98...83171c6. Read the comment docs.

@vatanaksoytezer
Copy link
Contributor

@DLu do you mind changing the target branch to main, now that we have the feature branch merged?

@mikeferguson
Copy link
Contributor

As a note - this adds some tests that will be wrong once we fix the format of the sensors_3d.yaml load/export in #1398

@mikeferguson
Copy link
Contributor

Looking at this PR, I'm also noticing a few other things about sensors_3d.yaml:

  • the templates/sensor_3d.yaml isn't actually used by the MSA at this point (so that description at the top of the file is somewhat misleading). It would actually be nice to load the default values for the text boxes from this file.
  • format of the templates/sensor_3d.yaml file is wrong - sensors should be a list of names, and then each of the sensor configurations should be namespaces under their name (lists of heterogenous items like this can't really be loaded as ROS2 params AFAIK).

@DLu DLu changed the base branch from feature/msa to main June 29, 2022 02:33
@DLu
Copy link
Contributor Author

DLu commented Jul 11, 2022

Updated description above, ready for review

Copy link
Contributor

@mikeferguson mikeferguson left a comment

Choose a reason for hiding this comment

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

Looks good to me - not sure what's going on with the test coverage showing as down 10% ?? Anybody know what's up with that?

@vatanaksoytezer
Copy link
Contributor

not sure what's going on with the test coverage showing as down 10% ?? Anybody know what's up with that?

Maybe because of some caching we were getting an inaccurate test coverage before, and this PR fixed the cache by having a lot of changes? That's my best bet.

@mikeferguson mikeferguson merged commit 4597319 into moveit:main Jul 12, 2022
@DLu DLu deleted the testing_framework_msa branch July 12, 2022 18:25
@DLu
Copy link
Contributor Author

DLu commented Jul 12, 2022

I've long wondered about the efficacy of the bot, and now I'm sure something's not right.

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.

None yet

3 participants