search

codecov / feedback

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

Partial coverage is broken in a C project #513

Closed bqqbarbhg closed 2 months ago

bqqbarbhg commented 2 months ago

Describe the bug

I've been using Codecov to test ufbx for years and it has been quite stable so far.

A few days ago my partial coverage fully disappeared in all new uploads, see this example PR, which does not change any code related to coverage.

@@               Coverage Diff               @@
##           integration     #156      +/-   ##
===============================================
+ Coverage        95.41%   99.14%   +3.73%     
===============================================
  Files                2        2              
  Lines            17081    17081              
  Branches          5339     5339              
===============================================
+ Hits             16297    16935     +638     
  Misses             146      146              
+ Partials           638        0     -638

Environment (please complete the following information):

Partial coverage is missing both in GitHub comments and in the Codecov website.

The coverage is uploaded from GitHub actions via https://codecov.io/bash, as the upload action is broken in self-hosted runners for some reason. There is an extra -y flag passed to the script, but it does detect the correct codecov.yml anyways. I have fixed that in another PR, where the issue reproduces as well (but in this one there are a lot of content changes as well).

To Reproduce

Expected behavior

Partially covered lines are retained in uploaded coverage, as they have been a few days ago.

bqqbarbhg commented 2 months ago

I did some further digging into this issue in case I could have worked around it:

I compared two uploads (master, commit, works correctly) and (codecov-issue, commit, no partial coverage):

Looking at line 1047 in the web viewer:

master Image

codecov-issue Image

Relevant parts of the uploaded files:

master

...
SF:/home/runner/work/ufbx/ufbx/ufbx.c
.. FN/FNDA lines
.. DA/BRDA lines
DA:1047,731837
BRDA:1047,0,0,0
BRDA:1047,0,1,1
...

codecov-issue

...
SF:/home/runner/work/ufbx/ufbx/ufbx.c
.. FN/FNDA lines
.. DA/BRDA lines
DA:1047,731835
BRDA:1047,0,0,0
BRDA:1047,0,1,1
...

As you can see, both uploads have practically the same content, and specifically for line 1047 both report branch 0 as not taken.

drazisil-codecov commented 2 months ago

Hi @bqqbarbhg

Can you check if https://github.com/codecov/feedback/issues/482#issuecomment-2340914676 fixes this?

bqqbarbhg commented 2 months ago

Heya, thanks for the reply!

I'm using codecov-cli instead of the action (in the PR that does not use the old bash uploader, as I assume that is out of date). I passed the equivalent argument to the CLI as --plugin noop, which seemed get parsed properly (Upload Coverage step here):

debug - 2024-09-23 12:51:56,992 -- Selected preparation plugins --- {"selected_plugins": ["<class 'codecov_cli.plugins.NoopPlugin'>"]}
debug - 2024-09-23 12:51:56,992 -- Running preparation plugin: <class 'codecov_cli.plugins.NoopPlugin'>

Unfortunately, the issue seems to persist in the uploaded coverage, as the reported files still have 0 partially covered lines: https://app.codecov.io/github/ufbx/ufbx/commit/b0f3a3e6b69b3ca28eb1de213e5f1d6164d2a99a/tree

The lines in the uploads won't match up to the previous comment, as the PR which uses codecov-cli has other changes included as well. However, the example line above still remains fully covered, even though the uploaded results show the same pattern as before (in the ci_coverage-10994634225 upload):

SF:/home/runner/work/ufbx/ufbx/ufbx.c
...
DA:1247,74467544
BRDA:1247,0,0,0
BRDA:1247,0,1,1

Image

bqqbarbhg commented 2 months ago

Oh also, not sure if this helps at all, but one thing I noticed is that in the PR comment, from the PR with other changes:

@@               Coverage Diff               @@
##           integration     #154      +/-   ##
===============================================
+ Coverage        95.41%   99.06%   +3.65%     
===============================================
  Files                2        2              
  Lines            17081    17309     +228     
  Branches          5339     5398      +59     
