Code coverage tests

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

Code coverage tests

Boost - Dev mailing list
A number of libraries have code coverage/project tests in Github. I am
not against such tests but I am against the idea that "failing" such a
test, whatever that means, should prevent a PR from being merged into a
Boost library. To me it is nonsense that a perfectly logical change to
software should lead to some code coverage "failure", and that therefore
the change should be deemed incorrect. I admit I do not know what the
criteria are for code coverage but I do not see code coverage
determining whether any change to a library is incorrect or not.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Code coverage tests

Boost - Dev mailing list
On 2020-04-13 at 17:52, Edward Diener via Boost wrote:
> A number of libraries have code coverage/project tests in Github. I am
> not against such tests but I am against the idea that "failing" such a
> test, whatever that means, should prevent a PR from being merged into a
> Boost library. To me it is nonsense that a perfectly logical change to
> software should lead to some code coverage "failure", and that therefore
> the change should be deemed incorrect. I admit I do not know what the
> criteria are for code coverage but I do not see code coverage
> determining whether any change to a library is incorrect or not.
>

Well, if some code change makes some other part of the code base unused,
perhaps the change is *incomplete* rather than incorrect?


     Bo Persson



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Code coverage tests

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Mon, Apr 13, 2020 at 8:53 AM Edward Diener via Boost
<[hidden email]> wrote:
> To me it is nonsense that a perfectly logical change to
> software should lead to some code coverage "failure"

Can you please provide specifics, perhaps with hyperlinks?

Thanks

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Code coverage tests

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 4/13/2020 11:58 AM, Bo Persson via Boost wrote:

> On 2020-04-13 at 17:52, Edward Diener via Boost wrote:
>> A number of libraries have code coverage/project tests in Github. I am
>> not against such tests but I am against the idea that "failing" such a
>> test, whatever that means, should prevent a PR from being merged into
>> a Boost library. To me it is nonsense that a perfectly logical change
>> to software should lead to some code coverage "failure", and that
>> therefore the change should be deemed incorrect. I admit I do not know
>> what the criteria are for code coverage but I do not see code coverage
>> determining whether any change to a library is incorrect or not.
>>
>
> Well, if some code change makes some other part of the code base unused,
> perhaps the change is *incomplete* rather than incorrect?

Maybe some part of the code base is being unused because it is no longer
relevant to all the compilers it was originally meant to serve. The case
where outdated compilers become less based on some more precise criteria
produces less code coverage because one or more modern versions of a
compiler implementation no longer need to be applied to some code. The
idea that this is some sort of failure because a modern version of
compiler XXX no longer needs some code workaround is just ridiculous.
Yet that appears to be what the code coverage test uses in considering
failure or not.

Some code:

xxx..
YYY..

Some compiler AAA workaround code:

zzz...

Now the compiler AAA workaround is reduced to compiler AAA , earlier
version NNN, where later versions of compiler AAA no longer need the
workaround code. Code coverage now produces a "failure" because it deems
this removal of compiler AAA, later version, from this workaround code,
as less code coverage.

Maybe code coverage is not all it is cracked up to be <g>. Sorry, but I
distrust something that is so unsubtle.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Code coverage tests

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 4/13/2020 12:24 PM, Vinnie Falco via Boost wrote:
> On Mon, Apr 13, 2020 at 8:53 AM Edward Diener via Boost
> <[hidden email]> wrote:
>> To me it is nonsense that a perfectly logical change to
>> software should lead to some code coverage "failure"
>
> Can you please provide specifics, perhaps with hyperlinks?

https://github.com/boostorg/property_map/pull/13


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Code coverage tests

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 2020-04-13 18:52, Edward Diener via Boost wrote:
> A number of libraries have code coverage/project tests in Github. I am
> not against such tests but I am against the idea that "failing" such a
> test, whatever that means, should prevent a PR from being merged into a
> Boost library. To me it is nonsense that a perfectly logical change to
> software should lead to some code coverage "failure", and that therefore
> the change should be deemed incorrect. I admit I do not know what the
> criteria are for code coverage but I do not see code coverage
> determining whether any change to a library is incorrect or not.

I agree. I consider code coverage and similar tools informative at best.
Therefore they should not block a commit or a PR merge.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Code coverage tests

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Mon, Apr 13, 2020 at 9:44 AM Edward Diener via Boost
<[hidden email]> wrote:
> Maybe code coverage is not all it is cracked up to be <g>. Sorry, but I
> distrust something that is so unsubtle.

Code coverage is simply tool, and like all good things requires effort
and attention to detail to use correctly. Depending on how the code is
structured, yes the tool will claim a lack of coverage if the
particular toolchain used to run the tests does not exercise a code
path used for a separate work-around. There are of course ways to
mitigate this. You can compile using both toolchains, and run the
resulting tests, and accumulate the coverage reports into one merged
report which does include the workaround code.

Another solution is to express the workaround code using function or
class templates. These don't count as uncovered lines unless they are
instantiated. Yes, I realize that this could be considered a "trick"
(i.e. sleight of hand) but as I said, the code coverage is a tool that
requires diligence. One cannot expect to just throw a code base at a
coverage tool and get perfect results. You have to pick through it and
identify things such as the case you described, and make small
adjustments and fixes. Ultimately you need to decide if and how the
coverage tool can enhance your project, and adopt a workflow that
makes the reports useful.

This is the approach that I have taken with Boost.Beast,
Not-Yet-In-Boost.JSON, and Not-Yet-In-Boost.URL. I have adopted a
style of code that helps the code coverage report make more sense. For
example by arranging conditionals to make it more clear when branches
are taken versus not. And I have tweaked the coverage command line
settings to eliminate false negatives. The coverage reports have been
incredibly successful in helping me identify which parts of code need
better test coverage, giving me a high degree of confidence that the
code in question is without defects. Boost.JSON for example has almost
99% coverage (I had a big refactor recently which caused a few new
uncovered lines, otherwise it would be over 99%):

<https://codecov.io/gh/vinniefalco/json/list/develop/>

Thanks

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Code coverage tests

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Mon, Apr 13, 2020 at 9:52 AM Edward Diener via Boost
<[hidden email]> wrote:
> https://github.com/boostorg/property_map/pull/13

Okay, I had a look at the coverage reports. They look quite sad, and
unloved! This is the canonical example of what you get when you just
throw a bunch of source code at a coverage reporting tool, without
accounting for the idiosyncrasies of the reporting tool and the
library in question. Right now the library is sitting at 53% but with
a day of work it could be pushed up to at least 90%, and I bet there
are some missing tests. For example, this line looks uncovered:

<https://codecov.io/gh/boostorg/property_map/src/develop/include/boost/property_map/dynamic_property_map.hpp#L312>

All this needs is one call to BOOST_TEST_EXCEPTION to make sure that
the proper exception is thrown when the dynamic property is not found.

Regards

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Code coverage tests

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Mon, Apr 13, 2020 at 9:53 AM Andrey Semashev via Boost
<[hidden email]> wrote:
> I agree. I consider code coverage and similar tools informative at best.
> Therefore they should not block a commit or a PR merge.

Well, I think that depends on the situation. If a contributor submits
a pull request to add some new public member functions for example,
and they don't write the tests then the coverage will go down. Or if
they do write the tests but they are incomplete, the coverage can also
go down. I think it is entirely reasonable in this case, rather than
merge the pull request simply to ask "why did coverage go down?"

If a project has set up the coverage reports correctly, and adopted a
coding style that enhances the precision of the reports, then soft
failures of reduced coverage on pull requests become much more
meaningful.

