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

Re-re-enable servo tests. #423

Merged
merged 7 commits into from
Apr 19, 2021
Merged

Conversation

vatanaksoytezer
Copy link
Contributor

Ongoing attempt to see what is going with the tests and fixing it

@codecov
Copy link

codecov bot commented Apr 15, 2021

Codecov Report

Merging #423 (549c732) into main (fc6e6fe) will increase coverage by 0.27%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #423      +/-   ##
==========================================
+ Coverage   51.72%   51.98%   +0.27%     
==========================================
  Files         220      223       +3     
  Lines       23167    23342     +175     
==========================================
+ Hits        11981    12133     +152     
- Misses      11186    11209      +23     
Impacted Files Coverage Δ
...meterization/work_space/pose_model_state_space.cpp 80.51% <0.00%> (-0.62%) ⬇️
...servo/include/moveit_servo/make_shared_from_pool.h 100.00% <0.00%> (ø)
moveit_ros/moveit_servo/src/pose_tracking.cpp 76.48% <0.00%> (ø)
.../moveit_servo/include/moveit_servo/pose_tracking.h 100.00% <0.00%> (ø)
moveit_ros/moveit_servo/src/servo_calcs.cpp 64.25% <0.00%> (+2.61%) ⬆️
moveit_ros/moveit_servo/src/servo.cpp 70.97% <0.00%> (+12.91%) ⬆️

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 fc6e6fe...549c732. Read the comment docs.

@vatanaksoytezer
Copy link
Contributor Author

After debugging with @JafarAbdi we traced the bug (for the enforceVelLimits function) to appear after #408. We don't exactly now what is causing this behaviour but we might want to check it out.

@vatanaksoytezer
Copy link
Contributor Author

Also flakiness seems to be something from run_test.py of colcon not being able to verify the result of the tests. Test itself passes but verification fails so it seems to be failing

@AndyZe
Copy link
Member

AndyZe commented Apr 16, 2021

I just started noticing warnings like this. Might be relevant here. Fixed by putting a .0 after the number in the yaml file --

parameter 'moveit_servo.lower_singularity_threshold' has invalid type: expected [double] got [integer]

@AndyZe
Copy link
Member

AndyZe commented Apr 18, 2021

There's a chance this moveit_resources PR will fix the issue: moveit/moveit_resources#63

@AndyZe
Copy link
Member

AndyZe commented Apr 18, 2021

@vatanaksoytezer I think it would be really great if you started a new PR for your port of joint_limits handling (porting this MoveIt1 PR)

@vatanaksoytezer
Copy link
Contributor Author

vatanaksoytezer commented Apr 18, 2021

@AndyZe I will open up a PR for this tomorrow (monday), I was working on porting moveit2_tutorials to github actions, and didn't have time to complete this on Friday.

@vatanaksoytezer
Copy link
Contributor Author

vatanaksoytezer commented Apr 19, 2021

@AndyZe @tylerjw @JafarAbdi I think I found the source of the flakiness. It seems we are starting google test before, we are starting the ros nodes, which causes comm problems sometimes and creates flakiness. I've observed this in three tests (singularity, pose_tracking and interface) which were the ones that are flaky. For eg. https://github.com/ros-planning/moveit2/blob/main/moveit_ros/moveit_servo/test/test_servo_interface.cpp#L225-L226 here, order of the lines 225 and 226 should be vice versa. See if this makes sense, and I am including these in this pr

@vatanaksoytezer
Copy link
Contributor Author

vatanaksoytezer commented Apr 19, 2021

I also disabled servo calcs test for now since @JafarAbdi and I traced it back to #408 .

@JafarAbdi JafarAbdi merged commit 2bd2261 into moveit:main Apr 19, 2021
@vatanaksoytezer
Copy link
Contributor Author

MikeWrock pushed a commit to MikeWrock/moveit2 that referenced this pull request Aug 15, 2022
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

4 participants