Refactor Servo velocity bounds enforcement. Disable flaky unit tests. #428

vatanaksoytezer · 2021-04-19T20:02:19Z

Port of moveit/moveit#2471 and an attempt to fix flakiness (again). Overall this PR:

Ports 2471.
Fixes most flakiness of the tests.
Adds low latency (named basic_servo_tests)

codecov · 2021-04-19T20:33:53Z

Codecov Report

Merging #428 (edfffa9) into main (d788700) will decrease coverage by 2.13%.
The diff coverage is 0.00%.

❗ Current head edfffa9 differs from pull request most recent head ff9eb0b. Consider uploading reports for the commit ff9eb0b to get more accurate results

@@            Coverage Diff             @@
##             main     #428      +/-   ##
==========================================
- Coverage   51.41%   49.29%   -2.12%     
==========================================
  Files         223      218       -5     
  Lines       23343    22985     -358     
==========================================
- Hits        12000    11328     -672     
- Misses      11343    11657     +314

Impacted Files	Coverage Δ
moveit_ros/moveit_servo/src/servo_calcs.cpp	`0.00% <0.00%> (-63.83%)`	⬇️
...oveit_servo/include/moveit_servo/collision_check.h	`0.00% <0.00%> (-100.00%)`	⬇️
...veit_servo/include/moveit_servo/servo_parameters.h	`0.00% <0.00%> (-100.00%)`	⬇️
moveit_ros/moveit_servo/src/collision_check.cpp	`0.00% <0.00%> (-75.36%)`	⬇️
moveit_ros/moveit_servo/src/servo.cpp	`0.00% <0.00%> (-70.96%)`	⬇️
..._core/collision_detection/src/collision_common.cpp	`0.00% <0.00%> (-42.85%)`	⬇️
...space/constrained_planning_state_space_factory.cpp	`66.67% <0.00%> (-33.33%)`	⬇️
...lanners/ompl/ompl_interface/src/ompl_interface.cpp	`50.48% <0.00%> (-26.44%)`	⬇️
...anning_scene_monitor/src/current_state_monitor.cpp	`36.25% <0.00%> (-19.65%)`	⬇️
... 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 0f94cfd...ff9eb0b. Read the comment docs.

vatanaksoytezer · 2021-04-20T13:04:01Z

Seems folks at nav2 had a similar case of flaky tests (see ros-navigation/navigation2#584). They solved it with seperating the test with the others.

In a related note this PR should solve the test failures related to servo_server not being opened.

vatanaksoytezer · 2021-04-20T19:24:30Z

This also includes #352

vatanaksoytezer · 2021-04-22T13:38:48Z

@JafarAbdi @AndyZe @tylerjw can you guys review this? It seems to pass for some time with multi thread and sleeps added to tests.

tylerjw

I glanced through the changes and left a couple comments. Thank-you for working on this. I'll test this soon and give you a more detailed review.

moveit_ros/moveit_servo/test/basic_servo_tests.cpp

moveit_ros/moveit_servo/test/launch/test_servo_collision.test.py

AndyZe · 2021-04-23T02:50:03Z

I think you should rename the PR because it's more than just porting the ROS1 PR now. Maybe something like:

Refactor Servo velocity bounds enforcement. Fix flaky unit tests.

AndyZe

Generally looks great!

moveit_ros/moveit_servo/include/moveit_servo/servo.h

moveit_ros/moveit_servo/test/launch/servo_launch_test_common.py

moveit_ros/moveit_servo/test/launch/test_servo_collision.test.py

moveit_ros/moveit_servo/test/launch/test_servo_integration.test.py

moveit_ros/moveit_servo/test/launch/test_servo_pose_tracking.test.py

moveit_ros/moveit_servo/test/launch/test_servo_singularity.test.py

moveit_ros/planning_interface/test/launch/move_group_ompl_constraints.test.py

moveit_ros/moveit_servo/test/launch/test_basic_servo.test.py

gavanderhoorn · 2021-04-23T12:14:49Z

@vatanaksoytezer: a small comment on logistics: it would be OK to git push commits in batches, instead of individually.

Pushing individual commits leads to the CI infrastructure starting jobs every time you push, and it also leads to everyone who Watches this repository receiving emails for every individual push as well.

Over the past few days, I've received 48 notifications about this PR alone, which seems a bit excessive.

vatanaksoytezer · 2021-04-23T12:24:08Z

@gavanderhoorn thanks for the heads up and sorry that you've received so many notifications. Although I am aware that I should not push that much, for this PR I was actually pushing on purpose to trigger actions and see if the flakiness on tests continue. I cannot reproduce the errors in my local machine and wanted to try as much as I can to see if it breaks with those changes.

On your comment I could probably test this in my own Github repository before opening the PR (which I did for the first 15 or so commits), but I found out tests be still failing after I opened the PR here. Will try my best to not push that much again.

gavanderhoorn · 2021-04-23T12:32:28Z

I was actually pushing on purpose to trigger actions and see if the flakiness on tests continue.

of course if you're trying to debug the GHA cfg, that would be a different situation.

It was more of a general comment though. The same happened in a few other PRs.

I could probably test this in my own Github repository before opening the PR (which I did for the first 15 or so commits)

that seems like a good approach indeed.

vatanaksoytezer · 2021-04-23T12:34:44Z

It was more of a general comment though. The same happened in a few other PRs.

No worries, I will be more careful with my other PRs. Thanks for giving the heads up.

vatanaksoytezer · 2021-04-24T10:35:13Z

@AndyZe @tylerjw @JafarAbdi I am thinking about disabling the tests again since CI still seems to fail with the same errors after my additions.

AndyZe · 2021-04-25T13:10:25Z

I am thinking about disabling the tests again since CI still seems to fail with the same errors after my additions.

Yep, I guess that is what we have to do.

AndyZe · 2021-04-27T13:10:01Z

@vatanaksoytezer can you go ahead and set the tests back as they were previously and we'll get this merged?
We're at 42 commits now. I think it would be a good idea to squash this all down to 1 commit that only updates velocity bounds enforcement.

vatanaksoytezer · 2021-04-27T13:17:31Z

@vatanaksoytezer can you go ahead and set the tests back as they were previously and we'll get this merged?
We're at 42 commits now. I think it would be a good idea to squash this all down to 1 commit that only updates velocity bounds enforcement.

@AndyZe I was waiting for today's meetup to confirm disabling tests with other people as well. Will squash and update this PR afterwards.

tylerjw · 2021-05-10T00:13:19Z

I'm sorry I've been slow to respond on this. I'll look more closely at this tonight.

vatanaksoytezer · 2021-05-10T16:57:17Z

Only remaining problem is: https://github.com/ros-planning/moveit2/runs/2547716859?check_suite_focus=true#step:7:7976 any ideas @tylerjw @AndyZe ? Every other test works fine and as expected. I would honestly prefer to just disable this and merge this PR and open up a new one to fix that issue.

AndyZe · 2021-05-10T16:58:05Z

Disabling that one test sounds fine to me. I checked the yaml file and it seems fine. It passes locally, too.

Net test coverage still improves by a lot.

vatanaksoytezer · 2021-05-10T18:47:37Z

The error seems to persist (see https://github.com/ros-planning/moveit2/runs/2548539403)

AndyZe

Blocking until flaky tests are resolved

AndyZe · 2021-05-11T13:28:10Z

OK, we have tried very hard to fix CI. I'm at the point where we just disable the tests again.

vatanaksoytezer · 2021-05-11T13:41:00Z

I think that's fair. Probably we will have to leave to another day.

henningkayser · 2021-05-20T19:26:37Z

I'm merging this, CI is fixed by #470.

vatanaksoytezer changed the title ~~Port of Refactor Servo velocity bounds enforcement~~ Port of Refactor Servo velocity bounds enforcement (Work in progress) Apr 19, 2021

vatanaksoytezer changed the title ~~Port of Refactor Servo velocity bounds enforcement (Work in progress)~~ Port of Refactor Servo velocity bounds enforcement Apr 20, 2021

vatanaksoytezer added the enhancement New feature or request label Apr 22, 2021

vatanaksoytezer requested review from tylerjw, JafarAbdi and AndyZe and removed request for tylerjw and JafarAbdi April 22, 2021 13:38

tylerjw reviewed Apr 22, 2021

View reviewed changes

moveit_ros/moveit_servo/test/basic_servo_tests.cpp Outdated Show resolved Hide resolved

moveit_ros/moveit_servo/test/launch/test_servo_collision.test.py Outdated Show resolved Hide resolved

AndyZe reviewed Apr 23, 2021

View reviewed changes

vatanaksoytezer changed the title ~~Port of Refactor Servo velocity bounds enforcement~~ Refactor Servo velocity bounds enforcement. Fix flaky unit tests. Apr 23, 2021

AndyZe force-pushed the feature/port2471 branch from 733ee87 to 5cca59b Compare April 23, 2021 19:15

AndyZe approved these changes Apr 23, 2021

View reviewed changes

vatanaksoytezer mentioned this pull request Apr 26, 2021

[Servo] Halt Servo command on Pose Tracking stop (#2501) #402

Closed

vatanaksoytezer changed the title ~~Refactor Servo velocity bounds enforcement. Fix flaky unit tests.~~ Refactor Servo velocity bounds enforcement. Disable flaky unit tests. Apr 28, 2021

vatanaksoytezer changed the title ~~Refactor Servo velocity bounds enforcement. Disable flaky unit tests.~~ Refactor Servo velocity bounds enforcement. Enable flaky unit tests. May 10, 2021

AndyZe requested changes May 10, 2021

View reviewed changes

AndyZe force-pushed the feature/port2471 branch 3 times, most recently from 554bb82 to 606abc0 Compare May 11, 2021 03:11

AndyZe force-pushed the feature/port2471 branch 7 times, most recently from ac06ed2 to ce84511 Compare May 11, 2021 18:45

AndyZe changed the title ~~Refactor Servo velocity bounds enforcement. Enable flaky unit tests.~~ Refactor Servo velocity bounds enforcement. Disable flaky unit tests. May 11, 2021

Refactor Servo velocity bounds enforcement. Disable flaky unit tests.

edfffa9

AndyZe force-pushed the feature/port2471 branch from ce84511 to edfffa9 Compare May 11, 2021 18:56

AndyZe approved these changes May 11, 2021

View reviewed changes

schornakj mentioned this pull request May 19, 2021

Action client feedback callback does not reliably trigger ros2/rclcpp#1679

Open

Merge branch 'main' into feature/port2471

ff9eb0b

henningkayser merged commit 0192dfd into moveit:main May 20, 2021

JafarAbdi mentioned this pull request Sep 28, 2021

Port moveit_servo low_latency tests #352

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Servo velocity bounds enforcement. Disable flaky unit tests. #428

Refactor Servo velocity bounds enforcement. Disable flaky unit tests. #428

vatanaksoytezer commented Apr 19, 2021 •

edited

Loading

codecov bot commented Apr 19, 2021 •

edited

Loading

vatanaksoytezer commented Apr 20, 2021

vatanaksoytezer commented Apr 20, 2021

vatanaksoytezer commented Apr 22, 2021

tylerjw left a comment

AndyZe commented Apr 23, 2021 •

edited

Loading

AndyZe left a comment

gavanderhoorn commented Apr 23, 2021

vatanaksoytezer commented Apr 23, 2021 •

edited

Loading

gavanderhoorn commented Apr 23, 2021

vatanaksoytezer commented Apr 23, 2021

vatanaksoytezer commented Apr 24, 2021

AndyZe commented Apr 25, 2021

AndyZe commented Apr 27, 2021

vatanaksoytezer commented Apr 27, 2021

tylerjw commented May 10, 2021

vatanaksoytezer commented May 10, 2021

AndyZe commented May 10, 2021 •

edited

Loading

vatanaksoytezer commented May 10, 2021

AndyZe left a comment

AndyZe commented May 11, 2021

vatanaksoytezer commented May 11, 2021

henningkayser commented May 20, 2021

Refactor Servo velocity bounds enforcement. Disable flaky unit tests. #428

Refactor Servo velocity bounds enforcement. Disable flaky unit tests. #428

Conversation

vatanaksoytezer commented Apr 19, 2021 • edited Loading

codecov bot commented Apr 19, 2021 • edited Loading

Codecov Report

vatanaksoytezer commented Apr 20, 2021

vatanaksoytezer commented Apr 20, 2021

vatanaksoytezer commented Apr 22, 2021

tylerjw left a comment

Choose a reason for hiding this comment

AndyZe commented Apr 23, 2021 • edited Loading

AndyZe left a comment

Choose a reason for hiding this comment

gavanderhoorn commented Apr 23, 2021

vatanaksoytezer commented Apr 23, 2021 • edited Loading

gavanderhoorn commented Apr 23, 2021

vatanaksoytezer commented Apr 23, 2021

vatanaksoytezer commented Apr 24, 2021

AndyZe commented Apr 25, 2021

AndyZe commented Apr 27, 2021

vatanaksoytezer commented Apr 27, 2021

tylerjw commented May 10, 2021

vatanaksoytezer commented May 10, 2021

AndyZe commented May 10, 2021 • edited Loading

vatanaksoytezer commented May 10, 2021

AndyZe left a comment

Choose a reason for hiding this comment

AndyZe commented May 11, 2021

vatanaksoytezer commented May 11, 2021

henningkayser commented May 20, 2021

vatanaksoytezer commented Apr 19, 2021 •

edited

Loading

codecov bot commented Apr 19, 2021 •

edited

Loading

AndyZe commented Apr 23, 2021 •

edited

Loading

vatanaksoytezer commented Apr 23, 2021 •

edited

Loading

AndyZe commented May 10, 2021 •

edited

Loading