Surface untested source files at 0% coverage in reports#9
Conversation
Introduces a reusable coverage toolchain based on llvm-cov: - coverage/merger.py: per-test profraw -> profdata + object file packaging - coverage/reporter.py: cross-test aggregation, HTML/LCOV/text reports - coverage/effective_coverage.py: justification overlay + effective metrics - coverage/justify.py: justification manifest resolution - coverage/defs.bzl: score_coverage_reporter macro for consumer wiring - coverage/coverage.bazelrc: shared coverage flags - coverage/filter_regexes.txt: baseline source exclusions - coverage/generate_coverage_html.sh: convenience entry point Adds an end-to-end example under tests/coverage exercising the pipeline with a small instrumented library, test, justification file, and consumer filter regexes. Known limitation: source files not linked into any cc_test are not yet included in the report (no instrumented object file -> invisible to llvm-cov).
llvm-cov only reports files linked into at least one test binary.
Sources that exist in the workspace but no test pulls in silently
disappear from the report, causing coverage to appear higher than
it actually is.
Three mechanisms work together to fix this:
Bazel aspect + manifest rule (coverage/defs.bzl):
_collect_sources_aspect walks the dependency graph of all configured
targets and collects C/C++ source files. score_instrumented_sources_manifest
writes a workspace-relative path-per-line manifest.
score_coverage_reporter gains an optional instrumented_sources_manifest
parameter that passes the manifest to the reporter via
--instrumented_sources_manifest.
Reporter augmentation (coverage/reporter.py):
After llvm-cov export, the reporter compares the manifest against
covered sources from the LCOV output. For each missing file a
synthetic 0%-coverage LCOV record is appended (SF + DA per
non-blank line + LF/LH). The llvm-cov text summary TOTALS line is
updated in-place using re.finditer to preserve fixed-width column
alignment. Per-file HTML pages and a "Not Linked Into Tests" index
section are generated for visibility.
Correctness and security fixes applied during review:
- Use str.replace() instead of str.format() for HTML template
rendering so that { and } in C++ source bodies do not crash the
reporter with KeyError/ValueError.
- Separate stderr from stdout for run_llvm_cov_export and
run_llvm_cov_report (separate_stderr=True) so that llvm-cov
warning messages are not mixed into LCOV/summary output.
- Validate that resolved manifest paths stay within workspace_root
via Path.is_relative_to() before reading files.
- Extend _escape_html to cover ' (') and " (") so that
file paths with apostrophes do not break HTML attributes.
- Count only non-blank lines for LF in synthetic LCOV records
to avoid inflating the denominator in aggregate metrics.
Test fixture (tests/coverage/uncovered.cpp, uncovered.h, BUILD.bazel):
cc_library intentionally not linked into any cc_test. Verifies that
the reporter surfaces the file at 0% coverage rather than omitting it.
Docs (coverage/README.md): new section 5a with usage example.
- Use heuristic to identify instrumentable lines instead of counting all non-blank lines. Filters comments, preprocessor directives, lone braces, namespace declarations, and access specifiers to avoid inflating LF values in synthetic LCOV records. - Augment summary.txt and console output with untested file line counts so the visible TOTALS reflect the true coverage including 0%-files. - Parse the llvm-cov column header to determine the Lines group index dynamically instead of hardcoding position 1. - Add workspace-bounds check after resolve() in _find_untested_sources to prevent path traversal via symlinks. - Escape single and double quotes in _escape_html to prevent attribute breakout in generated HTML pages.
- Narrow _NON_EXECUTABLE_RE block-comment pattern from `|\*.*` to `|\*(?:[/\s].*)?` so that pointer dereferences (`*ptr = value;`) are correctly classified as executable. - Add py_test with unit tests for all reporter augmentation helpers: _is_likely_executable, _count_instrumentable_lines, _covered_sources_from_lcov, _find_untested_sources, _append_zero_coverage_lcov, _augment_text_summary, _escape_html. Includes a path-traversal rejection test for _find_untested_sources.
- Add py_library target for reporter so unit tests can import it. - Remove coverable_test from instrumented_sources manifest targets to avoid testonly dependency violation (test sources don't need to appear in the manifest — they're tested by definition).
The heuristic line count (_count_instrumentable_lines) cannot replicate what llvm-cov would report for actually-instrumented objects. Rewriting TOTALS with approximate numbers gives false precision. - _augment_text_summary: no longer rewrites the TOTALS line; appends a clearly-labelled WARNING banner with ~N estimated lines instead. - _inject_untested_section_into_index: injects a prominent banner right after <body> so it is the first thing reviewers see. Detail table uses ~N notation and includes a disclaimer about the heuristic. - _append_zero_coverage_lcov docstring documents the approximation. - Tests updated to match banner-only behavior.
github-project-automation
Bot
moved this from Done
to In Progress
in INF - Infrastructure Community
nradakovic
left a comment
nradakovic
left a comment
There was a problem hiding this comment.
To be honest. I will need a lot of time (and coffee) to do proper review. Hopefully by Monday I will have feedback.
Thanks for the quick feedback Nikola. In fact, I took most of the code from this PR in the communication repository. Additionally, I fixed that files without unittests are shown in the report so not a higher coverage is show that is actually the case. Maybe for the review you seek help in the communication team. Happy review and hopefully good ☕😉 |
And this is also another problem. I'll for sure have some questions why certain implementation blocks are done in such way, but since communication PR is already merged (and in use?) not sure if I want to dive into large discussion and diverge from original PR. Anyway, let's see once I have concrete feedback. If something is on critical path, I will request that communication change their implementation. |
|
So something that I see by just looking at. This implementation relies on a generated Bash wrapper to reconstruct Bazel runfiles and workspace paths before invoking the actual reporter. This feels a bit non-Bazel-native. Can we instead model this as an executable Starlark rule, or move runfiles/path resolution into the reporter binary using Bazel runfiles libraries? |
nradakovic
left a comment
nradakovic
left a comment
There was a problem hiding this comment.
For start I left 2 comments. It's enough to discuss Bazel implementation blocks (I still haven't check Python scripts) but I think they look OK. Have to do better look.
I see two paths for this PR to get merged:
- I will approve as is but only if we have plan to improve current implementation (tickets to track it with clear plan how to improve it). What communication guys did is on them to support it. This will be untilized by every single module in S-CORE so we do not want to find nasty surprises 2 days before release of S-CORE.
- We start now changing blocks but this will require some time and probably we will need to include more people who have more exp. with Bazel and code coverage.
@RSingh1511 Please provide your thoughts on this as well.
|
Hello @olivembo , Thanks for this. Since you referenced communication, could you create an adoption PR in communication or logging showing how we can consume it? A real data example would help with verification and allow everyone to understand the use case. |
There was a problem hiding this comment.
Pull request overview
This PR extends the repo’s Bazel/llvm-cov coverage tooling so that C/C++ source files reachable from configured coverage roots—but not linked into any executed test binary—are still surfaced in coverage outputs as synthetic 0%-coverage entries (LCOV, HTML, and text summary). It does so by generating a manifest of reachable sources via a Bazel aspect and then augmenting the reporter outputs based on a manifest-vs-LCOV diff.
Changes:
- Adds a Bazel aspect +
score_instrumented_sources_manifestrule to enumerate transitive C/C++ sources and plumbs an optionalinstrumented_sources_manifestintoscore_coverage_reporter. - Implements report augmentation in
coverage/reporter.py(LCOV + HTML index/pages + text summary banner) and adds unit tests for the augmentation helpers. - Adds coverage adoption docs/config (
coverage/README.md,coverage.bazelrc, filter regex baseline) and a consumer smoke-test workspace undertests/coverage.
Reviewed changes
Copilot reviewed 5 out of 27 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/MODULE.bazel | Adds rules_python + Python toolchain setup for the test workspace. |
| tests/coverage/uncovered.h | Adds an intentionally untested header fixture for smoke-testing “0% untested file” surfacing. |
| tests/coverage/uncovered.cpp | Adds an intentionally untested source fixture for smoke-testing “0% untested file” surfacing. |
| tests/coverage/reporter_test.py | Adds unit tests for coverage reporter augmentation helpers (LCOV/HTML/text/escaping/path checks). |
| tests/coverage/coverage_justifications.yaml | Adds a sample justification DB used by the coverage smoke test. |
| tests/coverage/coverage_filter_regexes.txt | Adds smoke-test consumer-side extra ignore regexes. |
| tests/coverage/coverable.h | Adds a small API exercised by the coverage smoke test (with an intentionally uncovered branch). |
| tests/coverage/coverable.cpp | Adds the implementation for the smoke-test library. |
| tests/coverage/coverable_test.cpp | Adds a test that intentionally leaves one branch uncovered for report validation. |
| tests/coverage/BUILD.bazel | Wires the smoke test targets, manifest generation, and reporter wrapper. |
| tests/BUILD.bazel | Exports MODULE.bazel so the reporter wrapper can resolve workspace root at runtime. |
| tests/.bazelrc | Imports coverage bazelrc and adds coverage smoke-test settings for the tests workspace. |
| README.md | Updates top-level README to document coverage tooling availability and link to coverage guide. |
| MODULE.bazel | Adds rules_python, rules_shell, Python toolchain, and pip hub wiring for coverage tooling. |
| coverage/requirements.in | Adds pyyaml as the direct dependency for justification processing. |
| coverage/requirements_lock.txt | Adds a locked pyyaml pin with hashes for Bazel pip integration. |
| coverage/reporter.py | Implements final report generation + augmentation with synthetic 0%-coverage entries for untested sources. |
| coverage/README.md | Adds an adoption guide for consumers (setup, configs, optional untested-files manifest). |
| coverage/merger.py | Adds per-test merger that packages profdata + object file metadata for final reporting. |
| coverage/justify.py | Adds YAML + in-source marker processing to generate a justification manifest. |
| coverage/generate_coverage_html.sh | Adds a consumer-facing driver to extract reports, run justification/effective-coverage, and optionally archive artifacts. |
| coverage/filter_regexes.txt | Adds baseline ignore regexes (tests/mocks/fakes/external/benchmarks). |
| coverage/effective_coverage.py | Adds HTML post-processing + effective coverage computation incorporating justifications. |
| coverage/defs.bzl | Adds the manifest aspect/rule and updates score_coverage_reporter to pass the manifest into the reporter. |
| coverage/coverage.bazelrc | Adds generic Bazel coverage flags/config to be imported by consumers. |
| coverage/BUILD.bazel | Adds Bazel targets for merger/reporter/justify/effective_coverage and the driver script. |
| .gitignore | Ignores generated cpp_coverage/ output directories. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…e_reporter The previous implementation used a genrule + heredoc to generate the reporter wrapper shell script. This required three-level escaping (\\$$) to handle Bazel make-variable and bash variable expansion in the same string which makes the code fragile and hard to debug. Replace with a custom Starlark rule (`_score_coverage_reporter_rule`) that uses `ctx.actions.write()` to produce the wrapper script directly. A new `_rlocation_path()` helper computes `Runfiles.Rlocation()`-compatible paths from File objects, eliminating all genrule make-variable gymnastics. The `--workspace_root` argument (previously computed via `readlink -f` in bash) is replaced by `--module_bazel` (a plain rlocation string). The reporter resolves it with `r.Rlocation()` and derives the workspace root in Python. The generated wrapper is now a readable 7-line script with no escaping. `rules_shell` is no longer a dependency of defs.bzl. Addresses review comment: eclipse-score#9 (comment)
nradakovic
added
enhancement
|
|
Gentleman still wait or proceed with merge? |
|
Two cents from the guy where this was copied over from. As I wrote already in the Slack, the proper generation of baseline coverage requires a deeper look. Especially, as rules_cc is missing this support today - and I believe for a proper generation of baseline coverage, we need to look there first. But, this will require quite some time. Second, I am still in the workings to make the coverage also correct for QNX. First experiments also showed that the coverage for QNX will bring coverage results for rust included. Thus, I am also not sure yet how to best handle this and if this will be at the end an "cpp_policies" only thing, or might not include Rust also. Third, we still have a problem with constexpr functions, as they appear not covered. Fourth, my original plan would have been, once the three mentioned points are solved, to contribute this solution to toolchains_llvm, as obviously this can be reused by a way broader community. Anyhow, it can be decided that this is "good enough" for now, and these problems are tackled in an incremental way. On a personal note - while the licenses and copyrights that we utilize in this project fully allow a copying of the code to different places without touching the license + copyright, I still think it follows best practices to give at least some credit / blame for the original author. Feels quite bad if a topic where quite some time was invested is just copied over. I would have loved to see at least a clean commit with the original state and author, and then the improvements, refactorings on top of it. (I do not require to change this, but I would like to raise awareness). |
It's not approved yet |
Hi Jan, thanks for the detailed write-up. This is genuinely useful context. On the technical points:
On attribution: you're right to call this out, and I'll be direct: It should have been handled better from the start. A commit that establishes the original state with proper authorship, followed by the changes on top, would have been the clean way to do it. That ship has sailed for the commit history, but I've updated the PR description to explicitly credit your PR #549 as the origin of the pipeline. Not a substitute for getting the history right, but at least the record is there. The plan: Merge this as "good enough for now", and track the three open items as follow-ups. Does that work for you? |
score_instrumented_sources_manifest only emitted the manifest .txt listing source paths, never propagating the actual source Files as runfiles. Under the linux-sandbox strategy the reporter therefore couldn't find those files on disk, so _find_untested_sources() silently dropped every manifest entry and 0%-coverage files never appeared in the report. Even after fixing runfiles propagation, a second bug remained: _find_untested_sources() resolved each manifest path and rejected it if it fell outside workspace_root. Runfiles are frequently symlinks that legitimately resolve to the real on-disk source tree outside the sandboxed workspace_root, so this check rejected every legitimate entry as a false-positive path traversal. - _instrumented_sources_manifest_impl: return runfiles for the source Files (not just the manifest file) via DefaultInfo, and expose the InstrumentedSourcesInfo provider. - _score_coverage_reporter_impl: merge the manifest target's default_runfiles into the wrapper so the source files are present on disk when the reporter runs sandboxed. - _find_untested_sources: reject path traversal based on the raw manifest-relative path (no ".." components) instead of resolve()-then-containment, so legitimate runfiles symlinks are no longer dropped. Returns (abs_path, rel_path) pairs so callers can display the clean manifest-relative name instead of the resolved on-disk path. - _augment_html_with_untested: use the manifest-relative name for display instead of recomputing it from the resolved absolute path, which leaked the full host filesystem path once symlinks were followed. - Add regression test covering manifest entries reached only through symlinks.
…s symlink Clicking a file in the HTML coverage report, or opening a source referenced in lcov.dat (IDE coverage gutters, SonarQube, ...), resulted in file-not-found. The links pointed into a path that no longer existed once the coverage-report-generator action's sandbox was torn down. Root cause: workspace_root was computed by taking the parent of Runfiles.Rlocation(MODULE.bazel) directly. Under linux-sandbox, Rlocation() returns a path inside the runfiles tree, which is itself a symlink into the reporter action's own (ephemeral) sandbox - not the real workspace. Since workspace_root feeds --path-equivalence for both llvm-cov show and llvm-cov export, every SF: entry and every per-file HTML link for normal (tested) source files was built from that dead sandbox path (e.g. .../reporter_wrapper.sh.runfiles/_main/...) instead of the real, stable workspace directory. Extract the computation into _resolve_workspace_root() and resolve the symlink before taking its parent. Add a regression test that reproduces the runfiles-symlink layout.
Sums LH/LF across the final LCOV report (real + synthetic 0% records) to surface a combined coverage estimate in both the text summary and HTML index, clearly labeled as a heuristic.
The synthetic "Not Linked Into Tests" pages dumped the whole file into a single <pre> block, so they never used the line-number/uncovered-line classes style.css actually styles, next to a real llvm-cov page they looked broken/unstyled even though style.css loaded fine. Render one row per line instead, matching llvm-cov's own markup, and link control.js so keyboard navigation works on these pages too.
|
OK, so it took me a while to understand implementation design here (still haven't cover everything), to follow up on every discussion made (and probably I missed ton of them) and finally to review the PR. To be fair, the drive of the topic is in mess. Several people across different working community groups, across different modules are involved, everyone has it's own 5 cents to present and when you put all of this into mix, no wonder why no one wants to be maintainer of it. Anyway, let's at least try to fix this.
|
Thanks for taking the time @nradakovic I just got to know this PR: eclipse-score/baselibs#295 As mentioned: I thinks this PR at the moment is the most centralized one, but I also wrote with @castler and we agreed that a final solution should even be at a higher place (e.g. in rules_cpp), but that this is at least on step into the direction that we can provide a first coverage report to the modules do not fly blind and have no clue about there current coverage. |
| profdata_dir.mkdir(exist_ok=True) | ||
| profdata_file = profdata_dir / "target.profdata" | ||
|
|
||
| llvm_profdata = os.environ.get("LLVM_PROFDATA") |
There was a problem hiding this comment.
This could be an issue if the consumer doesn't sets it. I see reporter already has label for it, so I wonder why we don't do it for merger as well. We create a small executable script on the fly that has default label for it and we wire it to the command coverage --coverage_output_generator=//tools/coverage:merger_wrapper. Make sense?
There was a problem hiding this comment.
Makes sense. Addressed with #064db8173f96391cd4f1d68faa6f1eeb940d6a2
|
The eclipse-score/baselibs#295 is just to try things out before centralizing it. The findings are captured in eclipse-score/baselibs#289 Unfortunately, llvm-cov doesn't perform well when it comes to computing line coverage per file. If we are to adopt it in S-CORE we should find a solution for it, ideally fixing it upstream. I would also be interesting to see how this works together with lcov. Latest versions of genhtml have a differential view mode with git integration. |
LLVM_PROFDATA is never set by default anywhere, so any consumer pointing --coverage_output_generator directly at :merger had a silently broken coverage collection step. Add score_coverage_merger, mirroring score_coverage_reporter's pattern: it generates a wrapper script that resolves llvm-profdata from its own runfiles and passes it explicitly, falling back to LLVM_PROFDATA only for legacy direct-:merger consumers. The generated wrapper self-derives RUNFILES_DIR from its own path rather than trusting the env var value, since Bazel invokes --coverage_output_generator as a nested tool inside the per-test coverage action, which has its own (different) RUNFILES_DIR already set.
Same class of bug as the merger wrapper: if this wrapper is ever invoked as a nested tool from inside another action, the ambient RUNFILES_DIR would resolve paths against the wrong runfiles tree. reporter_wrapper is currently only ever invoked standalone so this was dormant, but self-derive from the script's own path first for consistency and defense-in-depth.
When coverage is built in a devcontainer (workspace at /workspace/...) and consumed on the host (at /home/user/...), or when an archive is created and unpacked elsewhere, absolute SF: paths in the LCOV file break IDE coverage gutters, SonarQube ingestion, and other tooling. Strip workspace_root prefix from all SF: entries so paths are relative to the project root, making the report portable across environments.
llvm-cov embeds absolute paths in the source-name-title div of every per-file coverage page. When a report is built in one environment (e.g. a devcontainer at /workspace/...) and viewed in another (host, unpacked archive), the displayed paths are misleading even though the intra-HTML navigation links remain functional. Post-process the HTML to strip workspace_root prefix from all source-name-title entries, making the displayed paths match the relative structure consumers see in their own environment.
|
Hi @nradakovic anything left to do from your side to get an approval? |
Summary
llvm-covonly reports files whose object files are linked into at least one test.Source files that exist in the workspace but no
cc_testpulls in silently disappear from the coverage report.This PR adds a Bazel aspect that walks the dependency graph to collect all C/C++ sources, compares them against what
llvm-covactually reported, and augments the LCOV, HTML, and text outputs with synthetic 0%-coverage entries for the missing files. With that Feature Request #6 should be resolved.The coverage pipeline (merger, reporter, justify, effective_coverage) was originally developed by Jan Schlosser in eclipse-score/communication#549.
This PR centralizes it in score_cpp_policies, adds the baseline-coverage aspect, and improves the reporter.
Approach
Why an aspect + manifest instead of patching
--instrumentation_filter?instrumentation_filtercontrols which targets get built with coverage instrumentation, butllvm-covstill only reports files whose.ois linked into a test binary. There is no Bazel-native way to surface the gap. The aspect walksdeps/srcs/implementation_depstransitively and writes a manifest of all reachable.cpp/.cc/.cxx/.cfiles. The reporter then diffs manifest vs. LCOV output.Why heuristic line counts instead of exact numbers?
Without running
llvm-covagainst actual instrumented object files (which don't exist for untested sources), we can't get exact instrumentable line counts. The heuristic (_count_instrumentable_lines) filters blank lines, comments, preprocessor directives, lone braces, and namespace declarations. All outputs explicitly label these as estimates(
~N, "estimated via heuristic") to avoid false precision. The TOTALS line insummary.txtis intentionally left untouched. Only a WARNING banner is appended.Known limitations (tracked for follow-up)
rules_ccfirst. Current heuristic line counts are estimates.constexprfunctions: These appear uncovered due to how llvm-cov handles them.What changed
coverage/defs.bzl:_collect_sources_aspect,score_instrumented_sources_manifestrule,instrumented_sources_manifestparameter onscore_coverage_reportermacrocoverage/reporter.py: LCOV augmentation, HTML augmentation (top-banner + per-file pages + detail table), text summary banner, workspace-bounds check, HTML escaping including quotescoverage/BUILD.bazel:reporter_libpy_library for testabilitytests/coverage/:uncovered.cpp/.hfixture (library with no test),reporter_test.pywith unit tests for all augmentation helpers including path-traversal rejectionConsumer usage