Project

General

Profile

Actions

action #125237

closed

os-autoinst codecov check "fully_covered" returns 99% but codecov reports look like 100% size:M

Added by okurz over 1 year ago. Updated over 1 year ago.

Status:
Resolved
Priority:
Low
Assignee:
Category:
Regressions/Crashes
Target version:
Start date:
2023-03-01
Due date:
% Done:

0%

Estimated time:

Description

Observation

So it looks like the problem first mentioned in https://github.com/os-autoinst/os-autoinst/pull/2260#issuecomment-1448414226 shows up in other pull requests as well now. Who can understand why codecov says that we have 99.28% coverage in the "fully_covered" section as configured in https://github.com/os-autoinst/os-autoinst/blob/master/codecov.yml#L18 even though https://app.codecov.io/gh/os-autoinst/os-autoinst/pull/2270/tree shows all referenced paths to be covered by 100%?

Steps to reproduce

Seems to happen in all pull requests that are either opened anew or updated

Acceptance criteria

  • AC1: It is known why the actual percentage doesn't match the expected 100%

Suggestions

Actions #2

Updated by okurz over 1 year ago

https://github.com/os-autoinst/os-autoinst/pull/2271 reached 100% with the following removed:

          - bmwqemu.pm
          - log.pm
          - mmapi.pm
          - osutils.pm
          - signalblocker.pm
          - t
          - xt

the other was still at 98.94%, updating with https://github.com/os-autoinst/os-autoinst/pull/2272#issuecomment-1450861262 to only exclude t/

Actions #3

Updated by okurz over 1 year ago

  • Status changed from In Progress to New
Actions #4

Updated by mkittler over 1 year ago

  • Subject changed from os-autoinst codecov check "fully_covered" returns 99% but codecov reports look like 100% to os-autoinst codecov check "fully_covered" returns 99% but codecov reports look like 100% size:M
  • Description updated (diff)
  • Status changed from New to Workable
Actions #5

Updated by tinita over 1 year ago

I think one reason could be the flaky test modules under t/:

t/data/tests/bar/baz.pm
t/data/tests/foo.pm

Sometimes the first is covered and the second not at all, dropping out of the coverage report completely; sometimes it's the other way round. Maybe this results in this 99.x percentage somehow, depending on codecov is calculating here.
I assume that if a file is not in coverage at all, it doesn't get considered for the complete coverage result.
e.g. we currently don't have t/data/tests/foo.pm covered in the current master, still it says 100% for that directory

Actions #6

Updated by okurz over 1 year ago

  • Status changed from Workable to In Progress

I could locally reproduce the problem regarding foo.pm with

while true; do rm -rf cover_db/ && OPENQA_TEST_TIMEOUT_DISABLE=1 tools/invoke-tests --coverage t/14-isotovideo.t && cover -report html_minimal  cover_db; done

showing that sometimes the file shows up and sometimes not regardless that it's always 100% if it's included.

Actions #7

Updated by openqa_review over 1 year ago

  • Due date set to 2023-03-21

Setting due date based on mean cycle time of SUSE QE Tools

Actions #10

Updated by okurz over 1 year ago

  • Status changed from In Progress to Feedback
Actions #11

Updated by okurz over 1 year ago

  • Due date deleted (2023-03-21)

PR merged, we are good

Actions #12

Updated by okurz over 1 year ago

  • Status changed from Feedback to Resolved
Actions #13

Updated by livdywan over 1 year ago

  • Status changed from Resolved to Feedback
  • Assignee changed from okurz to livdywan

https://github.com/os-autoinst/os-autoinst/pull/2282 would claim gaps in coverage despite my specifically adding and manually verifying that the code is covered.

I guess it's not working afterall? Maybe we need to deal with t/data/tests via PERL5OPT

Actions #15

Updated by livdywan over 1 year ago

  • Status changed from Feedback to Workable
  • Assignee deleted (livdywan)
  • Run codecov locally
  • Check the JSON on the individual test coverage
  • Fix?

So let's get a few instances fixed. We couldn't confirm how to ensure consistent results between develcov and codecov but we can evaluate that after that, and this is a necessary step anyway if we have gaps in coverage.

Actions #16

Updated by okurz over 1 year ago

  • Priority changed from Normal to High
Actions #17

Updated by tinita over 1 year ago

  • Status changed from Workable to In Progress
  • Assignee set to tinita
Actions #18

Updated by tinita over 1 year ago

I was able to reproduce locally:

  • Devel::Cover reports less than 100% coverage for 14-isotovideo.t
  • Still Devel::Cover reports every line as green
  • The lines which make the differences are the "uncoverable statement" lines
  • Devel::Cover::Report::Codecovbash reports those lines as uncovered

So I will check if something changed recently in Devel::Cover

Actions #19

Updated by openqa_review over 1 year ago

  • Due date set to 2023-04-15

Setting due date based on mean cycle time of SUSE QE Tools

Actions #20

Updated by tinita over 1 year ago

Actually the uncoverable lines are reported as undef, which means it doesn't count as code to cover, while 0 means not covered.

I just found something else:
All current pull requests have no coverage report because
https://github.com/os-autoinst/os-autoinst/pull/2289/checks?check_run_id=12439118095

Missing Base Report
Unable to compare commits because the base of the pull request did not upload a coverage report.

and that's because the last master run failed:
https://github.com/os-autoinst/os-autoinst/actions/runs/4481994082/jobs/7879448387

[2023-03-21T17:37:55.074Z] ['error'] There was an error running the uploader: Error uploading to https://codecov.io: Error: There was an error fetching the storage URL during POST: 500 - Unknown error, please try again later
95
[2023-03-21T17:37:55.075Z] ['verbose'] The error stack is: Error: Error uploading to https://codecov.io: Error: There was an error fetching the storage URL during POST: 500 - Unknown error, please try again later

