action #125237
closedos-autoinst codecov check "fully_covered" returns 99% but codecov reports look like 100% size:M
Added by okurz almost 2 years ago. Updated over 1 year ago.
0%
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¶
- Bisect like it is done in https://github.com/os-autoinst/os-autoinst/pull/2271 and similar PRs
- Click on "View details" in merged PRs to see the checks
Updated by okurz almost 2 years 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/
Updated by mkittler almost 2 years 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
Updated by tinita almost 2 years 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
Updated by okurz almost 2 years 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.
Updated by openqa_review almost 2 years ago
- Due date set to 2023-03-21
Setting due date based on mean cycle time of SUSE QE Tools
Updated by okurz almost 2 years ago
waiting for https://github.com/os-autoinst/os-autoinst/pull/2277 first
Updated by okurz almost 2 years ago
Updated by okurz almost 2 years ago
- Status changed from In Progress to Feedback
https://github.com/os-autoinst/os-autoinst/pull/2278 is good to go
Updated by livdywan almost 2 years 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
Updated by livdywan almost 2 years ago
Updated by livdywan almost 2 years 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.
Updated by tinita over 1 year ago
- Status changed from Workable to In Progress
- Assignee set to tinita
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
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
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.
Updated by tinita over 1 year ago
https://github.com/os-autoinst/os-autoinst/pull/2292
Should fix two issues:
- flaky test modules (e.g. see message
Devel::Cover: merging data for t/data/tests/bar/module2.pm into t/data/tests/module1.pm
in https://github.com/os-autoinst/os-autoinst/actions/runs/4583543527/jobs/8094455041) - fully_covered
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 ;-)
Updated by tinita over 1 year ago
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.
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!
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.
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.
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.
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.
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
Updated by tinita over 1 year ago
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
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.
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.
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
Updated by tinita over 1 year ago
- Status changed from Feedback to Resolved
Devel::Cover 1.40 is in Factory now