Repository: py-cov-action/python-coverage-comment-action (python)

Issue:
Unrelated files may appear in comment
The [diff calculation](https://github.com/py-cov-action/python-coverage-comment-action/blob/ffd1aa12992c2272b950cc6c882674e0c07bc8e8/coverage_comment/coverage.py#L296) is similar to the [two dot diff comparison](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-comparing-branches-in-pull-requests#three-dot-and-two-dot-git-diff-comparisons) and can result in files unrelated to a PR appearing in the coverage comment. This can happen if there is a merge to the base ref or default branch between the time the GHA was kicked off and this action is run.

I _think_ this is a bug and not the desired result.

Pull request:
use three dot notation to calculate diff
A tentative to reapply #532, and understand what went wrong

Fixes #529

Patch:
diff --git a/coverage_comment/coverage.py b/coverage_comment/coverage.py
index 591e9de..cd47b95 100644
--- a/coverage_comment/coverage.py
+++ b/coverage_comment/coverage.py
@@ -286,18 +286,7 @@ def get_diff_coverage_info(
     )
 
 
-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 parse_diff_output(diff: str) -> dict[pathlib.Path, list[int]]:
+def get_added_lines(diff: str) -> dict[pathlib.Path, list[int]]:
     current_file: pathlib.Path | None = None
     added_filename_prefix = "+++ b/"
     result: dict[pathlib.Path, list[int]] = {}
diff --git a/coverage_comment/github.py b/coverage_comment/github.py
index 9a06931..5612498 100644
--- a/coverage_comment/github.py
+++ b/coverage_comment/github.py
@@ -254,3 +254,27 @@ def append_to_file(content: str, filepath: pathlib.Path):
 
 def add_job_summary(content: str, github_step_summary: pathlib.Path):
     append_to_file(content=content, filepath=github_step_summary)
+
+
+def get_pr_diff(github: github_client.GitHub, repository: str, pr_number: int) -> str:
+    """
+    Get the diff of a pull request.
+    """
+    return (
+        github.repos(repository)
+        .pulls(pr_number)
+        .get(headers={"Accept": "application/vnd.github.v3.diff"})
+    )
+
+
+def get_branch_diff(
+    github: github_client.GitHub, repository: str, base_branch: str, head_branch: str
+) -> str:
+    """
+    Get the diff of branch.
+    """
+    return (
+        github.repos(repository)
+        .compare(f"{base_branch}...{head_branch}")
+        .get(headers={"Accept": "application/vnd.github.v3.diff"})
+    )
diff --git a/coverage_comment/github_client.py b/coverage_comment/github_client.py
index 92a4d9e..306edc7 100644
--- a/coverage_comment/github_client.py
+++ b/coverage_comment/github_client.py
@@ -83,10 +83,7 @@ class GitHub:
             **header_kwargs,
             **requests_kwargs,
         )
-        if bytes:
-            contents = response.content
-        else:
-            contents = response_contents(response)
+        contents = response_contents(response=response, bytes=bytes)
 
         try:
             response.raise_for_status()
@@ -103,14 +100,15 @@ class GitHub:
 
 def response_contents(
     response: httpx.Response,
+    bytes: bool,
 ) -> JsonObject | str | bytes:
+    if bytes:
+        return response.content
+
     if response.headers.get("content-type", "").startswith("application/json"):
         return response.json(object_hook=JsonObject)
-    if response.headers.get("content-type", "").startswith(
-        "application/vnd.github.raw+json"
-    ):
-        return response.text
-    return response.content
+
+    return response.text
 
 
 class JsonObject(dict):
diff --git a/coverage_comment/main.py b/coverage_comment/main.py
index 20f4646..5b679f8 100644
--- a/coverage_comment/main.py
+++ b/coverage_comment/main.py
@@ -132,7 +132,20 @@ def process_pr(
     )
     base_ref = config.GITHUB_BASE_REF or repo_info.default_branch
 
-    added_lines = coverage_module.get_added_lines(git=git, base_ref=base_ref)
+    if config.GITHUB_BRANCH_NAME:
+        diff = github.get_branch_diff(
+            github=gh,
+            repository=config.GITHUB_REPOSITORY,
+            base_branch=base_ref,
+            head_branch=config.GITHUB_BRANCH_NAME,
+        )
+    elif config.GITHUB_PR_NUMBER:
+        diff = github.get_pr_diff(
+            github=gh,
+            repository=config.GITHUB_REPOSITORY,
+            pr_number=config.GITHUB_PR_NUMBER,
+        )
+    added_lines = coverage_module.get_added_lines(diff=diff)
     diff_coverage = coverage_module.get_diff_coverage_info(
         coverage=coverage, added_lines=added_lines
     )


Test patch:
diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py
new file mode 100644
index 0000000..d6aace9
--- /dev/null
+++ b/tests/integration/conftest.py
@@ -0,0 +1,101 @@
+from __future__ import annotations
+
+import os
+import pathlib
+import subprocess
+import uuid
+
+import pytest
+
+
+@pytest.fixture
+def in_integration_env(integration_env, integration_dir):
+    curdir = os.getcwd()
+    os.chdir(integration_dir)
+    yield integration_dir
+    os.chdir(curdir)
+
+
+@pytest.fixture
+def integration_dir(tmp_path: pathlib.Path):
+    test_dir = tmp_path / "integration_test"
+    test_dir.mkdir()
+    return test_dir
+
+
+@pytest.fixture
+def file_path(integration_dir):
+    return integration_dir / "foo.py"
+
+
+@pytest.fixture
+def write_file(file_path):
+    def _(*variables):
+        content = "import os"
+        for i, var in enumerate(variables):
+            content += f"""\nif os.environ.get("{var}"):\n    {i}\n"""
+        file_path.write_text(content, encoding="utf8")
+
+    return _
+
+
+@pytest.fixture
+def run_coverage(file_path, integration_dir):
+    def _(*variables):
+        subprocess.check_call(
+            ["coverage", "run", "--parallel", file_path.name],
+            cwd=integration_dir,
+            env=os.environ | dict.fromkeys(variables, "1"),
+        )
+
+    return _
+
+
+@pytest.fixture
+def commit(integration_dir):
+    def _():
+        subprocess.check_call(
+            ["git", "add", "."],
+            cwd=integration_dir,
+        )
+        subprocess.check_call(
+            ["git", "commit", "-m", str(uuid.uuid4())],
+            cwd=integration_dir,
+            env={
+                "GIT_AUTHOR_NAME": "foo",
+                "GIT_AUTHOR_EMAIL": "foo",
+                "GIT_COMMITTER_NAME": "foo",
+                "GIT_COMMITTER_EMAIL": "foo",
+                "GIT_CONFIG_GLOBAL": "/dev/null",
+                "GIT_CONFIG_SYSTEM": "/dev/null",
+            },
+        )
+
+    return _
+
+
+@pytest.fixture
+def integration_env(integration_dir, write_file, run_coverage, commit, request):
+    subprocess.check_call(["git", "init", "-b", "main"], cwd=integration_dir)
+    # diff coverage reads the "origin/{...}" branch so we simulate an origin remote
+    subprocess.check_call(["git", "remote", "add", "origin", "."], cwd=integration_dir)
+    write_file("A", "B")
+    commit()
+
+    add_branch_mark = request.node.get_closest_marker("add_branches")
+    for additional_branch in add_branch_mark.args if add_branch_mark else []:
+        subprocess.check_call(
+            ["git", "switch", "-c", additional_branch],
+            cwd=integration_dir,
+        )
+
+    subprocess.check_call(
+        ["git", "switch", "-c", "branch"],
+        cwd=integration_dir,
+    )
+
+    write_file("A", "B", "C", "D")
+    commit()
+
+    run_coverage("A", "C")
+    subprocess.check_call(["git", "fetch", "origin"], cwd=integration_dir)
diff --git a/tests/integration/test_github.py b/tests/integration/test_github.py
index a5b99ec..39fb3c4 100644
--- a/tests/integration/test_github.py
+++ b/tests/integration/test_github.py
@@ -444,3 +444,29 @@ def test_annotations(capsys):
 ::endgroup::"""
     output = capsys.readouterr()
     assert output.err.strip() == expected
+
+
+def test_get_pr_diff(gh, session):
+    session.register(
+        "GET",
+        "/repos/foo/bar/pulls/123",
+        headers={"Accept": "application/vnd.github.v3.diff"},
+    )(text="diff --git a/foo.py b/foo.py...")
+
+    result = github.get_pr_diff(github=gh, repository="foo/bar", pr_number=123)
+
+    assert result == "diff --git a/foo.py b/foo.py..."
+
+
+def test_get_branch_diff(gh, session):
+    session.register(
+        "GET",
+        "/repos/foo/bar/compare/main...feature",
+        headers={"Accept": "application/vnd.github.v3.diff"},
+    )(text="diff --git a/foo.py b/foo.py...")
+
+    result = github.get_branch_diff(
+        github=gh, repository="foo/bar", base_branch="main", head_branch="feature"
+    )
+
+    assert result == "diff --git a/foo.py b/foo.py..."
diff --git a/tests/integration/test_main.py b/tests/integration/test_main.py
index f925e43..e98c949 100644
--- a/tests/integration/test_main.py
+++ b/tests/integration/test_main.py
@@ -1,59 +1,12 @@
 from __future__ import annotations
 
 import json
-import os
 import pathlib
-import subprocess
-import uuid
 
 import pytest
 
 from coverage_comment import main
 
-
-@pytest.fixture
-def in_integration_env(integration_env, integration_dir):
-    curdir = os.getcwd()
-    os.chdir(integration_dir)
-    yield integration_dir
-    os.chdir(curdir)
-
-
-@pytest.fixture
-def integration_dir(tmp_path: pathlib.Path):
-    test_dir = tmp_path / "integration_test"
-    test_dir.mkdir()
-    return test_dir
-
-
-@pytest.fixture
-def file_path(integration_dir):
-    return integration_dir / "foo.py"
-
-
-@pytest.fixture
-def write_file(file_path):
-    def _(*variables):
-        content = "import os"
-        for i, var in enumerate(variables):
-            content += f"""\nif os.environ.get("{var}"):\n    {i}\n"""
-        file_path.write_text(content, encoding="utf8")
-
-    return _
-
-
-@pytest.fixture
-def run_coverage(file_path, integration_dir):
-    def _(*variables):
-        subprocess.check_call(
-            ["coverage", "run", "--parallel", file_path.name],
-            cwd=integration_dir,
-            env=os.environ | dict.fromkeys(variables, "1"),
-        )
-
-    return _
-
-
 DIFF_STDOUT = """diff --git a/foo.py b/foo.py
 index 6c08c94..b65c612 100644
 --- a/foo.py
@@ -68,56 +21,6 @@ index 6c08c94..b65c612 100644
 """
 
 
-@pytest.fixture
-def commit(integration_dir):
-    def _():
-        subprocess.check_call(
-            ["git", "add", "."],
-            cwd=integration_dir,
-        )
-        subprocess.check_call(
-            ["git", "commit", "-m", str(uuid.uuid4())],
-            cwd=integration_dir,
-            env={
-                "GIT_AUTHOR_NAME": "foo",
-                "GIT_AUTHOR_EMAIL": "foo",
-                "GIT_COMMITTER_NAME": "foo",
-                "GIT_COMMITTER_EMAIL": "foo",
-                "GIT_CONFIG_GLOBAL": "/dev/null",
-                "GIT_CONFIG_SYSTEM": "/dev/null",
-            },
-        )
-
-    return _
-
-
-@pytest.fixture
-def integration_env(integration_dir, write_file, run_coverage, commit, request):
-    subprocess.check_call(["git", "init", "-b", "main"], cwd=integration_dir)
-    # diff coverage reads the "origin/{...}" branch so we simulate an origin remote
-    subprocess.check_call(["git", "remote", "add", "origin", "."], cwd=integration_dir)
-    write_file("A", "B")
-    commit()
-
-    add_branch_mark = request.node.get_closest_marker("add_branches")
-    for additional_branch in add_branch_mark.args if add_branch_mark else []:
-        subprocess.check_call(
-            ["git", "switch", "-c", additional_branch],
-            cwd=integration_dir,
-        )
-
-    subprocess.check_call(
-        ["git", "switch", "-c", "branch"],
-        cwd=integration_dir,
-    )
-
-    write_file("A", "B", "C", "D")
-    commit()
-
-    run_coverage("A", "C")
-    subprocess.check_call(["git", "fetch", "origin"], cwd=integration_dir)
-
-
 def test_action__invalid_event_name(session, push_config, in_integration_env, get_logs):
     session.register("GET", "/repos/py-cov-action/foobar")(
         json={"default_branch": "main", "visibility": "public"}
@@ -171,8 +74,8 @@ def test_action__pull_request__store_comment(
         "POST", "/repos/py-cov-action/foobar/issues/2/comments", json=checker
     )(status_code=403)
 
-    git.register("git fetch origin main --depth=1000")()
-    git.register("git diff --unified=0 FETCH_HEAD -- .")(stdout=DIFF_STDOUT)
+    # What is the diff of the PR
+    session.register("GET", "/repos/py-cov-action/foobar/pulls/2")(text=DIFF_STDOUT)
 
     result = main.action(
         config=pull_request_config(
@@ -252,8 +155,8 @@ def test_action__pull_request__store_comment_not_targeting_default(
         "POST", "/repos/py-cov-action/foobar/issues/2/comments", json=checker
     )(status_code=403)
 
-    git.register("git fetch origin foo --depth=1000")(stdout=DIFF_STDOUT)
-    git.register("git diff --unified=0 FETCH_HEAD -- .")(stdout=DIFF_STDOUT)
+    # What is the diff of the PR
+    session.register("GET", "/repos/py-cov-action/foobar/pulls/2")(text=DIFF_STDOUT)
 
     result = main.action(
         config=pull_request_config(
@@ -299,8 +202,8 @@ def test_action__pull_request__post_comment(
     # Are there already comments
     session.register("GET", "/repos/py-cov-action/foobar/issues/2/comments")(json=[])
 
-    git.register("git fetch origin main --depth=1000")()
-    git.register("git diff --unified=0 FETCH_HEAD -- .")(stdout=DIFF_STDOUT)
+    # What is the diff of the PR
+    session.register("GET", "/repos/py-cov-action/foobar/pulls/2")(text=DIFF_STDOUT)
 
     comment = None
 
@@ -346,8 +249,11 @@ def test_action__push__non_default_branch(
     session.register("GET", "/repos/py-cov-action/foobar")(
         json={"default_branch": "main", "visibility": "public"}
     )
-    git.register("git fetch origin main --depth=1000")(stdout=DIFF_STDOUT)
-    git.register("git diff --unified=0 FETCH_HEAD -- .")(stdout=DIFF_STDOUT)
+
+    # What is the diff of the `other` branch
+    session.register("GET", "/repos/py-cov-action/foobar/compare/main...other")(
+        text=DIFF_STDOUT
+    )
 
     payload = json.dumps({"coverage": 30.00})
     # There is an existing badge in this test, allowing to test the coverage evolution
@@ -435,8 +341,10 @@ def test_action__push__non_default_branch__no_pr(
     session.register("GET", "/repos/py-cov-action/foobar")(
         json={"default_branch": "main", "visibility": "public"}
     )
-    git.register("git fetch origin main --depth=1000")(stdout=DIFF_STDOUT)
-    git.register("git diff --unified=0 FETCH_HEAD -- .")(stdout=DIFF_STDOUT)
+    # What is the diff of the `other` branch
+    session.register("GET", "/repos/py-cov-action/foobar/compare/main...other")(
+        text=DIFF_STDOUT
+    )
 
     payload = json.dumps({"coverage": 30.00})
     # There is an existing badge in this test, allowing to test the coverage evolution
@@ -499,8 +407,8 @@ def test_action__pull_request__force_store_comment(
         "/repos/py-cov-action/foobar/contents/data.json",
     )(text=payload, headers={"content-type": "application/vnd.github.raw+json"})
 
-    git.register("git fetch origin main --depth=1000")()
-    git.register("git diff --unified=0 FETCH_HEAD -- .")(stdout=DIFF_STDOUT)
+    # What is the diff of the PR
+    session.register("GET", "/repos/py-cov-action/foobar/pulls/2")(text=DIFF_STDOUT)
 
     result = main.action(
         config=pull_request_config(FORCE_WORKFLOW_RUN=True, GITHUB_OUTPUT=output_file),
@@ -530,8 +438,8 @@ def test_action__pull_request__post_comment__no_marker(
         "/repos/py-cov-action/foobar/contents/data.json",
     )(status_code=404)
 
-    git.register("git fetch origin main --depth=1000")()
-    git.register("git diff --unified=0 FETCH_HEAD -- .")(stdout=DIFF_STDOUT)
+    # What is the diff of the PR
+    session.register("GET", "/repos/py-cov-action/foobar/pulls/2")(text=DIFF_STDOUT)
 
     result = main.action(
         config=pull_request_config(COMMENT_TEMPLATE="""foo"""),
@@ -555,8 +463,8 @@ def test_action__pull_request__annotations(
         "/repos/py-cov-action/foobar/contents/data.json",
     )(status_code=404)
 
-    git.register("git fetch origin main --depth=1000")()
-    git.register("git diff --unified=0 FETCH_HEAD -- .")(stdout=DIFF_STDOUT)
+    # What is the diff of the PR
+    session.register("GET", "/repos/py-cov-action/foobar/pulls/2")(text=DIFF_STDOUT)
 
     # Who am I
     session.register("GET", "/user")(json={"login": "foo"})
@@ -597,8 +505,8 @@ def test_action__pull_request__post_comment__template_error(
         "/repos/py-cov-action/foobar/contents/data.json",
     )(status_code=404)
 
-    git.register("git fetch origin main --depth=1000")()
-    git.register("git diff --unified=0 FETCH_HEAD -- .")(stdout=DIFF_STDOUT)
+    # What is the diff of the PR
+    session.register("GET", "/repos/py-cov-action/foobar/pulls/2")(text=DIFF_STDOUT)
 
     result = main.action(
         config=pull_request_config(COMMENT_TEMPLATE="""{%"""),
diff --git a/tests/unit/test_coverage.py b/tests/unit/test_coverage.py
index 621219d..c37ed50 100644
--- a/tests/unit/test_coverage.py
+++ b/tests/unit/test_coverage.py
@@ -304,17 +304,6 @@ def test_get_diff_coverage_info(make_coverage_obj, added_lines, update_obj, expe
     assert result == expected
 
 
-def test_get_added_lines(git):
-    diff = (
-        """+++ b/README.md\n@@ -1,2 +1,3 @@\n-# coverage-comment\n+coverage-comment\n"""
-    )
-    git.register("git fetch origin main --depth=1000")()
-    git.register("git diff --unified=0 FETCH_HEAD -- .")(stdout=diff)
-    assert coverage.get_added_lines(git=git, base_ref="main") == {
-        pathlib.Path("README.md"): [1, 2, 3]
-    }
-
-
 @pytest.mark.parametrize(
     "line_number_diff_line, expected",
     [
@@ -327,7 +316,7 @@ def test_parse_line_number_diff_line(git, line_number_diff_line, expected):
     assert result == expected
 
 
-def test_parse_diff_output(git):
+def test_get_added_lines(git):
     diff = """diff --git a/action.yml b/action.yml
 deleted file mode 100644
 index 42249d1..0000000
@@ -364,14 +353,14 @@ rename from coverage_comment/annotations.py
 rename to coverage_comment/annotations2.py
 """
     git.register("git fetch origin main --depth=1000")()
-    git.register("git diff --unified=0 FETCH_HEAD -- .")(stdout=diff)
-    assert coverage.parse_diff_output(diff=diff) == {
+    git.register("git diff --unified=0 FETCH_HEAD...HEAD")(stdout=diff)
+    assert coverage.get_added_lines(diff=diff) == {
         pathlib.Path("README.md"): [1, 3, 4, 5, 6],
         pathlib.Path("foo.txt"): [1],
     }
 
 
-def test_parse_diff_output__error(git):
+def test_get_added_lines__error(git):
     diff = """
 @@ -0,0 +1,1 @@
 +name: Python Coverage Comment
@@ -379,6 +368,6 @@ diff --git a/README.md b/README.md
 index 1f1d9a4..e69de29 100644
 """
     git.register("git fetch origin main --depth=1000")()
-    git.register("git diff --unified=0 FETCH_HEAD -- .")(stdout=diff)
+    git.register("git diff --unified=0 FETCH_HEAD...HEAD")(stdout=diff)
     with pytest.raises(ValueError):
-        coverage.parse_diff_output(diff=diff)
+        coverage.get_added_lines(diff=diff)
diff --git a/tests/unit/test_github_client.py b/tests/unit/test_github_client.py
index 3f07b27..6de9c38 100644
--- a/tests/unit/test_github_client.py
+++ b/tests/unit/test_github_client.py
@@ -79,4 +79,4 @@ def test_github_client__get_error_non_json(session, gh):
     with pytest.raises(github_client.ApiError) as exc_info:
         gh.repos.get()
 
-    assert str(exc_info.value) == "b'{foobar'"
+    assert str(exc_info.value) == "{foobar"