Skip to content

Support PHPUnit clover reports (silently reported as zero coverage) - #617

Merged
Bachmann1234 merged 2 commits into
Bachmann1234:mainfrom
Eljees:fix/272-phpunit-clover
Aug 8, 2026
Merged

Support PHPUnit clover reports (silently reported as zero coverage)#617
Bachmann1234 merged 2 commits into
Bachmann1234:mainfrom
Eljees:fix/272-phpunit-clover

Conversation

@Eljees

@Eljees Eljees commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Fixes #272.

A clover.xml written by PHPUnit comes out as zero coverage, without an error. Two independent reasons:

  1. Format detection. XmlCoverageReporter recognises Clover by .[@clover] on the root node. Atlassian Clover writes that attribute; PHPUnit writes a bare <coverage generated="…">, so the document falls through to the Cobertura branch, which finds nothing and returns an empty set instead of failing.
  2. File lookup. _get_src_path_line_nodes_clover() matches on file/@path. PHPUnit identifies files with @name, so even once detection is fixed the lookup finds nothing.

Detection now also accepts a document containing file/line[@num][@count] — the element shape both dialects always produce — and the file lookup falls back to @name, comparing normalised paths and accepting a path that ends with the requested source path (PHPUnit writes absolute paths).

Test added alongside the existing clover tests: a PHPUnit-shaped report whose covered lines are found. It fails on main (empty result) and passes with the fix.

AI-assisted (LLM used for drafting and for running the checks); the analysis and the runs are mine.

# Loop through the files that contain the xml roots
for i, xml_document in enumerate(self._xml_roots):
if xml_document.findall(".[@clover]"):
if self._is_clover_report(xml_document):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should not have to recompute this for every file.

Perhaps we just compute a mapping in init so we can just turn this check into a dict lookup rather then re-searchign though the xml file?

Comment on lines 149 to 151
for file_tree in files:
lines.append(file_tree.findall('./line[@type="stmt"]'))
lines.append(file_tree.findall('./line[@type="cond"]'))

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be clear, I know this was not in your changes, but I think it will prove important for ... honeslty both php clover and other clover reports

Two points from the review of Bachmann1234#617.

Detecting the report format searched the whole document, and it was done
once per source file inside _cache_file. The answer cannot change after
load, so classify each root once in __init__ and look the answer up.
This also folds in the JaCoCo probe, which had the same shape.

Clover marks an executable line as method, stmt or cond. PHPUnit emits
method for the declaration line of every function it measured, and
leaving that type out reported those lines as unmeasured. This part is
not specific to PHPUnit - it applies to any Clover writer.
@Eljees

Eljees commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Both fair, and the second one is a real gap rather than a nit — thanks.

On the per-file re-scan. You're right, and it's broader than the clover check I added: the whole dispatch in _cache_file (.[@clover] / .[@name] / fall through to cobertura) is re-evaluated for every source path, so each file walks all the XML roots again. I'll classify each root once and turn the dispatch into an index lookup — roughly a self._root_formats = [self._detect_format(root) for root in self._xml_roots] built where self._xml_roots is set, with _cache_file reading self._root_formats[i].

There's a second re-scan in the same path that I'd like to fix in the same pass: get_src_path_line_nodes_clover walks every .//file per source path. One caveat if you want it as a plain dict: the match isn't equality, it's a suffix test (…endswith(f"/{src_path}")), which is what makes the no-path-attribute PHPUnit reports resolve at all. So I'd build a mapping keyed by relative path, plus a basename → candidates mapping for the suffix case, which keeps it O(1) in the common case and O(few) in the fallback. Say the word if you'd rather I keep this PR to the format detection and do the lookup table separately.

On @type="method". Agreed, and it should go in here — dropping those lines is the same class of bug this PR is about. PHPUnit emits stmt, cond and method lines, and we only collect the first two, so a method declaration line reads as unmeasured; a diff that only touches a signature shows nothing covered. For method lines count is the number of times the method was entered, so it maps onto the existing hit/miss model with no special casing — it's one more findall. I'll add it with a test that a method line is reported as measured, and I'll note in the PR description that this changes reported numbers for existing Clover users, since signature lines move from unmeasured to measured.

@Bachmann1234

Copy link
Copy Markdown
Owner

alrighty, lets get this out!

@Bachmann1234
Bachmann1234 merged commit 5fc9e82 into Bachmann1234:main Aug 8, 2026
14 checks passed
@Bachmann1234

Copy link
Copy Markdown
Owner

Released as version 10.5.0 https://pypi.org/project/diff-cover/10.5.0/

thanks for the pr!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The script will fail If no string 'clover' in clover xml

2 participants