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.
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
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.
On Mon, Apr 13, 2020 at 9:44 AM Edward Diener via Boost <boost@lists.boost.org> 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
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 <boost@lists.boost.org> 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
On 13. Apr 2020, at 18:54, Vinnie Falco via Boost <boost@lists.boost.org> 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
On Mon, Apr 13, 2020 at 12:44 PM Edward Diener via Boost <boost@lists.boost.org> 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
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 <boost@lists.boost.org> 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.
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 < boost@lists.boost.org> 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 <boost@lists.boost.org> 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.
In article <r72aj3$2kka$1@ciao.gmane.io>, Edward Diener via Boost <boost@lists.boost.org> 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>
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
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 < boost@lists.boost.org> 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
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 <boost@lists.boost.org> 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?
On Mon, Apr 13, 2020 at 9:52 AM Edward Diener via Boost <boost@lists.boost.org> wrote:
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
Edward Diener wrote:
How is it possible for this change to affect coverage?
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.
On Mon, Apr 13, 2020 at 9:53 AM Andrey Semashev via Boost <boost@lists.boost.org> 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
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
On Mon, Apr 13, 2020 at 11:38 AM Raffi Enficiaud via Boost <boost@lists.boost.org> 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
participants (11)
-
Andrey Semashev -
Bo Persson -
Edward Diener -
Emil Dotchevski -
Glen Fernandes -
Hans Dembinski -
Jeff Garland -
Peter Dimov -
Raffi Enficiaud -
Richard -
Vinnie Falco