===============================================
+ Hits             16297    17148     +851     
- Misses             146      161      +15     
+ Partials           638        0     -638

It seems to have a reasonable increase in branch count given the code changes:

  Branches          5339     5398      +59

This would indicate that the branch information from the upload is indeed parsed correctly at some step, but the partial coverage is lost somewhere along the way.

drazisil-codecov commented 2 months ago

It's been a while since I dove into the lcov spec, but this looks very weird to me:

https://github.com/codecov/worker/blob/a7cb347f36b0fc45cfe29ac1390a7b58020f4dcd/services/report/languages/lcov.py#L135

DA:1247,50709271
BRDA:1247,0,0,0
BRDA:1247,0,1,1
DA:1248,0

I read that as, There's a branch at line 1247. The first branch was taken 0 times, and the second branch was taken 1 time. That's fine.

But as line coverage goes, line 1247 was hit 50709271 times. Like 1248 was hit 0 times.

I feel like the branch data isn't coming anywhere close to the line data. How would you interpret this?

bqqbarbhg commented 2 months ago

Yeah I noticed that as well when reading the uploaded lcov file. I feel like it might be a limitation of the coverage tool, as all of the BRDA lines only indicate binary taken/not taken. Maybe it's storing branch information as a bitmask or something else that can only store binary information per branch?

However, in the earlier comment, the old uploads that worked had identical behavior with the BRDA lines, but still had functional partial coverage, so to me it feels like something that should be supported.

bqqbarbhg commented 2 months ago

Also the, .gcov files have the hits in relative counts:

   731835: 1038:    char *ca = (char*)a, *cb = (char*)b;
        -: 1039:#if defined(UFBXI_HAS_UNALIGNED)
  1486051: 1040:    ufbxi_nounroll while (size >= 4) {
branch  0 taken 51%
branch  1 taken 49%
   754216: 1041:        uint32_t t = *(ufbxi_unaligned ufbxi_unaligned_u32*)ca;
   754216: 1042:        *(ufbxi_unaligned ufbxi_unaligned_u32*)ca = *(ufbxi_unaligned ufbxi_unaligned_u32*)cb;
   754216: 1043:        *(ufbxi_unaligned ufbxi_unaligned_u32*)cb = t;
   754216: 1044:        ca += 4; cb += 4; size -= 4;
        -: 1045:    }
        -: 1046:#endif
   731835: 1047:    ufbxi_nounroll while (size > 0) {
branch  0 taken 0%
branch  1 taken 100%
    #####: 1048:        char t = *ca; *ca = *cb; *cb = t;
    #####: 1049:        ca++; cb++; size--;
        -: 1050:    }
   731835: 1051:}

I tried to read the lcov source for handling those and that seems to strangely parse the percentages as counts. I'm not really proficient in Perl nor really dived to the codebase, but I think it's plausible it somehow flattens the branch counts to just 1/0 somewhere.

drazisil-codecov commented 2 months ago

@Swatinem Did https://github.com/codecov/worker/pull/726 possibly break lcov branch coverage?

We went from 638 partials in https://app.codecov.io/gh/ufbx/ufbx/commit/d08df1381472b8d29d22df351a01d6cea5a73d50/tree to 0 partials in https://app.codecov.io/gh/ufbx/ufbx/commit/60d9467635623dea3dfbe15ffbf1f4b7418b56ab/tree

bqqbarbhg commented 2 months ago

If it helps, here are the two commits and timestamps in between which the problem started:

Swatinem commented 2 months ago

Thanks for the report @bqqbarbhg.

This is indeed a recent regression, related to merging line records and branch records. A fix for this should land shortly in https://github.com/codecov/worker/pull/735.

bqqbarbhg commented 2 months ago

Looks like the fix worked on my end too: https://app.codecov.io/github/ufbx/ufbx/commit/14702ef36350708ab1b7b9822f418b5d8ab31105/tree

Never been happier to see -3.73% coverage on a commit, thanks a lot!