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

Disable actually failing flaky OMPL test #495

Merged

Conversation

vatanaksoytezer
Copy link
Contributor

Disable actually failing flaky OMPL test that causes CI failures all over moveit2. See https://github.com/ros-planning/moveit2/runs/2798018498?check_suite_focus=true for a sample output.

@vatanaksoytezer
Copy link
Contributor Author

Disables #481

@codecov
Copy link

codecov bot commented Jun 11, 2021

Codecov Report

Merging #495 (75bab8c) into main (9ca817d) will decrease coverage by 0.83%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #495      +/-   ##
==========================================
- Coverage   46.01%   45.19%   -0.82%     
==========================================
  Files         170      169       -1     
  Lines       18731    18729       -2     
==========================================
- Hits         8618     8462     -156     
- Misses      10113    10267     +154     
Impacted Files Coverage Δ
...mpl/ompl_interface/src/detail/ompl_constraints.cpp 0.00% <0.00%> (-77.18%) ⬇️
.../include/moveit/robot_model/revolute_joint_model.h 60.00% <0.00%> (-40.00%) ⬇️
...del/include/moveit/robot_model/joint_model_group.h 84.00% <0.00%> (-5.33%) ⬇️
moveit_core/robot_state/src/robot_state.cpp 28.99% <0.00%> (-2.48%) ⬇️
moveit_core/robot_model/src/joint_model_group.cpp 54.09% <0.00%> (-1.05%) ⬇️
...de/moveit/ompl_interface/detail/ompl_constraints.h

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 9ca817d...75bab8c. Read the comment docs.

@henningkayser henningkayser merged commit c029942 into moveit:main Jun 11, 2021
@davetcoleman
Copy link
Member

davetcoleman commented Jul 2, 2021

TODO (vatanaksoytezer): Uncomment once this is fixed

I think this is a pretty bad habit to get into... the chances of you ever re-enabling this flakey test is close to zero. Did a Clubhouse story get created to tackle this issue in the next sprint? Do we have a plan to fix it?

MoveIt already has low test coverage, but disabling existing ones only makes that worse.

Tagging @mamoll as this pertains to OMPL.

@vatanaksoytezer
Copy link
Contributor Author

vatanaksoytezer commented Jul 2, 2021

I think this is a pretty bad habit to get into... the chances of you ever re-enabling this flakey test is close to zero. Did a Clubhouse story get created to tackle this issue in the next sprint? Do we have a plan to fix it?

Thanks for the heads up @davetcoleman. I have a clubhouse story and an issue here to re-enable this. The reason I suggested to disable this was we didn't know the cause and this flakiness was becoming an issue with a lot of PRs failing in CI in the release sprint. #511 has more information from my debugging session. I think @mamoll maybe the right person to look into this.

For the OMPL and servo tests that we disabled I am trying to look whenever I can and really want to enable them back asap.

@v4hn
Copy link
Contributor

v4hn commented Jul 3, 2021

just a side remark: if you have to disable tests, you should usually not disable compiling the whole unit.
Instead you can selectively disable the affected gtests. The code will still be compiled (thus break on API changes) and during execution gtest explicitly mentions the disabled tests.

@vatanaksoytezer
Copy link
Contributor Author

just a side remark: if you have to disable tests, you should usually not disable compiling the whole unit.
Instead you can selectively disable the affected gtests. The code will still be compiled (thus break on API changes) and during execution gtest explicitly mentions the disabled tests.

Thanks a lot @v4hn. Definitely learned something new today! I will make sure I do it this way if we need to disable any test in the future. I might take my time to change this as well if I can't figure out a fix soon.

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.

4 participants