So we have the codecov upload error on master, which should actually have been fixed by using the token.

I have restarted the test, codecov upload succeeded, and now I get a proper report for my test PR.

Actions #21

Updated by tinita over 1 year ago

https://github.com/os-autoinst/os-autoinst/pull/2292
Should fix two issues:

Actions #22

Updated by tinita over 1 year ago

I tried to investigate more on the issue that having t/ in fully_covered results in less than 100%, while having t/*.t, t/data, t/fake results in 100%.
To investigate more one would probably have to remove test by test and observe what happens.
Running just one test, e.g. t/24-myjsonrpc.t and putting t/ in fully_covered works just fine.

Todo: try to reproduce the clash of the modules with the same content in a simple reproducer and file a bug (or talk to Paul personally in 3 weeks at the toolchain summit ;-)

Actions #24

Updated by tinita over 1 year ago

https://github.com/os-autoinst/os-autoinst/pull/2294 ci: Add slash to directories in codecov.yml (merged)

Apparently if one only specifies t then it counts all files
starting with t including testapi.pm and tools/update-deps etc.

It would be really good if there was a page on the codecov report where
it says which files it considered for the several groups.

Actions #25

Updated by okurz over 1 year ago

tinita wrote:

https://github.com/os-autoinst/os-autoinst/pull/2294 ci: Add slash to directories in codecov.yml

Apparently if one only specifies t then it counts all files
starting with t including testapi.pm and tools/update-deps etc.

It would be really good if there was a page on the codecov report where
it says which files it considered for the several groups.

I wonder if that behavior changed on the side of codecov in the past month. But regardless great that you could find this!

Actions #26

Updated by tinita over 1 year ago

I created the issue for the modules with the same content: https://github.com/pjcj/Devel--Cover/issues/322
I think it's probable that this might not be easy to fix and we have a very special, uncommon case anyway that was easy to fix on our side.

Now reproducing the case where the coverage for files with "# uncoverable statement" is less than 100% even when all lines are green in the report.

Actions #27

Updated by tinita over 1 year ago

Ok, so I reduced the reproducer regarding uncoverable to a few lines and reported it here: https://github.com/pjcj/Devel--Cover/issues/323
Basically it looks like Devel::Cover counts lines as uncovered, if they are covered && marked as uncoverable.
To add to the confusion, the line is still marked as green in the HTML report, so there is no quick way to find out which is the actual line that is responsible for less than 100% coverage.

It could be that the behaviour was always like that, but we didn't notice, because it doesn't affect the codecov reports, we had to actively look into the test logs.

Actions #28

Updated by tinita over 1 year ago

  • Status changed from In Progress to Feedback

I had a look into the code: https://github.com/pjcj/Devel--Cover/blob/main/lib/Devel/Cover/Statement.pm#L22

sub error       { $_[0][0] xor !$_[0][1] }

So error is supposed to return 1 if the statement is covered and marked as uncoverable.
The error is then later considered for the calculation of the coverage percentage.
Considering that it is hard to find out which line the culprit is (if there is more than one uncoverable line) this "feature" is not so helpful.
Also I don't consider it to be always an error, because some statements can be randomly covered due to timing issues, and then there is no way to get the percentage up to 100%.
The commit with this change is the first commit introducing uncoverable statements: https://github.com/pjcj/Devel--Cover/commit/b768db32b22959413639aea00b999a3d1134bf2a
(which also says "Allow uncoverable code to be specified. (Unfinished)" in the changelog :)

I added another comment to the issue.

For now I consider this done from our side because we don't rely on the reported overall percentage currently, but only on what gets reported to codecov.
I can keep it on Feedback until Paul lets us know what his plans are on this.

Actions #29

Updated by tinita over 1 year ago

  • Due date changed from 2023-04-15 to 2023-04-29
  • Priority changed from High to Low

I bumped the due date and set the Prio to low.

Actions #30

Updated by tinita over 1 year ago

Just a PR to correct all uncoverable markers in t/*.t files for now: https://github.com/os-autoinst/os-autoinst/pull/2297

Actions #32

Updated by okurz over 1 year ago

  • Due date deleted (2023-04-29)
  • Status changed from Feedback to Resolved

PR merged, no more problems observed

Actions #33

Updated by tinita over 1 year ago

  • Status changed from Resolved to Feedback

Why was this resolved?
This was still waiting on https://github.com/pjcj/Devel--Cover/issues/323

Can you be a bit more careful with closing other people's tickets please?
I didn't get an email about the due date because it was set to Resolved. Otherwise I would have commented on it myself.
I said I want to check this with Paul at the Perl Toolchain Summit, and I did.

Actions #34

Updated by okurz over 1 year ago

  • Status changed from Feedback to In Progress

https://build.opensuse.org/package/show/devel:languages:perl:autoupdate/perl-Devel-Cover has the new version. tinita will create according SRs and we will await those.

Actions #35

Updated by tinita over 1 year ago

  • Status changed from In Progress to Feedback

I tried it out, see my comment in https://github.com/pjcj/Devel--Cover/issues/323

Now waiting for autoupdate request to be approved: https://build.opensuse.org/request/show/1084395

When it's in Factory, we can then mark statements that might randomly be covered like that:

something that's sometimes called; # uncoverable statement class:ignore_covered_err
Actions #36

Updated by tinita over 1 year ago

  • Status changed from Feedback to Resolved

Devel::Cover 1.40 is in Factory now

Actions

Also available in: Atom PDF