Conversation
There was a problem hiding this comment.
Pull request overview
Updates the OVMS CI/build workflow to generate a filtered LCOV-based HTML coverage report (excluding tests/external code) and to support the make get_coverage flow that copies genhtml/ out of the -build image.
Changes:
- Update
run_unit_tests.shcoverage mode to post-process Bazel’s LCOV output withlcov --extract/--removeand generate HTML viagenhtml. - Split Dockerfile test execution so model preparation and unit-test execution run in separate
RUNsteps whenRUN_TESTS=1. - Ignore the generated
genhtml/coverage report directory in git.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| run_unit_tests.sh | Filters Bazel LCOV output to src/* (excluding src/test/* and external/*) and generates genhtml/ HTML report; adjusts coverage error handling flow. |
| Dockerfile.ubuntu | Splits model preparation and unit test execution into separate RUN steps for RUN_TESTS=1. |
| Dockerfile.redhat | Same as Ubuntu Dockerfile: separates model preparation from running unit tests. |
| .gitignore | Adds genhtml/ to ignored artifacts. |
|
how would we export coverage report? Using separate make targets and the build image as parameter? |
Dockerfile.ubuntu
Outdated
| ARG RUN_TESTS=0 | ||
| RUN if [ "$RUN_TESTS" == "1" ] ; then mkdir -p demos/common/export_models/ && mv export_model.py demos/common/export_models/ && ./prepare_llm_models.sh /ovms/src/test/llm_testing docker && ./run_unit_tests.sh ; fi | ||
| RUN if [ "$RUN_TESTS" == "1" ] ; then mkdir -p demos/common/export_models/ && mv export_model.py demos/common/export_models/ && ./prepare_llm_models.sh /ovms/src/test/llm_testing docker ; fi | ||
| RUN if [ "$RUN_TESTS" == "1" ] ; then ./run_unit_tests.sh ; fi |
There was a problem hiding this comment.
why this is separate command?
There was a problem hiding this comment.
It made testing easier for me. Reverted if you like
run_unit_tests.sh
Outdated
| genhtml --output genhtml "$(bazel info output_path)/_coverage/_coverage_report.dat" | ||
| local coverage_dat="$(bazel info output_path)/_coverage/_coverage_report.dat" | ||
|
|
||
| if ! lcov --extract "$coverage_dat" 'src/*' --output-file filtered_coverage.dat --ignore-errors negative; then |
There was a problem hiding this comment.
--instrumentation_filter="-src/test is not doing the same filtering?
There was a problem hiding this comment.
No, it didnt work
|
Added support for redhat as well. |
it is literally in the PR description, please take a look |
| # lcov 2.x supports --ignore-errors negative,unused; lcov 1.x does not | ||
| local lcov_ver | ||
| lcov_ver=$(lcov --version | grep -oP '\d+' | head -1) | ||
| local lcov_ignore="" | ||
| local genhtml_ignore="" | ||
| if [ "$lcov_ver" -ge 2 ] 2>/dev/null; then | ||
| lcov_ignore="--ignore-errors negative,unused" | ||
| genhtml_ignore="--ignore-errors negative" | ||
| fi | ||
|
|
||
| if ! lcov --extract "$coverage_dat" 'src/*' --output-file filtered_coverage.dat $lcov_ignore; then | ||
| echo "Error: lcov extraction failed" >&2 | ||
| return 1 | ||
| fi | ||
|
|
||
| if ! lcov --remove filtered_coverage.dat 'src/test/*' 'external/*' --output-file filtered_coverage.dat $lcov_ignore; then | ||
| echo "Error: lcov filtering failed" >&2 | ||
| return 1 | ||
| fi | ||
|
|
||
| if ! genhtml $genhtml_ignore --output genhtml filtered_coverage.dat; then | ||
| echo "Error: genhtml report generation failed" >&2 |
There was a problem hiding this comment.
Add comments why such fixes are required, beside that LGTM
There was a problem hiding this comment.
Pull request overview
Improves the repository’s code coverage generation and validation flow (CVS-183119) by filtering Bazel lcov output into a cleaner genhtml/ report and simplifying how coverage thresholds are checked.
Changes:
- Enhance
run_unit_tests.shto post-process Bazel coverage output withlcov/genhtml, including version-specific ignore flags and source/test/external filtering. - Update the coverage threshold parsing/check script and make it print clearer diagnostics.
- Adjust
Makefilecoverage check to run the script locally and ignore generatedgenhtml/output in git.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| run_unit_tests.sh | Adds lcov/genhtml-based filtering and report generation after bazel coverage. |
| ci/check_coverage.bat | Updates thresholds and parsing logic; adds clearer output and parse failure handling. |
| Makefile | Runs coverage threshold checks via the repo script after copying genhtml/ from the build image. |
| .gitignore | Ignores generated genhtml/ directory. |
You can also share your feedback on Copilot code review. Take the survey.
| #Ubuntu | ||
| MIN_LINES_COV=76.8 | ||
| MIN_FUNCTION_COV=87.6 | ||
| MIN_LINES_COV=76.0 | ||
| MIN_FUNCTION_COV=83.0 | ||
|
|
||
| #Rhel | ||
| MIN_LINES_COV=75.6 | ||
| MIN_FUNCTION_COV=73.0 | ||
| MIN_LINES_COV=76.0 | ||
| MIN_FUNCTION_COV=83.0 |
| if ! lcov --remove filtered_coverage.dat 'src/test/*' 'external/*' --output-file filtered_coverage.dat $lcov_ignore; then | ||
| echo "Error: lcov filtering failed" >&2 | ||
| return 1 | ||
| fi | ||
|
|
||
| if ! genhtml $genhtml_ignore --output genhtml filtered_coverage.dat; then |
🛠 Summary
CVS-183119