codecov / feedback

A place to discuss feedback about the pull request and web product experience.
38 stars 9 forks source link

Misreported branch coverage of C++ projects from gcov #334

Open Bronek opened 7 months ago

Bronek commented 7 months ago

Describe the bug

Branch coverage generated from .gcov files is under-reported for C++ projects with generic (template) classes/functions. When unit tests coverage data are being processed by the gcov tool, the results are grouped per instantiation of generic function. This results in the same block of lines being reported in .gcov file multiple times, each time for a different instantiation of a function, for example (from https://github.com/Bronek/market/blob/main/libs/market/book.hpp , MIT license)

------------------
        -:  238:
        -:  239:        template <side Side, typename ... Args>
      158:  240:        size_type emplace_back(Args&& ... a) {
     158*:  241:            ASSERT(freel != nullptr);
     156*:  242:            ASSERT(side_i[0] + side_i[1] + (size_type)(tail_i + 1) == size_i);
      150:  243:            size_type result = npos;
      150:  244:            auto& size = side_i[(size_t)Side];
      150:  245:            if (size < capacity) {
     115*:  246:                ASSERT(tail_i != npos);
        -:  247:                // Note: must post-decrement tail_i here. Will change to npos if it was 0
      113:  248:                const auto l = freel[tail_i--];
      113:  249:                common::emplace(&levels[l], std::forward<Args>(a) ...);
      113:  250:                sides[(size_t)Side * capacity + size] = l;
      113:  251:                result = size++; // Note: must post-increment side_i[Side] here
        -:  252:            }
      148:  253:            return result;
        -:  254:        }
------------------
_ZN6market4bookIN12_GLOBAL__N_110ConstLevelES2_E12emplace_backILNS_4sideE0EJiEEEhDpOT0_:
function _ZN6market4bookIN12_GLOBAL__N_110ConstLevelES2_E12emplace_backILNS_4sideE0EJiEEEhDpOT0_ called 2 returned 100% blocks executed 60%
        2:  240:        size_type emplace_back(Args&& ... a) {
       2*:  241:            ASSERT(freel != nullptr);
branch  0 taken 0 (fallthrough)
branch  1 taken 2
call    2 never executed
call    3 never executed
       2*:  242:            ASSERT(side_i[0] + side_i[1] + (size_type)(tail_i + 1) == size_i);
branch  0 taken 0 (fallthrough)
branch  1 taken 2
call    2 never executed
call    3 never executed
        2:  243:            size_type result = npos;
        2:  244:            auto& size = side_i[(size_t)Side];
        2:  245:            if (size < capacity) {
branch  0 taken 2 (fallthrough)
branch  1 taken 0
       2*:  246:                ASSERT(tail_i != npos);
branch  0 taken 0 (fallthrough)
branch  1 taken 2
call    2 never executed
call    3 never executed
        -:  247:                // Note: must post-decrement tail_i here. Will change to npos if it was 0
        2:  248:                const auto l = freel[tail_i--];
        2:  249:                common::emplace(&levels[l], std::forward<Args>(a) ...);
call    0 returned 2
call    1 returned 2
        2:  250:                sides[(size_t)Side * capacity + size] = l;
        2:  251:                result = size++; // Note: must post-increment side_i[Side] here
        -:  252:            }
        2:  253:            return result;
        -:  254:        }
------------------
_ZN6market4bookIN12_GLOBAL__N_110ConstLevelES2_E12emplace_backILNS_4sideE1EJiEEEhDpOT0_:
function _ZN6market4bookIN12_GLOBAL__N_110ConstLevelES2_E12emplace_backILNS_4sideE1EJiEEEhDpOT0_ called 14 returned 100% blocks executed 60%
       14:  240:        size_type emplace_back(Args&& ... a) {
      14*:  241:            ASSERT(freel != nullptr);
branch  0 taken 0 (fallthrough)
branch  1 taken 14
call    2 never executed
call    3 never executed
      14*:  242:            ASSERT(side_i[0] + side_i[1] + (size_type)(tail_i + 1) == size_i);
branch  0 taken 0 (fallthrough)
branch  1 taken 14
call    2 never executed
call    3 never executed
       14:  243:            size_type result = npos;
       14:  244:            auto& size = side_i[(size_t)Side];
       14:  245:            if (size < capacity) {
branch  0 taken 11 (fallthrough)
branch  1 taken 3
      11*:  246:                ASSERT(tail_i != npos);
branch  0 taken 0 (fallthrough)
branch  1 taken 11
call    2 never executed
call    3 never executed
        -:  247:                // Note: must post-decrement tail_i here. Will change to npos if it was 0
       11:  248:                const auto l = freel[tail_i--];
       11:  249:                common::emplace(&levels[l], std::forward<Args>(a) ...);
call    0 returned 11
call    1 returned 11
       11:  250:                sides[(size_t)Side * capacity + size] = l;
       11:  251:                result = size++; // Note: must post-increment side_i[Side] here
        -:  252:            }
       14:  253:            return result;
        -:  254:        }
------------------
_ZN6market4bookIN12_GLOBAL__N_15LevelES2_E12emplace_backILNS_4sideE1EJiiEEEhDpOT0_:
function _ZN6market4bookIN12_GLOBAL__N_15LevelES2_E12emplace_backILNS_4sideE1EJiiEEEhDpOT0_ called 65 returned 94% blocks executed 88%
       65:  240:        size_type emplace_back(Args&& ... a) {
       65:  241:            ASSERT(freel != nullptr);
branch  0 taken 1 (fallthrough)
branch  1 taken 64
call    2 returned 1
call    3 returned 0
       64:  242:            ASSERT(side_i[0] + side_i[1] + (size_type)(tail_i + 1) == size_i);
branch  0 taken 3 (fallthrough)
branch  1 taken 61
call    2 returned 3
call    3 returned 0
       61:  243:            size_type result = npos;
       61:  244:            auto& size = side_i[(size_t)Side];
       61:  245:            if (size < capacity) {
branch  0 taken 53 (fallthrough)
branch  1 taken 8
      53*:  246:                ASSERT(tail_i != npos);
branch  0 taken 0 (fallthrough)
branch  1 taken 53
call    2 never executed
call    3 never executed
        -:  247:                // Note: must post-decrement tail_i here. Will change to npos if it was 0
       53:  248:                const auto l = freel[tail_i--];
       53:  249:                common::emplace(&levels[l], std::forward<Args>(a) ...);
call    0 returned 53
call    1 returned 53
call    2 returned 53
       53:  250:                sides[(size_t)Side * capacity + size] = l;
       53:  251:                result = size++; // Note: must post-increment side_i[Side] here
        -:  252:            }
       61:  253:            return result;
        -:  254:        }
------------------

It appears that the gcov reporter https://github.com/codecov/worker/blob/main/services/report/languages/gcov.py is unable to handle such files correctly and only gathers branch coverage from the first instantiation of such generic functions, resulting in significant under-reporting of branch coverage inside the function.

Environment (please complete the following information):

To Reproduce

Steps to reproduce the behaviour:

  1. Build https://github.com/Bronek/market project with coverage enabled, e.g.
    mkdir .build
    cd .build
    export CC=$(which gcc); export CXX=$(which g++)
    cmake -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_FLAGS="-g -coverage  -fprofile-abs-path" -DCMAKE_CXX_FLAGS="-g -coverage  -fprofile-abs-path" ..
    cmake --build .
  2. Verify .gcno files are generated: find tests/ -type f -name '*.gcno'
  3. Run unit tests: tests/tests
  4. Verify .gcda files are generated: find tests/ -type f -name '*.gcda'
  5. Generate .gcov files: gcov -pbc $( find tests/ -type f -name '*.gcno' )
  6. Submit the generated .gcov files to codecov

Expected behavior

Expected unit test coverage is 100%

Screenshots

Reported unit test coverage is 98.35%. Example line 245 in function emplace_back is missing a branch

Screenshot 2024-04-16 at 14 14 57

Both branches of this line are exercised in unit tests, e.g. https://github.com/Bronek/market/blob/main/tests/book.cpp#L408

Additional context

Note the example project provided here is very small (basically just one file book.hpp). Assuming you have a recent C++ compiler (e.g. gcc-12 in Debian 12 "bookworm") you should be able to build and run tests of this project very quickly. This is also why the project has 100% unit test coverage (confirmed with gcovr tool). In this case the misreporting is tiny 1.65%. However for a non-trivial project the misreporting can have much larger impact.

For example by enabling branch reporting in .codecov.yml in https://github.com/libfn/functional/pull/63 I have found a drop by some 12 percent point. I do not know how much of this is actually missing coverage, and how much is the result of the bug being reported, but reading trough the .gcov files of this project I can see many cases where all the branches have been executed, and yet codecov is showing incomplete branch coverage. I have found similar impact of this bug in other non-trivial projects.

The additional annoyance is the fact that codecov/codecov-action runs gcov plugin by default (unless explicitly disabled, e.g. with plugin: noop), automatically generating .gcov files for submission, so it is sensible to assume that codecov would be able to process these files correctly.