-
Notifications
You must be signed in to change notification settings - Fork 58
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
Fix dvl integration test #348
Conversation
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #348 +/- ##
=======================================
Coverage 71.95% 71.96%
=======================================
Files 38 38
Lines 4868 4869 +1
=======================================
+ Hits 3503 3504 +1
Misses 1365 1365
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some fly by comments/questions on why manual management is necessary.
test/integration/dvl.cc
Outdated
@@ -181,6 +181,7 @@ class DopplerVelocityLogTest : public testing::Test, | |||
seabed->SetLocalPose(seabedPose); | |||
seabed->SetLocalScale(math::Vector3d(1e3, 1e3, 0.0)); | |||
seabed->SetMaterial(sand); | |||
scene->DestroyMaterial(sand); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be automatically handled when the reference count on the material goes to 0 or when the scene itself is destroyed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this call is not needed so removed.
@@ -342,6 +343,8 @@ TEST_P(DopplerVelocityLogTest, BottomTrackingWhileStatic) | |||
4 * config.trackingNoise)); | |||
} | |||
EXPECT_EQ(0, message.status()); | |||
|
|||
this->manager.Remove(sensor->Id()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should deleting the sensors be something the destructor of sensors::Manager
should handle instead of requiring the user to do it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a call to clear sensors in manager destructor. Segfault still happens when exiting test so kept the Remove
here. I believe this happens due to the same reason mentioned in gazebosim/gz-gui#535 (comment). Crash is likely due to shared pointers in gz-rendering removing invalid resources when they go out of scope. It's hard to debug where that's coming from since it only happens on Jammy github actions and not locally. Still need to track this down
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need to track this down
I'll do that separate from this PR.
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
🦟 Bug fix
Fixes dvl integration test that's been segfaulting on exit by making sure the sensor is destroyed before exiting, similar to what's done in #324.
Checklist
codecheck
passed (See contributing)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.