-
Notifications
You must be signed in to change notification settings - Fork 59
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
[fortress] Fix triggered camera test #215
[fortress] Fix triggered camera test #215
Conversation
Signed-off-by: Steve Peters <[email protected]>
Don't limit test to ogre1. Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
the triggered camera test is now failing on both Ubuntu and macOS:
@WilliamLewww the trigger topic |
Signed-off-by: William Lew <[email protected]>
Looks like when I merged the <trigger_topic> pull request, the integration test broke from a change in the topic name. |
Once merged, I'll open a forward port [to garden] for this fix. |
Codecov Report
@@ Coverage Diff @@
## ign-sensors6 #215 +/- ##
=============================================
Coverage 80.00% 80.00%
=============================================
Files 1 1
Lines 15 15
=============================================
Hits 12 12
Misses 3 3 Continue to review full report at Codecov.
|
Failing integration tests are unrelated to the triggered camera:
|
changes look good to me. Not sure why tests are failing on Jammy though |
@@ -7,6 +7,7 @@ | |||
<topic>/test/integration/TriggeredCameraPlugin_imagesWithBuiltinSDF</topic> | |||
<camera> | |||
<triggered>true</triggered> | |||
<trigger_topic>/test/integration/TriggeredCameraPlugin_imagesWithBuiltinSDF/trigger</trigger_topic> |
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.
I suppose this fixes it, but it's weird to me that the default trigger topic is __default__/trigger
. I think this will need to be fixed in sdformat. I'll take a look at that.
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.
Codecov Report
@@ Coverage Diff @@
## ign-sensors6 #215 +/- ##
=============================================
Coverage 80.00% 80.00%
=============================================
Files 1 1
Lines 15 15
=============================================
Hits 12 12
Misses 3 3 Continue to review full report at Codecov.
|
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.
Changes look good. The sdformat PR is merged and released (but looks like it does not necessary depend on it after fixing the topic name).
correct; I think we can handle the behavior when trigger topic is empty string in a separate PR |
🦟 Bug fix
Starts to fix a broken test for triggered cameras
Summary
A test for triggered cameras was added in #194, but it has a few issues.
test/integration/camera.cc
, so change it toTriggeredCameraTest
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.