On Mon, Apr 13, 2020 at 9:44 AM Edward Diener via Boost 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 13. Apr 2020, at 18:54, Vinnie Falco via Boost 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 4/13/2020 1:17 PM, Glen Fernandes via Boost wrote:

On Mon, Apr 13, 2020 at 12:44 PM 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.

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 , Edward Diener via Boost 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:

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 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

Edward Diener wrote:

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

How is it possible for this change to affect coverage?

On Mon, Apr 13, 2020 at 9:53 AM Andrey Semashev via Boost 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 Mon, Apr 13, 2020 at 11:38 AM Raffi Enficiaud via Boost 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