I agree that for this specific pull request
(https://github.com/boostorg/property_map/pull/13) that coverage
decrease should not hold up anything, because this project is not set
up for success with coverage reports in the first place.

Regards

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Code coverage tests

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Mon, Apr 13, 2020 at 12:44 PM Edward Diener via Boost
<[hidden email]> wrote:
>
> A number of libraries have code coverage/project tests in Github. I am
> not against such tests but I am against the idea that "failing" such a
> test, whatever that means, should prevent a PR from being merged into
> a Boost library.

Edward, are you referring to Boost libraries where a maintainer has
specifically chosen to have Code Coverage part of the CI? Or are there
libraries where the maintainer has not made this decision but their
libraries CI automatically involve coverage because they use some
shared CI configuration (boost-ci)?

Glen

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Code coverage tests

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Edward Diener wrote:

> https://github.com/boostorg/property_map/pull/13

How is it possible for this change to affect coverage?

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Code coverage tests

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 2020-04-13 at 18:54, Vinnie Falco via Boost wrote:

> On Mon, Apr 13, 2020 at 9:44 AM Edward Diener via Boost
> <[hidden email]> wrote:
>> Maybe code coverage is not all it is cracked up to be <g>. Sorry, but I
>> distrust something that is so unsubtle.
>
> Code coverage is simply tool, and like all good things requires effort
> and attention to detail to use correctly. Depending on how the code is
> structured, yes the tool will claim a lack of coverage if the
> particular toolchain used to run the tests does not exercise a code
> path used for a separate work-around. There are of course ways to
> mitigate this. You can compile using both toolchains, and run the
> resulting tests, and accumulate the coverage reports into one merged
> report which does include the workaround code.
>
> Another solution is to express the workaround code using function or
> class templates. These don't count as uncovered lines unless they are
> instantiated. Yes, I realize that this could be considered a "trick"
> (i.e. sleight of hand)

A problem here is that code coverage could be used either to see if the
tests are complete, or to see if some parts of the code is never used at
all.

If some new code (like Edward's PR) makes some other code redundant, it
is good if that shows up in the coverage report. Hiding dead code in
templates that are now never instantiated is not that good. :-)


    Bo Persson



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Code coverage tests

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 4/13/2020 1:17 PM, Glen Fernandes via Boost wrote:

> On Mon, Apr 13, 2020 at 12:44 PM Edward Diener via Boost
> <[hidden email]> wrote:
>>
>> A number of libraries have code coverage/project tests in Github. I am
>> not against such tests but I am against the idea that "failing" such a
>> test, whatever that means, should prevent a PR from being merged into
>> a Boost library.
>
> Edward, are you referring to Boost libraries where a maintainer has
> specifically chosen to have Code Coverage part of the CI? Or are there
> libraries where the maintainer has not made this decision but their
> libraries CI automatically involve coverage because they use some
> shared CI configuration (boost-ci)?

I am just creating a valid PR for a Boost library. How that library uses
code coverage is not my problem. I just do not want the validity of a PR
being affected by how code coverage works for a library. I do realize
that Github still allows a maintainer to merge a PR even when some CI
testing fails. I just do not want code coverage, rather than correctness
of code, determining whether a PR is merged or not. I am certainly not
going to change a correct PR based on a code coverage report, unless
that report shows me somehow that my change is logically incorrect, but
I doubt that code coverage can do that.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Code coverage tests

Boost - Dev mailing list
I'd add that if someone contributes a high quality PR, it's better for the
community if the library maintainers accept it rather than reject it based
only on gaps in a coverage report.

On Mon, Apr 13, 2020 at 11:21 AM Edward Diener via Boost <
[hidden email]> wrote:

>
> On 4/13/2020 1:17 PM, Glen Fernandes via Boost wrote:
> > On Mon, Apr 13, 2020 at 12:44 PM Edward Diener via Boost
> > <[hidden email]> wrote:
> >>
> >> A number of libraries have code coverage/project tests in Github. I am
> >> not against such tests but I am against the idea that "failing" such a
> >> test, whatever that means, should prevent a PR from being merged into
> >> a Boost library.
> >
> > Edward, are you referring to Boost libraries where a maintainer has
> > specifically chosen to have Code Coverage part of the CI? Or are there
> > libraries where the maintainer has not made this decision but their
> > libraries CI automatically involve coverage because they use some
> > shared CI configuration (boost-ci)?
>
> I am just creating a valid PR for a Boost library. How that library uses
> code coverage is not my problem. I just do not want the validity of a PR
> being affected by how code coverage works for a library. I do realize
> that Github still allows a maintainer to merge a PR even when some CI
> testing fails. I just do not want code coverage, rather than correctness
> of code, determining whether a PR is merged or not. I am certainly not
> going to change a correct PR based on a code coverage report, unless
> that report shows me somehow that my change is logically incorrect, but
> I doubt that code coverage can do that.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Code coverage tests

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 13.04.20 17:52, Edward Diener via Boost wrote:
> A number of libraries have code coverage/project tests in Github. I am
> not against such tests but I am against the idea that "failing" such a
> test, whatever that means, should prevent a PR from being merged into a
> Boost library. To me it is nonsense that a perfectly logical change to
> software should lead to some code coverage "failure", and that therefore
> the change should be deemed incorrect. I admit I do not know what the
> criteria are for code coverage but I do not see code coverage
> determining whether any change to a library is incorrect or not.

Even if the code coverage is set as a "required check" to have in order
to enable the merge, an admin of that repo can always bypass those (even
if the branch is protected).

A problem to me is that most of the repos that have code coverage today
did not really participate in having it: coverage tests were given
during the effort to have Travis based CI. How coverage is calculated is
unknown to me (and in my case why Boost.Test numbers are so bad).

Raffi


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Code coverage tests

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
In article <r72aj3$2kka$[hidden email]>,
    Edward Diener via Boost <[hidden email]> writes:

> I am just creating a valid PR for a Boost library. How that library uses
> code coverage is not my problem. I just do not want the validity of a PR
> being affected by how code coverage works for a library.

Am I missing something?

I don't see anything on the pull request that is rejecting your PR.

Did someone tell you outside of github that your PR was being rejected
for code coverage reasons?
--
"The Direct3D Graphics Pipeline" free book <http://tinyurl.com/d3d-pipeline>
            The Terminals Wiki <http://terminals-wiki.org>
     The Computer Graphics Museum <http://ComputerGraphicsMuseum.org>
  Legalize Adulthood! (my blog) <http://LegalizeAdulthood.wordpress.com>

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Code coverage tests

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Mon, Apr 13, 2020 at 11:38 AM Raffi Enficiaud via Boost
<[hidden email]> wrote:
> How coverage is calculated is unknown to me
> (and in my case why Boost.Test numbers are so bad).

Part of the reason why numbers are bad is because "branch coverage" is
enabled by default. I believe it should be disabled by default, for
the reasons I described here:

<https://github.com/boostorg/boost-ci/pull/38>

Regards

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Code coverage tests

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Mon, Apr 13, 2020 at 2:21 PM Edward Diener wrote:

> On 4/13/2020 1:17 PM, Glen Fernandes via Boost wrote:
> > Edward, are you referring to Boost libraries where a maintainer has
> > specifically chosen to have Code Coverage part of the CI? Or are there
> > libraries where the maintainer has not made this decision but their
> > libraries CI automatically involve coverage because they use some
> > shared CI configuration (boost-ci)?
>
> I am just creating a valid PR for a Boost library. How that library uses
> code coverage is not my problem. I just do not want the validity of a PR
> being affected by how code coverage works for a library. I do realize
> that Github still allows a maintainer to merge a PR even when some CI
> testing fails. I just do not want code coverage, rather than correctness
> of code, determining whether a PR is merged or not.

Understood. I don't believe you're going to find yourself in this
situation. i.e. I cannot imagine any maintainer is going to reject
your pull request just because a red light on a CI does not turn green
due to code coverage.

> I am certainly not
> going to change a correct PR based on a code coverage report

Absolutely.

Glen

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Code coverage tests

Boost - Dev mailing list
On Mon, Apr 13, 2020 at 2:21 PM Edward Diener wrote:
>
> I am just creating a valid PR for a Boost library. How that library uses
> > code coverage is not my problem.
>

Well, it might be -- if you've added a new method, for example, without a
test the coverage will go down -- that's what the CI is looking at.  I'm
sure there's cases where a good request will make it go down, but there are
also cases where it's flagging a valid issue with the PR.

On Mon, Apr 13, 2020 at 4:14 PM Glen Fernandes via Boost <
[hidden email]> wrote:

> Understood. I don't believe you're going to find yourself in this
> situation. i.e. I cannot imagine any maintainer is going to reject
> your pull request just because a red light on a CI does not turn green
> due to code coverage.
>

And also true -- think most of these aren't 'required' -- so it's really
informational an nothing more.

Jeff

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Code coverage tests

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list

> On 13. Apr 2020, at 18:54, Vinnie Falco via Boost <[hidden email]> wrote:
>
> I have adopted a
> style of code that helps the code coverage report make more sense. For
> example by arranging conditionals to make it more clear when branches
> are taken versus not. And I have tweaked the coverage command line
> settings to eliminate false negatives. The coverage reports have been
> incredibly successful in helping me identify which parts of code need
> better test coverage [...]

I had 99 % line coverage for a while in Boost.Histogram, eventually I got it to 100 %. The Pareto principle applies, getting 80 % requires 20 % of the work, while the remaining 20 % required 80 % of the work. To get the last 1 %, I had to write a mocked allocator that throws std::bad_alloc in a controlled way, and finally I also had to add some LCOV_EXCL_LINE, LCOV_EXCL_START, LCOV_EXCL_STOP, because gcc-8 reports some lines as missed which cannot possibly be missed and which gcc-5 did report correctly.

My points:

1) I agree with Vinnie that achieving line coverage close to 100 % is a worthy goal, and yes that requires also to understand how the coverage reporting works. I found many bugs in my code this way, including some subtle ones. I cannot guarantee that the code is bug-free, but I sleep rather well at night. Maybe we should write a guidelines about this to share what we learned.

2) Getting branch coverage to 100 % is nearly impossible with reasonable effort (I consider the effort I put into getting 100 % line coverage already barely reasonable), and that's why I am in the camp with Vinnie that it is not worth it. Consider this code

if (cond1 && cond2 && cond3)
   // do X
else
   // do Y

Branch coverage will insist that you check all(*) combinations of true/false for cond1, cond2, and cond3 for 100 %, while in most cases, it really only matters that both "do X" and "do Y" are executed in tests and work correctly.

(*) It may detect the short-circuiting of the "&&", I am not sure. So maybe you don't have to test all combinations, but at least four (TTT, FXX, TFX, TTF).

3) Achieving nearly full coverage is much easier for libraries that do not have to support workarounds for old compilers. It is considerably easier to achieve for the C++11 and C++14 libraries than for the older ones. Perhaps older Boost libraries should not attempt this.

Best regards,
Hans

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost