Project

General

Profile

Actions

action #105759

closed

flaky code coverage in os-autoinst consoles/virtio_terminal.pm

Added by okurz almost 3 years ago. Updated over 2 years ago.

Status:
Resolved
Priority:
High
Assignee:
Category:
Regressions/Crashes
Target version:
Start date:
2022-01-31
Due date:
2022-04-13
% Done:

0%

Estimated time:

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

virtio-coverage-drop.png (37.8 KB) virtio-coverage-drop.png tinita, 2022-03-15 11:44

Related issues 1 (0 open1 closed)

Copied to openQA Project (public) - action #107254: Flaky code coverage in consoles/VNC.pmResolvedmkittler

Actions
Actions #1

Updated by kraih almost 3 years ago

  • Assignee set to kraih
Actions #2

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
Actions #3

Updated by kraih almost 3 years ago

  • Status changed from Workable to In Progress
Actions #4

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

Actions #5

Updated by kraih almost 3 years ago

  • Status changed from In Progress to Feedback
Actions #6

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:     # [2022-02-09T14:22:53.494340Z] [info] ::: consoles::virtio_terminal::open_pipe: Set PIPE_SZ from 1048576 to 1048576
[  236s] 3:     # [2022-02-09T14:22:53.494557Z] [info] ::: consoles::virtio_terminal::open_pipe: Set PIPE_SZ from 1048576 to 1048576
[  236s] 3:     # 
[  236s] 3:     # don't match:
[  236s] 3:     # (?^u:Set PIPE_SZ from 65536 to 131072)
Actions #7

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.

Actions #8

Updated by kraih almost 3 years ago

  • Status changed from In Progress to Feedback
Actions #9

Updated by kraih almost 3 years ago

Looks like package tests are passing again.

Actions #10

Updated by kraih almost 3 years ago

  • Status changed from Feedback to Resolved
Actions #11

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 }
Actions #12

Updated by tinita almost 3 years ago

  • Status changed from Resolved to Workable
Actions #13

Updated by tinita almost 3 years ago

  • Copied to action #107254: Flaky code coverage in consoles/VNC.pm added
Actions #14

Updated by tinita almost 3 years ago

  • Assignee deleted (kraih)
Actions #15

Updated by okurz almost 3 years ago

  • Due date deleted (2022-02-22)
Actions #16

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.

Actions #17

Updated by kraih almost 3 years ago

  • Status changed from Workable to In Progress
Actions #18

Updated by openqa_review over 2 years ago

  • Due date set to 2022-03-26

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

Actions #19

Updated by kraih over 2 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.

Actions #20

Updated by tinita over 2 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.

Actions #21

Updated by tinita over 2 years ago

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

Actions #22

Updated by kraih over 2 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

Actions #23

Updated by kraih over 2 years ago

  • Status changed from In Progress to Feedback
Actions #24

Updated by kraih over 2 years ago

  • Status changed from Feedback to Resolved
Actions #25

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.

Actions #26

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.

Actions #27

Updated by kraih over 2 years ago

Maybe someone else should give this ticket a try, fresh eyes might help.

Actions #28

Updated by mkittler over 2 years ago

  • Assignee set to mkittler

I'll have a look.

Actions #29

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

Actions #30

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.

Actions #31

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

Actions #32

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.

Actions #33

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.

Actions #34

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.

Actions

Also available in: Atom PDF