action #42074
closedcoverage output in os-autoinst seems incomplete
0%
Description
Observation¶
(cd t && cover -report html_basic)
Reading database from t/cover_db
…
---------------------------- ------ ------ ------ ------ ------ ------ ------
File stmt bran cond sub pod time total
---------------------------- ------ ------ ------ ------ ------ ------ ------
...QA/Benchmark/Stopwatch.pm 71.6 100.0 16.6 83.3 87.5 0.0 71.5
...inst/OpenQA/Exceptions.pm 100.0 n/a n/a 100.0 n/a 0.0 100.0
...tovideo/CommandHandler.pm 58.1 40.9 29.4 74.0 0.0 0.0 55.0
...A/Isotovideo/Interface.pm 100.0 n/a n/a 100.0 n/a 0.0 100.0
...penQA/Isotovideo/Utils.pm 100.0 n/a 50.0 100.0 0.0 0.0 95.4
...t/OpenQA/Qemu/BlockDev.pm 83.6 68.7 0.0 84.6 60.0 0.0 78.0
...enQA/Qemu/BlockDevConf.pm 60.3 10.0 22.2 70.9 77.2 0.0 59.5
...QA/Qemu/ControllerConf.pm 56.7 n/a n/a 63.6 57.1 0.0 58.1
...A/Qemu/DriveController.pm 42.8 n/a n/a 33.3 100.0 0.0 45.4
…
Total 63.3 33.4 25.4 69.9 37.7 100.0 55.9
---------------------------- ------ ------ ------ ------ ------ ------ ------
## Please see file -
Perltidy version is 20180220
1: Syntax error near ? :
## Please see file -
Perltidy version is 20180220
1: Syntax error near ? :
<repeats many times more>
HTML output written to t/cover_db/coverage.html
Opening an html file with browser for single files just shows one line with "perltidy" and not the expected source code with the coverage information
Problem¶
Seems to be a regression, was working better once.
Files
Updated by okurz over 5 years ago
- File coverage.html coverage.html added
- Subject changed from coverage output in os-autoinst seems incomplete and spills errors about tidy to coverage output in os-autoinst seems incomplete
Trying out with make check && (cd t && cover -report html_basic)
the error about tidy seems to have vanished and the coverage seems to be a bit "more complete" as more files are mentioned but I could see two issues:
- Files outside the project are covered as well, e.g. /usr/share/automake-1.15/Automake/ChannelDefs.pm and such
- Not all test files are included (or none)
Updated by okurz about 5 years ago
- Status changed from New to Feedback
- Assignee set to okurz
- Target version set to Current Sprint
okurz wrote:
Trying out with
make check && (cd t && cover -report html_basic)
the error about tidy seems to have vanished and the coverage seems to be a bit "more complete" as more files are mentioned but I could see two issues:
- Files outside the project are covered as well, e.g. /usr/share/automake-1.15/Automake/ChannelDefs.pm and such
This seems to have been a problem on my side as there was still an old directory t/cover_db/ around where probably I played around with cover manually without any exclude/include rules. Deleting the directory and running tests again collecting coverage the result is reliable and including only files within the os-autoinst working copy.
- Not all test files are included (or none)
That we can change, e.g. with https://github.com/os-autoinst/os-autoinst/pull/1289
So what I tried in https://github.com/os-autoinst/os-autoinst/pull/1289#issuecomment-559862318 is to remove the include use Devel::Cover
from one test and it reduced coverage. I still do not understand why. What I tried then is to add use Devel::Cover
in simply all test files and this further increases coverage so I created
https://github.com/os-autoinst/os-autoinst/pull/1291
EDIT: 2019-11-30: coolo prefers to have test files themselves to be not recorded in code coverage, fine.
So I conducted another two runs locally with tests not included, one with use Devel::Cover
in each file and one without:
rm -rf cover_db/ t/cover_db/ && ./configure && make && make VERBOSE=1 coverage && mv t/cover_db{/,_all_devel_cover_no_tests_included/} && firefox t/cover_db_all_devel_cover_no_tests_included/coverage.html && git co origin/master && rm -rf cover_db/ t/cover_db/ && ./configure && make && make VERBOSE=1 coverage && mv t/cover_db{/,_master_no_tests_included/} && firefox t/cover_db_master_no_tests_included/coverage.html
There are differences in reports but they do not seem to correlate with the change but rather a timing based non-deterministic execution of certain code paths. E.g. in one case I observed that "autotest.pm" was imported 58 times, in the other case 59 times.
I can see that some lines are only covered sometimes, e.g.
autotest.pm:
128 8 50 0 10 unless (blessed($args{run_args}) && $args{run_args}->isa('OpenQA::Test::RunArgs')) {
129 8 16 die 'The run_args must be a sub-class of OpenQA::Test::RunArgs';
130 }
131 0 0 $test->{run_args} = $args{run_args};
132 0 0 delete $args{run_args};
133 }
134
135 429 576 my $nr = '';
136 421 896 while (exists $tests{$fullname . $nr}) {
137 # to all perl hardcore hackers: fuck off!
138 8 50 11 $nr = $nr eq '' ? 1 : $nr + 1;
139 8 23 $test->{name} = join("#", $name, $nr);
The lines 128-129 and 138-139 were not executed in the other case. Probably we can look for the test that should check the loading of the same test module multiple times as it seems to be not fully deterministic.
Another test again where I have use Devel::Cover
removed from 07-commands.t:
sudo rm -rf cover_db/ t/cover_db/ && ./configure && make && make VERBOSE=1 coverage && mv t/cover_db{/,_devel_cover_use_removed/} && firefox t/cover_db_devel_cover_use_removed/coverage.html
The overall coverage reduced from 55.0+-0.1 to 53.3. Could it be that some subprocesses do not get covered by Devel::Cover unless use Devel::Cover
is explicitly stated somewhere? Especially in 07-commands.t where we spawn a separate server instance. By the way, the original PR explained all these problems already in https://github.com/os-autoinst/os-autoinst/pull/987 , seems mkittler has forgot that ;) Seems also https://stackoverflow.com/questions/56856646/how-do-i-collect-coverage-from-child-processes-when-running-cover-test-and-n tries to answer that question and the best they could come up with it seems is:
my $is_covering = !!(eval 'Devel::Cover::get_coverage()');
my @perl = ($^X, $is_covering ? ('-MDevel::Cover=-silent,1') : ());
diag $is_covering ? 'Devel::Cover running' : 'Devel::Cover not covering';
# Pass the Devel::Cover option, if any, to the child process
system(@perl, 'prt', ...);
to start processes with coverage. Now also perlpunk started some experiments in https://github.com/os-autoinst/os-autoinst/pull/1295 so I will wait for this
EDIT: 2019-12-01: https://github.com/os-autoinst/os-autoinst/pull/1297 created to explain better why we have "use Devel::Cover" in t/07-commands.t but nowhere else. I closed https://github.com/os-autoinst/os-autoinst/pull/1292
Updated by tinita about 5 years ago
One reason I can think of is that PERL5OPT is not passed to the child processes, but I can't explain why a "use Devel::Cover" in a test file fixes this...
Updated by tinita about 5 years ago
I just found this note here https://rt.cpan.org/Ticket/Display.html?id=42532
Starting with version 0.63, Devel::Cover calls Devel::Cover::report
automatically when a process does an exec.
Updated by okurz about 5 years ago
but we have Devel::Cover >= 1.0 since 2015 so I doubt it makes any recent difference for us.
https://stackoverflow.com/questions/56856646/how-do-i-collect-coverage-from-child-processes-when-running-cover-test-and-n also describes that environment variables do not seem to do the trick.
Updated by coolo about 5 years ago
This can only work if we exit
processes normally - but often we have _exit
in place
Updated by tinita about 5 years ago
I created a PR: https://github.com/os-autoinst/os-autoinst/pull/1307 - t: Adjust PERL5LIB and Devel::Cover options
This has shown broader and higher coverage so far.
Updated by tinita about 5 years ago
Two more PRs have been merged:
https://github.com/os-autoinst/os-autoinst/pull/1312 - t: Enable 04-check_vars_docu.t to be run with prove
https://github.com/os-autoinst/os-autoinst/pull/1311 - t: Enable 10-terminal.t to be run with prove
Updated by tinita about 5 years ago
We now have all 66 perl files covered (before we were missing 25 files), and also the coverage is higher.
The flaky coverage is still there. Not clear yet what is the reason.
It could be the forking, but also mocking functions can be problematic for coverage.
Updated by okurz about 5 years ago
also https://github.com/os-autoinst/os-autoinst/pull/1305 , just merged. I have a good feeling regarding the completeness of coverage and also its stability. Do you have any further plans?
Updated by tinita about 5 years ago
I would like to remove "use Devel::Cover" from 07-commands, and long term it would be nice to be able to run tests without cd t
.
Not sure yet about stability of the coverage...
Updated by tinita about 5 years ago
https://github.com/os-autoinst/os-autoinst/pull/1320 - Remove "use Devel::Cover" from 07-commands.t
Updated by tinita about 5 years ago
Updated by livdywan almost 5 years ago
- Status changed from Feedback to Resolved
Updated by livdywan almost 5 years ago
The Devel::Cover stuff is gone, and we seem to be in good shape. I'm assuming this is all we want to achieve here.