Codecov seems to report false positives

[elliottbiondo]

During recent MR reviews I observed two Codecov issues related to macros:

CELER_DISCARD and CELER_NOT IMPLEMENTED: SolidConverter.cc lines 815 and 816 (from #2032)

EXPECT_EQ: IntersectRegion.test.cc line 2527 (from #2032)

[pcanal]

As seen/hinted here: https://app.codecov.io/gh/celeritas-project/celeritas/pull/2043?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celeritas-project

It seems that the issue might be triggered by Real3{0, 0, 0} as the lines is counted correct for Real3{0.1, 0.1, 0.1}

[sethrj]

Weird, so maybe it's some sort of compiler optimization? 😕

[elliottbiondo]

Maybe whatever CI job runs Codecov could be down without compiler optimizations. That would eliminate having to decorate the code with pragmas for loops that may be unrolled, for example.

[sethrj]

Here's another false positive in #2048: https://app.codecov.io/gh/celeritas-project/celeritas/pull/2048

I know every loop here and conditional is being tested in PolySolid.test.cc. And what's up with the "missing" line of coverage?

@pcanal Is there a tolerance parameter that lets us assume codecov will have a few false positives per PR so that it doesn't get a nasty "❌" marker?

[pcanal]

Is there a tolerance parameter that lets us assume codecov will have a few false positives per PR so that it doesn't get a nasty "❌" marker?

Hide it from 'which' part of the interface? On specific lines, I suppose the comment is the right direction. On the codecov web site, a percentage wouldn't help since it has the marker next to each line ...

[sethrj]

We definitely do not have the time and overhead to wait for code coverage to give us back a bunch of false positive and then add ugly markup to the code to hide them… that could be reasonable if it's one in every thousand lines, but it seems to be one in every 50 😢

[pcanal]

Explanation of: EXPECT_EQ: IntersectRegion.test.cc line 2527 (from #2032)

The report is 'technically' sort of correct (albeit not that relevant). We have some use of EXPEC_EQ on one line:

    EXPECT_EQ(SignedSense::inside, this->calc_sense(result.node_id, Real3{0, 0, 0}));

and some time in multiple lines:

    EXPECT_EQ(SignedSense::inside,
              this->calc_sense(result.node_id, Real3{0, 0, 0.1}));

which epands to:

# 2538 "/home/pcanal/geant/sources/celeritas/test/orange/orangeinp/IntersectRegion.test.cc" 3 4
...
# 2538 "/home/pcanal/geant/sources/celeritas/test/orange/orangeinp/IntersectRegion.test.cc" 3 4
   ))) ; else ::testing::internal::AssertHelper(::testing::TestPartResult::kNonFatalFailure, "/home/pcanal/geant/sources/celeritas/test/orange/orangeinp/IntersectRegion.test.cc", 253
8, gtest_ar.failure_message()) = ::testing::Message()
                                                                 
# 2539 "/home/pcanal/geant/sources/celeritas/test/orange/orangeinp/IntersectRegion.test.cc"
                                                                ;

where the else clause is indeed not exercised ... and the macro being split across multi line make the result different and (possibly due to the expansion that shuffles things around) make the reporting 'murky'.

I am investigating ways to mitigate this.

[sethrj]

Aha, very sneaky! Good diagnosis.