Fix potentially flaky integration component test case

[adlarkin]

🦟 Bug fix

Summary

I noticed that the components integration test fails at times for me locally. I found that for the LogicalMicrophone component test I wrote last year, I forgot to initialize one of the streams with the relevant data 🙃 I believe that this should make the test pass consistently now.

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• 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

[codecov]

Codecov Report

Merging #848 (481e413) into ign-gazebo3 (01fc2ff) will increase coverage by 12.27%.
The diff coverage is 90.91%.

@@               Coverage Diff                @@
##           ign-gazebo3     #848       +/-   ##
================================================
+ Coverage        65.71%   77.98%   +12.27%     
================================================
  Files              127      216       +89     
  Lines             6238    12131     +5893     
================================================
+ Hits              4099     9460     +5361     
- Misses            2139     2671      +532     

Impacted Files	Coverage Δ
include/ignition/gazebo/EntityComponentManager.hh	100.00% <ø> (ø)
include/ignition/gazebo/Link.hh	100.00% <ø> (ø)
include/ignition/gazebo/Model.hh	100.00% <ø> (ø)
include/ignition/gazebo/SdfEntityCreator.hh	100.00% <ø> (ø)
include/ignition/gazebo/Server.hh	100.00% <ø> (ø)
include/ignition/gazebo/ServerConfig.hh	100.00% <ø> (ø)
include/ignition/gazebo/System.hh	100.00% <ø> (ø)
include/ignition/gazebo/components/Component.hh	100.00% <ø> (ø)
include/ignition/gazebo/gui/GuiEvents.hh	0.00% <0.00%> (ø)
src/SimulationRunner.hh	100.00% <ø> (ø)
... and 199 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 ee895b8...481e413. Read the comment docs.

[scpeters]

out of curiosity, what console output do you see from a failure of this test?

[adlarkin]

out of curiosity, what console output do you see from a failure of this test?

Sometimes, I will see this:

Expected equality of these values:
  comp1
    Which is: 32-byte object <E0-D7 33-3B AC-55 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 E0-3F>
  comp3
    Which is: 32-byte object <E0-D7 33-3B AC-55 00-00 00-00 00-00 00-00 00-00 D0-2B 80-3B AC-55 00-00 60-D0 14-84 D3-7F 00-00>

If you look at the code I am changing, before this PR, I am just comparing a deserialized default-constructed std::istringstream with comp1, which is a logical_audio::Microphone object that is not default-constructed. I'm honestly amazed that this comparison passes most of the time, because it's an incorrect comparison.

[scpeters]

your change looks right to me, but I wanted to understand why it wasn't always failing. I think it's because the logical_audio::Microphone == operator only compares the id values and not the volumeDetectionThreshold. I think it would be worth adding some additional EXPECT_NEAR or EXPECT_DOUBLE_EQ statements for the volumeDetectionThreshold to confirm the equality

[adlarkin]

your change looks right to me, but I wanted to understand why it wasn't always failing. I think it's because the logical_audio::Microphone == operator only compares the id values and not the volumeDetectionThreshold. I think it would be worth adding some additional EXPECT_NEAR or EXPECT_DOUBLE_EQ statements for the volumeDetectionThreshold to confirm the equality

Yeah, that's a good point! I also think an issue here was that mic1.id was set to 0, which I believe is (usually) the default value given to unsigned ints. So, I am guessing that before the changes in this PR, the default-constructed comp3 was right most of the time because comp3.id was set to 0. However, since there's no guarantee regarding what a data type will be initialized to unless explicitly assigned, there are probably times where comp3.id doesn't default to 0, which results in the test failure.

Regarding the comment about the == operator only comparing IDs: I believe the reason for this is because for use cases like the LogicalAudioSensorPlugin, the responsibility is on the plugin/user to ensure that no two microphones are created with the same ID. So, when comparing for equality in the component context, IDs should be enough since IDs are what determine uniqueness for a logical microphone..

Anyways, I think that it's still good to check the values of the volumeDetectionThreshold after de-serialization to make sure that those are still what we expect them to be. So, I went ahead and made a few changes in 481e413:

• set the IDs of the two test microphone components to something other than 0 so that we don't have the default-initialized value issues we've been seeing
• compare the components when checking equality operators, not the data structs that make up the component (I realized the equality checks were being done between mic1 and mic2 instead of comp1 and comp2)
• checking the volumeDetectionThreshold values after de-serialization to make sure they haven't changed

@scpeters, does this look good to you?

[mjcarroll]

@osrf-jenkins retest this please

[chapulina]

sneaky