Merge pull request #257 from py-cov-action/remove-diff-cover

Remove diff cover

.github/workflows/release.yml

@@ -26,7 +26,7 @@ jobs:
         uses: docker/build-push-action@v5
         with:
           # See Dockerfile.build for instructions on bumping this.
-          tags: ewjoachim/python-coverage-comment-action-base:v3
+          tags: ewjoachim/python-coverage-comment-action-base:v4
           push: true
           file: Dockerfile.build
 

Dockerfile

@@ -1,5 +1,5 @@
 # See Dockerfile.build for instructions on bumping this.
-FROM ewjoachim/python-coverage-comment-action-base:v3
+FROM ewjoachim/python-coverage-comment-action-base:v4
 
 COPY coverage_comment ./coverage_comment
 RUN pip install .

Dockerfile.build

@@ -1,6 +1,6 @@
 # If you change anything here, bump the version in:
 # - Dockerfile
-# - .github/workflows/docker.yml
+# - .github/workflows/release.yml
 
 FROM python:3.11-slim
 

coverage_comment/coverage.py

@@ -3,7 +3,7 @@
 import decimal
 import json
 import pathlib
-import tempfile
+from collections.abc import Iterable
 
 from coverage_comment import log, subprocess
 
@@ -45,6 +45,12 @@ class Coverage:
     files: dict[pathlib.Path, FileCoverage]
 
 
+# The format for Diff Coverage objects may seem a little weird, because it
+# was originally copied from diff-cover schema. In order to keep the
+# compatibility for existing custom template, we kept the same format.
+# Maybe in v4, we can change it to a simpler format.
+
+
 @dataclasses.dataclass
 class FileDiffCoverage:
     path: pathlib.Path
@@ -211,57 +217,90 @@ def extract_info(data: dict, coverage_path: pathlib.Path) -> Coverage:
     )
 
 
