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

Fix most servo tests, cleanup #511

Closed

Conversation

vatanaksoytezer
Copy link
Contributor

@vatanaksoytezer vatanaksoytezer commented Jun 19, 2021

Servo tests were already passing on Rolling and Galactic when enabled as is. This fixes them in Foxy too with two additions to upstream repos. This PR will:

  • Fix all servo tests except unit_test_servo_calcs which is somewhat still flaky / failing on all distros.
  • Cleans up two unnecessary files in servo folder (deleted tests were already included in basic_servo_tests)
  • OMPL test is still flaky but I've added timeout without enabling the test.

See below for more information for what I've figured out:

The tests are not failing because test file is not generated. I figured they are actually timing out way up in the raw Github action log output. I naturally tried to increase the timeout values, with that I figured the following out:

move_group_ompl_constraint was also timing out. When I increase the timeout from default 60 seconds to 120 seconds:

  • The test is still flaky on all distros (Galactic, Rolling and Foxy) but now gives out an error. The source of the flakiness is 2021-06-19T12:56:32.5818472Z [move_group-1] [WARN] [1624107338.958080006] [ompl]: /tmp/binarydeb/ros-rolling-ompl-1.5.2/src/ompl/tools/multiplan/src/ParallelPlan.cpp:138 - ParallelPlan::solve(): Unable to find solution by any of the threads in 60.002458 seconds. I don't have much idea why this is happening, but when I checked ParallelPlan I couldn't find any mutexes, maybe this?
  • We never got this error before because the planning time was 60 seconds and test timeout was 60 seconds, thus we were always getting timeouts when plan is stalled.

@vatanaksoytezer vatanaksoytezer requested review from AndyZe and tylerjw and removed request for AndyZe June 19, 2021 20:59
@codecov
Copy link

codecov bot commented Jun 19, 2021

Codecov Report

Merging #511 (eb1365d) into main (1d12504) will increase coverage by 7.32%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #511      +/-   ##
==========================================
+ Coverage   45.18%   52.50%   +7.32%     
==========================================
  Files         169      174       +5     
  Lines       18732    19079     +347     
==========================================
+ Hits         8462    10015    +1553     
+ Misses      10270     9064    -1206     
Impacted Files Coverage Δ
.../moveit_servo/include/moveit_servo/pose_tracking.h 100.00% <0.00%> (ø)
moveit_ros/moveit_servo/src/pose_tracking.cpp 75.89% <0.00%> (ø)
...servo/include/moveit_servo/make_shared_from_pool.h 100.00% <0.00%> (ø)
moveit_ros/moveit_servo/src/servo_server.cpp 78.95% <0.00%> (ø)
moveit_ros/moveit_servo/src/servo_parameters.cpp 64.92% <0.00%> (ø)
...e/collision_detection_fcl/src/collision_common.cpp 74.94% <0.00%> (+0.25%) ⬆️
moveit_core/robot_state/src/conversions.cpp 28.91% <0.00%> (+0.79%) ⬆️
moveit_core/robot_model/src/joint_model_group.cpp 55.41% <0.00%> (+1.32%) ⬆️
...ene/include/moveit/planning_scene/planning_scene.h 55.18% <0.00%> (+1.73%) ⬆️
...bot_state/include/moveit/robot_state/robot_state.h 87.72% <0.00%> (+1.76%) ⬆️
... and 25 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 1d12504...eb1365d. Read the comment docs.

MikeWrock pushed a commit to MikeWrock/moveit2 that referenced this pull request Aug 15, 2022
* Update chomp_planner_tutorial.rst

Fix the order of planning_adapters.
Add TimeParameterization should be on the top of the list.
See moveit/moveit#2053

* Update planning_adapters_tutorial.rst
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.

1 participant