Add expand_coverage_report option to for #270

[MathieuLamiot]

Description

Issue

This PR provides a work-around that fixes #270
The issue reported there is that diff data contains all modified lines, while most coverage tools report by statement and not lines.
As an example, let's take this code change:

429        if worker is None:
430            metrics.UNEXPECTED_RETURN_VALUE_COUNTER.labels(
- 431                code=self._UNKNOWN_WORKER_RESPONSE["returnvalue"]["code"],
+ 431                code=self._UNKNOWN_WORKER_RESPONSE["code"],
432                method=request.method,
433                endpoint=request.path,
434            ).inc()
435            return Response(self._UNKNOWN_WORKER_RESPONSE)

Diff file will report line 431 as being changed.
However, most coverage tools will report lines 429, 430 and 435 being hit or not by test, but nothing for line 431.

As a result, the Violation reporters do not report line 431 has a violation, even if it does not run during tests.
But diff reports don't report line 430 has being changed. So in the end, this changes goes silent in the diff-cover analysis.

Fix implemented

Following this suggestion: #270 (comment), it seems most coverage reports use the line opening a statement as the reference. Also, untested statements are reported in the coverage files, with 0 hits (they are not just missing).

The implemented solution is to:

• add an option --expand-coverage-report (not on by default because this workaround might not work in all situations, to be further tested with different coverage reporters and languages.)
• When the option is set, all line numbers not listed in the XML reports are extrapolated from the previous line reported.

In the example above, line 431 is not in coverage.xml. But line 430 is, so we add line 431 in the data extracted from the XML report, with the same number of hits as 430.

Risks & limitations

• The option is only implemented for XML reports.
• This fix works as expected under the assumptions that:
  • The coverage tool reports untested statements too, with hits set to 0.
  • The coverage tool reports statements based on the opening line.
• Blank lines/empty lines might be reported?
• There are no unit/integration tests

Tests performed

I tested this PR on the example presented above (so in Python), and with other changes covered in the same file ; with:

• pytest --cov=. --cov-report=xml
• diff-cover coverage.xml --compare-branch=origin/develop --markdown-report diff-cover-report.md --exclude 'test*.py' --fail-under=50 --expand_coverage_report

When line 430 is not tested

• coverage.xml contains

						<line number="424" hits="0"/>
						<line number="426" hits="1"/>
						<line number="427" hits="1"/>
						<line number="428" hits="1"/>
						<line number="429" hits="1"/>
						<line number="430" hits="0"/>
						<line number="435" hits="0"/>
						<line number="436" hits="1"/>
						<line number="438" hits="1"/>
						<line number="439" hits="1"/>

and diff-coverage returns:

---

Diff Coverage
Diff: origin/develop...HEAD, staged and unstaged changes

saas/views.py (95.5%): Missing lines 431

Total: 22 lines
Missing: 1 line
Coverage: 95%

The markdown:

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

• saas/views.py (95.5%): Missing lines 431

Summary

• Total: 22 lines
• Missing: 1 line
• Coverage: 95%

saas/views.py

Lines 427-435

  427         params["id"] = job_id
  428         worker = settings.WORKERS.get(worker_name.lower())
  429         if worker is None:
  430             metrics.UNEXPECTED_RETURN_VALUE_COUNTER.labels(
! 431                 code=self._UNKNOWN_WORKER_RESPONSE["code"],
  432                 method=request.method,
  433                 endpoint=request.path,
  434             ).inc()
  435             return Response(self._UNKNOWN_WORKER_RESPONSE)

---

When line 431 is tested

• coverage.xml contains

						<line number="424" hits="0"/>
						<line number="426" hits="1"/>
						<line number="427" hits="1"/>
						<line number="428" hits="1"/>
						<line number="429" hits="1"/>
						<line number="430" hits="1"/>
						<line number="435" hits="0"/>
						<line number="436" hits="1"/>
						<line number="438" hits="1"/>
						<line number="439" hits="1"/>

and diff-coverage returns:

---

Diff Coverage
Diff: origin/develop...HEAD, staged and unstaged changes

saas/views.py (100%)

Total: 22 lines
Missing: 0 lines
Coverage: 100%

The markdown:

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

• saas/views.py (100%)

Summary

• Total: 22 lines
• Missing: 0 lines
• Coverage: 100%

[m-aciek]

I would add it as a default, without a flag. I think with the assumptions it will always be correct.

[Bachmann1234]

I'm gonna need a bit of time before I get to this. But I see it. Just a busy time this week.

[Bachmann1234]

This is pretty interesting. I feel a bit iffy about trying to interpret data not in the report. But this seems sensible.

The flag should be documented in the readme
and ill need a few tests before I can merge this

[MathieuLamiot]

Thank you for the feedback. I won't have time right now for the test but I'll try to do something about this in the upcoming weeks for sure!

[MathieuLamiot]

@Bachmann1234 I added a readme paragraph, unit & integration tests.
I also changed the param name to use - instead of _ to comply with other parameters.

I also added .venv to .gitignore. I hope it's OK

Let me know what you think! Also, can you make the CI run? the verify.sh is passing locally but just to be sure

[Bachmann1234]

Annoyingly my readme formatting check had some nitpicks, I made the suggestions that I think will fix these.

Pr looks good, once we get this merged ill try to get it out there asap.

[MathieuLamiot]

Good catch, thanks. It should be good for the 2 feedbacks you shared now 👍

[Bachmann1234]

Alright. Let's merge this in. I'll get it released in the next day or so

[Bachmann1234]

Thank you! Try out https://pypi.org/project/diff-cover/9.2.0/