action #105759
closedflaky code coverage in os-autoinst consoles/virtio_terminal.pm
Added by okurz almost 3 years ago. Updated over 2 years ago.
Description
Observation¶
See https://github.com/os-autoinst/os-autoinst/pull/1939#issuecomment-1023298597
https://app.codecov.io/gh/os-autoinst/os-autoinst/compare/1939/changes shows that the method set_pipe_sz would not be covered anymore:
sub set_pipe_sz ($self, $fd, $newsize) {
no autodie;
return fcntl($fd, F_SETPIPE_SZ(), int($newsize)); # uncoverable statement
as the return line already has "uncoverable statement" likely we should just add more such statements so that the complete function is excluded
Acceptance criteria¶
- AC1: Reported code coverage is stable
Files
Updated by livdywan almost 3 years ago
- Subject changed from flaky code coverage in os-autoinst consoles/virtio_terminal.pm to flaky code coverage in os-autoinst consoles/virtio_terminal.pm size:S
- Description updated (diff)
- Status changed from New to Workable
Updated by kraih almost 3 years ago
- Status changed from Workable to In Progress
Updated by openqa_review almost 3 years ago
- Due date set to 2022-02-22
Setting due date based on mean cycle time of SUSE QE Tools
Updated by okurz almost 3 years ago
- Priority changed from Normal to High
PR is merged. Now I see failures in package tests, e.g. in https://build.opensuse.org/package/live_build_log/devel:openQA/os-autoinst/openSUSE_Factory_PowerPC/ppc64le
I see
[ 236s] 3: # Failed test 'Log mention new size'
[ 236s] 3: # at ./10-virtio_terminal.t line 125.
[ 236s] 3: # STDOUT & STDERR:
[ 236s] 3: # [2022-02-09T14:22:53.491278Z] [debug] <<< consoles::virtio_terminal::open_pipe(pipe_prefix="./virtio_console_open_test")
[ 236s] 3: # [33m[2022-02-09T14:22:53.494340Z] [info] ::: consoles::virtio_terminal::open_pipe: Set PIPE_SZ from 1048576 to 1048576
[ 236s] 3: # [0m[33m[2022-02-09T14:22:53.494557Z] [info] ::: consoles::virtio_terminal::open_pipe: Set PIPE_SZ from 1048576 to 1048576
[ 236s] 3: # [0m
[ 236s] 3: # don't match:
[ 236s] 3: # (?^u:Set PIPE_SZ from 65536 to 131072)
Updated by kraih almost 3 years ago
- Status changed from Feedback to In Progress
Looks like the test doesn't quite work right, i'll need to find a more portable way to deal with pipe size.
Updated by kraih almost 3 years ago
- Status changed from In Progress to Feedback
Updated by tinita almost 3 years ago
Somehow this problem is back:
https://github.com/os-autoinst/os-autoinst/pull/1961
https://app.codecov.io/gh/os-autoinst/os-autoinst/compare/1961/changes complains about lost coverage for those lines:
112 sub set_pipe_sz ($self, $fd, $newsize) {
113 no autodie;
114 return fcntl($fd, F_SETPIPE_SZ(), int($newsize));
115 }
Updated by tinita almost 3 years ago
- Copied to action #107254: Flaky code coverage in consoles/VNC.pm added
Updated by kraih almost 3 years ago
- Assignee set to kraih
Since nobody else has picked this up yet i guess i'll take another look.
Updated by kraih almost 3 years ago
- Status changed from Workable to In Progress
Updated by openqa_review almost 3 years ago
- Due date set to 2022-03-26
Setting due date based on mean cycle time of SUSE QE Tools
Updated by kraih almost 3 years ago
tinita wrote:
Somehow this problem is back:
https://github.com/os-autoinst/os-autoinst/pull/1961
https://app.codecov.io/gh/os-autoinst/os-autoinst/compare/1961/changes complains about lost coverage for those lines:
These links are not helpful if you re-run the tests until the problem does not show up anymore.
Updated by tinita almost 3 years ago
kraih wrote:
tinita wrote:
Somehow this problem is back:
https://github.com/os-autoinst/os-autoinst/pull/1961
https://app.codecov.io/gh/os-autoinst/os-autoinst/compare/1961/changes complains about lost coverage for those lines:These links are not helpful if you re-run the tests until the problem does not show up anymore.
I know, that's why I quoted the lines that codecov was complaining about.
I will attach a screenshot next time.
Updated by tinita almost 3 years ago
- File virtio-coverage-drop.png virtio-coverage-drop.png added
I digged into recent reports and found one with the same coverage drop:
https://github.com/os-autoinst/os-autoinst/pull/1967
https://app.codecov.io/gh/os-autoinst/os-autoinst/compare/1967/changes
Attached a screenshot
Updated by kraih almost 3 years ago
Only explanation for the coverage being flaky i can think of would be an unexpectedly small max pipe size value. So i've made a PR to fix that case, and a test that should give more details in case the fix doesn't work. https://github.com/os-autoinst/os-autoinst/pull/1991
Updated by kraih almost 3 years ago
- Status changed from In Progress to Feedback
Updated by okurz over 2 years ago
- Subject changed from flaky code coverage in os-autoinst consoles/virtio_terminal.pm size:S to flaky code coverage in os-autoinst consoles/virtio_terminal.pm
- Due date deleted (
2022-03-26) - Status changed from Resolved to New
- Assignee deleted (
kraih)
Unfortunately the above didn't really help as https://github.com/os-autoinst/os-autoinst/pull/2006#issuecomment-1081654863= just showed.
Updated by kraih over 2 years ago
okurz wrote:
Unfortunately the above didn't really help as https://github.com/os-autoinst/os-autoinst/pull/2006#issuecomment-1081654863= just showed.
I'm starting to think that maybe something might be off with the coverage metrics and it's not a race condition. Because if i look at the current coverage of virtio_terminal.pm and compare that to the changes from your PR, the only difference is that sub set_pipe_sz is no longer called. But the line that actually calls set_pipe_sz is still covered. Something does not add up here.
Updated by kraih over 2 years ago
Maybe someone else should give this ticket a try, fresh eyes might help.
Updated by mkittler over 2 years ago
- Status changed from New to In Progress
Maybe it helps to simply invoke this function explicitly: https://github.com/os-autoinst/os-autoinst/pull/2008
Updated by kraih over 2 years ago
mkittler wrote:
Maybe it helps to simply invoke this function explicitly: https://github.com/os-autoinst/os-autoinst/pull/2008
Good idea. That didn't work previously because this line also had flaky coverage. But maybe that's fixed by the other changes.
Updated by openqa_review over 2 years ago
- Due date set to 2022-04-13
Setting due date based on mean cycle time of SUSE QE Tools
Updated by mkittler over 2 years ago
- Status changed from In Progress to Feedback
I cannot really make sense of the locally generated coverage report so I'd just wait a while to see whether the problem shows up again in the CI env.
Updated by okurz over 2 years ago
mkittler and me worked on this together. We have fixed the problems mkittler had generating the coverage locally which is a known upstream issue https://github.com/pjcj/Devel--Cover/issues/292 . If you also run into warnings, especially with openSUSE Tumbleweed with perl 5.34 you can apply the hotfix referenced in the github issue. And then we improved the situation, see https://github.com/os-autoinst/os-autoinst/pull/2013 and https://github.com/os-autoinst/os-autoinst/pull/2014 and https://github.com/os-autoinst/os-autoinst/pull/2015
I suggest to check the stability of coverage now locally with multiple runs.
Updated by mkittler over 2 years ago
- Status changed from Feedback to Resolved
Tested it locally with for i in {00..50}; do echo "run $i" && rm -rf cover_db/ && cover -coverage stmt -test -make 'prove -I. --timer -v t/10-virtio_terminal.t; echo $?' | grep 'consoles/virtio_terminal.pm 100.0 100.0' || break ; done and it went though all iterations. This should be good enough to consider the ticket resolved.
I've also created https://github.com/os-autoinst/os-autoinst/pull/2016 for a CI check.