-def get_diff_coverage_info(base_ref: str, coverage_path: pathlib.Path) -> DiffCoverage:
-    subprocess.run("git", "fetch", "--depth=1000", path=pathlib.Path.cwd())
-    subprocess.run("coverage", "xml", path=coverage_path)
-    with tempfile.NamedTemporaryFile("r") as f:
-        subprocess.run(
-            "diff-cover",
-            "coverage.xml",
-            f"--compare-branch=origin/{base_ref}",
-            f"--json-report={f.name}",
-            "--diff-range-notation=..",
-            "--quiet",
-            path=coverage_path,
+def get_diff_coverage_info(
+    added_lines: dict[pathlib.Path, list[int]], coverage: Coverage
+) -> DiffCoverage:
+    files = {}
+    total_num_lines = 0
+    total_num_violations = 0
+    num_changed_lines = 0
+
+    for path, added_lines_for_file in added_lines.items():
+        num_changed_lines += len(added_lines_for_file)
+
+        try:
+            file = coverage.files[path]
+        except KeyError:
+            continue
+
+        executed = set(file.executed_lines) & set(added_lines_for_file)
+        count_executed = len(executed)
+
+        missing = set(file.missing_lines) & set(added_lines_for_file)
+        count_missing = len(missing)
+        # Even partially covered lines are considered as covered, no line
+        # appears in both counts
+        count_total = count_executed + count_missing
+
+        total_num_lines += count_total
+        total_num_violations += count_missing
+
+        percent_covered = compute_coverage(
+            num_covered=count_executed, num_total=count_total
         )
-        diff_json = json.loads(pathlib.Path(f.name).read_text())
 
-    return extract_diff_info(diff_json)
+        files[path] = FileDiffCoverage(
+            path=path,
+            percent_covered=percent_covered,
+            violation_lines=sorted(missing),
+        )
+    final_percentage = compute_coverage(
+        num_covered=total_num_lines - total_num_violations,
+        num_total=total_num_lines,
+    )
+
+    return DiffCoverage(
+        total_num_lines=total_num_lines,
+        total_num_violations=total_num_violations,
+        total_percent_covered=final_percentage,
+        num_changed_lines=num_changed_lines,
+        files=files,
+    )
+
+
+def get_added_lines(
+    git: subprocess.Git, base_ref: str
+) -> dict[pathlib.Path, list[int]]:
+    # --unified=0 means we don't get any context lines for chunk, and we
+    # don't merge chunks. This means the headers that describe line number
+    # are always enough to derive what line numbers were added.
+    git.fetch("origin", base_ref, "--depth=1000")
+    diff = git.diff("--unified=0", "FETCH_HEAD", "--", ".")
+    return parse_diff_output(diff)
 
 
-def extract_diff_info(data) -> DiffCoverage:
+def parse_diff_output(diff: str) -> dict[pathlib.Path, list[int]]:
+    current_file: pathlib.Path | None = None
+    added_filename_prefix = "+++ b/"
+    result: dict[pathlib.Path, list[int]] = {}
+    for line in diff.splitlines():
+        if line.startswith(added_filename_prefix):
+            current_file = pathlib.Path(line.removeprefix(added_filename_prefix))
+            continue
+        if line.startswith("@@"):
+            assert current_file
+            lines = parse_line_number_diff_line(line)
+            result.setdefault(current_file, []).extend(lines)
+            continue
+
+    return result
+
+
+def parse_line_number_diff_line(line: str) -> Iterable[int]:
     """
-    {
-        "report_name": "XML",
-        "diff_name": "master...HEAD, staged and unstaged changes",
-        "src_stats": {
-            "codebase/code.py": {
-                "percent_covered": 80.0,
-                "violation_lines": [9],
-                "violations": [[9, null]],
-            }
-        },
-        "total_num_lines": 5,
-        "total_num_violations": 1,
-        "total_percent_covered": 80,
-        "num_changed_lines": 39,
-    }
+    Parse the "added" part of the line number diff text:
+        @@ -60,0 +61 @@ def compute_files(  -> [61]
+        @@ -60,0 +61,3 @@ def compute_files(  -> [61, 62, 63]
     """
-    return DiffCoverage(
-        total_num_lines=data["total_num_lines"],
-        total_num_violations=data["total_num_violations"],
-        total_percent_covered=compute_coverage(
-            data["total_num_lines"] - data["total_num_violations"],
-            data["total_num_lines"],
-        ),
-        num_changed_lines=data["num_changed_lines"],
-        files={
-            pathlib.Path(path): FileDiffCoverage(
-                path=pathlib.Path(path),
-                percent_covered=decimal.Decimal(str(file_data["percent_covered"]))
-                / decimal.Decimal("100"),
-                violation_lines=file_data["violation_lines"],
-            )
-            for path, file_data in data["src_stats"].items()
-        },
-    )
+    start, length = (int(i) for i in (line.split()[2][1:] + ",1").split(",")[:2])
+    return range(start, start + length)

coverage_comment/main.py

@@ -93,6 +93,7 @@ def action(
             config=config,
             gh=gh,
             repo_info=repo_info,
+            git=git,
         )
 
     else:
@@ -107,6 +108,7 @@ def process_pr(
     config: settings.Config,
     gh: github_client.GitHub,
     repo_info: github.RepositoryInfo,
+    git: subprocess.Git,
 ) -> int:
     log.info("Generating comment for PR")
 
@@ -115,10 +117,11 @@ def process_pr(
         coverage_path=config.COVERAGE_PATH,
     )
     base_ref = config.GITHUB_BASE_REF or repo_info.default_branch
+
+    added_lines = coverage_module.get_added_lines(git=git, base_ref=base_ref)
     diff_coverage = coverage_module.get_diff_coverage_info(
-        base_ref=base_ref, coverage_path=config.COVERAGE_PATH
+        coverage=coverage, added_lines=added_lines
     )
-
     # It only really makes sense to display a comparison with the previous
     # coverage if the PR target is the branch in which the coverage data is
     # stored, e.g. the default branch.

coverage_comment/subprocess.py

@@ -48,7 +48,7 @@ def _git(self, *args: str, env: dict[str, str] | None = None, **kwargs) -> str:
         # When setting the `env` argument to run, instead of inheriting env
         # vars from the current process, the whole environment of the
         # subprocess is whatever we pass. In other words, we can either
-        # conditionnaly pass an `env` parameter, but it's less readable,
+        # conditionally pass an `env` parameter, but it's less readable,
         # or we can always pass an `env` parameter, but in this case, we
         # need to always merge `os.environ` to it (and ensure our variables
         # have precedence)

pyproject.toml

@@ -16,7 +16,6 @@ coverage_comment = 'coverage_comment.main:main'
 [tool.poetry.dependencies]
 python = "^3.11"
 coverage = { version = "*", extras = ["toml"] }
-diff-cover = "*"
 httpx = { version = "*", extras = ["http2"] }
 Jinja2 = "*"