search

Bachmann1234 / diff_cover

Automatically find diff lines that need test coverage.
Apache License 2.0
713 stars 191 forks source link

Add expand_coverage_report option to for #270 #416

Closed MathieuLamiot closed 2 months ago

MathieuLamiot commented 3 months ago

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: https://github.com/Bachmann1234/diff_cover/issues/270#issuecomment-1017563602, 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:

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

Tests performed

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

When line 430 is not tested

                        <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

                        <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 commented 3 months ago

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

Bachmann1234 commented 3 months ago

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

Bachmann1234 commented 3 months ago

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 commented 3 months ago

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 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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

Bachmann1234 commented 2 months ago

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

Bachmann1234 commented 2 months ago

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