action #95170
closedcoordination #93883: [epic] Speedup openQA coverage tests with running minion jobs synchronously using new upstream "perform_jobs_in_foreground" mojo function
Increase code coverage of critical component OpenQA::Worker::Job without introducing slow-down due to subprocess coverage collection
Description
Motivation¶
https://codecov.io/gh/os-autoinst/openQA/src/master/lib/OpenQA/Worker/Job.pm currently reports a statement coverage of 77% which is below our average. Similar as in the parent #93883 we are using subprocesses with Mojo::IOLoop suffering from the same problem as minion jobs: Collecting coverage in system-level tests is hard and if we ask Devel::Cover to collect coverage then it can be quite slow. Better if we have explicit unit-level testing for code intended to run in subprocesses and/or run subprocesses as synchronous foreground code.
Acceptance criteria¶
- AC1: Code coverage of https://codecov.io/gh/os-autoinst/openQA/src/master/lib/OpenQA/Worker/Job.pm is significantly higher than the current 77%
- AC2: Tests like t/24-worker-jobs.t still run fast, e.g.
make coverage TESTS=t/24-worker-jobs.ttakes less than 30s locally
Suggestions¶
- Run
make coverage KEEP_DB=1 TESTS=t/24-worker-jobs.tlocally - Extend the current test coverage before applying any changes
- Refactor to simplify OpenQA::Worker::Job
- Review the design regarding the use of subprocesses
Updated by mkittler almost 3 years ago
- Status changed from New to In Progress
PR for collecting coverage of image upload code: https://github.com/os-autoinst/openQA/pull/4026
Updated by openqa_review almost 3 years ago
- Due date set to 2021-07-23
Setting due date based on mean cycle time of SUSE QE Tools
Updated by mkittler almost 3 years ago
- Status changed from In Progress to Feedback
The PR has been merged and has increased the coverage by 8.66 % (absolute). The file's coverage is now at 85.92 %.
Updated by okurz almost 3 years ago
good improvement. Do you want to use the opportunity and try to reach 100% statement coverage including considering the use of # uncoverable statement?
Updated by mkittler almost 3 years ago
I can try. Here's another PR for a slight improvement: https://github.com/os-autoinst/openQA/pull/4040 - This leaves only the functions _upload_asset and _upload_log_file (and _log_upload_error which is used by them) completely uncovered.
EDIT: _upload_asset is now covered as well.
Updated by mkittler almost 3 years ago
PR for _upload_log_file: https://github.com/os-autoinst/openQA/pull/4046
With that the file's coverage is now at 99.55 % which is actually quite good. The few remaining lines would be possible to cover as well but of course the effort/benefit ratio is much worse (as they're spread over different functions/branches).
Updated by mkittler almost 3 years ago
- Status changed from Feedback to Resolved
The PR has been merged. I'll leave it at 99.55 %. I wouldn't mark the remaining lines as uncoverable because they should all be coverable. It is just some effort.