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.

Sign up for GitHub

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

Jump to bottom

Fix segfaulting tests #429

Merged
merged 21 commits into from
Aug 22, 2023
Merged

Fix segfaulting tests #429

merged 21 commits into from
Aug 22, 2023

Conversation

mjcarroll
Copy link
Contributor

@mjcarroll mjcarroll commented Jun 28, 2022 •
edited
Loading

🦟 Bug fix

Summary

This removes the deprecated scene3d tests for garden. These have been a source of flakiness for quite some time in CI.

Additionally, this removes the unload calls for the rendering engines, which can cause issues.

Checklist

  • Signed all commits for DCO
  • Added tests
  • 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

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@github-actions github-actions bot added the 🌱 garden Ignition Garden label Jun 28, 2022
ahcorde
ahcorde requested changes Jun 28, 2022
View reviewed changes
test/integration/scene3d_load.cc Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
test/integration/scene3d_config.cc Outdated Show resolved Hide resolved
test/integration/scene3d_config.cc Outdated Show resolved Hide resolved
test/integration/scene3d_load.cc Outdated Show resolved Hide resolved
test/integration/transport_scene_manager.cc Outdated Show resolved Hide resolved
test/integration/transport_scene_manager.cc Outdated Show resolved Hide resolved
mjcarroll added 2 commits June 28, 2022 16:32
@mjcarroll
Fix include paths
bce5f0a 
Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll
Remove scene3d as it is deprecated
872a45c 
Signed-off-by: Michael Carroll <[email protected]>
@codecov
Copy link

codecov bot commented Jun 29, 2022 •
edited
Loading

Codecov Report

Merging #429 (c0857f5) into gz-gui7 (5bbfc63) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

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

@@             Coverage Diff             @@
##           gz-gui7     #429      +/-   ##
===========================================
- Coverage    70.96%   70.91%   -0.05%     
===========================================
  Files           45       45              
  Lines         4939     4941       +2     
===========================================
- Hits          3505     3504       -1     
- Misses        1434     1437       +3
Files Changed Coverage Δ
src/plugins/minimal_scene/MinimalScene.cc 61.99% <100.00%> (-0.37%) ⬇️

@mjcarroll
Remove message header
961f98b 
Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll mjcarroll requested a review from ahcorde June 29, 2022 15:27
ahcorde
ahcorde previously requested changes Jun 29, 2022
View reviewed changes
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

conflicts

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

Removing the unload engine is related to: gazebosim/gz-sim#1410 as well.

@chapulina chapulina added the bug Something isn't working label Jul 23, 2022
@chapulina chapulina changed the base branch from main to gz-gui7 August 5, 2022 19:06
@iche033 iche033 mentioned this pull request Apr 14, 2023
Merged
8 tasks
@iche033
Copy link
Contributor

iche033 commented Apr 14, 2023

follow up PR in #535

azeey
azeey approved these changes Apr 21, 2023
View reviewed changes
Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Just one minor issue. It'd be best if we merge #535 into this before merging this.

src/plugins/minimal_scene/MinimalScene.hh Outdated Show resolved Hide resolved
@azeey azeey mentioned this pull request Jun 13, 2023
Merged
@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 31, 2023
@iche033 iche033 mentioned this pull request Aug 18, 2023
Merged
8 tasks
@azeey
Copy link
Contributor

azeey commented Aug 18, 2023

@iche033 is this good to go?

@iche033
Fix rendering tests (#561)
bb82bb8 
* debugging

Signed-off-by: Ian Chen <[email protected]>

* reset cam

Signed-off-by: Ian Chen <[email protected]>

* more testing

Signed-off-by: Ian Chen <[email protected]>

* no unload

Signed-off-by: Ian Chen <[email protected]>

* more testing with no unload

Signed-off-by: Ian Chen <[email protected]>

* unload engine in gzrenderer

Signed-off-by: Ian Chen <[email protected]>

* unload engine in gzrenderer debugging

Signed-off-by: Ian Chen <[email protected]>

* unload engine in gzrenderer reset scene

Signed-off-by: Ian Chen <[email protected]>

* comment out unload

Signed-off-by: Ian Chen <[email protected]>

* increase timeout

Signed-off-by: Ian Chen <[email protected]>

* test timing

Signed-off-by: Ian Chen <[email protected]>

* cleanup

Signed-off-by: Ian Chen <[email protected]>

* cleanup

Signed-off-by: Ian Chen <[email protected]>

* more cleanup

Signed-off-by: Ian Chen <[email protected]>

---------

Signed-off-by: Ian Chen <[email protected]>
@mjcarroll
Copy link
Contributor Author

I think this is good now, going to wait for CI to turn over, first.

@mjcarroll
Copy link
Contributor Author

Windows is backed up, so I'm going without.

@mjcarroll mjcarroll merged commit c020e25 into gz-gui7 Aug 22, 2023
@mjcarroll mjcarroll deleted the fix_tests branch August 22, 2023 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iche033 iche033 iche033 left review comments

@azeey azeey azeey approved these changes

@ahcorde ahcorde ahcorde left review comments

@chapulina chapulina Awaiting requested review from chapulina

@jennuine jennuine Awaiting requested review from jennuine jennuine is a code owner

Assignees
No one assigned
Labels
beta Targeting beta release of upcoming collection bug Something isn't working 🌱 garden Ignition Garden tests Broken or missing tests / testing infra
Projects
Core development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mjcarroll @iche033 @azeey @ahcorde @chapulina