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 failing test on Citadel #416

Merged
merged 1 commit into from
Jun 14, 2022
Merged

Disable failing test on Citadel #416

merged 1 commit into from
Jun 14, 2022

Conversation

Blast545
Copy link
Contributor

Signed-off-by: Jorge Perez [email protected]

Summary

Temporary disabling test described on #415 to help with gazebo-tooling/release-tools#398

Checklist

  • Signed all commits for DCO
  • Added tests -> Actually disables a test.
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

@Blast545 Blast545 changed the base branch from ign-gui6 to ign-gui3 June 10, 2022 21:55
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Jun 10, 2022
@Blast545 Blast545 added 🏰 citadel Ignition Citadel and removed 🏯 fortress Ignition Fortress labels Jun 10, 2022
@codecov
Copy link

codecov bot commented Jun 10, 2022

Codecov Report

Merging #416 (58bafc5) into ign-gui3 (5cd426d) will increase coverage by 34.19%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##           ign-gui3     #416       +/-   ##
=============================================
+ Coverage     31.12%   65.32%   +34.19%     
=============================================
  Files            29       39       +10     
  Lines          1343     5182     +3839     
=============================================
+ Hits            418     3385     +2967     
- Misses          925     1797      +872     
Impacted Files Coverage Δ
src/plugins/topic_echo/qrc_TopicEcho.cpp
include/ignition/gui/moc_Dialog.cpp
src/plugins/world_stats/moc_WorldStats.cpp
src/plugins/topic_echo/moc_TopicEcho.cpp
src/plugins/key_publisher/qrc_KeyPublisher.cpp
src/plugins/key_publisher/moc_KeyPublisher.cpp
src/plugins/scene3d/moc_Scene3D.cpp
src/plugins/shutdown_button/qrc_ShutdownButton.cpp
src/plugins/topic_viewer/qrc_TopicViewer.cpp
src/plugins/world_control/qrc_WorldControl.cpp
... and 58 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 5cd426d...58bafc5. Read the comment docs.

@Blast545
Copy link
Contributor Author

@ros-pull-request-builder retest this please

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I think we may be able to get away with just commenting out the EXPECT_FALSE(common::exists(this->kFakeHome)); expectation, but I think the larger approach is ok for now if we tackle #415 soon.

@chapulina chapulina added the tests Broken or missing tests / testing infra label Jun 13, 2022
@Blast545
Copy link
Contributor Author

I took some time with #415 and it looks odd to me, like I may be missing something obvious 😢

I am seeing with this PR CI various errors that I haven't seen in the https://build.osrfoundation.org/job/ignition_gui-ci-ign-gui3-focal-amd64 jobs. Should I disable them with this PR instead of opening multiple PRs?

@Blast545
Copy link
Contributor Author

@ros-pull-request-builder retest this please

@Blast545
Copy link
Contributor Author

I installed a local setup with Citadel and wasn't able to reproduce the errors that appeared on github actions CI. Re-running them with logging mode enable features to see if I can get more information about the failure.

@chapulina
Copy link
Contributor

wasn't able to reproduce the errors that appeared on github actions CI.

Ah I should have mentioned this to you before. A lot of our flaky tests on GitHub actions happens due to #58. I'll add some notes there.

@Blast545
Copy link
Contributor Author

I'm glad I mentioned it, I was about to take more time investigating it.

@chapulina do we want to enable the required checks for those tests as well? Although the failing tests seem to vary a lot with github actions.

As this PR itself, if it's OK with you I'll merge it.

@chapulina
Copy link
Contributor

do we want to enable the required checks for those tests as well?

We do. I think the quickest thing we can do is implement the CMake argument that I mentioned on the issue. We can see if someone else on the team has time for this if you feel like it's out of scope for the buildfarmer work.

@chapulina chapulina merged commit e3cf225 into ign-gui3 Jun 14, 2022
@chapulina chapulina deleted the blast545/disable_415 branch June 14, 2022 18:10
@Blast545
Copy link
Contributor Author

I'd be glad to help, I will assign the issue to myself and take a look what can be done there.

@chapulina chapulina mentioned this pull request Jun 16, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel tests Broken or missing tests / testing infra
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants