From 58e0ed5f66ed3716778e44ee7fe1019d8a5fdc4e Mon Sep 17 00:00:00 2001 From: Joachim Jablon Date: Sat, 19 Aug 2023 09:06:26 +0200 Subject: [PATCH 1/9] Fix imprecision in error message --- coverage_comment/main.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coverage_comment/main.py b/coverage_comment/main.py index b5fa459b..ed0721c9 100644 --- a/coverage_comment/main.py +++ b/coverage_comment/main.py @@ -64,7 +64,7 @@ def action( event_name = config.GITHUB_EVENT_NAME if event_name not in {"pull_request", "push", "workflow_run"}: log.error( - 'This action has only been designed to work for "pull_request", "branch" ' + 'This action has only been designed to work for "pull_request", "push" ' f'or "workflow_run" actions, not "{event_name}". Because there are security ' "implications. If you have a different usecase, please open an issue, " "we'll be glad to add compatibility." From 52f1ded91aa3f4e0be5823e07a0ea0a96d9a3151 Mon Sep 17 00:00:00 2001 From: Joachim Jablon Date: Sun, 3 Sep 2023 03:07:20 +0200 Subject: [PATCH 2/9] Introduce the activity module for decoupling the routing logic from the glue code --- coverage_comment/activity.py | 28 ++++++++++++++++++++++++++++ tests/unit/test_activity.py | 25 +++++++++++++++++++++++++ 2 files changed, 53 insertions(+) create mode 100644 coverage_comment/activity.py create mode 100644 tests/unit/test_activity.py diff --git a/coverage_comment/activity.py b/coverage_comment/activity.py new file mode 100644 index 00000000..230a6e39 --- /dev/null +++ b/coverage_comment/activity.py @@ -0,0 +1,28 @@ +""" +This module is responsible for identifying what the action should be doing +based on the github event type and repository. + +The code in main should be as straightforward as possible, we're offloading +the branching logic to this module. +""" + + +class ActivityNotFound(Exception): + pass + + +def find_activity( + event_name: str, + is_default_branch: bool, +) -> str: + """Find the activity to perform based on the event type and payload.""" + if event_name == "workflow_run": + return "post_comment" + + if event_name == "push" and is_default_branch: + return "save_coverage_data_files" + + if event_name not in {"pull_request", "push"}: + raise ActivityNotFound + + return "process_pr" diff --git a/tests/unit/test_activity.py b/tests/unit/test_activity.py new file mode 100644 index 00000000..cfb37d7a --- /dev/null +++ b/tests/unit/test_activity.py @@ -0,0 +1,25 @@ +import pytest + +from coverage_comment import activity + + +@pytest.mark.parametrize( + "event_name, is_default_branch, expected_activity", + [ + ("workflow_run", True, "post_comment"), + ("push", True, "save_coverage_data_files"), + ("push", False, "process_pr"), + ("pull_request", True, "process_pr"), + ("pull_request", False, "process_pr"), + ], +) +def test_find_activity(event_name, is_default_branch, expected_activity): + result = activity.find_activity( + event_name=event_name, is_default_branch=is_default_branch + ) + assert result == expected_activity + + +def test_find_activity_not_found(): + with pytest.raises(activity.ActivityNotFound): + activity.find_activity(event_name="not_found", is_default_branch=False) From 3b7d4e1fdf53f28a06acd812d3a026dea597db8d Mon Sep 17 00:00:00 2001 From: Joachim Jablon Date: Sun, 3 Sep 2023 03:08:59 +0200 Subject: [PATCH 3/9] get_pr_number_from_workflow_run > get_branch_from_workflow_run + find_pr_for_branch --- coverage_comment/github.py | 65 ++++++++++++++------------ tests/integration/test_github.py | 78 ++++++++++++++++++-------------- 2 files changed, 78 insertions(+), 65 deletions(-) diff --git a/coverage_comment/github.py b/coverage_comment/github.py index 5653a189..14b8fdb6 100644 --- a/coverage_comment/github.py +++ b/coverage_comment/github.py @@ -1,5 +1,4 @@ import dataclasses -import functools import io import json import pathlib @@ -73,47 +72,53 @@ def download_artifact( raise NoArtifact(f"File named {filename} not found in artifact {artifact_name}") -def get_pr_number_from_workflow_run( +def get_branch_from_workflow_run( github: github_client.GitHub, repository: str, run_id: int -) -> int: - # It's quite horrendous to access the PR number from a workflow run, - # especially when it's not the "pull_request" workflow run itself but a - # "workflow_run" workflow run that runs after the "pull_request" workflow - # run. - # - # 1. We need the user to give us access to the "pull_request" workflow run - # id. That's why we request to be sent the following as input: - # GITHUB_PR_RUN_ID: ${{ github.event.workflow_run.id }} - # 2. From that run, we get the corresponding branch, and the owner of the branch - # 3. We list open PRs that have that branch as head branch. There should be only - # one. - # 4. If there's no open PRs, we look at all PRs. We take the most recently - # updated one - +) -> tuple[str, str]: repo_path = github.repos(repository) run = repo_path.actions.runs(run_id).get() branch = run.head_branch - repo_name = run.head_repository.full_name - full_branch = f"{repo_name}:{branch}" - get_prs = functools.partial( - repo_path.pulls.get, - head=full_branch, - sort="updated", - direction="desc", - ) + owner = run.head_repository.owner.login + return owner, branch + + +def find_pr_for_branch( + github: github_client.GitHub, repository: str, owner: str, branch: str +) -> int: + # The full branch is in the form of "owner:branch" as specified in + # https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#list-pull-requests + # but it seems to also work with "owner/repo:branch" + + full_branch = f"{owner}:{branch}" + + common_kwargs = {"head": full_branch, "sort": "updated", "direction": "desc"} try: - return next(iter(pr.number for pr in get_prs(state="open"))) + return next( + iter( + pr.number + for pr in github.repos(repository).pulls.get( + state="open", **common_kwargs + ) + ) + ) except StopIteration: pass - log.info(f"No open PR found for branch {full_branch}, defaulting to all PRs") + log.info(f"No open PR found for branch {branch}, defaulting to all PRs") try: - return next(iter(pr.number for pr in get_prs(state="all"))) + return next( + iter( + pr.number + for pr in github.repos(repository).pulls.get( + state="all", **common_kwargs + ) + ) + ) except StopIteration: - raise CannotDeterminePR(f"No open PR found for branch {full_branch}") + raise CannotDeterminePR(f"No open PR found for branch {branch}") -def get_my_login(github: github_client.GitHub): +def get_my_login(github: github_client.GitHub) -> str: try: response = github.user.get() except github_client.Forbidden: diff --git a/tests/integration/test_github.py b/tests/integration/test_github.py index f43c20e4..7cfc62c0 100644 --- a/tests/integration/test_github.py +++ b/tests/integration/test_github.py @@ -107,14 +107,24 @@ def test_download_artifact__no_file(gh, session, zip_bytes): ) -def test_get_pr_number_from_workflow_run(gh, session): +def test_get_branch_from_workflow_run(gh, session): json = { "head_branch": "other", - "head_repository": {"full_name": "someone/repo-name"}, + "head_repository": {"owner": {"login": "someone"}}, } session.register("GET", "/repos/foo/bar/actions/runs/123")(json=json) + + owner, branch = github.get_branch_from_workflow_run( + github=gh, repository="foo/bar", run_id=123 + ) + + assert owner == "someone" + assert branch == "other" + + +def test_find_pr_for_branch(gh, session): params = { - "head": "someone/repo-name:other", + "head": "someone:other", "sort": "updated", "direction": "desc", "state": "open", @@ -123,58 +133,56 @@ def test_get_pr_number_from_workflow_run(gh, session): json=[{"number": 456}] ) - result = github.get_pr_number_from_workflow_run( - github=gh, repository="foo/bar", run_id=123 + result = github.find_pr_for_branch( + github=gh, repository="foo/bar", owner="someone", branch="other" ) assert result == 456 -def test_get_pr_number_from_workflow_run__no_open_pr(gh, session): - json = { - "head_branch": "other", - "head_repository": {"full_name": "someone/repo-name"}, - } - session.register("GET", "/repos/foo/bar/actions/runs/123")(json=json) +def test_find_pr_for_branch__no_open_pr(gh, session): params = { - "head": "someone/repo-name:other", + "head": "someone:other", "sort": "updated", "direction": "desc", } - session.register("GET", "/repos/foo/bar/pulls", params=params | {"state": "open"})( - json=[] - ) - session.register("GET", "/repos/foo/bar/pulls", params=params | {"state": "all"})( - json=[{"number": 456}] - ) + session.register( + "GET", + "/repos/foo/bar/pulls", + params=params | {"state": "open"}, + )(json=[]) + session.register( + "GET", + "/repos/foo/bar/pulls", + params=params | {"state": "all"}, + )(json=[{"number": 456}]) - result = github.get_pr_number_from_workflow_run( - github=gh, repository="foo/bar", run_id=123 + result = github.find_pr_for_branch( + github=gh, repository="foo/bar", owner="someone", branch="other" ) assert result == 456 -def test_get_pr_number_from_workflow_run__no_pr(gh, session): - json = { - "head_branch": "other", - "head_repository": {"full_name": "someone/repo-name"}, - } - session.register("GET", "/repos/foo/bar/actions/runs/123")(json=json) +def test_find_pr_for_branch__no_pr(gh, session): params = { - "head": "someone/repo-name:other", + "head": "someone:other", "sort": "updated", "direction": "desc", } - session.register("GET", "/repos/foo/bar/pulls", params=params | {"state": "open"})( - json=[] - ) - session.register("GET", "/repos/foo/bar/pulls", params=params | {"state": "all"})( - json=[] - ) + session.register( + "GET", + "/repos/foo/bar/pulls", + params=params | {"state": "open"}, + )(json=[]) + session.register( + "GET", + "/repos/foo/bar/pulls", + params=params | {"state": "all"}, + )(json=[]) with pytest.raises(github.CannotDeterminePR): - github.get_pr_number_from_workflow_run( - github=gh, repository="foo/bar", run_id=123 + github.find_pr_for_branch( + github=gh, repository="foo/bar", owner="someone", branch="other" ) From 641fe038a21e9d55a84d1981280bc312ace3807d Mon Sep 17 00:00:00 2001 From: Joachim Jablon Date: Sun, 3 Sep 2023 03:09:19 +0200 Subject: [PATCH 4/9] Add comments for confusing settings --- coverage_comment/settings.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/coverage_comment/settings.py b/coverage_comment/settings.py index 94aadce1..4610d891 100644 --- a/coverage_comment/settings.py +++ b/coverage_comment/settings.py @@ -33,9 +33,15 @@ def str_to_bool(value: str) -> bool: class Config: """This object defines the environment variables""" + # A branch name, not a fully-formed ref. For example, `main`. GITHUB_BASE_REF: str GITHUB_TOKEN: str = dataclasses.field(repr=False) GITHUB_REPOSITORY: str + # > The ref given is fully-formed, meaning that for branches the format is + # > `refs/heads/`, for pull requests it is + # > `refs/pull//merge`, and for tags it is `refs/tags/`. + # > For example, `refs/heads/feature-branch-1`. + # (from https://docs.github.com/en/actions/learn-github-actions/variables#default-environment-variables ) GITHUB_REF: str GITHUB_EVENT_NAME: str GITHUB_PR_RUN_ID: int | None From 16d1b36a19e133d28dda8cdfc80ab59e39c420b8 Mon Sep 17 00:00:00 2001 From: Joachim Jablon Date: Sun, 3 Sep 2023 03:09:36 +0200 Subject: [PATCH 5/9] Add GITHUB_BRANCH_NAME --- coverage_comment/settings.py | 7 +++++++ tests/unit/test_settings.py | 11 +++++++++++ 2 files changed, 18 insertions(+) diff --git a/coverage_comment/settings.py b/coverage_comment/settings.py index 4610d891..21a55a8b 100644 --- a/coverage_comment/settings.py +++ b/coverage_comment/settings.py @@ -125,6 +125,13 @@ def GITHUB_PR_NUMBER(self) -> int | None: return int(self.GITHUB_REF.split("/")[2]) return None + @property + def GITHUB_BRANCH_NAME(self) -> str | None: + # "refs/head/my_branch_name" + if "heads" in self.GITHUB_REF: + return self.GITHUB_REF.split("/", 2)[2] + return None + # We need to type environ as a MutableMapping because that's what # os.environ is, and just saying `dict[str, str]` is not enough to make # mypy happy diff --git a/tests/unit/test_settings.py b/tests/unit/test_settings.py index edab1716..d45f6ba9 100644 --- a/tests/unit/test_settings.py +++ b/tests/unit/test_settings.py @@ -130,6 +130,17 @@ def test_config__GITHUB_PR_NUMBER(config, github_ref, github_pr_number): assert config(GITHUB_REF=github_ref).GITHUB_PR_NUMBER == github_pr_number +@pytest.mark.parametrize( + "github_ref, github_branch_name", + [ + ("refs/pull/2/merge", None), + ("refs/heads/a/b", "a/b"), + ], +) +def test_config__GITHUB_BRANCH_NAME(config, github_ref, github_branch_name): + assert config(GITHUB_REF=github_ref).GITHUB_BRANCH_NAME == github_branch_name + + def test_config__from_environ__error(): with pytest.raises(ValueError): settings.Config.from_environ({"COMMENT_FILENAME": "/a"}) From 9758f02202303c6bd4b86c104ac61838bf89f387 Mon Sep 17 00:00:00 2001 From: Joachim Jablon Date: Sun, 3 Sep 2023 03:09:59 +0200 Subject: [PATCH 6/9] Reorganize main using activity module --- coverage_comment/main.py | 136 ++++++++++++--------- tests/integration/test_main.py | 211 +++++++++++++++++++++++++++++---- tests/unit/test_main.py | 22 ---- 3 files changed, 268 insertions(+), 101 deletions(-) diff --git a/coverage_comment/main.py b/coverage_comment/main.py index ed0721c9..a3de74f0 100644 --- a/coverage_comment/main.py +++ b/coverage_comment/main.py @@ -5,6 +5,7 @@ import httpx +from coverage_comment import activity as activity_module from coverage_comment import annotations, comment_file, communication from coverage_comment import coverage as coverage_module from coverage_comment import ( @@ -60,9 +61,17 @@ def action( git: subprocess.Git, ) -> int: log.debug(f"Operating on {config.GITHUB_REF}") - + gh = github_client.GitHub(session=github_session) event_name = config.GITHUB_EVENT_NAME - if event_name not in {"pull_request", "push", "workflow_run"}: + repo_info = github.get_repository_info( + github=gh, repository=config.GITHUB_REPOSITORY + ) + try: + activity = activity_module.find_activity( + event_name=event_name, + is_default_branch=repo_info.is_default_branch(ref=config.GITHUB_REF), + ) + except activity_module.ActivityNotFound: log.error( 'This action has only been designed to work for "pull_request", "push" ' f'or "workflow_run" actions, not "{event_name}". Because there are security ' @@ -71,52 +80,49 @@ def action( ) return 1 - if event_name in {"pull_request", "push"}: - raw_coverage, coverage = coverage_module.get_coverage_info( - merge=config.MERGE_COVERAGE_FILES, coverage_path=config.COVERAGE_PATH + if activity == "save_coverage_data_files": + return save_coverage_data_files( + config=config, + git=git, + http_session=http_session, + repo_info=repo_info, + ) + + elif activity == "process_pr": + return process_pr( + config=config, + gh=gh, + repo_info=repo_info, ) - if event_name == "pull_request": - diff_coverage = coverage_module.get_diff_coverage_info( - base_ref=config.GITHUB_BASE_REF, coverage_path=config.COVERAGE_PATH - ) - if config.ANNOTATE_MISSING_LINES: - annotations.create_pr_annotations( - annotation_type=config.ANNOTATION_TYPE, diff_coverage=diff_coverage - ) - return generate_comment( - config=config, - coverage=coverage, - diff_coverage=diff_coverage, - github_session=github_session, - ) - else: - # event_name == "push" - return save_coverage_data_files( - config=config, - coverage=coverage, - raw_coverage_data=raw_coverage, - github_session=github_session, - git=git, - http_session=http_session, - ) else: - # event_name == "workflow_run" + # activity == "post_comment": return post_comment( config=config, - github_session=github_session, + gh=gh, ) -def generate_comment( +def process_pr( config: settings.Config, - coverage: coverage_module.Coverage, - diff_coverage: coverage_module.DiffCoverage, - github_session: httpx.Client, + gh: github_client.GitHub, + repo_info: github.RepositoryInfo, ) -> int: log.info("Generating comment for PR") - gh = github_client.GitHub(session=github_session) + _, coverage = coverage_module.get_coverage_info( + merge=config.MERGE_COVERAGE_FILES, + coverage_path=config.COVERAGE_PATH, + ) + base_ref = config.GITHUB_BASE_REF or repo_info.default_branch + diff_coverage = coverage_module.get_diff_coverage_info( + base_ref=base_ref, coverage_path=config.COVERAGE_PATH + ) + + if config.ANNOTATE_MISSING_LINES: + annotations.create_pr_annotations( + annotation_type=config.ANNOTATION_TYPE, diff_coverage=diff_coverage + ) previous_coverage_data_file = storage.get_datafile_contents( github=gh, @@ -152,21 +158,35 @@ def generate_comment( ) return 1 - assert config.GITHUB_PR_NUMBER - github.add_job_summary( content=comment, github_step_summary=config.GITHUB_STEP_SUMMARY ) + pr_number: int | None = config.GITHUB_PR_NUMBER + if not pr_number: + # If we don't have a PR number, we're launched from a push event, + # so we need to find the PR number from the branch name + assert config.GITHUB_BRANCH_NAME + try: + pr_number = github.find_pr_for_branch( + github=gh, + # A push event cannot be initiated from a forked repository + repository=config.GITHUB_REPOSITORY, + owner=config.GITHUB_REPOSITORY.split("/")[0], + branch=config.GITHUB_BRANCH_NAME, + ) + except github.CannotDeterminePR: + pr_number = None + try: - if config.FORCE_WORKFLOW_RUN: + if config.FORCE_WORKFLOW_RUN or not pr_number: raise github.CannotPostComment github.post_comment( github=gh, me=github.get_my_login(github=gh), repository=config.GITHUB_REPOSITORY, - pr_number=config.GITHUB_PR_NUMBER, + pr_number=pr_number, contents=comment, marker=template.MARKER, ) @@ -193,21 +213,29 @@ def generate_comment( return 0 -def post_comment(config: settings.Config, github_session: httpx.Client) -> int: +def post_comment( + config: settings.Config, + gh: github_client.GitHub, +) -> int: log.info("Posting comment to PR") if not config.GITHUB_PR_RUN_ID: log.error("Missing input GITHUB_PR_RUN_ID. Please consult the documentation.") return 1 - gh = github_client.GitHub(session=github_session) me = github.get_my_login(github=gh) log.info(f"Search for PR associated with run id {config.GITHUB_PR_RUN_ID}") + owner, branch = github.get_branch_from_workflow_run( + github=gh, + run_id=config.GITHUB_PR_RUN_ID, + repository=config.GITHUB_REPOSITORY, + ) try: - pr_number = github.get_pr_number_from_workflow_run( + pr_number = github.find_pr_for_branch( github=gh, - run_id=config.GITHUB_PR_RUN_ID, repository=config.GITHUB_REPOSITORY, + owner=owner, + branch=branch, ) except github.CannotDeterminePR: log.error( @@ -250,25 +278,17 @@ def post_comment(config: settings.Config, github_session: httpx.Client) -> int: def save_coverage_data_files( config: settings.Config, - coverage: coverage_module.Coverage, - raw_coverage_data: dict, - github_session: httpx.Client, git: subprocess.Git, http_session: httpx.Client, + repo_info: github.RepositoryInfo, ) -> int: - gh = github_client.GitHub(session=github_session) - repo_info = github.get_repository_info( - github=gh, - repository=config.GITHUB_REPOSITORY, - ) - is_default_branch = repo_info.is_default_branch(ref=config.GITHUB_REF) - log.debug(f"On default branch: {is_default_branch}") + log.info("Computing coverage files & badge") - if not is_default_branch: - log.info("Skipping badge save as we're not on the default branch") - return 0 + raw_coverage_data, coverage = coverage_module.get_coverage_info( + merge=config.MERGE_COVERAGE_FILES, + coverage_path=config.COVERAGE_PATH, + ) - log.info("Computing coverage files & badge") operations: list[files.Operation] = files.compute_files( line_rate=coverage.info.percent_covered, raw_coverage_data=raw_coverage_data, diff --git a/tests/integration/test_main.py b/tests/integration/test_main.py index 8481801c..f25f438f 100644 --- a/tests/integration/test_main.py +++ b/tests/integration/test_main.py @@ -96,9 +96,28 @@ def integration_env(integration_dir, write_file, run_coverage, commit): 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"} + ) + + result = main.action( + config=push_config(GITHUB_EVENT_NAME="pull_request_target"), + github_session=session, + http_session=session, + git=None, + ) + + assert result == 1 + assert get_logs("ERROR", "This action has only been designed to work for") + + def test_action__pull_request__store_comment( pull_request_config, session, in_integration_env, output_file, summary_file, capsys ): + session.register("GET", "/repos/py-cov-action/foobar")( + json={"default_branch": "main", "visibility": "public"} + ) # No existing badge in this test session.register( "GET", @@ -161,6 +180,10 @@ def checker(payload): def test_action__pull_request__post_comment( pull_request_config, session, in_integration_env, output_file, summary_file ): + session.register("GET", "/repos/py-cov-action/foobar")( + json={"default_branch": "main", "visibility": "public"} + ) + payload = json.dumps({"coverage": 30.00}) # There is an existing badge in this test, allowing to test the coverage evolution session.register( @@ -210,9 +233,135 @@ def checker(payload): assert output_file.read_text() == expected_output +def test_action__push__non_default_branch( + push_config, session, in_integration_env, output_file, summary_file +): + session.register("GET", "/repos/py-cov-action/foobar")( + json={"default_branch": "main", "visibility": "public"} + ) + + payload = json.dumps({"coverage": 30.00}) + # There is an existing badge in this test, allowing to test the coverage evolution + session.register( + "GET", + "/repos/py-cov-action/foobar/contents/data.json", + )(json={"content": base64.b64encode(payload.encode()).decode()}) + + session.register( + "GET", + "/repos/py-cov-action/foobar/pulls", + params={ + "state": "open", + "head": "py-cov-action:other", + "sort": "updated", + "direction": "desc", + }, + )(json=[{"number": 2}]) + + # Who am I + session.register("GET", "/user")(json={"login": "foo"}) + # Are there already comments + session.register("GET", "/repos/py-cov-action/foobar/issues/2/comments")(json=[]) + + comment = None + + def checker(payload): + body = payload["body"] + assert "## Coverage report" in body + nonlocal comment + comment = body + return True + + # Post a new comment + session.register( + "POST", + "/repos/py-cov-action/foobar/issues/2/comments", + json=checker, + )( + status_code=200, + ) + result = main.action( + config=push_config( + GITHUB_REF="refs/heads/other", + GITHUB_STEP_SUMMARY=summary_file, + GITHUB_OUTPUT=output_file, + ), + github_session=session, + http_session=session, + git=None, + ) + assert result == 0 + + assert not pathlib.Path("python-coverage-comment-action.txt").exists() + assert "The coverage rate went from `30%` to `77.77%` :arrow_up:" in comment + assert comment == summary_file.read_text() + + expected_output = "COMMENT_FILE_WRITTEN=false\n" + + assert output_file.read_text() == expected_output + + +def test_action__push__non_default_branch__no_pr( + push_config, session, in_integration_env, output_file, summary_file +): + session.register("GET", "/repos/py-cov-action/foobar")( + json={"default_branch": "main", "visibility": "public"} + ) + + payload = json.dumps({"coverage": 30.00}) + # There is an existing badge in this test, allowing to test the coverage evolution + session.register( + "GET", + "/repos/py-cov-action/foobar/contents/data.json", + )(json={"content": base64.b64encode(payload.encode()).decode()}) + + session.register( + "GET", + "/repos/py-cov-action/foobar/pulls", + params={ + "state": "open", + "head": "py-cov-action:other", + "sort": "updated", + "direction": "desc", + }, + )(json=[]) + session.register( + "GET", + "/repos/py-cov-action/foobar/pulls", + params={ + "state": "all", + "head": "py-cov-action:other", + "sort": "updated", + "direction": "desc", + }, + )(json=[]) + + result = main.action( + config=push_config( + GITHUB_REF="refs/heads/other", + GITHUB_STEP_SUMMARY=summary_file, + GITHUB_OUTPUT=output_file, + ), + github_session=session, + http_session=session, + git=None, + ) + assert result == 0 + + assert pathlib.Path("python-coverage-comment-action.txt").exists() + + expected_output = "COMMENT_FILE_WRITTEN=true\n" + + assert output_file.read_text() == expected_output + + def test_action__pull_request__force_store_comment( pull_request_config, session, in_integration_env, output_file ): + session.register("GET", "/repos/py-cov-action/foobar")( + json={"default_branch": "main", "visibility": "public"} + ) + payload = json.dumps({"coverage": 30.00}) # There is an existing badge in this test, allowing to test the coverage evolution session.register( @@ -238,6 +387,10 @@ def test_action__pull_request__force_store_comment( def test_action__pull_request__post_comment__no_marker( pull_request_config, session, in_integration_env, get_logs ): + session.register("GET", "/repos/py-cov-action/foobar")( + json={"default_branch": "main", "visibility": "public"} + ) + # No existing badge in this test session.register( "GET", @@ -257,6 +410,9 @@ def test_action__pull_request__post_comment__no_marker( def test_action__pull_request__annotations( pull_request_config, session, in_integration_env, capsys ): + session.register("GET", "/repos/py-cov-action/foobar")( + json={"default_branch": "main", "visibility": "public"} + ) # No existing badge in this test session.register( "GET", @@ -292,6 +448,10 @@ def test_action__pull_request__annotations( def test_action__pull_request__post_comment__template_error( pull_request_config, session, in_integration_env, get_logs ): + session.register("GET", "/repos/py-cov-action/foobar")( + json={"default_branch": "main", "visibility": "public"} + ) + # No existing badge in this test session.register( "GET", @@ -308,24 +468,6 @@ def test_action__pull_request__post_comment__template_error( assert get_logs("ERROR", "There was a rendering error") -def test_action__push__non_default_branch( - push_config, session, in_integration_env, get_logs -): - session.register("GET", "/repos/py-cov-action/foobar")( - json={"default_branch": "main", "visibility": "public"} - ) - - result = main.action( - config=push_config(GITHUB_REF="refs/heads/master"), - github_session=session, - http_session=session, - git=None, - ) - assert result == 0 - - assert get_logs("INFO", "Skipping badge") - - def test_action__push__default_branch( push_config, session, in_integration_env, get_logs, git, summary_file ): @@ -435,14 +577,35 @@ def test_action__push__default_branch__private( assert log == expected +def test_action__workflow_run__no_pr_number( + workflow_run_config, session, in_integration_env, get_logs +): + session.register("GET", "/repos/py-cov-action/foobar")( + json={"default_branch": "main", "visibility": "public"} + ) + + result = main.action( + config=workflow_run_config(GITHUB_PR_RUN_ID=None), + github_session=session, + http_session=session, + git=None, + ) + + assert result == 1 + assert get_logs("ERROR", "Missing input GITHUB_PR_RUN_ID") + + def test_action__workflow_run__no_pr( workflow_run_config, session, in_integration_env, get_logs ): + session.register("GET", "/repos/py-cov-action/foobar")( + json={"default_branch": "main", "visibility": "public"} + ) session.register("GET", "/user")(json={"login": "foo"}) session.register("GET", "/repos/py-cov-action/foobar/actions/runs/123")( json={ "head_branch": "branch", - "head_repository": {"full_name": "bar/repo-name"}, + "head_repository": {"owner": {"login": "bar/repo-name"}}, } ) @@ -481,11 +644,14 @@ def test_action__workflow_run__no_pr( def test_action__workflow_run__no_artifact( workflow_run_config, session, in_integration_env, get_logs ): + session.register("GET", "/repos/py-cov-action/foobar")( + json={"default_branch": "main", "visibility": "public"} + ) session.register("GET", "/user")(json={"login": "foo"}) session.register("GET", "/repos/py-cov-action/foobar/actions/runs/123")( json={ "head_branch": "branch", - "head_repository": {"full_name": "bar/repo-name"}, + "head_repository": {"owner": {"login": "bar/repo-name"}}, } ) @@ -519,11 +685,14 @@ def test_action__workflow_run__no_artifact( def test_action__workflow_run__post_comment( workflow_run_config, session, in_integration_env, get_logs, zip_bytes, summary_file ): + session.register("GET", "/repos/py-cov-action/foobar")( + json={"default_branch": "main", "visibility": "public"} + ) session.register("GET", "/user")(json={"login": "foo"}) session.register("GET", "/repos/py-cov-action/foobar/actions/runs/123")( json={ "head_branch": "branch", - "head_repository": {"full_name": "bar/repo-name"}, + "head_repository": {"owner": {"login": "bar/repo-name"}}, } ) diff --git a/tests/unit/test_main.py b/tests/unit/test_main.py index a98b4391..28d45a05 100644 --- a/tests/unit/test_main.py +++ b/tests/unit/test_main.py @@ -5,28 +5,6 @@ from coverage_comment import main, settings, subprocess -def test_action__invalid_event_name(push_config, get_logs): - result = main.action( - config=push_config(GITHUB_EVENT_NAME="pull_request_target"), - github_session=None, - http_session=None, - git=None, - ) - - assert result == 1 - assert get_logs("ERROR", "This action has only been designed to work for") - - -def test_post_comment__no_run_id(workflow_run_config, get_logs): - result = main.post_comment( - config=workflow_run_config(GITHUB_PR_RUN_ID=""), - github_session=None, - ) - - assert result == 1 - assert get_logs("ERROR", "Missing input GITHUB_PR_RUN_ID") - - def test_main(mocker, get_logs): # This test is a mock festival. The idea is that all the things that are hard # to simulate without mocks have been pushed up the stack up to this function From 380791beb0623f5d913cd05da1efe034467c6709 Mon Sep 17 00:00:00 2001 From: Joachim Jablon Date: Sun, 10 Sep 2023 13:01:41 +0200 Subject: [PATCH 7/9] Code review comments --- coverage_comment/main.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/coverage_comment/main.py b/coverage_comment/main.py index a3de74f0..338ef00d 100644 --- a/coverage_comment/main.py +++ b/coverage_comment/main.py @@ -119,11 +119,6 @@ def process_pr( base_ref=base_ref, coverage_path=config.COVERAGE_PATH ) - if config.ANNOTATE_MISSING_LINES: - annotations.create_pr_annotations( - annotation_type=config.ANNOTATION_TYPE, diff_coverage=diff_coverage - ) - previous_coverage_data_file = storage.get_datafile_contents( github=gh, repository=config.GITHUB_REPOSITORY, @@ -163,7 +158,7 @@ def process_pr( ) pr_number: int | None = config.GITHUB_PR_NUMBER - if not pr_number: + if pr_number is None: # If we don't have a PR number, we're launched from a push event, # so we need to find the PR number from the branch name assert config.GITHUB_BRANCH_NAME @@ -177,6 +172,11 @@ def process_pr( ) except github.CannotDeterminePR: pr_number = None + else: + if config.ANNOTATE_MISSING_LINES: + annotations.create_pr_annotations( + annotation_type=config.ANNOTATION_TYPE, diff_coverage=diff_coverage + ) try: if config.FORCE_WORKFLOW_RUN or not pr_number: From 1792b2c9496fc018c873ef90def267c80b3d05aa Mon Sep 17 00:00:00 2001 From: Joachim Jablon Date: Sun, 10 Sep 2023 13:10:41 +0200 Subject: [PATCH 8/9] Addressing the issue of PRs not targetting the default branch --- coverage_comment/main.py | 21 ++++++++++++++----- coverage_comment/template.py | 2 ++ coverage_comment/template_files/comment.md.j2 | 16 ++++++++++++-- 3 files changed, 32 insertions(+), 7 deletions(-) diff --git a/coverage_comment/main.py b/coverage_comment/main.py index 338ef00d..41efbf07 100644 --- a/coverage_comment/main.py +++ b/coverage_comment/main.py @@ -119,11 +119,21 @@ def process_pr( base_ref=base_ref, coverage_path=config.COVERAGE_PATH ) - previous_coverage_data_file = storage.get_datafile_contents( - github=gh, - repository=config.GITHUB_REPOSITORY, - branch=config.COVERAGE_DATA_BRANCH, - ) + # 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. + # In the case we're running on a branch without a PR yet, we can't know + # if it's going to target the default branch, so we display it. + previous_coverage_data_file = None + pr_targets_default_branch = base_ref == repo_info.default_branch + + if pr_targets_default_branch: + previous_coverage_data_file = storage.get_datafile_contents( + github=gh, + repository=config.GITHUB_REPOSITORY, + branch=config.COVERAGE_DATA_BRANCH, + ) + previous_coverage = None if previous_coverage_data_file: previous_coverage = files.parse_datafile(contents=previous_coverage_data_file) @@ -135,6 +145,7 @@ def process_pr( previous_coverage_rate=previous_coverage, base_template=template.read_template_file("comment.md.j2"), custom_template=config.COMMENT_TEMPLATE, + pr_targets_default_branch=pr_targets_default_branch, ) except template.MissingMarker: log.error( diff --git a/coverage_comment/template.py b/coverage_comment/template.py index 8cf83381..f212204d 100644 --- a/coverage_comment/template.py +++ b/coverage_comment/template.py @@ -45,6 +45,7 @@ def get_comment_markdown( previous_coverage_rate: decimal.Decimal | None, base_template: str, custom_template: str | None = None, + pr_targets_default_branch: bool = True, ): loader = CommentLoader(base_template=base_template, custom_template=custom_template) env = SandboxedEnvironment(loader=loader) @@ -56,6 +57,7 @@ def get_comment_markdown( coverage=coverage, diff_coverage=diff_coverage, marker=MARKER, + pr_targets_default_branch=pr_targets_default_branch, ) except jinja2.exceptions.TemplateError as exc: raise TemplateError from exc diff --git a/coverage_comment/template_files/comment.md.j2 b/coverage_comment/template_files/comment.md.j2 index 95543145..1151d57c 100644 --- a/coverage_comment/template_files/comment.md.j2 +++ b/coverage_comment/template_files/comment.md.j2 @@ -15,8 +15,20 @@ The coverage rate went from `{{ previous_coverage_rate | pct }}` to `{{ coverage {%- endblock emoji_coverage -%} {%- else -%} {% block no_comparison_info -%} -> **Note** -> No coverage data of the default branch was found for comparison. A possible reason for this is that the coverage action has not yet run after a push event and the data is therefore not yet initialized. +{%- if pr_targets_default_branch -%} +{% block no_comparison_info_data_not_found_message -%} +> [!NOTE] +> Coverage data for the default branch was not found. +> This usually happens when the action has not run on the default +> branch yet, for example right after deploying it into the workflows. +{%- endblock no_comparison_info_data_not_found_message -%} +{% else %} +{% block no_comparison_info_non_default_target -%} +> [!NOTE] +> Coverage evolution disabled because this PR targets a different branch +> than the default branch, for which coverage data is not available. +{%- endblock no_comparison_info_non_default_target -%} +{% endif %} {%- endblock no_comparison_info %} {% block coverage_value_wording -%} From 66ad832cd3b00f6579bb313761f13e5356ec01cf Mon Sep 17 00:00:00 2001 From: Joachim Jablon Date: Sun, 10 Sep 2023 13:40:42 +0200 Subject: [PATCH 9/9] Fix associated tests & add missing coverage --- coverage_comment/main.py | 11 ++- coverage_comment/template_files/comment.md.j2 | 4 +- pyproject.toml | 1 + tests/integration/test_main.py | 68 ++++++++++++++++++- tests/unit/test_template.py | 6 +- 5 files changed, 77 insertions(+), 13 deletions(-) diff --git a/coverage_comment/main.py b/coverage_comment/main.py index 41efbf07..8d55ea29 100644 --- a/coverage_comment/main.py +++ b/coverage_comment/main.py @@ -167,7 +167,6 @@ def process_pr( github.add_job_summary( content=comment, github_step_summary=config.GITHUB_STEP_SUMMARY ) - pr_number: int | None = config.GITHUB_PR_NUMBER if pr_number is None: # If we don't have a PR number, we're launched from a push event, @@ -183,11 +182,11 @@ def process_pr( ) except github.CannotDeterminePR: pr_number = None - else: - if config.ANNOTATE_MISSING_LINES: - annotations.create_pr_annotations( - annotation_type=config.ANNOTATION_TYPE, diff_coverage=diff_coverage - ) + + if pr_number is not None and config.ANNOTATE_MISSING_LINES: + annotations.create_pr_annotations( + annotation_type=config.ANNOTATION_TYPE, diff_coverage=diff_coverage + ) try: if config.FORCE_WORKFLOW_RUN or not pr_number: diff --git a/coverage_comment/template_files/comment.md.j2 b/coverage_comment/template_files/comment.md.j2 index 1151d57c..860911e4 100644 --- a/coverage_comment/template_files/comment.md.j2 +++ b/coverage_comment/template_files/comment.md.j2 @@ -27,8 +27,8 @@ The coverage rate went from `{{ previous_coverage_rate | pct }}` to `{{ coverage > [!NOTE] > Coverage evolution disabled because this PR targets a different branch > than the default branch, for which coverage data is not available. -{%- endblock no_comparison_info_non_default_target -%} -{% endif %} +{%- endblock no_comparison_info_non_default_target %} +{%- endif %} {%- endblock no_comparison_info %} {% block coverage_value_wording -%} diff --git a/pyproject.toml b/pyproject.toml index a757c43e..85139034 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -49,6 +49,7 @@ markers = [ "repo_suffix: Allows to use an additional suffix for the e2e test repo.", "code_path: Allows to place the code in a subdirectory for the e2e test repo.", "subproject_id: Allows to use a different subproject id for the e2e test repo.", + "add_branches: Adds branches besides 'main' and 'branch' to integration tests setup.", ] [tool.coverage.run] diff --git a/tests/integration/test_main.py b/tests/integration/test_main.py index f25f438f..123d569a 100644 --- a/tests/integration/test_main.py +++ b/tests/integration/test_main.py @@ -77,13 +77,20 @@ def _(): @pytest.fixture -def integration_env(integration_dir, write_file, run_coverage, commit): +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, @@ -160,7 +167,7 @@ def checker(payload): comment_file = pathlib.Path("python-coverage-comment-action.txt").read_text() assert comment == comment_file assert comment == summary_file.read_text() - assert "No coverage data of the default branch was found for comparison" in comment + assert "Coverage data for the default branch was not found." in comment assert "The coverage rate is `77.77%`" in comment assert "`75%` of new lines are covered." in comment assert ( @@ -177,6 +184,61 @@ def checker(payload): assert output_file.read_text() == expected_output +@pytest.mark.add_branches("foo") +def test_action__pull_request__store_comment_not_targeting_default( + pull_request_config, session, in_integration_env, output_file, summary_file, capsys +): + session.register("GET", "/repos/py-cov-action/foobar")( + json={"default_branch": "main", "visibility": "public"} + ) + payload = json.dumps({"coverage": 30.00}) + + session.register( + "GET", + "/repos/py-cov-action/foobar/contents/data.json", + )(json={"content": base64.b64encode(payload.encode()).decode()}) + + # Who am I + session.register("GET", "/user")(json={"login": "foo"}) + # Are there already comments + session.register("GET", "/repos/py-cov-action/foobar/issues/2/comments")(json=[]) + + comment = None + + def checker(payload): + body = payload["body"] + assert "## Coverage report" in body + nonlocal comment + comment = body + return True + + # Post a new comment + session.register( + "POST", "/repos/py-cov-action/foobar/issues/2/comments", json=checker + )(status_code=403) + + result = main.action( + config=pull_request_config( + GITHUB_OUTPUT=output_file, + GITHUB_STEP_SUMMARY=summary_file, + GITHUB_BASE_REF="foo", + ), + github_session=session, + http_session=session, + git=None, + ) + assert result == 0 + + # Check that no annotations were made + output = capsys.readouterr() + assert output.err.strip() == "" + + comment_file = pathlib.Path("python-coverage-comment-action.txt").read_text() + assert comment == comment_file + assert comment == summary_file.read_text() + assert "Coverage evolution disabled because this PR targets" in comment + + def test_action__pull_request__post_comment( pull_request_config, session, in_integration_env, output_file, summary_file ): diff --git a/tests/unit/test_template.py b/tests/unit/test_template.py index 78a298b6..52bda8db 100644 --- a/tests/unit/test_template.py +++ b/tests/unit/test_template.py @@ -204,8 +204,10 @@ def test_template__no_branch_no_previous(coverage_obj_no_branch, diff_coverage_o base_template=template.read_template_file("comment.md.j2"), ) expected = """## Coverage report -> **Note** -> No coverage data of the default branch was found for comparison. A possible reason for this is that the coverage action has not yet run after a push event and the data is therefore not yet initialized. +> [!NOTE] +> Coverage data for the default branch was not found. +> This usually happens when the action has not run on the default +> branch yet, for example right after deploying it into the workflows. The coverage rate is `75%`.