From eb48a53b18f301996a23a3636cd9dd03b0d799f1 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Fri, 5 Jan 2024 17:43:54 -0800 Subject: [PATCH 01/12] get diff of suggestions for each files (from each clang tool) --- cpp_linter/__init__.py | 2 ++ cpp_linter/clang_tools/__init__.py | 13 ++++++++-- cpp_linter/clang_tools/clang_format.py | 23 +++++++++++++--- cpp_linter/clang_tools/clang_tidy.py | 26 ++++++++++++++++++- cpp_linter/cli.py | 16 ++++++++++++ cpp_linter/rest_api/__init__.py | 6 +++++ cpp_linter/rest_api/github_api.py | 6 +++-- .../test_database_path.py | 6 ++++- .../capture_tools_output/test_tools_output.py | 10 ++++++- tests/comments/test_comments.py | 4 +++ tests/test_cli_args.py | 2 ++ 11 files changed, 104 insertions(+), 10 deletions(-) diff --git a/cpp_linter/__init__.py b/cpp_linter/__init__.py index 3d3bc43..644e076 100644 --- a/cpp_linter/__init__.py +++ b/cpp_linter/__init__.py @@ -67,6 +67,8 @@ def main(): args.lines_changed_only, args.database, args.extra_arg, + args.tidy_review, + args.format_review, ) start_log_group("Posting comment(s)") diff --git a/cpp_linter/clang_tools/__init__.py b/cpp_linter/clang_tools/__init__.py index ca2b015..53ee70e 100644 --- a/cpp_linter/clang_tools/__init__.py +++ b/cpp_linter/clang_tools/__init__.py @@ -39,6 +39,8 @@ def capture_clang_tools_output( lines_changed_only: int, database: str, extra_args: List[str], + tidy_review: bool, + format_review: bool, ) -> Tuple[List[FormatAdvice], List[TidyAdvice]]: """Execute and capture all output from clang-tidy and clang-format. This aggregates results in the :attr:`~cpp_linter.Globals.OUTPUT`. @@ -54,6 +56,10 @@ def capture_clang_tools_output( :param database: The path to the compilation database. :param extra_args: A list of extra arguments used by clang-tidy as compiler arguments. + :param tidy_review: A flag to enable/disable creating a diff suggestion for + PR review comments using clang-tidy. + :param format_review: A flag to enable/disable creating a diff suggestion for + PR review comments using clang-format. """ def show_tool_version_output(cmd: str): # show version output for executable used @@ -81,7 +87,7 @@ def show_tool_version_output(cmd: str): # show version output for executable us db_json = json.loads(db_path.read_text(encoding="utf-8")) # temporary cache of parsed notifications for use in log commands - tidy_notes: List[TidyAdvice] = [] + tidy_notes = [] format_advice = [] for file in files: start_log_group(f"Performing checkup on {file.name}") @@ -95,11 +101,14 @@ def show_tool_version_output(cmd: str): # show version output for executable us database, extra_args, db_json, + tidy_review, ) ) if format_cmd is not None: format_advice.append( - run_clang_format(format_cmd, file, style, lines_changed_only) + run_clang_format( + format_cmd, file, style, lines_changed_only, format_review + ) ) end_log_group() return (format_advice, tidy_notes) diff --git a/cpp_linter/clang_tools/clang_format.py b/cpp_linter/clang_tools/clang_format.py index 4b065c2..974c153 100644 --- a/cpp_linter/clang_tools/clang_format.py +++ b/cpp_linter/clang_tools/clang_format.py @@ -1,8 +1,11 @@ """Parse output from clang-format's XML suggestions.""" -from pathlib import PurePath +from pathlib import PurePath, Path import subprocess -from typing import List, cast +from typing import List, cast, Optional + +from pygit2 import Patch # type: ignore[import] import xml.etree.ElementTree as ET + from ..common_fs import get_line_cnt_from_cols, FileObj from ..loggers import logger @@ -66,6 +69,8 @@ def __init__(self, filename: str): """A list of `FormatReplacementLine` representing replacement(s) on a single line.""" + self.suggestion: Optional[Patch] = None + def __repr__(self) -> str: return ( f" FormatAdvice: """Run clang-format on a certain file @@ -149,6 +155,8 @@ def run_clang_format( use the relative-most .clang-format configuration file. :param lines_changed_only: A flag that forces focus on only changes in the event's diff info. + :param format_review: A flag to enable/disable creating a diff suggestion for + PR review comments. """ cmds = [ command, @@ -168,6 +176,15 @@ def run_clang_format( logger.debug( "%s raised the following error(s):\n%s", cmds[0], results.stderr.decode() ) - return parse_format_replacements_xml( + advice = parse_format_replacements_xml( results.stdout.decode(encoding="utf-8").strip(), file_obj, lines_changed_only ) + if format_review: + del cmds[2] # remove `--output-replacements-xml` flag + # get formatted file from stdout + formatted_output = subprocess.run(cmds, capture_output=True) + # create and store a patch between original file and formatted_output + advice.suggestion = Patch.create_from( + Path(file_obj.name).read_bytes(), formatted_output.stdout + ) + return advice diff --git a/cpp_linter/clang_tools/clang_tidy.py b/cpp_linter/clang_tools/clang_tidy.py index 1bed197..0421787 100644 --- a/cpp_linter/clang_tools/clang_tidy.py +++ b/cpp_linter/clang_tools/clang_tidy.py @@ -104,6 +104,7 @@ def run_clang_tidy( database: str, extra_args: List[str], db_json: Optional[List[Dict[str, str]]], + tidy_review: bool, ) -> TidyAdvice: """Run clang-tidy on a certain file. @@ -134,6 +135,8 @@ def run_clang_tidy( :param db_json: The compilation database deserialized from JSON, only if ``database`` parameter points to a valid path containing a ``compile_commands.json file``. + :param tidy_review: A flag to enable/disable creating a diff suggestion for + PR review comments. """ filename = file_obj.name.replace("/", os.sep) cmds = [command] @@ -162,7 +165,28 @@ def run_clang_tidy( logger.debug( "clang-tidy made the following summary:\n%s", results.stderr.decode() ) - return parse_tidy_output(results.stdout.decode(), database=db_json) + + advice = parse_tidy_output(results.stdout.decode(), database=db_json) + + if tidy_review: + # clang-tidy overwrites the file contents when applying fixes. + original_buf = Path( + file_obj.name + ).read_bytes() # create a cache of original contents + cmds.insert(1, "--fix-errors") # include compiler-suggested fixes + # run clang-tidy again to apply any fixes + fixed_result = subprocess.run(cmds, capture_output=True) + if ( + fixed_result.returncode + ): # log if any problems encountered (whatever they are) + logger.error("clang-tidy had problems applying fixes to %s", file_obj.name) + # create a patch for the file changes + advice.suggestion = Patch.create_from( + original_buf, Path(file_obj.name).read_bytes() + ) + # re-write original file contents (can probably skip this on CI runners) + Path(file_obj.name).write_bytes(original_buf) + return advice def parse_tidy_output( diff --git a/cpp_linter/cli.py b/cpp_linter/cli.py index b20c50e..28b8605 100644 --- a/cpp_linter/cli.py +++ b/cpp_linter/cli.py @@ -281,6 +281,22 @@ specified as positional arguments will be exempt from explicitly ignored domains (see :std:option:`--ignore`).""", ) +cli_arg_parser.add_argument( + "--tidy-review", + "-tr", + default="false", + type=lambda input: input.lower() == "true", + help="""Set to ``true`` to enable PR review suggestions +from clang-tidy.""", +) +cli_arg_parser.add_argument( + "--format-review", + "-fr", + default="false", + type=lambda input: input.lower() == "true", + help="""Set to ``true`` to enable PR review suggestions +from clang-format.""", +) def parse_ignore_option( diff --git a/cpp_linter/rest_api/__init__.py b/cpp_linter/rest_api/__init__.py index 533f6d4..e5345bf 100644 --- a/cpp_linter/rest_api/__init__.py +++ b/cpp_linter/rest_api/__init__.py @@ -143,6 +143,8 @@ def post_feedback( step_summary: bool, file_annotations: bool, style: str, + tidy_review: bool, + format_review: bool, ): """Post action's results using REST API. @@ -161,5 +163,9 @@ def post_feedback( :param file_annotations: A flag that describes if file annotations should be posted. See :std:option:`--file-annotations`. :param style: The style used for clang-format. See :std:option:`--style`. + :param tidy_review: A flag to enable/disable creating a diff suggestion for + PR review comments using clang-tidy. + :param format_review: A flag to enable/disable creating a diff suggestion for + PR review comments using clang-format. """ raise NotImplementedError("Must be defined in the derivative") diff --git a/cpp_linter/rest_api/github_api.py b/cpp_linter/rest_api/github_api.py index 882277d..1c734ff 100644 --- a/cpp_linter/rest_api/github_api.py +++ b/cpp_linter/rest_api/github_api.py @@ -146,6 +146,8 @@ def post_feedback( step_summary: bool, file_annotations: bool, style: str, + tidy_review: bool, + format_review: bool, ): (comment, format_checks_failed, tidy_checks_failed) = super().make_comment( files, format_advice, tidy_advice @@ -227,8 +229,8 @@ def make_annotations( output += f" does not conform to {style_guide} style guidelines. " output += "(lines {lines})".format(lines=", ".join(line_list)) log_commander.info(output) - for concern, file in zip(tidy_advice, files): - for note in concern.notes: + for concerns, file in zip(tidy_advice, files): + for note in concerns.notes: if note.filename == file.name: output = "::{} ".format( "notice" if note.severity.startswith("note") else note.severity diff --git a/tests/capture_tools_output/test_database_path.py b/tests/capture_tools_output/test_database_path.py index 2d92d1d..3838957 100644 --- a/tests/capture_tools_output/test_database_path.py +++ b/tests/capture_tools_output/test_database_path.py @@ -40,7 +40,7 @@ def test_db_detection( monkeypatch.chdir(PurePath(__file__).parent.parent.as_posix()) CACHE_PATH.mkdir(exist_ok=True) caplog.set_level(logging.DEBUG, logger=logger.name) - demo_src = "../demo/demo.cpp" + demo_src = "demo/demo.cpp" files = [FileObj(demo_src, [], [])] _ = capture_clang_tools_output( @@ -51,6 +51,8 @@ def test_db_detection( lines_changed_only=0, # analyze complete file database=database, extra_args=[], + tidy_review=False, + format_review=False, ) matched_args = [] for record in caplog.records: @@ -99,6 +101,8 @@ def test_ninja_database( lines_changed_only=0, # analyze complete file database="build", # point to generated compile_commands.txt extra_args=[], + tidy_review=False, + format_review=False, ) found_project_file = False for concern in tidy_advice: diff --git a/tests/capture_tools_output/test_tools_output.py b/tests/capture_tools_output/test_tools_output.py index 347e218..f387886 100644 --- a/tests/capture_tools_output/test_tools_output.py +++ b/tests/capture_tools_output/test_tools_output.py @@ -252,6 +252,8 @@ def test_format_annotations( lines_changed_only=lines_changed_only, database="", extra_args=[], + tidy_review=False, + format_review=False, ) assert [note for note in format_advice] assert not [note for concern in tidy_advice for note in concern.notes] @@ -328,6 +330,8 @@ def test_tidy_annotations( lines_changed_only=lines_changed_only, database="", extra_args=[], + tidy_review=False, + format_review=False, ) assert [note for concern in tidy_advice for note in concern.notes] assert not [note for note in format_advice] @@ -379,6 +383,8 @@ def test_all_ok_comment(tmp_path: Path, monkeypatch: pytest.MonkeyPatch): lines_changed_only=0, database="", extra_args=[], + tidy_review=False, + format_review=False, ) comment, format_checks_failed, tidy_checks_failed = GithubApiClient.make_comment( files, format_advice, tidy_advice @@ -461,13 +467,15 @@ def test_tidy_extra_args(caplog: pytest.LogCaptureFixture, user_input: List[str] args = cli_arg_parser.parse_args(cli_in) assert len(user_input) == len(args.extra_arg) _, _ = capture_clang_tools_output( - files=[FileObj("test/demo/demo.cpp", [], [])], + files=[FileObj("tests/demo/demo.cpp", [], [])], version=CLANG_VERSION, checks="", # use .clang-tidy config style="", # disable clang-format lines_changed_only=0, database="", extra_args=args.extra_arg, + tidy_review=False, + format_review=False, ) messages = [ r.message diff --git a/tests/comments/test_comments.py b/tests/comments/test_comments.py index 83b2071..d9d187d 100644 --- a/tests/comments/test_comments.py +++ b/tests/comments/test_comments.py @@ -43,6 +43,8 @@ def test_post_feedback( lines_changed_only=0, database="", extra_args=[], + tidy_review=False, + format_review=False, ) # add a non project file to tidy_advice to intentionally cover a log.debug() assert tidy_advice @@ -122,4 +124,6 @@ def test_post_feedback( step_summary=True, file_annotations=True, style="llvm", + tidy_review=False, + format_review=False, ) diff --git a/tests/test_cli_args.py b/tests/test_cli_args.py index 76cfe7c..6376497 100644 --- a/tests/test_cli_args.py +++ b/tests/test_cli_args.py @@ -40,6 +40,8 @@ class Args: extra_arg: List[str] = [] no_lgtm: bool = True files: List[str] = [] + tidy_review: bool = False + format_review: bool = False def test_defaults(): From 35cac88757a3a191e84c2087c8e9239c52c34492 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Sun, 7 Jan 2024 01:01:43 -0800 Subject: [PATCH 02/12] support PR reviews for individual tool's output satisfy some sonarcloud notes only get PR review info for actual PR events keep suggestions within 1 diff hunk - only dismiss reviews that can be dismissed - use default SHA for review commit_id - only use start_line if it precedes end line - use actual line numbers - prevent posting blank suggestions - don't add a new line at end of each suggestion - some code cleanup post review payload as json -- oops only create a Patch when creating a review use 1 review for both tools don't post reviews for PRs marked as "draft" --- cpp_linter/__init__.py | 7 +- cpp_linter/clang_tools/clang_format.py | 12 +- cpp_linter/clang_tools/clang_tidy.py | 21 +-- cpp_linter/common_fs.py | 25 +++- cpp_linter/loggers.py | 3 +- cpp_linter/rest_api/github_api.py | 140 +++++++++++++++++- .../test_database_path.py | 4 +- .../capture_tools_output/test_tools_output.py | 2 +- 8 files changed, 181 insertions(+), 33 deletions(-) diff --git a/cpp_linter/__init__.py b/cpp_linter/__init__.py index 644e076..3c86e3f 100644 --- a/cpp_linter/__init__.py +++ b/cpp_linter/__init__.py @@ -23,6 +23,7 @@ def main(): rest_api_client = GithubApiClient() logger.info("processing %s event", rest_api_client.event_name) + is_pr_event = rest_api_client.event_name == "pull_request" # set logging verbosity logger.setLevel(10 if args.verbosity or rest_api_client.debug_enabled else 20) @@ -67,8 +68,8 @@ def main(): args.lines_changed_only, args.database, args.extra_arg, - args.tidy_review, - args.format_review, + is_pr_event and args.tidy_review, + is_pr_event and args.format_review, ) start_log_group("Posting comment(s)") @@ -81,6 +82,8 @@ def main(): args.step_summary, args.file_annotations, args.style, + is_pr_event and args.format_review, + is_pr_event and args.tidy_review, ) end_log_group() diff --git a/cpp_linter/clang_tools/clang_format.py b/cpp_linter/clang_tools/clang_format.py index 974c153..5f93a01 100644 --- a/cpp_linter/clang_tools/clang_format.py +++ b/cpp_linter/clang_tools/clang_format.py @@ -1,9 +1,8 @@ """Parse output from clang-format's XML suggestions.""" -from pathlib import PurePath, Path +from pathlib import PurePath import subprocess from typing import List, cast, Optional -from pygit2 import Patch # type: ignore[import] import xml.etree.ElementTree as ET from ..common_fs import get_line_cnt_from_cols, FileObj @@ -69,7 +68,8 @@ def __init__(self, filename: str): """A list of `FormatReplacementLine` representing replacement(s) on a single line.""" - self.suggestion: Optional[Patch] = None + #: A buffer of the applied fixes from clang-format + self.patched: Optional[bytes] = None def __repr__(self) -> str: return ( @@ -183,8 +183,6 @@ def run_clang_format( del cmds[2] # remove `--output-replacements-xml` flag # get formatted file from stdout formatted_output = subprocess.run(cmds, capture_output=True) - # create and store a patch between original file and formatted_output - advice.suggestion = Patch.create_from( - Path(file_obj.name).read_bytes(), formatted_output.stdout - ) + # store formatted_output (for comparing later) + advice.patched = formatted_output.stdout return advice diff --git a/cpp_linter/clang_tools/clang_tidy.py b/cpp_linter/clang_tools/clang_tidy.py index 0421787..ac76a8d 100644 --- a/cpp_linter/clang_tools/clang_tidy.py +++ b/cpp_linter/clang_tools/clang_tidy.py @@ -5,7 +5,6 @@ import re import subprocess from typing import Tuple, Union, List, cast, Optional, Dict -from pygit2 import Patch # type: ignore[import] from ..loggers import logger from ..common_fs import FileObj @@ -91,8 +90,8 @@ def __repr__(self) -> str: class TidyAdvice: def __init__(self, notes: List[TidyNotification]) -> None: - #: A patch of the suggested fixes from clang-tidy - self.suggestion: Optional[Patch] = None + #: A buffer of the applied fixes from clang-tidy + self.patched: Optional[bytes] = None self.notes = notes @@ -170,20 +169,16 @@ def run_clang_tidy( if tidy_review: # clang-tidy overwrites the file contents when applying fixes. - original_buf = Path( - file_obj.name - ).read_bytes() # create a cache of original contents + # create a cache of original contents + original_buf = Path(file_obj.name).read_bytes() cmds.insert(1, "--fix-errors") # include compiler-suggested fixes # run clang-tidy again to apply any fixes fixed_result = subprocess.run(cmds, capture_output=True) - if ( - fixed_result.returncode - ): # log if any problems encountered (whatever they are) + if fixed_result.returncode: + # log if any problems encountered (whatever they are) logger.error("clang-tidy had problems applying fixes to %s", file_obj.name) - # create a patch for the file changes - advice.suggestion = Patch.create_from( - original_buf, Path(file_obj.name).read_bytes() - ) + # store the modified output from clang-tidy + advice.patched = Path(file_obj.name).read_bytes() # re-write original file contents (can probably skip this on CI runners) Path(file_obj.name).write_bytes(original_buf) return advice diff --git a/cpp_linter/common_fs.py b/cpp_linter/common_fs.py index 4b9142b..dda2afa 100644 --- a/cpp_linter/common_fs.py +++ b/cpp_linter/common_fs.py @@ -1,7 +1,7 @@ from os import environ from os.path import commonpath from pathlib import PurePath, Path -from typing import List, Dict, Any, Union, Tuple +from typing import List, Dict, Any, Union, Tuple, Optional from .loggers import logger, start_log_group #: A path to generated cache artifacts. (only used when verbosity is in debug mode) @@ -19,16 +19,21 @@ class FileObj: for all hunks in the diff. """ - def __init__(self, name: str, additions: List[int], diff_chunks: List[List[int]]): + def __init__( + self, + name: str, + additions: Optional[List[int]] = None, + diff_chunks: Optional[List[List[int]]] = None, + ): self.name: str = name #: The file name - self.additions: List[int] = additions + self.additions: List[int] = additions or [] """A list of line numbers that contain added changes. This will be empty if not focusing on lines changed only.""" - self.diff_chunks: List[List[int]] = diff_chunks + self.diff_chunks: List[List[int]] = diff_chunks or [] """A list of line numbers that define the beginning and ending of hunks in the diff. This will be empty if not focusing on lines changed only.""" self.lines_added: List[List[int]] = FileObj._consolidate_list_to_ranges( - additions + additions or [] ) """A list of line numbers that define the beginning and ending of ranges that have added changes. This will be empty if not focusing on lines changed only. @@ -93,6 +98,14 @@ def serialize(self) -> Dict[str, Any]: }, } + def is_in_1_hunk(self, start: int, end: int) -> bool: + """Is a given ``start`` and ``end`` line numbers within a single diff hunk?""" + for hunk in self.diff_chunks: + chunk_range = range(hunk[0], hunk[1]) + if start in chunk_range and end in chunk_range: + return True + return False + def is_file_in_list(paths: List[str], file_name: str, prompt: str) -> bool: """Determine if a file is specified in a list of paths and/or filenames. @@ -195,7 +208,7 @@ def list_source_files( if is_file_in_list( not_ignored, file_path, "not ignored" ) or not is_file_in_list(ignored, file_path, "ignored"): - files.append(FileObj(file_path, [], [])) + files.append(FileObj(file_path)) return files diff --git a/cpp_linter/loggers.py b/cpp_linter/loggers.py index eb066c6..6b90c46 100644 --- a/cpp_linter/loggers.py +++ b/cpp_linter/loggers.py @@ -51,8 +51,9 @@ def log_response_msg(response_buffer: Response) -> bool: """ if response_buffer.status_code >= 400: logger.error( - "response returned %d message: %s", + "response returned %d from %s with message: %s", response_buffer.status_code, + response_buffer.url, response_buffer.text, ) return False diff --git a/cpp_linter/rest_api/github_api.py b/cpp_linter/rest_api/github_api.py index 1c734ff..5e7a60f 100644 --- a/cpp_linter/rest_api/github_api.py +++ b/cpp_linter/rest_api/github_api.py @@ -13,8 +13,9 @@ from pathlib import Path import urllib.parse import sys -from typing import Dict, List, Any, cast, Optional, Tuple +from typing import Dict, List, Any, cast, Optional, Tuple, Union, Sequence +from pygit2 import Patch # type: ignore from ..common_fs import FileObj, CACHE_PATH from ..clang_tools.clang_format import FormatAdvice, formalize_style_name from ..clang_tools.clang_tidy import TidyAdvice @@ -172,6 +173,9 @@ def post_feedback( comment, comments_url, count, no_lgtm, update_only, is_lgtm ) + if self.event_name == "pull_request" and (tidy_review or format_review): + self.post_review(files, tidy_advice, format_advice) + if file_annotations: self.make_annotations(files, format_advice, tidy_advice, style) @@ -344,3 +348,137 @@ def remove_bot_comments( if not delete: comment_url = cast(str, comment["url"]) return comment_url + + def post_review( + self, + files: List[FileObj], + tidy_advice: List[TidyAdvice], + format_advice: List[FormatAdvice], + ): + url = f"{self.api_url}/repos/{self.repo}/pulls/{self.event_payload['number']}" + response_buffer = self.session.get(url, headers=self.make_headers()) + url += "/reviews" + is_draft = True + if log_response_msg(response_buffer): + is_draft = cast(Dict[str, bool], response_buffer.json()).get("draft", False) + if "GITHUB_TOKEN" not in environ: + logger.error("A GITHUB_TOKEN env var is required to post review comments") + return + self._dismiss_stale_reviews(url) + if is_draft: + return # don't post reviews for PRs marked as "draft" + body = "\n## Cpp-linter Review\n" + payload_comments = [] + total_changes = 0 + for index, tool_advice in enumerate([format_advice, tidy_advice]): + comments, total, patch = self.create_review_comments( + files, + tool_advice, # type: ignore[arg-type] + ) + tool = "clang-tidy" if index else "clang-format" + total_changes += total + payload_comments.extend(comments) + if total and total != len(comments): + body += f"Only {len(comments)} out of {total} {tool} " + body += "suggestions fit within this pull request's diff.\n" + if patch: + body += f"\n
Click here for the full {tool} patch" + body += f"\n\n\n```diff\n{patch}\n```\n\n\n
\n\n" + if total_changes: + event = "REQUEST_CHANGES" + else: + event = "APPROVE" + payload = { + "body": body, + "event": event, + "comments": payload_comments, + } + response_buffer = self.session.post( + url, headers=self.make_headers(), data=json.dumps(payload) + ) + log_response_msg(response_buffer) + + @staticmethod + def create_review_comments( + files: List[FileObj], + tool_advice: Sequence[Union[FormatAdvice, TidyAdvice]], + ) -> Tuple[List[Dict[str, Any]], int, str]: + """Creates a batch of comments for a specific clang tool's PR review""" + total = 0 + comments = [] + full_patch = "" + for file, advice in zip(files, tool_advice): + if not advice.patched: + continue + patch = Patch.create_from( + old=Path(file.name).read_bytes(), + new=advice.patched, + old_as_path=file.name, + new_as_path=file.name, + context_lines=0, # trim all unchanged lines from start/end of hunks + ) + full_patch += patch.text + for hunk in patch.hunks: + total += 1 + start_lines, end_lines = ( + hunk.old_start, + hunk.old_start + hunk.old_lines, + ) + if not file.is_in_1_hunk(start_lines, end_lines): + logger.warning( + "lines %d - %d are not within a single diff hunk for file %s.", + start_lines, + end_lines, + file.name, + ) + continue + comment: Dict[str, Any] = {"path": file.name} + body = "" + if isinstance(advice, TidyAdvice): + body += "### clang-tidy diagnostics\n" + for note in advice.notes: + if note.line in range(start_lines, end_lines): + body += f"- {note.rationale} [{note.diagnostic_link}]\n" + else: + body += "### clang-format suggestions\n" + if start_lines < end_lines: + comment["start_line"] = start_lines + comment["line"] = end_lines + suggestion = "" + for line in hunk.lines: + if line.origin in ["+", " "]: + suggestion += line.content + if not suggestion: + body += "\nPlease remove this line(s)." + else: + body += f"\n```suggestion\n{suggestion}```" + comment["body"] = body + comments.append(comment) + return (comments, total, full_patch) + + def _dismiss_stale_reviews(self, url: str): + """Dismiss all reviews that were previously created by cpp-linter""" + response_buffer = self.session.get(url, headers=self.make_headers()) + if not log_response_msg(response_buffer): + logger.error("Failed to poll existing reviews for dismissal") + else: + headers = self.make_headers() + reviews: List[Dict[str, Any]] = response_buffer.json() + for review in reviews: + if ( + "body" in review + and cast(str, review["body"]).startswith( + "\n" + ) + and "state" in review + and review["state"] not in ["PENDING", "DISMISSED"] + ): + assert "id" in review + response_buffer = self.session.put( + f"{url}/{review['id']}/dismissals", + headers=headers, + data=json.dumps( + {"message": "outdated suggestion", "event": "DISMISS"} + ), + ) + log_response_msg(response_buffer) diff --git a/tests/capture_tools_output/test_database_path.py b/tests/capture_tools_output/test_database_path.py index 3838957..9d7293b 100644 --- a/tests/capture_tools_output/test_database_path.py +++ b/tests/capture_tools_output/test_database_path.py @@ -41,7 +41,7 @@ def test_db_detection( CACHE_PATH.mkdir(exist_ok=True) caplog.set_level(logging.DEBUG, logger=logger.name) demo_src = "demo/demo.cpp" - files = [FileObj(demo_src, [], [])] + files = [FileObj(demo_src)] _ = capture_clang_tools_output( files, @@ -89,7 +89,7 @@ def test_ninja_database( meson() caplog.set_level(logging.DEBUG, logger=logger.name) - files = [FileObj("demo.cpp", [], [])] + files = [FileObj("demo.cpp")] gh_client = GithubApiClient() # run clang-tidy and verify paths of project files were matched with database paths diff --git a/tests/capture_tools_output/test_tools_output.py b/tests/capture_tools_output/test_tools_output.py index f387886..5f1997f 100644 --- a/tests/capture_tools_output/test_tools_output.py +++ b/tests/capture_tools_output/test_tools_output.py @@ -467,7 +467,7 @@ def test_tidy_extra_args(caplog: pytest.LogCaptureFixture, user_input: List[str] args = cli_arg_parser.parse_args(cli_in) assert len(user_input) == len(args.extra_arg) _, _ = capture_clang_tools_output( - files=[FileObj("tests/demo/demo.cpp", [], [])], + files=[FileObj("tests/demo/demo.cpp")], version=CLANG_VERSION, checks="", # use .clang-tidy config style="", # disable clang-format From a23cc35e3eddd7fed5e238bcc95352133144b0ae Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Sun, 7 Jan 2024 01:32:04 -0800 Subject: [PATCH 03/12] add test --- .pre-commit-config.yaml | 2 +- tests/comments/test_comments.py | 3 + tests/reviews/.clang-format | 3 + tests/reviews/.clang-tidy | 185 +++++++++++++ tests/reviews/pr_27.diff | 108 ++++++++ tests/reviews/pr_27.json | 372 ++++++++++++++++++++++++++ tests/reviews/pr_review_comments.json | 206 ++++++++++++++ tests/reviews/pr_reviews.json | 41 +++ tests/reviews/test_pr_review.py | 143 ++++++++++ 9 files changed, 1062 insertions(+), 1 deletion(-) create mode 100644 tests/reviews/.clang-format create mode 100644 tests/reviews/.clang-tidy create mode 100644 tests/reviews/pr_27.diff create mode 100644 tests/reviews/pr_27.json create mode 100644 tests/reviews/pr_review_comments.json create mode 100644 tests/reviews/pr_reviews.json create mode 100644 tests/reviews/test_pr_review.py diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index a7bb57e..4e4c34d 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -3,7 +3,7 @@ repos: rev: v4.5.0 hooks: - id: trailing-whitespace - exclude: tests/capture_tools_output/cpp-linter/cpp-linter/test_git_lib.patch + exclude: ^tests/.*\.(?:patch|diff)$ - id: end-of-file-fixer - id: check-docstring-first - id: check-added-large-files diff --git a/tests/comments/test_comments.py b/tests/comments/test_comments.py index d9d187d..34cccb4 100644 --- a/tests/comments/test_comments.py +++ b/tests/comments/test_comments.py @@ -91,6 +91,9 @@ def test_post_feedback( text=(cache_path / f"pr_comments_pg{i}.json").read_text( encoding="utf-8" ), + # to trigger a logged error, we'll modify the response when + # fetching page 2 of old comments and thread-comments is true + status_code=404 if i == 2 and thread_comments == "true" else 200 ) # load mock responses for push event diff --git a/tests/reviews/.clang-format b/tests/reviews/.clang-format new file mode 100644 index 0000000..1dd236c --- /dev/null +++ b/tests/reviews/.clang-format @@ -0,0 +1,3 @@ +--- +Language: Cpp +BasedOnStyle: WebKit diff --git a/tests/reviews/.clang-tidy b/tests/reviews/.clang-tidy new file mode 100644 index 0000000..36fb3ab --- /dev/null +++ b/tests/reviews/.clang-tidy @@ -0,0 +1,185 @@ +--- +Checks: 'clang-diagnostic-*,clang-analyzer-*,-*,performance-*,bugprone-*,clang-analyzer-*,mpi-*,misc-*,readability-*' +WarningsAsErrors: '' +HeaderFilterRegex: '' +AnalyzeTemporaryDtors: false +FormatStyle: 'file' +CheckOptions: + - key: bugprone-argument-comment.CommentBoolLiterals + value: '0' + - key: bugprone-argument-comment.CommentCharacterLiterals + value: '0' + - key: bugprone-argument-comment.CommentFloatLiterals + value: '0' + - key: bugprone-argument-comment.CommentIntegerLiterals + value: '0' + - key: bugprone-argument-comment.CommentNullPtrs + value: '0' + - key: bugprone-argument-comment.CommentStringLiterals + value: '0' + - key: bugprone-argument-comment.CommentUserDefinedLiterals + value: '0' + - key: bugprone-argument-comment.IgnoreSingleArgument + value: '0' + - key: bugprone-argument-comment.StrictMode + value: '0' + - key: bugprone-assert-side-effect.AssertMacros + value: assert + - key: bugprone-assert-side-effect.CheckFunctionCalls + value: '0' + - key: bugprone-dangling-handle.HandleClasses + value: 'std::basic_string_view;std::experimental::basic_string_view' + - key: bugprone-dynamic-static-initializers.HeaderFileExtensions + value: ',h,hh,hpp,hxx' + - key: bugprone-exception-escape.FunctionsThatShouldNotThrow + value: '' + - key: bugprone-exception-escape.IgnoredExceptions + value: '' + - key: bugprone-misplaced-widening-cast.CheckImplicitCasts + value: '0' + - key: bugprone-not-null-terminated-result.WantToUseSafeFunctions + value: '1' + - key: bugprone-signed-char-misuse.CharTypdefsToIgnore + value: '' + - key: bugprone-sizeof-expression.WarnOnSizeOfCompareToConstant + value: '1' + - key: bugprone-sizeof-expression.WarnOnSizeOfConstant + value: '1' + - key: bugprone-sizeof-expression.WarnOnSizeOfIntegerExpression + value: '0' + - key: bugprone-sizeof-expression.WarnOnSizeOfThis + value: '1' + - key: bugprone-string-constructor.LargeLengthThreshold + value: '8388608' + - key: bugprone-string-constructor.WarnOnLargeLength + value: '1' + - key: bugprone-suspicious-enum-usage.StrictMode + value: '0' + - key: bugprone-suspicious-missing-comma.MaxConcatenatedTokens + value: '5' + - key: bugprone-suspicious-missing-comma.RatioThreshold + value: '0.200000' + - key: bugprone-suspicious-missing-comma.SizeThreshold + value: '5' + - key: bugprone-suspicious-string-compare.StringCompareLikeFunctions + value: '' + - key: bugprone-suspicious-string-compare.WarnOnImplicitComparison + value: '1' + - key: bugprone-suspicious-string-compare.WarnOnLogicalNotComparison + value: '0' + - key: bugprone-too-small-loop-variable.MagnitudeBitsUpperLimit + value: '16' + - key: bugprone-unhandled-self-assignment.WarnOnlyIfThisHasSuspiciousField + value: '1' + - key: bugprone-unused-return-value.CheckedFunctions + value: '::std::async;::std::launder;::std::remove;::std::remove_if;::std::unique;::std::unique_ptr::release;::std::basic_string::empty;::std::vector::empty' + - key: cert-dcl16-c.NewSuffixes + value: 'L;LL;LU;LLU' + - key: cert-oop54-cpp.WarnOnlyIfThisHasSuspiciousField + value: '0' + - key: cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors + value: '1' + - key: cppcoreguidelines-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic + value: '1' + - key: google-readability-braces-around-statements.ShortStatementLines + value: '1' + - key: google-readability-function-size.StatementThreshold + value: '800' + - key: google-readability-namespace-comments.ShortNamespaceLines + value: '10' + - key: google-readability-namespace-comments.SpacesBeforeComments + value: '2' + - key: misc-definitions-in-headers.HeaderFileExtensions + value: ',h,hh,hpp,hxx' + - key: misc-definitions-in-headers.UseHeaderFileExtension + value: '1' + - key: misc-throw-by-value-catch-by-reference.CheckThrowTemporaries + value: '1' + - key: misc-unused-parameters.StrictMode + value: '0' + - key: modernize-loop-convert.MaxCopySize + value: '16' + - key: modernize-loop-convert.MinConfidence + value: reasonable + - key: modernize-loop-convert.NamingStyle + value: CamelCase + - key: modernize-pass-by-value.IncludeStyle + value: llvm + - key: modernize-replace-auto-ptr.IncludeStyle + value: llvm + - key: modernize-use-nullptr.NullMacros + value: 'NULL' + - key: performance-faster-string-find.StringLikeClasses + value: 'std::basic_string' + - key: performance-for-range-copy.AllowedTypes + value: '' + - key: performance-for-range-copy.WarnOnAllAutoCopies + value: '0' + - key: performance-inefficient-string-concatenation.StrictMode + value: '0' + - key: performance-inefficient-vector-operation.EnableProto + value: '0' + - key: performance-inefficient-vector-operation.VectorLikeClasses + value: '::std::vector' + - key: performance-move-const-arg.CheckTriviallyCopyableMove + value: '1' + - key: performance-move-constructor-init.IncludeStyle + value: llvm + - key: performance-no-automatic-move.AllowedTypes + value: '' + - key: performance-type-promotion-in-math-fn.IncludeStyle + value: llvm + - key: performance-unnecessary-copy-initialization.AllowedTypes + value: '' + - key: performance-unnecessary-value-param.AllowedTypes + value: '' + - key: performance-unnecessary-value-param.IncludeStyle + value: llvm + - key: readability-braces-around-statements.ShortStatementLines + value: '0' + - key: readability-else-after-return.WarnOnUnfixable + value: '1' + - key: readability-function-size.BranchThreshold + value: '4294967295' + - key: readability-function-size.LineThreshold + value: '4294967295' + - key: readability-function-size.NestingThreshold + value: '4294967295' + - key: readability-function-size.ParameterThreshold + value: '4294967295' + - key: readability-function-size.StatementThreshold + value: '800' + - key: readability-function-size.VariableThreshold + value: '4294967295' + - key: readability-identifier-naming.IgnoreFailedSplit + value: '0' + - key: readability-implicit-bool-conversion.AllowIntegerConditions + value: '0' + - key: readability-implicit-bool-conversion.AllowPointerConditions + value: '0' + - key: readability-inconsistent-declaration-parameter-name.IgnoreMacros + value: '1' + - key: readability-inconsistent-declaration-parameter-name.Strict + value: '0' + - key: readability-magic-numbers.IgnoredFloatingPointValues + value: '1.0;100.0;' + - key: readability-magic-numbers.IgnoredIntegerValues + value: '1;2;3;4;' + - key: readability-redundant-member-init.IgnoreBaseInCopyConstructors + value: '0' + - key: readability-redundant-smartptr-get.IgnoreMacros + value: '1' + - key: readability-redundant-string-init.StringNames + value: '::std::basic_string' + - key: readability-simplify-boolean-expr.ChainedConditionalAssignment + value: '0' + - key: readability-simplify-boolean-expr.ChainedConditionalReturn + value: '0' + - key: readability-simplify-subscript-expr.Types + value: '::std::basic_string;::std::basic_string_view;::std::vector;::std::array' + - key: readability-static-accessed-through-instance.NameSpecifierNestingThreshold + value: '3' + - key: readability-uppercase-literal-suffix.IgnoreMacros + value: '1' + - key: readability-uppercase-literal-suffix.NewSuffixes + value: '' diff --git a/tests/reviews/pr_27.diff b/tests/reviews/pr_27.diff new file mode 100644 index 0000000..3c5dd0b --- /dev/null +++ b/tests/reviews/pr_27.diff @@ -0,0 +1,108 @@ +diff --git a/.github/workflows/cpp-lint-package.yml b/.github/workflows/cpp-lint-package.yml +index 0418957..3b8c454 100644 +--- a/.github/workflows/cpp-lint-package.yml ++++ b/.github/workflows/cpp-lint-package.yml +@@ -7,6 +7,7 @@ on: + description: 'which branch to test' + default: 'main' + required: true ++ pull_request: + + jobs: + cpp-linter: +@@ -14,9 +15,9 @@ jobs: + + strategy: + matrix: +- clang-version: ['7', '8', '9','10', '11', '12', '13', '14', '15', '16', '17'] ++ clang-version: ['10', '11', '12', '13', '14', '15', '16', '17'] + repo: ['cpp-linter/cpp-linter'] +- branch: ['${{ inputs.branch }}'] ++ branch: ['pr-review-suggestions'] + fail-fast: false + + steps: +@@ -62,10 +63,12 @@ jobs: + -i=build + -p=build + -V=${{ runner.temp }}/llvm +- -f=false + --extra-arg="-std=c++14 -Wall" +- --thread-comments=${{ matrix.clang-version == '12' }} +- -a=${{ matrix.clang-version == '12' }} ++ --file-annotations=false ++ --lines-changed-only=true ++ --thread-comments=${{ matrix.clang-version == '16' }} ++ --tidy-review=${{ matrix.clang-version == '16' }} ++ --format-review=${{ matrix.clang-version == '16' }} + + - name: Fail fast?! + if: steps.linter.outputs.checks-failed > 0 +diff --git a/src/demo.cpp b/src/demo.cpp +index 0c1db60..1bf553e 100644 +--- a/src/demo.cpp ++++ b/src/demo.cpp +@@ -1,17 +1,18 @@ + /** This is a very ugly test code (doomed to fail linting) */ + #include "demo.hpp" +-#include +-#include ++#include + +-// using size_t from cstddef +-size_t dummyFunc(size_t i) { return i; } + +-int main() +-{ +- for (;;) +- break; ++ ++ ++int main(){ ++ ++ for (;;) break; ++ + + printf("Hello world!\n"); + +- return 0; +-} ++ ++ ++ ++ return 0;} +diff --git a/src/demo.hpp b/src/demo.hpp +index 2695731..f93d012 100644 +--- a/src/demo.hpp ++++ b/src/demo.hpp +@@ -5,12 +5,10 @@ + class Dummy { + char* useless; + int numb; ++ Dummy() :numb(0), useless("\0"){} + + public: +- void *not_usefull(char *str){ +- useless = str; +- return 0; +- } ++ void *not_useful(char *str){useless = str;} + }; + + +@@ -28,14 +26,11 @@ class Dummy { + + + +- +- +- +- + + + struct LongDiff + { ++ + long diff; + + }; diff --git a/tests/reviews/pr_27.json b/tests/reviews/pr_27.json new file mode 100644 index 0000000..1ce3675 --- /dev/null +++ b/tests/reviews/pr_27.json @@ -0,0 +1,372 @@ +{ + "url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/27", + "id": 1667524645, + "node_id": "PR_kwDOFY2uzM5jZGgl", + "html_url": "https://github.com/cpp-linter/test-cpp-linter-action/pull/27", + "diff_url": "https://github.com/cpp-linter/test-cpp-linter-action/pull/27.diff", + "patch_url": "https://github.com/cpp-linter/test-cpp-linter-action/pull/27.patch", + "issue_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/issues/27", + "number": 27, + "state": "open", + "locked": false, + "title": "Test pr reviews (attempt 2)", + "user": { + "login": "2bndy5", + "id": 14963867, + "node_id": "MDQ6VXNlcjE0OTYzODY3", + "avatar_url": "https://avatars.githubusercontent.com/u/14963867?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/2bndy5", + "html_url": "https://github.com/2bndy5", + "followers_url": "https://api.github.com/users/2bndy5/followers", + "following_url": "https://api.github.com/users/2bndy5/following{/other_user}", + "gists_url": "https://api.github.com/users/2bndy5/gists{/gist_id}", + "starred_url": "https://api.github.com/users/2bndy5/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/2bndy5/subscriptions", + "organizations_url": "https://api.github.com/users/2bndy5/orgs", + "repos_url": "https://api.github.com/users/2bndy5/repos", + "events_url": "https://api.github.com/users/2bndy5/events{/privacy}", + "received_events_url": "https://api.github.com/users/2bndy5/received_events", + "type": "User", + "site_admin": false + }, + "body": null, + "created_at": "2024-01-06T22:20:55Z", + "updated_at": "2024-01-06T22:22:32Z", + "closed_at": null, + "merged_at": null, + "merge_commit_sha": "faf42f6e55e8e7e4c5e8db55eba2bacd270de362", + "assignee": null, + "assignees": [ + + ], + "requested_reviewers": [ + + ], + "requested_teams": [ + + ], + "labels": [ + + ], + "milestone": null, + "draft": true, + "commits_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/27/commits", + "review_comments_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/27/comments", + "review_comment_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/comments{/number}", + "comments_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/issues/27/comments", + "statuses_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/statuses/a09a2032511f6a61f9216b24d2cd480c923d333f", + "head": { + "label": "cpp-linter:test-pr-reviews", + "ref": "test-pr-reviews", + "sha": "a09a2032511f6a61f9216b24d2cd480c923d333f", + "user": { + "login": "cpp-linter", + "id": 103884627, + "node_id": "O_kgDOBjEnUw", + "avatar_url": "https://avatars.githubusercontent.com/u/103884627?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/cpp-linter", + "html_url": "https://github.com/cpp-linter", + "followers_url": "https://api.github.com/users/cpp-linter/followers", + "following_url": "https://api.github.com/users/cpp-linter/following{/other_user}", + "gists_url": "https://api.github.com/users/cpp-linter/gists{/gist_id}", + "starred_url": "https://api.github.com/users/cpp-linter/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/cpp-linter/subscriptions", + "organizations_url": "https://api.github.com/users/cpp-linter/orgs", + "repos_url": "https://api.github.com/users/cpp-linter/repos", + "events_url": "https://api.github.com/users/cpp-linter/events{/privacy}", + "received_events_url": "https://api.github.com/users/cpp-linter/received_events", + "type": "Organization", + "site_admin": false + }, + "repo": { + "id": 361606860, + "node_id": "MDEwOlJlcG9zaXRvcnkzNjE2MDY4NjA=", + "name": "test-cpp-linter-action", + "full_name": "cpp-linter/test-cpp-linter-action", + "private": false, + "owner": { + "login": "cpp-linter", + "id": 103884627, + "node_id": "O_kgDOBjEnUw", + "avatar_url": "https://avatars.githubusercontent.com/u/103884627?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/cpp-linter", + "html_url": "https://github.com/cpp-linter", + "followers_url": "https://api.github.com/users/cpp-linter/followers", + "following_url": "https://api.github.com/users/cpp-linter/following{/other_user}", + "gists_url": "https://api.github.com/users/cpp-linter/gists{/gist_id}", + "starred_url": "https://api.github.com/users/cpp-linter/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/cpp-linter/subscriptions", + "organizations_url": "https://api.github.com/users/cpp-linter/orgs", + "repos_url": "https://api.github.com/users/cpp-linter/repos", + "events_url": "https://api.github.com/users/cpp-linter/events{/privacy}", + "received_events_url": "https://api.github.com/users/cpp-linter/received_events", + "type": "Organization", + "site_admin": false + }, + "html_url": "https://github.com/cpp-linter/test-cpp-linter-action", + "description": "Test cpp-linter-action", + "fork": false, + "url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action", + "forks_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/forks", + "keys_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/keys{/key_id}", + "collaborators_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/collaborators{/collaborator}", + "teams_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/teams", + "hooks_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/hooks", + "issue_events_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/issues/events{/number}", + "events_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/events", + "assignees_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/assignees{/user}", + "branches_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/branches{/branch}", + "tags_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/tags", + "blobs_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/git/blobs{/sha}", + "git_tags_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/git/tags{/sha}", + "git_refs_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/git/refs{/sha}", + "trees_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/git/trees{/sha}", + "statuses_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/statuses/{sha}", + "languages_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/languages", + "stargazers_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/stargazers", + "contributors_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contributors", + "subscribers_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/subscribers", + "subscription_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/subscription", + "commits_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/commits{/sha}", + "git_commits_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/git/commits{/sha}", + "comments_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/comments{/number}", + "issue_comment_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/issues/comments{/number}", + "contents_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contents/{+path}", + "compare_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/compare/{base}...{head}", + "merges_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/merges", + "archive_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/{archive_format}{/ref}", + "downloads_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/downloads", + "issues_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/issues{/number}", + "pulls_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls{/number}", + "milestones_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/milestones{/number}", + "notifications_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/notifications{?since,all,participating}", + "labels_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/labels{/name}", + "releases_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/releases{/id}", + "deployments_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/deployments", + "created_at": "2021-04-26T03:34:07Z", + "updated_at": "2022-06-19T12:00:43Z", + "pushed_at": "2024-01-06T22:20:56Z", + "git_url": "git://github.com/cpp-linter/test-cpp-linter-action.git", + "ssh_url": "git@github.com:cpp-linter/test-cpp-linter-action.git", + "clone_url": "https://github.com/cpp-linter/test-cpp-linter-action.git", + "svn_url": "https://github.com/cpp-linter/test-cpp-linter-action", + "homepage": "https://github.com/cpp-linter/cpp-linter-action", + "size": 86, + "stargazers_count": 0, + "watchers_count": 0, + "language": "C++", + "has_issues": true, + "has_projects": true, + "has_downloads": true, + "has_wiki": true, + "has_pages": false, + "has_discussions": false, + "forks_count": 1, + "mirror_url": null, + "archived": false, + "disabled": false, + "open_issues_count": 1, + "license": { + "key": "mit", + "name": "MIT License", + "spdx_id": "MIT", + "url": "https://api.github.com/licenses/mit", + "node_id": "MDc6TGljZW5zZTEz" + }, + "allow_forking": true, + "is_template": false, + "web_commit_signoff_required": false, + "topics": [ + "clang-format", + "clang-tidy", + "cpp", + "demo" + ], + "visibility": "public", + "forks": 1, + "open_issues": 1, + "watchers": 0, + "default_branch": "master" + } + }, + "base": { + "label": "cpp-linter:master", + "ref": "master", + "sha": "1202cbe585a650dfb6f79ecafe85e0d352e11288", + "user": { + "login": "cpp-linter", + "id": 103884627, + "node_id": "O_kgDOBjEnUw", + "avatar_url": "https://avatars.githubusercontent.com/u/103884627?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/cpp-linter", + "html_url": "https://github.com/cpp-linter", + "followers_url": "https://api.github.com/users/cpp-linter/followers", + "following_url": "https://api.github.com/users/cpp-linter/following{/other_user}", + "gists_url": "https://api.github.com/users/cpp-linter/gists{/gist_id}", + "starred_url": "https://api.github.com/users/cpp-linter/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/cpp-linter/subscriptions", + "organizations_url": "https://api.github.com/users/cpp-linter/orgs", + "repos_url": "https://api.github.com/users/cpp-linter/repos", + "events_url": "https://api.github.com/users/cpp-linter/events{/privacy}", + "received_events_url": "https://api.github.com/users/cpp-linter/received_events", + "type": "Organization", + "site_admin": false + }, + "repo": { + "id": 361606860, + "node_id": "MDEwOlJlcG9zaXRvcnkzNjE2MDY4NjA=", + "name": "test-cpp-linter-action", + "full_name": "cpp-linter/test-cpp-linter-action", + "private": false, + "owner": { + "login": "cpp-linter", + "id": 103884627, + "node_id": "O_kgDOBjEnUw", + "avatar_url": "https://avatars.githubusercontent.com/u/103884627?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/cpp-linter", + "html_url": "https://github.com/cpp-linter", + "followers_url": "https://api.github.com/users/cpp-linter/followers", + "following_url": "https://api.github.com/users/cpp-linter/following{/other_user}", + "gists_url": "https://api.github.com/users/cpp-linter/gists{/gist_id}", + "starred_url": "https://api.github.com/users/cpp-linter/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/cpp-linter/subscriptions", + "organizations_url": "https://api.github.com/users/cpp-linter/orgs", + "repos_url": "https://api.github.com/users/cpp-linter/repos", + "events_url": "https://api.github.com/users/cpp-linter/events{/privacy}", + "received_events_url": "https://api.github.com/users/cpp-linter/received_events", + "type": "Organization", + "site_admin": false + }, + "html_url": "https://github.com/cpp-linter/test-cpp-linter-action", + "description": "Test cpp-linter-action", + "fork": false, + "url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action", + "forks_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/forks", + "keys_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/keys{/key_id}", + "collaborators_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/collaborators{/collaborator}", + "teams_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/teams", + "hooks_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/hooks", + "issue_events_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/issues/events{/number}", + "events_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/events", + "assignees_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/assignees{/user}", + "branches_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/branches{/branch}", + "tags_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/tags", + "blobs_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/git/blobs{/sha}", + "git_tags_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/git/tags{/sha}", + "git_refs_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/git/refs{/sha}", + "trees_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/git/trees{/sha}", + "statuses_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/statuses/{sha}", + "languages_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/languages", + "stargazers_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/stargazers", + "contributors_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contributors", + "subscribers_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/subscribers", + "subscription_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/subscription", + "commits_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/commits{/sha}", + "git_commits_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/git/commits{/sha}", + "comments_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/comments{/number}", + "issue_comment_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/issues/comments{/number}", + "contents_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contents/{+path}", + "compare_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/compare/{base}...{head}", + "merges_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/merges", + "archive_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/{archive_format}{/ref}", + "downloads_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/downloads", + "issues_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/issues{/number}", + "pulls_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls{/number}", + "milestones_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/milestones{/number}", + "notifications_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/notifications{?since,all,participating}", + "labels_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/labels{/name}", + "releases_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/releases{/id}", + "deployments_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/deployments", + "created_at": "2021-04-26T03:34:07Z", + "updated_at": "2022-06-19T12:00:43Z", + "pushed_at": "2024-01-06T22:20:56Z", + "git_url": "git://github.com/cpp-linter/test-cpp-linter-action.git", + "ssh_url": "git@github.com:cpp-linter/test-cpp-linter-action.git", + "clone_url": "https://github.com/cpp-linter/test-cpp-linter-action.git", + "svn_url": "https://github.com/cpp-linter/test-cpp-linter-action", + "homepage": "https://github.com/cpp-linter/cpp-linter-action", + "size": 86, + "stargazers_count": 0, + "watchers_count": 0, + "language": "C++", + "has_issues": true, + "has_projects": true, + "has_downloads": true, + "has_wiki": true, + "has_pages": false, + "has_discussions": false, + "forks_count": 1, + "mirror_url": null, + "archived": false, + "disabled": false, + "open_issues_count": 1, + "license": { + "key": "mit", + "name": "MIT License", + "spdx_id": "MIT", + "url": "https://api.github.com/licenses/mit", + "node_id": "MDc6TGljZW5zZTEz" + }, + "allow_forking": true, + "is_template": false, + "web_commit_signoff_required": false, + "topics": [ + "clang-format", + "clang-tidy", + "cpp", + "demo" + ], + "visibility": "public", + "forks": 1, + "open_issues": 1, + "watchers": 0, + "default_branch": "master" + } + }, + "_links": { + "self": { + "href": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/27" + }, + "html": { + "href": "https://github.com/cpp-linter/test-cpp-linter-action/pull/27" + }, + "issue": { + "href": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/issues/27" + }, + "comments": { + "href": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/issues/27/comments" + }, + "review_comments": { + "href": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/27/comments" + }, + "review_comment": { + "href": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/comments{/number}" + }, + "commits": { + "href": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/27/commits" + }, + "statuses": { + "href": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/statuses/a09a2032511f6a61f9216b24d2cd480c923d333f" + } + }, + "author_association": "COLLABORATOR", + "auto_merge": null, + "active_lock_reason": null, + "merged": false, + "mergeable": true, + "rebaseable": true, + "mergeable_state": "clean", + "merged_by": null, + "comments": 1, + "review_comments": 3, + "maintainer_can_modify": false, + "commits": 3, + "additions": 22, + "deletions": 23, + "changed_files": 3 +} diff --git a/tests/reviews/pr_review_comments.json b/tests/reviews/pr_review_comments.json new file mode 100644 index 0000000..4ca723b --- /dev/null +++ b/tests/reviews/pr_review_comments.json @@ -0,0 +1,206 @@ +[ + { + "url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/comments/1443866659", + "pull_request_review_id": 1807607546, + "id": 1443866659, + "node_id": "PRRC_kwDOFY2uzM5WD6gj", + "diff_hunk": "@@ -1,17 +1,18 @@\n /** This is a very ugly test code (doomed to fail linting) */\n #include \"demo.hpp\"\n-#include \n-#include \n+#include \n \n-// using size_t from cstddef\n-size_t dummyFunc(size_t i) { return i; }\n \n-int main()\n-{\n- for (;;)\n- break;\n+\n+\n+int main(){\n+\n+ for (;;) break;\n+\n \n printf(\"Hello world!\\n\");", + "path": "src/demo.cpp", + "commit_id": "a09a2032511f6a61f9216b24d2cd480c923d333f", + "original_commit_id": "a09a2032511f6a61f9216b24d2cd480c923d333f", + "user": { + "login": "github-actions[bot]", + "id": 41898282, + "node_id": "MDM6Qm90NDE4OTgyODI=", + "avatar_url": "https://avatars.githubusercontent.com/in/15368?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/github-actions%5Bbot%5D", + "html_url": "https://github.com/apps/github-actions", + "followers_url": "https://api.github.com/users/github-actions%5Bbot%5D/followers", + "following_url": "https://api.github.com/users/github-actions%5Bbot%5D/following{/other_user}", + "gists_url": "https://api.github.com/users/github-actions%5Bbot%5D/gists{/gist_id}", + "starred_url": "https://api.github.com/users/github-actions%5Bbot%5D/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/github-actions%5Bbot%5D/subscriptions", + "organizations_url": "https://api.github.com/users/github-actions%5Bbot%5D/orgs", + "repos_url": "https://api.github.com/users/github-actions%5Bbot%5D/repos", + "events_url": "https://api.github.com/users/github-actions%5Bbot%5D/events{/privacy}", + "received_events_url": "https://api.github.com/users/github-actions%5Bbot%5D/received_events", + "type": "Bot", + "site_admin": false + }, + "body": "### clang-format suggestions\n\n```suggestion\n\r\nint main()\r\n{\r\n\r\n for (;;)\r\n break;\r\n\r\n```", + "created_at": "2024-01-06T22:22:31Z", + "updated_at": "2024-01-06T22:22:32Z", + "html_url": "https://github.com/cpp-linter/test-cpp-linter-action/pull/27#discussion_r1443866659", + "pull_request_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/27", + "author_association": "NONE", + "_links": { + "self": { + "href": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/comments/1443866659" + }, + "html": { + "href": "https://github.com/cpp-linter/test-cpp-linter-action/pull/27#discussion_r1443866659" + }, + "pull_request": { + "href": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/27" + } + }, + "reactions": { + "url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/comments/1443866659/reactions", + "total_count": 0, + "+1": 0, + "-1": 0, + "laugh": 0, + "hooray": 0, + "confused": 0, + "heart": 0, + "rocket": 0, + "eyes": 0 + }, + "start_line": 4, + "original_start_line": 4, + "start_side": "RIGHT", + "line": 13, + "original_line": 13, + "side": "RIGHT", + "original_position": 21, + "position": 21, + "subject_type": "line" + }, + { + "url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/comments/1443866660", + "pull_request_review_id": 1807607546, + "id": 1443866660, + "node_id": "PRRC_kwDOFY2uzM5WD6gk", + "diff_hunk": "@@ -1,17 +1,18 @@\n /** This is a very ugly test code (doomed to fail linting) */\n #include \"demo.hpp\"\n-#include \n-#include \n+#include \n \n-// using size_t from cstddef\n-size_t dummyFunc(size_t i) { return i; }\n \n-int main()\n-{\n- for (;;)\n- break;\n+\n+\n+int main(){\n+\n+ for (;;) break;\n+\n \n printf(\"Hello world!\\n\");", + "path": "src/demo.cpp", + "commit_id": "a09a2032511f6a61f9216b24d2cd480c923d333f", + "original_commit_id": "a09a2032511f6a61f9216b24d2cd480c923d333f", + "user": { + "login": "github-actions[bot]", + "id": 41898282, + "node_id": "MDM6Qm90NDE4OTgyODI=", + "avatar_url": "https://avatars.githubusercontent.com/in/15368?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/github-actions%5Bbot%5D", + "html_url": "https://github.com/apps/github-actions", + "followers_url": "https://api.github.com/users/github-actions%5Bbot%5D/followers", + "following_url": "https://api.github.com/users/github-actions%5Bbot%5D/following{/other_user}", + "gists_url": "https://api.github.com/users/github-actions%5Bbot%5D/gists{/gist_id}", + "starred_url": "https://api.github.com/users/github-actions%5Bbot%5D/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/github-actions%5Bbot%5D/subscriptions", + "organizations_url": "https://api.github.com/users/github-actions%5Bbot%5D/orgs", + "repos_url": "https://api.github.com/users/github-actions%5Bbot%5D/repos", + "events_url": "https://api.github.com/users/github-actions%5Bbot%5D/events{/privacy}", + "received_events_url": "https://api.github.com/users/github-actions%5Bbot%5D/received_events", + "type": "Bot", + "site_admin": false + }, + "body": "### clang-tidy diagnostics\n- inclusion of deprecated C++ header 'stdio.h'; consider using 'cstdio' instead [[modernize-deprecated-headers](https://clang.llvm.org/extra/clang-tidy/checks/modernize/deprecated-headers.html)]\n- use a trailing return type for this function [[modernize-use-trailing-return-type](https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-trailing-return-type.html)]\n- statement should be inside braces [[readability-braces-around-statements](https://clang.llvm.org/extra/clang-tidy/checks/readability/braces-around-statements.html)]\n\n```suggestion\n#include \"demo.hpp\"\r\n#include \r\n\r\nauto main() -> int\r\n{\r\n\r\n for (;;) {\r\n break;\r\n }\r\n\r\n```", + "created_at": "2024-01-06T22:22:31Z", + "updated_at": "2024-01-06T22:22:32Z", + "html_url": "https://github.com/cpp-linter/test-cpp-linter-action/pull/27#discussion_r1443866660", + "pull_request_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/27", + "author_association": "NONE", + "_links": { + "self": { + "href": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/comments/1443866660" + }, + "html": { + "href": "https://github.com/cpp-linter/test-cpp-linter-action/pull/27#discussion_r1443866660" + }, + "pull_request": { + "href": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/27" + } + }, + "reactions": { + "url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/comments/1443866660/reactions", + "total_count": 0, + "+1": 0, + "-1": 0, + "laugh": 0, + "hooray": 0, + "confused": 0, + "heart": 0, + "rocket": 0, + "eyes": 0 + }, + "start_line": 2, + "original_start_line": 2, + "start_side": "RIGHT", + "line": 13, + "original_line": 13, + "side": "RIGHT", + "original_position": 21, + "position": 21, + "subject_type": "line" + }, + { + "url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/comments/1443866662", + "pull_request_review_id": 1807607546, + "id": 1443866662, + "node_id": "PRRC_kwDOFY2uzM5WD6gm", + "diff_hunk": "@@ -5,12 +5,10 @@\n class Dummy {\n char* useless;\n int numb;\n+ Dummy() :numb(0), useless(\"\\0\"){}\n \n public:\n- void *not_usefull(char *str){\n- useless = str;\n- return 0;\n- }\n+ void *not_useful(char *str){useless = str;}\n };\n ", + "path": "src/demo.hpp", + "commit_id": "a09a2032511f6a61f9216b24d2cd480c923d333f", + "original_commit_id": "a09a2032511f6a61f9216b24d2cd480c923d333f", + "user": { + "login": "github-actions[bot]", + "id": 41898282, + "node_id": "MDM6Qm90NDE4OTgyODI=", + "avatar_url": "https://avatars.githubusercontent.com/in/15368?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/github-actions%5Bbot%5D", + "html_url": "https://github.com/apps/github-actions", + "followers_url": "https://api.github.com/users/github-actions%5Bbot%5D/followers", + "following_url": "https://api.github.com/users/github-actions%5Bbot%5D/following{/other_user}", + "gists_url": "https://api.github.com/users/github-actions%5Bbot%5D/gists{/gist_id}", + "starred_url": "https://api.github.com/users/github-actions%5Bbot%5D/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/github-actions%5Bbot%5D/subscriptions", + "organizations_url": "https://api.github.com/users/github-actions%5Bbot%5D/orgs", + "repos_url": "https://api.github.com/users/github-actions%5Bbot%5D/repos", + "events_url": "https://api.github.com/users/github-actions%5Bbot%5D/events{/privacy}", + "received_events_url": "https://api.github.com/users/github-actions%5Bbot%5D/received_events", + "type": "Bot", + "site_admin": false + }, + "body": "### clang-tidy diagnostics\n- use a trailing return type for this function [[modernize-use-trailing-return-type](https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-trailing-return-type.html)]\n\n```suggestion\n public:\r\n auto not_useful(char* str) -> void* { useless = str; }\r\n};\r\n```", + "created_at": "2024-01-06T22:22:31Z", + "updated_at": "2024-01-06T22:22:32Z", + "html_url": "https://github.com/cpp-linter/test-cpp-linter-action/pull/27#discussion_r1443866662", + "pull_request_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/27", + "author_association": "NONE", + "_links": { + "self": { + "href": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/comments/1443866662" + }, + "html": { + "href": "https://github.com/cpp-linter/test-cpp-linter-action/pull/27#discussion_r1443866662" + }, + "pull_request": { + "href": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/27" + } + }, + "reactions": { + "url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/comments/1443866662/reactions", + "total_count": 0, + "+1": 0, + "-1": 0, + "laugh": 0, + "hooray": 0, + "confused": 0, + "heart": 0, + "rocket": 0, + "eyes": 0 + }, + "start_line": 10, + "original_start_line": 10, + "start_side": "RIGHT", + "line": 13, + "original_line": 13, + "side": "RIGHT", + "original_position": 13, + "position": 13, + "subject_type": "line" + } +] diff --git a/tests/reviews/pr_reviews.json b/tests/reviews/pr_reviews.json new file mode 100644 index 0000000..9e46886 --- /dev/null +++ b/tests/reviews/pr_reviews.json @@ -0,0 +1,41 @@ +[ + { + "id": 1807607546, + "node_id": "PRR_kwDOFY2uzM5rveb6", + "user": { + "login": "github-actions[bot]", + "id": 41898282, + "node_id": "MDM6Qm90NDE4OTgyODI=", + "avatar_url": "https://avatars.githubusercontent.com/in/15368?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/github-actions%5Bbot%5D", + "html_url": "https://github.com/apps/github-actions", + "followers_url": "https://api.github.com/users/github-actions%5Bbot%5D/followers", + "following_url": "https://api.github.com/users/github-actions%5Bbot%5D/following{/other_user}", + "gists_url": "https://api.github.com/users/github-actions%5Bbot%5D/gists{/gist_id}", + "starred_url": "https://api.github.com/users/github-actions%5Bbot%5D/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/github-actions%5Bbot%5D/subscriptions", + "organizations_url": "https://api.github.com/users/github-actions%5Bbot%5D/orgs", + "repos_url": "https://api.github.com/users/github-actions%5Bbot%5D/repos", + "events_url": "https://api.github.com/users/github-actions%5Bbot%5D/events{/privacy}", + "received_events_url": "https://api.github.com/users/github-actions%5Bbot%5D/received_events", + "type": "Bot", + "site_admin": false + }, + "body": "\n## Cpp-linter Review\nOnly 1 out of 4 clang-format suggestions fit within this pull request's diff.\n\n
Click here for the full clang-format patch\n\n\n```diff\ndiff --git a/src/demo.cpp b/src/demo.cpp\nindex fc295c3..c522998 100644\n--- a/src/demo.cpp\n+++ b/src/demo.cpp\n@@ -4,9 +4,7 @@\n \r\n+int main()\r\n+{\r\n \r\n-\r\n-\r\n-int main(){\r\n-\r\n- for (;;) break;\r\n-\r\n+ for (;;)\r\n+ break;\r\n \r\n@@ -14,5 +12,3 @@ int main(){\n \r\n-\r\n-\r\n-\r\n- return 0;}\r\n+ return 0;\r\n+}\r\ndiff --git a/src/demo.hpp b/src/demo.hpp\nindex a429f5c..8f92cac 100644\n--- a/src/demo.hpp\n+++ b/src/demo.hpp\n@@ -7,25 +7,12 @@ class Dummy {\n int numb;\r\n- Dummy() :numb(0), useless(\"\\0\"){}\r\n+ Dummy()\r\n+ : numb(0)\r\n+ , useless(\"\\0\")\r\n+ {\r\n+ }\r\n \r\n public:\r\n- void *not_useful(char *str){useless = str;}\r\n+ void* not_useful(char* str) { useless = str; }\r\n };\r\n \r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n struct LongDiff\r\n@@ -33,4 +20,3 @@ struct LongDiff\n \r\n- long diff;\r\n-\r\n+ long diff;\r\n };\r\n\n```\n\n\n
\n\nOnly 2 out of 3 clang-tidy suggestions fit within this pull request's diff.\n\n
Click here for the full clang-tidy patch\n\n\n```diff\ndiff --git a/src/demo.cpp b/src/demo.cpp\nindex fc295c3..b160609 100644\n--- a/src/demo.cpp\n+++ b/src/demo.cpp\n@@ -2,11 +2,10 @@\n #include \"demo.hpp\"\r\n-#include \r\n+#include \r\n \r\n+auto main() -> int\r\n+{\r\n \r\n-\r\n-\r\n-int main(){\r\n-\r\n- for (;;) break;\r\n-\r\n+ for (;;) {\r\n+ break;\r\n+ }\r\n \r\n@@ -17,2 +16,3 @@ int main(){\n \r\n- return 0;}\r\n+ return 0;\r\n+}\r\ndiff --git a/src/demo.hpp b/src/demo.hpp\nindex a429f5c..2591c48 100644\n--- a/src/demo.hpp\n+++ b/src/demo.hpp\n@@ -10,3 +10,3 @@ class Dummy {\n public:\r\n- void *not_useful(char *str){useless = str;}\r\n+ auto not_useful(char* str) -> void* { useless = str; }\r\n };\r\n\n```\n\n\n
\n\n", + "state": "CHANGES_REQUESTED", + "html_url": "https://github.com/cpp-linter/test-cpp-linter-action/pull/27#pullrequestreview-1807607546", + "pull_request_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/27", + "author_association": "NONE", + "_links": { + "html": { + "href": "https://github.com/cpp-linter/test-cpp-linter-action/pull/27#pullrequestreview-1807607546" + }, + "pull_request": { + "href": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/27" + } + }, + "submitted_at": "2024-01-06T22:22:32Z", + "commit_id": "a09a2032511f6a61f9216b24d2cd480c923d333f" + } +] diff --git a/tests/reviews/test_pr_review.py b/tests/reviews/test_pr_review.py new file mode 100644 index 0000000..f65ab36 --- /dev/null +++ b/tests/reviews/test_pr_review.py @@ -0,0 +1,143 @@ +import json +from os import environ +from pathlib import Path +import shutil +import requests_mock +import pytest + +from cpp_linter.rest_api.github_api import GithubApiClient +from cpp_linter.clang_tools import capture_clang_tools_output + +TEST_REPO = "cpp-linter/test-cpp-linter-action" +TEST_PR = 27 + + +@pytest.mark.parametrize("is_draft", [True, False], ids=["draft", "ready"]) +@pytest.mark.parametrize("with_token", [True, False], ids=["has_token", "no_token"]) +@pytest.mark.parametrize("tidy_review", [True, False], ids=["yes_tidy", "no_tidy"]) +@pytest.mark.parametrize( + "format_review", [True, False], ids=["yes_format", "no_format"] +) +def test_post_review( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + is_draft: bool, + with_token: bool, + tidy_review: bool, + format_review: bool, +): + """A mock test of posting PR reviews""" + # patch env vars + event_payload = {"number": TEST_PR, "repository": {"private": False}} + event_payload_path = tmp_path / "event_payload.json" + event_payload_path.write_text(json.dumps(event_payload), encoding="utf-8") + monkeypatch.setenv("GITHUB_EVENT_PATH", str(event_payload_path)) + monkeypatch.setenv("CI", "true") + if with_token: + monkeypatch.setenv("GITHUB_TOKEN", "123456") + monkeypatch.chdir(str(tmp_path)) + (tmp_path / "src").mkdir() + demo_dir = Path(__file__).parent.parent / "demo" + shutil.copyfile(str(demo_dir / "demo.cpp"), str(tmp_path / "src" / "demo.cpp")) + shutil.copyfile(str(demo_dir / "demo.hpp"), str(tmp_path / "src" / "demo.hpp")) + cache_path = Path(__file__).parent + shutil.copyfile( + str(cache_path / ".clang-format"), str(tmp_path / "src" / ".clang-format") + ) + shutil.copyfile( + str(cache_path / ".clang-tidy"), str(tmp_path / "src" / ".clang-tidy") + ) + + gh_client = GithubApiClient() + gh_client.repo = TEST_REPO + gh_client.event_name = "pull_request" + + with requests_mock.Mocker() as mock: + base_url = f"{gh_client.api_url}/repos/{TEST_REPO}/pulls/{TEST_PR}" + # load mock responses for pull_request event + mock.get( + base_url, + headers={"Accept": "application/vnd.github.diff"}, + text=(cache_path / f"pr_{TEST_PR}.diff").read_text(encoding="utf-8"), + ) + reviews = (cache_path / "pr_reviews.json").read_text(encoding="utf-8") + mock.get( + f"{base_url}/reviews", + text=reviews, + # to trigger a logged error, we'll modify the status code here + status_code=404 if tidy_review and not format_review else 200, + ) + mock.get( + f"{base_url}/comments", + text=(cache_path / "pr_review_comments.json").read_text(encoding="utf-8"), + ) + + # acknowledge any PUT and POST requests about specific reviews + mock.post(f"{base_url}/reviews") + for review_id in [r["id"] for r in json.loads(reviews) if "id" in r]: + mock.put(f"{base_url}/reviews/{review_id}/dismissals") + + # run the actual test + files = gh_client.get_list_of_changed_files( + extensions=["cpp", "hpp"], + ignored=[], + not_ignored=[], + lines_changed_only=1, + ) + assert files + for file_obj in files: + assert file_obj.additions + format_advice, tidy_advice = capture_clang_tools_output( + files, + version=environ.get("CLANG_VERSION", "16"), + checks="", + style="file", + lines_changed_only=1, + database="", + extra_args=[], + tidy_review=tidy_review, + format_review=format_review, + ) + assert [note for concern in tidy_advice for note in concern.notes] + assert [note for note in format_advice] + + # simulate draft PR by changing the request response + cache_pr_response = (cache_path / f"pr_{TEST_PR}.json").read_text( + encoding="utf-8" + ) + cache_pr_response = cache_pr_response.replace( + ' "draft": true,', f' "draft": {str(is_draft).lower()},', 1 + ) + mock.get( + base_url, + headers={"Accept": "application/vnd.github.text+json"}, + text=cache_pr_response, + ) + gh_client.post_feedback( + files, + format_advice, + tidy_advice, + thread_comments="false", + no_lgtm=True, + step_summary=False, + file_annotations=False, + style="file", + tidy_review=tidy_review, + format_review=format_review, + ) + # save the body of the review json for manual inspection + last_request = mock.last_request + if (tidy_review or format_review) and not is_draft and with_token: + assert hasattr(last_request, "json") + json_payload = last_request.json() + assert "body" in json_payload + if tidy_review: + assert "clang-tidy" in json_payload["body"] + elif format_review: + assert "clang-format" in json_payload["body"] + else: # pragma: no cover + raise RuntimeError("no review payload sent!") + assert hasattr(last_request, "text") + (tmp_path / "review.json").write_text( + json.dumps(json_payload, indent=2), encoding="utf-8" + ) From 0cf2e5ae5fe86f2ed2075903f2e0c55fde7ac1c3 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Sun, 7 Jan 2024 12:05:41 -0800 Subject: [PATCH 04/12] allow 2% drop (maximum) in coverage on CI checks make patch coverage informational (not a gate) --- codecov.yml | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 codecov.yml diff --git a/codecov.yml b/codecov.yml new file mode 100644 index 0000000..448ae18 --- /dev/null +++ b/codecov.yml @@ -0,0 +1,11 @@ +coverage: + status: + patch: + default: + informational: true + project: + default: + target: auto + # adjust accordingly based on how flaky your tests are + # this allows a 2% drop from the previous base commit coverage + threshold: 2% From e9c449dc7691425f61a5bd79be506b9476390e6f Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Sun, 7 Jan 2024 15:01:54 -0800 Subject: [PATCH 05/12] handle odd hunk headers --- cpp_linter/common_fs.py | 22 +++++++++++++++++++--- cpp_linter/git/git_str.py | 8 ++++++-- cpp_linter/rest_api/github_api.py | 14 +++----------- tests/test_git_str.py | 27 +++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 16 deletions(-) diff --git a/cpp_linter/common_fs.py b/cpp_linter/common_fs.py index dda2afa..5a41608 100644 --- a/cpp_linter/common_fs.py +++ b/cpp_linter/common_fs.py @@ -2,6 +2,7 @@ from os.path import commonpath from pathlib import PurePath, Path from typing import List, Dict, Any, Union, Tuple, Optional +from pygit2 import DiffHunk # type: ignore from .loggers import logger, start_log_group #: A path to generated cache artifacts. (only used when verbosity is in debug mode) @@ -98,13 +99,28 @@ def serialize(self) -> Dict[str, Any]: }, } - def is_in_1_hunk(self, start: int, end: int) -> bool: + def is_hunk_contained(self, hunk: DiffHunk) -> Optional[Tuple[int, int]]: """Is a given ``start`` and ``end`` line numbers within a single diff hunk?""" + if hunk.old_lines > 0: + start = hunk.old_start + # span of old_lines is an inclusive range + end = hunk.old_start + hunk.old_lines - 1 + else: # if number of old lines is 0 + # start hunk at new line number + start = hunk.new_start + # make it span 1 line + end = start for hunk in self.diff_chunks: chunk_range = range(hunk[0], hunk[1]) if start in chunk_range and end in chunk_range: - return True - return False + return (start, end) + logger.warning( + "lines %d - %d are not within a single diff hunk for file %s.", + start, + end, + self.name, + ) + return None def is_file_in_list(paths: List[str], file_name: str, prompt: str) -> bool: diff --git a/cpp_linter/git/git_str.py b/cpp_linter/git/git_str.py index 6447e7a..d30bad1 100644 --- a/cpp_linter/git/git_str.py +++ b/cpp_linter/git/git_str.py @@ -11,7 +11,7 @@ DIFF_FILE_NAME = re.compile(r"^\+\+\+\sb?/(.*)$", re.MULTILINE) DIFF_RENAMED_FILE = re.compile(r"^rename to (.*)$", re.MULTILINE) DIFF_BINARY_FILE = re.compile(r"^Binary\sfiles\s", re.MULTILINE) -HUNK_INFO = re.compile(r"@@\s\-\d+,\d+\s\+(\d+,\d+)\s@@", re.MULTILINE) +HUNK_INFO = re.compile(r"^@@\s\-\d+,?\d*\s\+(\d+,?\d*)\s@@", re.MULTILINE) def _get_filename_from_diff(front_matter: str) -> Optional[re.Match]: @@ -94,7 +94,11 @@ def _parse_patch(full_patch: str) -> Tuple[List[List[int]], List[int]]: for index, chunk in enumerate(chunks): if index % 2 == 1: # each odd element holds the starting line number and number of lines - start_line, hunk_length = [int(x) for x in chunk.split(",")] + if "," in chunk: + start_line, hunk_length = [int(x) for x in chunk.split(",")] + else: + start_line = int(chunk) + hunk_length = 1 ranges.append([start_line, hunk_length + start_line]) line_numb_in_diff = start_line continue diff --git a/cpp_linter/rest_api/github_api.py b/cpp_linter/rest_api/github_api.py index 5e7a60f..c51a281 100644 --- a/cpp_linter/rest_api/github_api.py +++ b/cpp_linter/rest_api/github_api.py @@ -420,18 +420,10 @@ def create_review_comments( full_patch += patch.text for hunk in patch.hunks: total += 1 - start_lines, end_lines = ( - hunk.old_start, - hunk.old_start + hunk.old_lines, - ) - if not file.is_in_1_hunk(start_lines, end_lines): - logger.warning( - "lines %d - %d are not within a single diff hunk for file %s.", - start_lines, - end_lines, - file.name, - ) + new_hunk_range = file.is_hunk_contained(hunk) + if new_hunk_range is None: continue + start_lines, end_lines = new_hunk_range comment: Dict[str, Any] = {"path": file.name} body = "" if isinstance(advice, TidyAdvice): diff --git a/tests/test_git_str.py b/tests/test_git_str.py index d4d455c..294313e 100644 --- a/tests/test_git_str.py +++ b/tests/test_git_str.py @@ -75,3 +75,30 @@ def test_ignored_diff(): files = parse_diff_str(TYPICAL_DIFF, ["hpp"], [], [], 0) # binary files are ignored during parsing assert not files + + +def test_terse_hunk_header(): + """For coverage completeness""" + diff_str = "\n".join( + [ + "diff --git a/src/demo.cpp b/src/demo.cpp", + "--- a/src/demo.cpp", + "+++ b/src/demo.cpp", + "@@ -3 +3 @@", + "-#include ", + "+#include ", + "@@ -4,0 +5,2 @@", + "+auto main() -> int", + "+{", + "@@ -18 +17,2 @@ int main(){", + "- return 0;}", + "+ return 0;", + "+}", + ] + ) + files = parse_diff_str(diff_str, ["cpp"], [], [], 0) + assert files + assert files[0].diff_chunks == [[3, 4], [5, 7], [17, 19]] + git_files = parse_diff(diff_str, ["cpp"], [], [], 0) + assert git_files + assert files[0].diff_chunks == git_files[0].diff_chunks From 580d3859e4f4d1f9666757be820b82d70eb29bd3 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Sun, 7 Jan 2024 16:17:31 -0800 Subject: [PATCH 06/12] get files changes for review when analyzing all files --- cpp_linter/__init__.py | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/cpp_linter/__init__.py b/cpp_linter/__init__.py index 3c86e3f..08b7526 100644 --- a/cpp_linter/__init__.py +++ b/cpp_linter/__init__.py @@ -47,10 +47,27 @@ def main(): not_ignored, args.lines_changed_only, ) - if files: - rest_api_client.verify_files_are_present(files) + rest_api_client.verify_files_are_present(files) else: files = list_source_files(args.extensions, ignored, not_ignored) + # at this point, files have no info about git changes. + # for PR reviews, we need this info + if is_pr_event and (args.tidy_review or args.format_review): + # get file changes from diff + git_changes = rest_api_client.get_list_of_changed_files( + args.extensions, + ignored, + not_ignored, + lines_changed_only=0, # prevent filtering out unchanged files + ) + # merge info from git changes into list of all files + for git_file in git_changes: + for file in files: + if git_file.name == file.name: + file.additions = git_file.additions + file.diff_chunks = git_file.diff_chunks + file.lines_added = git_file.lines_added + break if not files: logger.info("No source files need checking!") else: @@ -82,8 +99,8 @@ def main(): args.step_summary, args.file_annotations, args.style, - is_pr_event and args.format_review, - is_pr_event and args.tidy_review, + args.format_review, + args.tidy_review, ) end_log_group() From 455acc674ab0ffb5c3215bf2727329ff84e62153 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Sun, 7 Jan 2024 17:13:35 -0800 Subject: [PATCH 07/12] reviewing the reviews --- cpp_linter/rest_api/github_api.py | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/cpp_linter/rest_api/github_api.py b/cpp_linter/rest_api/github_api.py index c51a281..6d9cae4 100644 --- a/cpp_linter/rest_api/github_api.py +++ b/cpp_linter/rest_api/github_api.py @@ -427,21 +427,32 @@ def create_review_comments( comment: Dict[str, Any] = {"path": file.name} body = "" if isinstance(advice, TidyAdvice): - body += "### clang-tidy diagnostics\n" + body += "### clang-tidy " + diagnostics = "" for note in advice.notes: - if note.line in range(start_lines, end_lines): - body += f"- {note.rationale} [{note.diagnostic_link}]\n" + if note.line in range(start_lines, end_lines + 1): + diagnostics += ( + f"- {note.rationale} [{note.diagnostic_link}]\n" + ) + if diagnostics: + body += "diagnostics\n" + diagnostics + else: + body += "suggestions\n" else: body += "### clang-format suggestions\n" if start_lines < end_lines: comment["start_line"] = start_lines comment["line"] = end_lines suggestion = "" + removed = [] for line in hunk.lines: if line.origin in ["+", " "]: suggestion += line.content - if not suggestion: - body += "\nPlease remove this line(s)." + else: + removed.append(line.old_lineno) + if not suggestion and removed: + body += "\nPlease remove the line(s)\n- " + body += "\n- ".join([str(x) for x in removed]) else: body += f"\n```suggestion\n{suggestion}```" comment["body"] = body From 30691d3d1e83f85df43b477ebbc525738211750f Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Sun, 7 Jan 2024 23:57:26 -0800 Subject: [PATCH 08/12] Some refactoring; Improve review comment content --- cpp_linter/clang_tools/clang_tidy.py | 9 ++++++++ cpp_linter/common_fs.py | 12 +++++++++- cpp_linter/rest_api/__init__.py | 14 ++++++++---- cpp_linter/rest_api/github_api.py | 21 ++++++++--------- tests/reviews/pr_reviews.json | 2 +- tests/reviews/test_pr_review.py | 34 ++++++++++++++++++++++------ 6 files changed, 66 insertions(+), 26 deletions(-) diff --git a/cpp_linter/clang_tools/clang_tidy.py b/cpp_linter/clang_tools/clang_tidy.py index ac76a8d..a4ca2fb 100644 --- a/cpp_linter/clang_tools/clang_tidy.py +++ b/cpp_linter/clang_tools/clang_tidy.py @@ -94,6 +94,15 @@ def __init__(self, notes: List[TidyNotification]) -> None: self.patched: Optional[bytes] = None self.notes = notes + def diagnostics_in_range(self, start: int, end: int) -> str: + """Get a markdown formatted list of diagnostics found between a ``start`` + and ``end`` range of lines.""" + diagnostics = "" + for note in self.notes: + if note.line in range(start, end + 1): # range is inclusive + diagnostics += f"- {note.rationale} [{note.diagnostic_link}]\n" + return diagnostics + def run_clang_tidy( command: str, diff --git a/cpp_linter/common_fs.py b/cpp_linter/common_fs.py index 5a41608..6120dd2 100644 --- a/cpp_linter/common_fs.py +++ b/cpp_linter/common_fs.py @@ -100,7 +100,17 @@ def serialize(self) -> Dict[str, Any]: } def is_hunk_contained(self, hunk: DiffHunk) -> Optional[Tuple[int, int]]: - """Is a given ``start`` and ``end`` line numbers within a single diff hunk?""" + """Does a given ``hunk`` start and end within a single diff hunk? + + This also includes some compensations for hunk headers that are oddly formed. + + .. tip:: This is mostly useful to create comments that can be posted within a + git changes' diff. Ideally, designed for PR reviews based on patches + generated by clang tools' output. + + :returns: The appropriate starting and ending line numbers of the given hunk. + If hunk cannot fit in a single hunk, this returns `None`. + """ if hunk.old_lines > 0: start = hunk.old_start # span of old_lines is an inclusive range diff --git a/cpp_linter/rest_api/__init__.py b/cpp_linter/rest_api/__init__.py index e5345bf..1d47c0c 100644 --- a/cpp_linter/rest_api/__init__.py +++ b/cpp_linter/rest_api/__init__.py @@ -8,6 +8,13 @@ from ..loggers import logger +USER_OUTREACH = ( + "\n\nHave any feedback or feature suggestions? [Share it here.]" + + "(https://github.com/cpp-linter/cpp-linter-action/issues)" +) +COMMENT_MARKER = "\n" + + class RestApiClient(ABC): def __init__(self) -> None: self.session = requests.Session() @@ -114,7 +121,7 @@ def make_comment( else: logger.debug("%s != %s", file_obj.name, note.filename) - comment = "\n# Cpp-Linter Report " + comment = f"{COMMENT_MARKER}# Cpp-Linter Report " if format_comment or tidy_comment: comment += ":warning:\nSome files did not pass the configured checks!\n" if format_comment: @@ -127,10 +134,7 @@ def make_comment( comment += f"{tidy_comment}\n" else: comment += ":heavy_check_mark:\nNo problems need attention." - comment += ( - "\n\nHave any feedback or feature suggestions? [Share it here.]" - + "(https://github.com/cpp-linter/cpp-linter-action/issues)" - ) + comment += USER_OUTREACH return (comment, format_checks_failed, tidy_checks_failed) def post_feedback( diff --git a/cpp_linter/rest_api/github_api.py b/cpp_linter/rest_api/github_api.py index 6d9cae4..7de052b 100644 --- a/cpp_linter/rest_api/github_api.py +++ b/cpp_linter/rest_api/github_api.py @@ -21,7 +21,7 @@ from ..clang_tools.clang_tidy import TidyAdvice from ..loggers import start_log_group, logger, log_response_msg, log_commander from ..git import parse_diff, get_diff -from . import RestApiClient +from . import RestApiClient, USER_OUTREACH, COMMENT_MARKER class GithubApiClient(RestApiClient): @@ -323,7 +323,7 @@ def remove_bot_comments( for comment in comments: # only search for comments that begin with a specific html comment. # the specific html comment is our action's name - if cast(str, comment["body"]).startswith(""): + if cast(str, comment["body"]).startswith(COMMENT_MARKER): logger.debug( "comment id %d from user %s (%d)", comment["id"], @@ -367,7 +367,7 @@ def post_review( self._dismiss_stale_reviews(url) if is_draft: return # don't post reviews for PRs marked as "draft" - body = "\n## Cpp-linter Review\n" + body = f"{COMMENT_MARKER}## Cpp-linter Review\n" payload_comments = [] total_changes = 0 for index, tool_advice in enumerate([format_advice, tidy_advice]): @@ -384,10 +384,14 @@ def post_review( if patch: body += f"\n
Click here for the full {tool} patch" body += f"\n\n\n```diff\n{patch}\n```\n\n\n
\n\n" + else: + body += f"No objections from {tool}." if total_changes: event = "REQUEST_CHANGES" else: + body += "\nGreat job!" event = "APPROVE" + body += USER_OUTREACH payload = { "body": body, "event": event, @@ -428,12 +432,7 @@ def create_review_comments( body = "" if isinstance(advice, TidyAdvice): body += "### clang-tidy " - diagnostics = "" - for note in advice.notes: - if note.line in range(start_lines, end_lines + 1): - diagnostics += ( - f"- {note.rationale} [{note.diagnostic_link}]\n" - ) + diagnostics = advice.diagnostics_in_range(start_lines, end_lines) if diagnostics: body += "diagnostics\n" + diagnostics else: @@ -470,9 +469,7 @@ def _dismiss_stale_reviews(self, url: str): for review in reviews: if ( "body" in review - and cast(str, review["body"]).startswith( - "\n" - ) + and cast(str, review["body"]).startswith(COMMENT_MARKER) and "state" in review and review["state"] not in ["PENDING", "DISMISSED"] ): diff --git a/tests/reviews/pr_reviews.json b/tests/reviews/pr_reviews.json index 9e46886..6e6210f 100644 --- a/tests/reviews/pr_reviews.json +++ b/tests/reviews/pr_reviews.json @@ -22,7 +22,7 @@ "type": "Bot", "site_admin": false }, - "body": "\n## Cpp-linter Review\nOnly 1 out of 4 clang-format suggestions fit within this pull request's diff.\n\n
Click here for the full clang-format patch\n\n\n```diff\ndiff --git a/src/demo.cpp b/src/demo.cpp\nindex fc295c3..c522998 100644\n--- a/src/demo.cpp\n+++ b/src/demo.cpp\n@@ -4,9 +4,7 @@\n \r\n+int main()\r\n+{\r\n \r\n-\r\n-\r\n-int main(){\r\n-\r\n- for (;;) break;\r\n-\r\n+ for (;;)\r\n+ break;\r\n \r\n@@ -14,5 +12,3 @@ int main(){\n \r\n-\r\n-\r\n-\r\n- return 0;}\r\n+ return 0;\r\n+}\r\ndiff --git a/src/demo.hpp b/src/demo.hpp\nindex a429f5c..8f92cac 100644\n--- a/src/demo.hpp\n+++ b/src/demo.hpp\n@@ -7,25 +7,12 @@ class Dummy {\n int numb;\r\n- Dummy() :numb(0), useless(\"\\0\"){}\r\n+ Dummy()\r\n+ : numb(0)\r\n+ , useless(\"\\0\")\r\n+ {\r\n+ }\r\n \r\n public:\r\n- void *not_useful(char *str){useless = str;}\r\n+ void* not_useful(char* str) { useless = str; }\r\n };\r\n \r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n struct LongDiff\r\n@@ -33,4 +20,3 @@ struct LongDiff\n \r\n- long diff;\r\n-\r\n+ long diff;\r\n };\r\n\n```\n\n\n
\n\nOnly 2 out of 3 clang-tidy suggestions fit within this pull request's diff.\n\n
Click here for the full clang-tidy patch\n\n\n```diff\ndiff --git a/src/demo.cpp b/src/demo.cpp\nindex fc295c3..b160609 100644\n--- a/src/demo.cpp\n+++ b/src/demo.cpp\n@@ -2,11 +2,10 @@\n #include \"demo.hpp\"\r\n-#include \r\n+#include \r\n \r\n+auto main() -> int\r\n+{\r\n \r\n-\r\n-\r\n-int main(){\r\n-\r\n- for (;;) break;\r\n-\r\n+ for (;;) {\r\n+ break;\r\n+ }\r\n \r\n@@ -17,2 +16,3 @@ int main(){\n \r\n- return 0;}\r\n+ return 0;\r\n+}\r\ndiff --git a/src/demo.hpp b/src/demo.hpp\nindex a429f5c..2591c48 100644\n--- a/src/demo.hpp\n+++ b/src/demo.hpp\n@@ -10,3 +10,3 @@ class Dummy {\n public:\r\n- void *not_useful(char *str){useless = str;}\r\n+ auto not_useful(char* str) -> void* { useless = str; }\r\n };\r\n\n```\n\n\n
\n\n", + "body": "\n## Cpp-linter Review\nOnly 1 out of 4 clang-format suggestions fit within this pull request's diff.\n\n
Click here for the full clang-format patch\n\n\n```diff\ndiff --git a/src/demo.cpp b/src/demo.cpp\nindex fc295c3..c522998 100644\n--- a/src/demo.cpp\n+++ b/src/demo.cpp\n@@ -4,9 +4,7 @@\n \r\n+int main()\r\n+{\r\n \r\n-\r\n-\r\n-int main(){\r\n-\r\n- for (;;) break;\r\n-\r\n+ for (;;)\r\n+ break;\r\n \r\n@@ -14,5 +12,3 @@ int main(){\n \r\n-\r\n-\r\n-\r\n- return 0;}\r\n+ return 0;\r\n+}\r\ndiff --git a/src/demo.hpp b/src/demo.hpp\nindex a429f5c..8f92cac 100644\n--- a/src/demo.hpp\n+++ b/src/demo.hpp\n@@ -7,25 +7,12 @@ class Dummy {\n int numb;\r\n- Dummy() :numb(0), useless(\"\\0\"){}\r\n+ Dummy()\r\n+ : numb(0)\r\n+ , useless(\"\\0\")\r\n+ {\r\n+ }\r\n \r\n public:\r\n- void *not_useful(char *str){useless = str;}\r\n+ void* not_useful(char* str) { useless = str; }\r\n };\r\n \r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n struct LongDiff\r\n@@ -33,4 +20,3 @@ struct LongDiff\n \r\n- long diff;\r\n-\r\n+ long diff;\r\n };\r\n\n```\n\n\n
\n\nOnly 2 out of 3 clang-tidy suggestions fit within this pull request's diff.\n\n
Click here for the full clang-tidy patch\n\n\n```diff\ndiff --git a/src/demo.cpp b/src/demo.cpp\nindex fc295c3..b160609 100644\n--- a/src/demo.cpp\n+++ b/src/demo.cpp\n@@ -2,11 +2,10 @@\n #include \"demo.hpp\"\r\n-#include \r\n+#include \r\n \r\n+auto main() -> int\r\n+{\r\n \r\n-\r\n-\r\n-int main(){\r\n-\r\n- for (;;) break;\r\n-\r\n+ for (;;) {\r\n+ break;\r\n+ }\r\n \r\n@@ -17,2 +16,3 @@ int main(){\n \r\n- return 0;}\r\n+ return 0;\r\n+}\r\ndiff --git a/src/demo.hpp b/src/demo.hpp\nindex a429f5c..2591c48 100644\n--- a/src/demo.hpp\n+++ b/src/demo.hpp\n@@ -10,3 +10,3 @@ class Dummy {\n public:\r\n- void *not_useful(char *str){useless = str;}\r\n+ auto not_useful(char* str) -> void* { useless = str; }\r\n };\r\n\n```\n\n\n
\n\n", "state": "CHANGES_REQUESTED", "html_url": "https://github.com/cpp-linter/test-cpp-linter-action/pull/27#pullrequestreview-1807607546", "pull_request_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/27", diff --git a/tests/reviews/test_pr_review.py b/tests/reviews/test_pr_review.py index f65ab36..0da4dca 100644 --- a/tests/reviews/test_pr_review.py +++ b/tests/reviews/test_pr_review.py @@ -18,6 +18,12 @@ @pytest.mark.parametrize( "format_review", [True, False], ids=["yes_format", "no_format"] ) +@pytest.mark.parametrize( + "force_approved", [True, False], ids=["approved", "request_changes"] +) +@pytest.mark.parametrize( + "lines_changed_only", [0, 1, 2], ids=["all_lines", "lines_added", "diff_lines"] +) def test_post_review( monkeypatch: pytest.MonkeyPatch, tmp_path: Path, @@ -25,6 +31,8 @@ def test_post_review( with_token: bool, tidy_review: bool, format_review: bool, + force_approved: bool, + lines_changed_only: int, ): """A mock test of posting PR reviews""" # patch env vars @@ -82,24 +90,28 @@ def test_post_review( extensions=["cpp", "hpp"], ignored=[], not_ignored=[], - lines_changed_only=1, + lines_changed_only=lines_changed_only, ) assert files for file_obj in files: - assert file_obj.additions + assert file_obj.diff_chunks + if force_approved: + files.clear() + format_advice, tidy_advice = capture_clang_tools_output( files, version=environ.get("CLANG_VERSION", "16"), checks="", style="file", - lines_changed_only=1, + lines_changed_only=lines_changed_only, database="", extra_args=[], tidy_review=tidy_review, format_review=format_review, ) - assert [note for concern in tidy_advice for note in concern.notes] - assert [note for note in format_advice] + if not force_approved: + assert [note for concern in tidy_advice for note in concern.notes] + assert [note for note in format_advice] # simulate draft PR by changing the request response cache_pr_response = (cache_path / f"pr_{TEST_PR}.json").read_text( @@ -125,18 +137,26 @@ def test_post_review( tidy_review=tidy_review, format_review=format_review, ) - # save the body of the review json for manual inspection + + # inspect the review payload for correctness last_request = mock.last_request if (tidy_review or format_review) and not is_draft and with_token: assert hasattr(last_request, "json") json_payload = last_request.json() assert "body" in json_payload + assert "event" in json_payload if tidy_review: assert "clang-tidy" in json_payload["body"] elif format_review: assert "clang-format" in json_payload["body"] else: # pragma: no cover - raise RuntimeError("no review payload sent!") + raise RuntimeError("review payload is incorrect") + if force_approved: + assert json_payload["event"] == "APPROVE" + else: + assert json_payload["event"] == "REQUEST_CHANGES" + + # save the body of the review json for manual inspection assert hasattr(last_request, "text") (tmp_path / "review.json").write_text( json.dumps(json_payload, indent=2), encoding="utf-8" From 34710e2fe539783f30730b3023314804707423c0 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Mon, 8 Jan 2024 00:18:15 -0800 Subject: [PATCH 09/12] try using check=True on subprocess calls --- cpp_linter/clang_tools/clang_format.py | 2 +- cpp_linter/clang_tools/clang_tidy.py | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/cpp_linter/clang_tools/clang_format.py b/cpp_linter/clang_tools/clang_format.py index 5f93a01..193e309 100644 --- a/cpp_linter/clang_tools/clang_format.py +++ b/cpp_linter/clang_tools/clang_format.py @@ -182,7 +182,7 @@ def run_clang_format( if format_review: del cmds[2] # remove `--output-replacements-xml` flag # get formatted file from stdout - formatted_output = subprocess.run(cmds, capture_output=True) + formatted_output = subprocess.run(cmds, capture_output=True, check=True) # store formatted_output (for comparing later) advice.patched = formatted_output.stdout return advice diff --git a/cpp_linter/clang_tools/clang_tidy.py b/cpp_linter/clang_tools/clang_tidy.py index a4ca2fb..8c1eca6 100644 --- a/cpp_linter/clang_tools/clang_tidy.py +++ b/cpp_linter/clang_tools/clang_tidy.py @@ -182,10 +182,7 @@ def run_clang_tidy( original_buf = Path(file_obj.name).read_bytes() cmds.insert(1, "--fix-errors") # include compiler-suggested fixes # run clang-tidy again to apply any fixes - fixed_result = subprocess.run(cmds, capture_output=True) - if fixed_result.returncode: - # log if any problems encountered (whatever they are) - logger.error("clang-tidy had problems applying fixes to %s", file_obj.name) + subprocess.run(cmds, check=True) # store the modified output from clang-tidy advice.patched = Path(file_obj.name).read_bytes() # re-write original file contents (can probably skip this on CI runners) From ab15cc5233a8b55b673e6c9702e3f9e9740152a3 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Mon, 8 Jan 2024 09:43:31 -0800 Subject: [PATCH 10/12] do not review closed PRs reduce test permutations to only what is needed --- cpp_linter/rest_api/github_api.py | 10 +++--- tests/reviews/pr_27.json | 2 +- tests/reviews/test_pr_review.py | 58 ++++++++++++++++++++++--------- 3 files changed, 48 insertions(+), 22 deletions(-) diff --git a/cpp_linter/rest_api/github_api.py b/cpp_linter/rest_api/github_api.py index 7de052b..565e8a3 100644 --- a/cpp_linter/rest_api/github_api.py +++ b/cpp_linter/rest_api/github_api.py @@ -360,13 +360,15 @@ def post_review( url += "/reviews" is_draft = True if log_response_msg(response_buffer): - is_draft = cast(Dict[str, bool], response_buffer.json()).get("draft", False) + pr_payload = response_buffer.json() + is_draft = cast(Dict[str, bool], pr_payload).get("draft", False) + is_open = cast(Dict[str, str], pr_payload).get("state", "open") == "open" if "GITHUB_TOKEN" not in environ: logger.error("A GITHUB_TOKEN env var is required to post review comments") - return + sys.exit(self.set_exit_code(1)) self._dismiss_stale_reviews(url) - if is_draft: - return # don't post reviews for PRs marked as "draft" + if is_draft or not is_open: # is PR open and ready for review + return # don't post reviews body = f"{COMMENT_MARKER}## Cpp-linter Review\n" payload_comments = [] total_changes = 0 diff --git a/tests/reviews/pr_27.json b/tests/reviews/pr_27.json index 1ce3675..4648845 100644 --- a/tests/reviews/pr_27.json +++ b/tests/reviews/pr_27.json @@ -50,7 +50,7 @@ ], "milestone": null, - "draft": true, + "draft": false, "commits_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/27/commits", "review_comments_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/27/comments", "review_comment_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/comments{/number}", diff --git a/tests/reviews/test_pr_review.py b/tests/reviews/test_pr_review.py index 0da4dca..36efa13 100644 --- a/tests/reviews/test_pr_review.py +++ b/tests/reviews/test_pr_review.py @@ -12,27 +12,41 @@ TEST_PR = 27 -@pytest.mark.parametrize("is_draft", [True, False], ids=["draft", "ready"]) -@pytest.mark.parametrize("with_token", [True, False], ids=["has_token", "no_token"]) -@pytest.mark.parametrize("tidy_review", [True, False], ids=["yes_tidy", "no_tidy"]) @pytest.mark.parametrize( - "format_review", [True, False], ids=["yes_format", "no_format"] -) -@pytest.mark.parametrize( - "force_approved", [True, False], ids=["approved", "request_changes"] -) -@pytest.mark.parametrize( - "lines_changed_only", [0, 1, 2], ids=["all_lines", "lines_added", "diff_lines"] + "is_draft,is_closed,with_token,force_approved,tidy_review,format_review,changes", + [ + (True, False, True, False, False, True, 2), + (False, True, True, False, False, True, 2), + pytest.param( + False, False, False, False, False, True, 2, marks=pytest.mark.xfail + ), + (False, False, True, True, False, True, 2), + (False, False, True, False, True, False, 2), + (False, False, True, False, False, True, 2), + (False, False, True, False, True, True, 1), + (False, False, True, False, True, True, 0), + ], + ids=[ + "draft", + "closed", + "no_token", + "approved", + "tidy", # changes == diff_chunks only + "format", # changes == diff_chunks only + "lines_added", + "all_lines", + ], ) def test_post_review( monkeypatch: pytest.MonkeyPatch, tmp_path: Path, is_draft: bool, + is_closed: bool, with_token: bool, tidy_review: bool, format_review: bool, force_approved: bool, - lines_changed_only: int, + changes: int, ): """A mock test of posting PR reviews""" # patch env vars @@ -90,7 +104,7 @@ def test_post_review( extensions=["cpp", "hpp"], ignored=[], not_ignored=[], - lines_changed_only=lines_changed_only, + lines_changed_only=changes, ) assert files for file_obj in files: @@ -103,7 +117,7 @@ def test_post_review( version=environ.get("CLANG_VERSION", "16"), checks="", style="file", - lines_changed_only=lines_changed_only, + lines_changed_only=changes, database="", extra_args=[], tidy_review=tidy_review, @@ -117,9 +131,14 @@ def test_post_review( cache_pr_response = (cache_path / f"pr_{TEST_PR}.json").read_text( encoding="utf-8" ) - cache_pr_response = cache_pr_response.replace( - ' "draft": true,', f' "draft": {str(is_draft).lower()},', 1 - ) + if is_draft: + cache_pr_response = cache_pr_response.replace( + ' "draft": false,', ' "draft": true,', 1 + ) + if is_closed: + cache_pr_response = cache_pr_response.replace( + ' "state": "open",', ' "state": "closed",', 1 + ) mock.get( base_url, headers={"Accept": "application/vnd.github.text+json"}, @@ -140,7 +159,12 @@ def test_post_review( # inspect the review payload for correctness last_request = mock.last_request - if (tidy_review or format_review) and not is_draft and with_token: + if ( + (tidy_review or format_review) + and not is_draft + and with_token + and not is_closed + ): assert hasattr(last_request, "json") json_payload = last_request.json() assert "body" in json_payload From 2e889db8ddb00daa4a35906fd38ff9aee220607a Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Mon, 8 Jan 2024 10:27:25 -0800 Subject: [PATCH 11/12] reduce permutation of post comments test --- tests/comments/test_comments.py | 83 +++++++++++++++++++-------------- 1 file changed, 49 insertions(+), 34 deletions(-) diff --git a/tests/comments/test_comments.py b/tests/comments/test_comments.py index 34cccb4..af09fde 100644 --- a/tests/comments/test_comments.py +++ b/tests/comments/test_comments.py @@ -16,11 +16,26 @@ @pytest.mark.parametrize("event_name", ["pull_request", "push"]) @pytest.mark.parametrize( - "thread_comments", - ["update", "true", "false", pytest.param("fail", marks=pytest.mark.xfail)], - ids=["updated_only", "only_new", "disable_comment", "no_token"], + "thread_comments,no_lgtm", + [ + ("update", True), + ("update", False), + ("true", True), + ("true", False), + ("false", True), + ("false", False), + pytest.param("fail", False, marks=pytest.mark.xfail), + ], + ids=[ + "updated-lgtm", + "updated-no_lgtm", + "new-lgtm", + "new-no_lgtm", + "disabled-lgtm", + "disabled-no_lgtm", + "no_token", + ], ) -@pytest.mark.parametrize("no_lgtm", [True, False], ids=["no_lgtm", "yes_lgtm"]) def test_post_feedback( monkeypatch: pytest.MonkeyPatch, tmp_path: Path, @@ -80,41 +95,41 @@ def test_post_feedback( with requests_mock.Mocker() as mock: cache_path = Path(__file__).parent base_url = f"{gh_client.api_url}/repos/{TEST_REPO}/" - # load mock responses for pull_request event - mock.get( - f"{base_url}issues/{TEST_PR}", - text=(cache_path / f"pr_{TEST_PR}.json").read_text(encoding="utf-8"), - ) - for i in [1, 2]: + + if event_name == "pull_request": + # load mock responses for pull_request event + mock.get( + f"{base_url}issues/{TEST_PR}", + text=(cache_path / f"pr_{TEST_PR}.json").read_text(encoding="utf-8"), + ) + for i in [1, 2]: + mock.get( + f"{base_url}issues/{TEST_PR}/comments?page={i}", + text=(cache_path / f"pr_comments_pg{i}.json").read_text( + encoding="utf-8" + ), + # to trigger a logged error, we'll modify the response when + # fetching page 2 of old comments and thread-comments is true + status_code=404 if i == 2 and thread_comments == "true" else 200, + ) + else: + # load mock responses for push event + mock.get( + f"{base_url}commits/{TEST_SHA}", + text=(cache_path / f"push_{TEST_SHA}.json").read_text(encoding="utf-8"), + ) mock.get( - f"{base_url}issues/{TEST_PR}/comments?page={i}", - text=(cache_path / f"pr_comments_pg{i}.json").read_text( + f"{base_url}commits/{TEST_SHA}/comments", + text=(cache_path / f"push_comments_{TEST_SHA}.json").read_text( encoding="utf-8" ), - # to trigger a logged error, we'll modify the response when - # fetching page 2 of old comments and thread-comments is true - status_code=404 if i == 2 and thread_comments == "true" else 200 ) - # load mock responses for push event - mock.get( - f"{base_url}commits/{TEST_SHA}", - text=(cache_path / f"push_{TEST_SHA}.json").read_text(encoding="utf-8"), - ) - mock.get( - f"{base_url}commits/{TEST_SHA}/comments", - text=(cache_path / f"push_comments_{TEST_SHA}.json").read_text( - encoding="utf-8" - ), - ) - # acknowledge any DELETE, PATCH, and POST requests about specific comments comment_url = f"{base_url}comments/" - for comment_id in [ - 76453652, - ]: - mock.delete(f"{comment_url}{comment_id}") - mock.patch(f"{comment_url}{comment_id}") + comment_id = 76453652 + mock.delete(f"{comment_url}{comment_id}") + mock.patch(f"{comment_url}{comment_id}") mock.post(f"{base_url}commits/{TEST_SHA}/comments") mock.post(f"{base_url}issues/{TEST_PR}/comments") @@ -124,8 +139,8 @@ def test_post_feedback( tidy_advice, thread_comments, no_lgtm, - step_summary=True, - file_annotations=True, + step_summary=thread_comments == "update" and not no_lgtm, + file_annotations=thread_comments == "update" and no_lgtm, style="llvm", tidy_review=False, format_review=False, From 15d87a77f6d0a1b06d9cb1856c611b6ed609b691 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Wed, 10 Jan 2024 00:24:42 -0800 Subject: [PATCH 12/12] add missing LF for approved comment --- cpp_linter/rest_api/github_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp_linter/rest_api/github_api.py b/cpp_linter/rest_api/github_api.py index 565e8a3..8aed395 100644 --- a/cpp_linter/rest_api/github_api.py +++ b/cpp_linter/rest_api/github_api.py @@ -387,7 +387,7 @@ def post_review( body += f"\n
Click here for the full {tool} patch" body += f"\n\n\n```diff\n{patch}\n```\n\n\n
\n\n" else: - body += f"No objections from {tool}." + body += f"No objections from {tool}.\n" if total_changes: event = "REQUEST_CHANGES" else: