diff --git a/.github/workflows/run-dev-tests.yml b/.github/workflows/run-dev-tests.yml index 2dbedc8d..b7022ecd 100644 --- a/.github/workflows/run-dev-tests.yml +++ b/.github/workflows/run-dev-tests.yml @@ -43,7 +43,7 @@ jobs: matrix: py: ['3.8', '3.9', '3.10', '3.11'] os: ['windows-latest', ubuntu-22.04] - version: ['17', '16', '15', '14', '13', '12', '11', '10', '9', '8', '7'] + version: ['18', '17', '16', '15', '14', '13', '12', '11', '10', '9', '8', '7'] runs-on: ${{ matrix.os }} steps: @@ -102,6 +102,7 @@ jobs: with: name: coverage-data-${{ runner.os }}-py${{ matrix.py }}-${{ matrix.version }} path: .coverage* + include-hidden-files: true coverage-report: needs: [test] diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 0fe15440..030c5ae7 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,6 +1,6 @@ repos: - repo: https://github.com/pre-commit/pre-commit-hooks - rev: v4.5.0 + rev: v4.6.0 hooks: - id: trailing-whitespace exclude: ^tests/.*\.(?:patch|diff)$ @@ -15,14 +15,14 @@ repos: args: ["--fix=lf"] - repo: https://github.com/astral-sh/ruff-pre-commit # Ruff version. - rev: v0.3.3 + rev: v0.6.4 hooks: # Run the linter. - id: ruff # Run the formatter. - id: ruff-format - repo: https://github.com/pre-commit/mirrors-mypy - rev: 'v1.9.0' + rev: 'v1.11.2' hooks: - id: mypy additional_dependencies: @@ -32,9 +32,7 @@ repos: - pytest - requests-mock - '.' - - repo: https://github.com/codespell-project/codespell - rev: v2.2.6 + - repo: https://github.com/streetsidesoftware/cspell-cli + rev: v8.13.3 hooks: - - id: codespell - additional_dependencies: - - tomli + - id: cspell diff --git a/cpp_linter/clang_tools/__init__.py b/cpp_linter/clang_tools/__init__.py index 7d607de1..be7bb339 100644 --- a/cpp_linter/clang_tools/__init__.py +++ b/cpp_linter/clang_tools/__init__.py @@ -59,6 +59,7 @@ def _run_on_single_file( extra_args=args.extra_arg, db_json=db_json, tidy_review=args.tidy_review, + style=args.style, ) format_advice = None diff --git a/cpp_linter/clang_tools/clang_format.py b/cpp_linter/clang_tools/clang_format.py index 913cf300..a32139cf 100644 --- a/cpp_linter/clang_tools/clang_format.py +++ b/cpp_linter/clang_tools/clang_format.py @@ -2,12 +2,13 @@ from pathlib import PurePath import subprocess -from typing import List, cast, Optional +from typing import List, cast import xml.etree.ElementTree as ET from ..common_fs import get_line_cnt_from_cols, FileObj from ..loggers import logger +from .patcher import PatchMixin class FormatReplacement: @@ -53,7 +54,7 @@ def __repr__(self): ) -class FormatAdvice: +class FormatAdvice(PatchMixin): """A single object to represent each suggestion. :param filename: The source file's name for which the contents of the xml @@ -69,8 +70,7 @@ def __init__(self, filename: str): """A list of `FormatReplacementLine` representing replacement(s) on a single line.""" - #: A buffer of the applied fixes from clang-format - self.patched: Optional[bytes] = None + super().__init__() def __repr__(self) -> str: return ( @@ -78,6 +78,12 @@ def __repr__(self) -> str: f"replacements for {self.filename}>" ) + def get_suggestion_help(self, start, end) -> str: + return super().get_suggestion_help(start, end) + "suggestion\n" + + def get_tool_name(self) -> str: + return "clang-format" + def tally_format_advice(files: List[FileObj]) -> int: """Returns the sum of clang-format errors""" diff --git a/cpp_linter/clang_tools/clang_tidy.py b/cpp_linter/clang_tools/clang_tidy.py index 896585b9..af4821ba 100644 --- a/cpp_linter/clang_tools/clang_tidy.py +++ b/cpp_linter/clang_tools/clang_tidy.py @@ -8,6 +8,7 @@ from typing import Tuple, Union, List, cast, Optional, Dict, Set from ..loggers import logger from ..common_fs import FileObj +from .patcher import PatchMixin, ReviewComments, Suggestion NOTE_HEADER = re.compile(r"^(.+):(\d+):(\d+):\s(\w+):(.*)\[([a-zA-Z\d\-\.]+)\]$") FIXED_NOTE = re.compile(r"^.+:(\d+):\d+:\snote: FIX-IT applied suggested code changes$") @@ -85,6 +86,10 @@ def diagnostic_link(self) -> str: if self.diagnostic.startswith("clang-diagnostic-"): return self.diagnostic link = f"[{self.diagnostic}](https://clang.llvm.org/extra/clang-tidy/checks/" + if self.diagnostic.startswith("clang-analyzer-"): + check_name_parts = self.diagnostic.split("-", maxsplit=2) + assert len(check_name_parts) > 2, "diagnostic name malformed" + return link + "clang-analyzer/{}.html)".format(check_name_parts[2]) return link + "{}/{}.html)".format(*self.diagnostic.split("-", maxsplit=1)) def __repr__(self) -> str: @@ -94,10 +99,10 @@ def __repr__(self) -> str: ) -class TidyAdvice: +class TidyAdvice(PatchMixin): def __init__(self, notes: List[TidyNotification]) -> None: #: A buffer of the applied fixes from clang-tidy - self.patched: Optional[bytes] = None + super().__init__() self.notes = notes def diagnostics_in_range(self, start: int, end: int) -> str: @@ -111,6 +116,50 @@ def diagnostics_in_range(self, start: int, end: int) -> str: break return diagnostics + def get_suggestion_help(self, start: int, end: int) -> str: + diagnostics = self.diagnostics_in_range(start, end) + prefix = super().get_suggestion_help(start, end) + if diagnostics: + return prefix + "diagnostics\n" + diagnostics + return prefix + "suggestion\n" + + def get_tool_name(self) -> str: + return "clang-tidy" + + def get_suggestions_from_patch( + self, file_obj: FileObj, summary_only: bool, review_comments: ReviewComments + ): + super().get_suggestions_from_patch(file_obj, summary_only, review_comments) + + def _has_related_suggestion(suggestion: Suggestion) -> bool: + for known in review_comments.suggestions: + if known.line_end >= suggestion.line_end >= known.line_start: + known.comment += f"\n{suggestion.comment}" + return True + return False + + # now check for clang-tidy warnings with no fixes applied + for note in self.notes: + if not note.applied_fixes: # if no fix was applied + line_numb = int(note.line) + if not summary_only and file_obj.is_range_contained( + start=line_numb, end=line_numb + 1 + ): + suggestion = Suggestion(file_obj.name) + suggestion.line_end = line_numb + body = f"### clang-tidy diagnostic\n**{file_obj.name}:" + body += f"{note.line}:{note.cols}:** {note.severity}: " + body += f"[{note.diagnostic_link}]\n> {note.rationale}\n" + if note.fixit_lines: + body += f'```{Path(file_obj.name).suffix.lstrip(".")}\n' + for fixit_line in note.fixit_lines: + body += f"{fixit_line}\n" + body += "```\n" + suggestion.comment = body + review_comments.tool_total["clang-tidy"] += 1 + if not _has_related_suggestion(suggestion): + review_comments.suggestions.append(suggestion) + def tally_tidy_advice(files: List[FileObj]) -> int: """Returns the sum of clang-format errors""" @@ -135,6 +184,7 @@ def run_clang_tidy( extra_args: List[str], db_json: Optional[List[Dict[str, str]]], tidy_review: bool, + style: str, ) -> TidyAdvice: """Run clang-tidy on a certain file. @@ -179,6 +229,8 @@ def run_clang_tidy( "name": filename, "lines": file_obj.range_of_changed_lines(lines_changed_only, get_ranges=True), } + if style: + cmds.extend(["--format-style", style]) if line_ranges["lines"]: # logger.info("line_filter = %s", json.dumps([line_ranges])) cmds.append(f"--line-filter={json.dumps([line_ranges])}") diff --git a/cpp_linter/clang_tools/patcher.py b/cpp_linter/clang_tools/patcher.py new file mode 100644 index 00000000..7354f773 --- /dev/null +++ b/cpp_linter/clang_tools/patcher.py @@ -0,0 +1,195 @@ +"""A module to contain the abstractions about creating suggestions from a diff generated +by the clang tool's output.""" + +from abc import ABC +from pathlib import Path +from typing import Optional, Dict, Any, List, Tuple +from pygit2 import Patch # type: ignore +from ..common_fs import FileObj + +try: + from pygit2.enums import DiffOption # type: ignore + + INDENT_HEURISTIC = DiffOption.INDENT_HEURISTIC +except ImportError: # if pygit2.__version__ < 1.14 + from pygit2 import GIT_DIFF_INDENT_HEURISTIC # type: ignore + + INDENT_HEURISTIC = GIT_DIFF_INDENT_HEURISTIC + + +class Suggestion: + """A data structure to contain information about a single suggestion. + + :param file_name: The path to the file that this suggestion pertains. + This should use posix path separators. + """ + + def __init__(self, file_name: str) -> None: + #: The file's line number starting the suggested change. + self.line_start: int = -1 + #: The file's line number ending the suggested change. + self.line_end: int = -1 + #: The file's path about the suggested change. + self.file_name: str = file_name + #: The markdown comment about the suggestion. + self.comment: str = "" + + def serialize_to_github_payload(self) -> Dict[str, Any]: + """Serialize this object into a JSON compatible with Github's REST API.""" + assert self.line_end > 0, "ending line number unknown" + result = {"path": self.file_name, "body": self.comment, "line": self.line_end} + if self.line_start != self.line_end and self.line_start > 0: + result["start_line"] = self.line_start + return result + + +class ReviewComments: + """A data structure to contain PR review comments from a specific clang tool.""" + + def __init__(self) -> None: + #: The list of actual comments + self.suggestions: List[Suggestion] = [] + + self.tool_total: Dict[str, int] = {"clang-tidy": 0, "clang-format": 0} + """The total number of concerns about a specific clang tool. + + This may not equate to the length of `suggestions` because + 1. There is no guarantee that all suggestions will fit within the PR's diff. + 2. Suggestions are a combined result of advice from both tools. + """ + + self.full_patch: Dict[str, str] = {"clang-tidy": "", "clang-format": ""} + """The full patch of all the suggestions (including those that will not + fit within the diff)""" + + def merge_similar_suggestion(self, suggestion: Suggestion) -> bool: + """Merge a given ``suggestion`` into a similar `Suggestion` + + :returns: `True` if the suggestion was merged, otherwise `False`. + """ + for known in self.suggestions: + if ( + known.line_end == suggestion.line_end + and known.line_start == suggestion.line_start + ): + known.comment += f"\n{suggestion.comment}" + return True + return False + + def serialize_to_github_payload(self) -> Tuple[str, List[Dict[str, Any]]]: + """Serialize this object into a summary and list of comments compatible + with Github's REST API. + + :returns: The returned tuple contains a brief summary (at index ``0``) + that contains markdown text describing the summary of the review + comments. + + The list of `suggestions` (at index ``1``) is the serialized JSON + object. + """ + summary = "" + comments = [] + posted_tool_advice = {"clang-tidy": 0, "clang-format": 0} + for comment in self.suggestions: + comments.append(comment.serialize_to_github_payload()) + if "### clang-format" in comment.comment: + posted_tool_advice["clang-format"] += 1 + if "### clang-tidy" in comment.comment: + posted_tool_advice["clang-tidy"] += 1 + + for tool_name in ("clang-tidy", "clang-format"): + if ( + len(comments) + and posted_tool_advice[tool_name] != self.tool_total[tool_name] + ): + summary += ( + f"Only {posted_tool_advice[tool_name]} out of " + + f"{self.tool_total[tool_name]} {tool_name}" + + " concerns fit within this pull request's diff.\n" + ) + if self.full_patch[tool_name]: + summary += ( + f"\n
Click here for the full {tool_name} patch" + + f"\n\n\n```diff\n{self.full_patch[tool_name]}\n" + + "```\n\n\n
\n\n" + ) + elif not self.tool_total[tool_name]: + summary += f"No concerns from {tool_name}.\n" + result = (summary, comments) + return result + + +class PatchMixin(ABC): + """An abstract mixin that unified parsing of the suggestions into + PR review comments.""" + + def __init__(self) -> None: + #: A unified diff of the applied fixes from the clang tool's output + self.patched: Optional[bytes] = None + + def get_suggestion_help(self, start, end) -> str: + """Create helpful text about what the suggestion aims to fix. + + The parameters ``start`` and ``end`` are the line numbers (relative to file's + original content) encapsulating the suggestion. + """ + + return f"### {self.get_tool_name()} " + + def get_tool_name(self) -> str: + """A function that must be implemented by derivatives to + get the clang tool's name that generated the `patched` data.""" + + raise NotImplementedError("must be implemented by derivative") + + def get_suggestions_from_patch( + self, file_obj: FileObj, summary_only: bool, review_comments: ReviewComments + ): + """Create a list of suggestions from the tool's `patched` output. + + Results are stored in the ``review_comments`` parameter (passed by reference). + """ + assert ( + self.patched + ), f"{self.__class__.__name__} has no suggestions for {file_obj.name}" + patch = Patch.create_from( + Path(file_obj.name).read_bytes(), + self.patched, + file_obj.name, + file_obj.name, + context_lines=0, # exclude any surrounding unchanged lines + flag=INDENT_HEURISTIC, + ) + tool_name = self.get_tool_name() + assert tool_name in review_comments.full_patch + review_comments.full_patch[tool_name] += f"{patch.text}" + assert tool_name in review_comments.tool_total + for hunk in patch.hunks: + review_comments.tool_total[tool_name] += 1 + if summary_only: + continue + new_hunk_range = file_obj.is_hunk_contained(hunk) + if new_hunk_range is None: + continue + start_line, end_line = new_hunk_range + comment = Suggestion(file_obj.name) + body = self.get_suggestion_help(start=start_line, end=end_line) + if start_line < end_line: + comment.line_start = start_line + comment.line_end = end_line + removed = [] + suggestion = "" + for line in hunk.lines: + if line.origin in ("+", " "): + suggestion += f"{line.content}" + else: + line_numb = line.old_lineno + removed.append(line_numb) + 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.comment = body + if not review_comments.merge_similar_suggestion(comment): + review_comments.suggestions.append(comment) diff --git a/cpp_linter/cli.py b/cpp_linter/cli.py index 7c812fa7..f95d4225 100644 --- a/cpp_linter/cli.py +++ b/cpp_linter/cli.py @@ -117,6 +117,13 @@ class Args(UserDict): See `clang-format docs `_ for more info. +.. note:: + If this is not a blank string, then it is also + passed to clang-tidy (if :std:option:`--tidy-checks` + is not ``-*``). This is done ensure a more consistent + output about suggested fixes between clang-tidy and + clang-format. + Defaults to ``%(default)s``""", ) _parser_args[("-c", "--tidy-checks")] = dict( diff --git a/cpp_linter/common_fs/__init__.py b/cpp_linter/common_fs/__init__.py index 9199ce49..974b8dd6 100644 --- a/cpp_linter/common_fs/__init__.py +++ b/cpp_linter/common_fs/__init__.py @@ -48,6 +48,9 @@ def __init__( #: The results from clang-format self.format_advice: Optional["FormatAdvice"] = None + def __repr__(self) -> str: + return f"" + @staticmethod def _consolidate_list_to_ranges(numbers: List[int]) -> List[List[int]]: """A helper function that is only used after parsing the lines from a diff that diff --git a/cpp_linter/rest_api/github_api.py b/cpp_linter/rest_api/github_api.py index 70a438de..f2aa102a 100644 --- a/cpp_linter/rest_api/github_api.py +++ b/cpp_linter/rest_api/github_api.py @@ -15,17 +15,16 @@ from pathlib import Path import urllib.parse import sys -from typing import Dict, List, Any, cast, Optional, Tuple, Union +from typing import Dict, List, Any, cast, Optional -from pygit2 import Patch # type: ignore from ..common_fs import FileObj, CACHE_PATH from ..common_fs.file_filter import FileFilter from ..clang_tools.clang_format import ( - FormatAdvice, formalize_style_name, tally_format_advice, ) -from ..clang_tools.clang_tidy import TidyAdvice, tally_tidy_advice +from ..clang_tools.clang_tidy import tally_tidy_advice +from ..clang_tools.patcher import ReviewComments, PatchMixin from ..cli import Args from ..loggers import logger, log_commander from ..git import parse_diff, get_diff @@ -102,11 +101,52 @@ def get_list_of_changed_files( files_link += f"commits/{self.sha}" logger.info("Fetching files list from url: %s", files_link) response = self.api_request( - url=files_link, headers=self.make_headers(use_diff=True) + url=files_link, headers=self.make_headers(use_diff=True), strict=False ) - files = parse_diff(response.text, file_filter, lines_changed_only) - else: - files = parse_diff(get_diff(), file_filter, lines_changed_only) + if response.status_code != 200: + return self._get_changed_files_paginated( + files_link, lines_changed_only, file_filter + ) + return parse_diff(response.text, file_filter, lines_changed_only) + return parse_diff(get_diff(), file_filter, lines_changed_only) + + def _get_changed_files_paginated( + self, url: Optional[str], lines_changed_only: int, file_filter: FileFilter + ) -> List[FileObj]: + """A fallback implementation of getting file changes using a paginated + REST API endpoint.""" + logger.info( + "Could not get raw diff of the %s event. " + "Perhaps there are too many changes?", + self.event_name, + ) + assert url is not None + if self.event_name == "pull_request": + url += "/files" + files = [] + while url is not None: + response = self.api_request(url) + url = RestApiClient.has_more_pages(response) + file_list: List[Dict[str, Any]] + if self.event_name == "pull_request": + file_list = response.json() + else: + file_list = response.json()["files"] + for file in file_list: + assert "filename" in file + file_name = file["filename"] + if not file_filter.is_source_or_ignored(file_name): + continue + old_name = file_name + if "previous_filename" in file: + old_name = file["previous_filename"] + assert "patch" in file + file_diff = ( + f"diff --git a/{file_name} b/{old_name}\n" + + f"--- a/{file_name}\n+++ b/{old_name}\n" + + file["patch"] + ) + files.extend(parse_diff(file_diff, file_filter, lines_changed_only)) return files def verify_files_are_present(self, files: List[FileObj]) -> None: @@ -363,31 +403,27 @@ def post_review( return # don't post reviews body = f"{COMMENT_MARKER}## Cpp-linter Review\n" payload_comments = [] - total_changes = 0 - summary_only = ( - environ.get("CPP_LINTER_PR_REVIEW_SUMMARY_ONLY", "false") == "true" - ) - advice: Dict[str, bool] = {} + summary_only = environ.get( + "CPP_LINTER_PR_REVIEW_SUMMARY_ONLY", "false" + ).lower() in ("true", "on", "1") + advice = [] if format_review: - advice["clang-format"] = False + advice.append("clang-format") if tidy_review: - advice["clang-tidy"] = True - for tool_name, tidy_tool in advice.items(): - comments, total, patch = self.create_review_comments( - files, tidy_tool, summary_only + advice.append("clang-tidy") + review_comments = ReviewComments() + for tool_name in advice: + self.create_review_comments( + files=files, + tidy_tool=tool_name == "clang-tidy", + summary_only=summary_only, + review_comments=review_comments, ) - total_changes += total - if not summary_only: - payload_comments.extend(comments) - if total and total != len(comments): - body += f"Only {len(comments)} out of {total} {tool_name} " - body += "concerns fit within this pull request's diff.\n" - if patch: - body += f"\n
Click here for the full {tool_name} patch" - body += f"\n\n\n```diff\n{patch}\n```\n\n\n
\n\n" - elif not total: - body += f"No concerns from {tool_name}.\n" - if total_changes: + (summary, comments) = review_comments.serialize_to_github_payload() + if not summary_only: + payload_comments.extend(comments) + body += summary + if sum(review_comments.tool_total.values()): event = "REQUEST_CHANGES" else: if no_lgtm: @@ -410,91 +446,28 @@ def create_review_comments( files: List[FileObj], tidy_tool: bool, summary_only: bool, - ) -> 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 = "" + review_comments: ReviewComments, + ): + """Creates a batch of comments for a specific clang tool's PR review. + + :param files: The list of files to traverse. + :param tidy_tool: A flag to indicate if the suggestions should originate + from clang-tidy. + :param summary_only: A flag to indicate if only the review summary is desired. + :param review_comments: An object (passed by reference) that is used to store + the results. + """ for file_obj in files: - tool_advice: Optional[Union[TidyAdvice, FormatAdvice]] + tool_advice: Optional[PatchMixin] if tidy_tool: tool_advice = file_obj.tidy_advice else: tool_advice = file_obj.format_advice if not tool_advice: continue - assert tool_advice.patched, f"No suggested patch found for {file_obj.name}" - patch = Patch.create_from( - old=Path(file_obj.name).read_bytes(), - new=tool_advice.patched, - old_as_path=file_obj.name, - new_as_path=file_obj.name, - context_lines=0, # trim all unchanged lines from start/end of hunks + tool_advice.get_suggestions_from_patch( + file_obj, summary_only, review_comments ) - full_patch += patch.text - for hunk in patch.hunks: - total += 1 - if summary_only: - continue - new_hunk_range = file_obj.is_hunk_contained(hunk) - if new_hunk_range is None: - continue - start_lines, end_lines = new_hunk_range - comment: Dict[str, Any] = {"path": file_obj.name} - body = "" - if tidy_tool and file_obj.tidy_advice: - body += "### clang-tidy " - diagnostics = file_obj.tidy_advice.diagnostics_in_range( - start_lines, end_lines - ) - if diagnostics: - body += "diagnostics\n" + diagnostics - else: - body += "suggestions\n" - elif not tidy_tool: - 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 - 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 - comments.append(comment) - - # now check for clang-tidy warnings with no fixes applied - if tidy_tool and file_obj.tidy_advice: - for note in file_obj.tidy_advice.notes: - if not note.applied_fixes: # if no fix was applied - total += 1 - line_numb = int(note.line) - if file_obj.is_range_contained( - start=line_numb, end=line_numb + 1 - ): - diag: Dict[str, Any] = { - "path": file_obj.name, - "line": note.line, - } - body = f"### clang-tidy diagnostic\n**{file_obj.name}:" - body += f"{note.line}:{note.cols}:** {note.severity}: " - body += f"[{note.diagnostic_link}]\n> {note.rationale}\n" - if note.fixit_lines: - body += f'```{Path(file_obj.name).suffix.lstrip(".")}\n' - for line in note.fixit_lines: - body += f"{line}\n" - body += "```\n" - diag["body"] = body - comments.append(diag) - return (comments, total, full_patch) def _dismiss_stale_reviews(self, url: str): """Dismiss all reviews that were previously created by cpp-linter""" diff --git a/cspell.config.yml b/cspell.config.yml new file mode 100644 index 00000000..563d93c8 --- /dev/null +++ b/cspell.config.yml @@ -0,0 +1,64 @@ +version: "0.2" +language: en +words: + - argnames + - argvalues + - automodule + - bndy + - bugprone + - bysource + - caplog + - capsys + - codecov + - codespell + - consts + - cppcoreguidelines + - cstdio + - docutils + - endgroup + - Fixit + - fontawesome + - gitmodules + - gmtime + - intersphinx + - iomanip + - keepends + - levelno + - libgit + - libvips + - markdownlint + - maxsplit + - mktime + - mypy + - posix + - pybind + - pygit + - pypi + - pyproject + - pytest + - ratelimit + - revparse + - seealso + - setenv + - shenxianpeng + - srcdir + - stddef + - tada + - toctree + - tofile + - tomli + - undoc + - vararg + - venv + - viewcode +ignorePaths: + - .env/** + - .venv/** + - env/** + - venv/** + - tests/**/*.{json,h,c,cpp,hpp,patch,diff} + - "**.clang-tidy" + - "**.clang-format" + - pyproject.toml + - .gitignore + - "**/*.{yml,yaml,txt}" diff --git a/docs/API-Reference/cpp_linter.clang_tools.patcher.rst b/docs/API-Reference/cpp_linter.clang_tools.patcher.rst new file mode 100644 index 00000000..e7165b6c --- /dev/null +++ b/docs/API-Reference/cpp_linter.clang_tools.patcher.rst @@ -0,0 +1,5 @@ +``clang_tools.patcher`` +======================= + +.. automodule:: cpp_linter.clang_tools.patcher + :members: diff --git a/docs/index.rst b/docs/index.rst index b65a4815..e51602b1 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -16,6 +16,7 @@ API-Reference/cpp_linter.clang_tools API-Reference/cpp_linter.clang_tools.clang_format API-Reference/cpp_linter.clang_tools.clang_tidy + API-Reference/cpp_linter.clang_tools.patcher API-Reference/cpp_linter.rest_api API-Reference/cpp_linter.rest_api.github_api API-Reference/cpp_linter.git diff --git a/docs/pr_review_caveats.rst b/docs/pr_review_caveats.rst index 77888bcc..fcf024b5 100644 --- a/docs/pr_review_caveats.rst +++ b/docs/pr_review_caveats.rst @@ -65,8 +65,8 @@ GitHub REST API does not provide a way to hide comments or mark review suggestio .. tip:: We do support an environment variable named ``CPP_LINTER_PR_REVIEW_SUMMARY_ONLY``. - If the variable is set to ``true``, then the review only contains a summary comment - with no suggestions posted in the diff. + If the variable is set either ``true``, ``on``, or ``1``, then the review only + contains a summary comment with no suggestions posted in the diff. Probable non-exhaustive reviews ------------------------------- diff --git a/pyproject.toml b/pyproject.toml index b1f7f673..bf5348b6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,5 +1,5 @@ [build-system] -requires = ["setuptools>=61", "setuptools-scm"] +requires = ["setuptools>=62.6", "setuptools-scm"] build-backend = "setuptools.build_meta" [project] @@ -12,11 +12,6 @@ authors = [ { name = "Brendan Doherty", email = "2bndy5@gmail.com" }, { name = "Peter Shen", email = "xianpeng.shen@gmail.com" }, ] -dependencies = [ - "requests", - "pyyaml", - "pygit2", -] classifiers = [ # https://pypi.org/pypi?%3Aaction=list_classifiers "Development Status :: 5 - Production/Stable", @@ -31,7 +26,7 @@ classifiers = [ "Programming Language :: Python :: 3", "Topic :: Software Development :: Build Tools", ] -dynamic = ["version"] +dynamic = ["version", "dependencies"] [project.scripts] cpp-linter = "cpp_linter:main" @@ -47,6 +42,9 @@ tracker = "https://github.com/cpp-linter/cpp-linter/issues" zip-safe = false packages = ["cpp_linter"] +[tool.setuptools.dynamic] +dependencies = {file = ["requirements.txt"]} + [tool.setuptools_scm] # It would be nice to include the commit hash in the version, but that # can't be done in a PEP 440-compatible way. @@ -61,7 +59,7 @@ show_column_numbers = true [tool.pytest.ini_options] minversion = "6.0" -addopts = "-vv" +addopts = "-vv --durations=8 --color=yes" testpaths = ["tests"] [tool.coverage] diff --git a/tests/demo/.clang-tidy b/tests/demo/.clang-tidy index d3865ade..ba113044 100644 --- a/tests/demo/.clang-tidy +++ b/tests/demo/.clang-tidy @@ -2,7 +2,6 @@ Checks: 'clang-diagnostic-*,clang-analyzer-*,-*,performance-*,bugprone-*,clang-analyzer-*,mpi-*,misc-*,readability-*' WarningsAsErrors: '' HeaderFilterRegex: '' -AnalyzeTemporaryDtors: false FormatStyle: 'file' CheckOptions: - key: bugprone-argument-comment.CommentBoolLiterals diff --git a/tests/list_changes/patch.diff b/tests/list_changes/patch.diff new file mode 100644 index 00000000..7bda2e1b --- /dev/null +++ b/tests/list_changes/patch.diff @@ -0,0 +1,142 @@ +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/src/demo.c b/src/demo.c +index 0c1db60..1bf553e 100644 +--- a/src/demo.c ++++ b/src/demo.c +@@ -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/tests/list_changes/pull_request_files_pg1.json b/tests/list_changes/pull_request_files_pg1.json new file mode 100644 index 00000000..5a70fd9c --- /dev/null +++ b/tests/list_changes/pull_request_files_pg1.json @@ -0,0 +1,27 @@ +[ + { + "sha": "52501fa1dc96d6bc6f8a155816df041b1de975d9", + "filename": ".github/workflows/cpp-lint-package.yml", + "status": "modified", + "additions": 9, + "deletions": 5, + "changes": 14, + "blob_url": "https://github.com/cpp-linter/test-cpp-linter-action/blob/635a9c57bdcca07b99ddef52c2640337c50280b1/.github%2Fworkflows%2Fcpp-lint-package.yml", + "raw_url": "https://github.com/cpp-linter/test-cpp-linter-action/raw/635a9c57bdcca07b99ddef52c2640337c50280b1/.github%2Fworkflows%2Fcpp-lint-package.yml", + "contents_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contents/.github%2Fworkflows%2Fcpp-lint-package.yml?ref=635a9c57bdcca07b99ddef52c2640337c50280b1", + "patch": "@@ -7,16 +7,17 @@ on:\n description: 'which branch to test'\n default: 'main'\n required: true\n+ pull_request:\n \n jobs:\n cpp-linter:\n runs-on: windows-latest\n \n strategy:\n matrix:\n- clang-version: ['7', '8', '9','10', '11', '12', '13', '14', '15', '16', '17']\n+ clang-version: ['10', '11', '12', '13', '14', '15', '16', '17']\n repo: ['cpp-linter/cpp-linter']\n- branch: ['${{ inputs.branch }}']\n+ branch: ['pr-review-suggestions']\n fail-fast: false\n \n steps:\n@@ -62,10 +63,13 @@ jobs:\n -i=build \n -p=build \n -V=${{ runner.temp }}/llvm \n- -f=false \n --extra-arg=\"-std=c++14 -Wall\" \n- --thread-comments=${{ matrix.clang-version == '12' }} \n- -a=${{ matrix.clang-version == '12' }}\n+ --file-annotations=false\n+ --lines-changed-only=false\n+ --extension=h,c\n+ --thread-comments=${{ matrix.clang-version == '16' }} \n+ --tidy-review=${{ matrix.clang-version == '16' }}\n+ --format-review=${{ matrix.clang-version == '16' }}\n \n - name: Fail fast?!\n if: steps.linter.outputs.checks-failed > 0" + }, + { + "sha": "1bf553e06e4b7c6c9a9be5da4845acbdeb04f6a5", + "filename": "src/demo.cpp", + "previous_filename": "src/demo.c", + "status": "modified", + "additions": 11, + "deletions": 10, + "changes": 21, + "blob_url": "https://github.com/cpp-linter/test-cpp-linter-action/blob/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.cpp", + "raw_url": "https://github.com/cpp-linter/test-cpp-linter-action/raw/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.cpp", + "contents_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contents/src%2Fdemo.cpp?ref=635a9c57bdcca07b99ddef52c2640337c50280b1", + "patch": "@@ -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\");\n \n- return 0;\n-}\n+\n+\n+\n+ return 0;}" + } +] diff --git a/tests/list_changes/pull_request_files_pg2.json b/tests/list_changes/pull_request_files_pg2.json new file mode 100644 index 00000000..987a2395 --- /dev/null +++ b/tests/list_changes/pull_request_files_pg2.json @@ -0,0 +1,14 @@ +[ + { + "sha": "f93d0122ae2e3c1952c795837d71c432036b55eb", + "filename": "src/demo.hpp", + "status": "modified", + "additions": 3, + "deletions": 8, + "changes": 11, + "blob_url": "https://github.com/cpp-linter/test-cpp-linter-action/blob/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.hpp", + "raw_url": "https://github.com/cpp-linter/test-cpp-linter-action/raw/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.hpp", + "contents_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contents/src%2Fdemo.hpp?ref=635a9c57bdcca07b99ddef52c2640337c50280b1", + "patch": "@@ -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 \n \n@@ -28,14 +26,11 @@ class Dummy {\n \n \n \n-\n-\n-\n-\n \n \n struct LongDiff\n {\n+\n long diff;\n \n };" + } +] diff --git a/tests/list_changes/push_files_pg1.json b/tests/list_changes/push_files_pg1.json new file mode 100644 index 00000000..8022a1e1 --- /dev/null +++ b/tests/list_changes/push_files_pg1.json @@ -0,0 +1,29 @@ +{ + "files": [ + { + "sha": "52501fa1dc96d6bc6f8a155816df041b1de975d9", + "filename": ".github/workflows/cpp-lint-package.yml", + "status": "modified", + "additions": 9, + "deletions": 5, + "changes": 14, + "blob_url": "https://github.com/cpp-linter/test-cpp-linter-action/blob/635a9c57bdcca07b99ddef52c2640337c50280b1/.github%2Fworkflows%2Fcpp-lint-package.yml", + "raw_url": "https://github.com/cpp-linter/test-cpp-linter-action/raw/635a9c57bdcca07b99ddef52c2640337c50280b1/.github%2Fworkflows%2Fcpp-lint-package.yml", + "contents_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contents/.github%2Fworkflows%2Fcpp-lint-package.yml?ref=635a9c57bdcca07b99ddef52c2640337c50280b1", + "patch": "@@ -7,16 +7,17 @@ on:\n description: 'which branch to test'\n default: 'main'\n required: true\n+ pull_request:\n \n jobs:\n cpp-linter:\n runs-on: windows-latest\n \n strategy:\n matrix:\n- clang-version: ['7', '8', '9','10', '11', '12', '13', '14', '15', '16', '17']\n+ clang-version: ['10', '11', '12', '13', '14', '15', '16', '17']\n repo: ['cpp-linter/cpp-linter']\n- branch: ['${{ inputs.branch }}']\n+ branch: ['pr-review-suggestions']\n fail-fast: false\n \n steps:\n@@ -62,10 +63,13 @@ jobs:\n -i=build \n -p=build \n -V=${{ runner.temp }}/llvm \n- -f=false \n --extra-arg=\"-std=c++14 -Wall\" \n- --thread-comments=${{ matrix.clang-version == '12' }} \n- -a=${{ matrix.clang-version == '12' }}\n+ --file-annotations=false\n+ --lines-changed-only=false\n+ --extension=h,c\n+ --thread-comments=${{ matrix.clang-version == '16' }} \n+ --tidy-review=${{ matrix.clang-version == '16' }}\n+ --format-review=${{ matrix.clang-version == '16' }}\n \n - name: Fail fast?!\n if: steps.linter.outputs.checks-failed > 0" + }, + { + "sha": "1bf553e06e4b7c6c9a9be5da4845acbdeb04f6a5", + "filename": "src/demo.cpp", + "previous_filename": "src/demo.c", + "status": "modified", + "additions": 11, + "deletions": 10, + "changes": 21, + "blob_url": "https://github.com/cpp-linter/test-cpp-linter-action/blob/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.cpp", + "raw_url": "https://github.com/cpp-linter/test-cpp-linter-action/raw/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.cpp", + "contents_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contents/src%2Fdemo.cpp?ref=635a9c57bdcca07b99ddef52c2640337c50280b1", + "patch": "@@ -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\");\n \n- return 0;\n-}\n+\n+\n+\n+ return 0;}" + } + ] +} diff --git a/tests/list_changes/push_files_pg2.json b/tests/list_changes/push_files_pg2.json new file mode 100644 index 00000000..573f532a --- /dev/null +++ b/tests/list_changes/push_files_pg2.json @@ -0,0 +1,16 @@ +{ + "files": [ + { + "sha": "f93d0122ae2e3c1952c795837d71c432036b55eb", + "filename": "src/demo.hpp", + "status": "modified", + "additions": 3, + "deletions": 8, + "changes": 11, + "blob_url": "https://github.com/cpp-linter/test-cpp-linter-action/blob/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.hpp", + "raw_url": "https://github.com/cpp-linter/test-cpp-linter-action/raw/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.hpp", + "contents_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contents/src%2Fdemo.hpp?ref=635a9c57bdcca07b99ddef52c2640337c50280b1", + "patch": "@@ -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 \n \n@@ -28,14 +26,11 @@ class Dummy {\n \n \n \n-\n-\n-\n-\n \n \n struct LongDiff\n {\n+\n long diff;\n \n };" + } + ] +} diff --git a/tests/list_changes/test_get_file_changes.py b/tests/list_changes/test_get_file_changes.py new file mode 100644 index 00000000..5e15d8b0 --- /dev/null +++ b/tests/list_changes/test_get_file_changes.py @@ -0,0 +1,128 @@ +import json +import logging +from pathlib import Path +import pytest +import requests_mock +from cpp_linter import GithubApiClient, logger, FileFilter +import cpp_linter.rest_api.github_api + + +TEST_PR = 27 +TEST_REPO = "cpp-linter/test-cpp-linter-action" +TEST_SHA = "708a1371f3a966a479b77f1f94ec3b7911dffd77" +TEST_API_URL = "https://api.mock.com" +TEST_ASSETS = Path(__file__).parent +TEST_DIFF = (TEST_ASSETS / "patch.diff").read_text(encoding="utf-8") + + +@pytest.mark.parametrize( + "event_name,paginated,fake_runner", + [ + # push event (full diff) + ( + "unknown", # let coverage include logged warning about unknown event + False, + True, + ), + # pull request event (full diff) + ( + "pull_request", + False, + True, + ), + # push event (paginated diff) + ( + "push", # let coverage include logged warning about unknown event + True, + True, + ), + # pull request event (paginated diff) + ( + "pull_request", + True, + True, + ), + # local dev env + ("", False, False), + ], + ids=[ + "push", + "pull_request", + "push(paginated)", + "pull_request(paginated)", + "local_dev", + ], +) +def test_get_changed_files( + caplog: pytest.LogCaptureFixture, + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + event_name: str, + paginated: bool, + fake_runner: bool, +): + """test getting a list of changed files for an event.""" + caplog.set_level(logging.DEBUG, logger=logger.name) + + # setup test to act as though executed in user's repo's CI + event_payload = {"number": TEST_PR} + 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("GITHUB_EVENT_NAME", event_name) + monkeypatch.setenv("GITHUB_REPOSITORY", TEST_REPO) + monkeypatch.setenv("GITHUB_SHA", TEST_SHA) + monkeypatch.setenv("GITHUB_API_URL", TEST_API_URL) + monkeypatch.setenv("CI", str(fake_runner).lower()) + monkeypatch.setenv("GITHUB_TOKEN", "123456") + gh_client = GithubApiClient() + + if not fake_runner: + # getting a diff in CI (on a shallow checkout) fails + # monkey patch the .git.get_diff() to return the test's diff asset + monkeypatch.setattr( + cpp_linter.rest_api.github_api, + "get_diff", + lambda *args: TEST_DIFF, + ) + + endpoint = f"{TEST_API_URL}/repos/{TEST_REPO}/commits/{TEST_SHA}" + if event_name == "pull_request": + endpoint = f"{TEST_API_URL}/repos/{TEST_REPO}/pulls/{TEST_PR}" + + with requests_mock.Mocker() as mock: + mock.get( + endpoint, + request_headers={ + "Authorization": "token 123456", + "Accept": "application/vnd.github.diff", + }, + text=TEST_DIFF if not paginated else "", + status_code=200 if not paginated else 403, + ) + + if paginated: + mock_endpoint = endpoint + if event_name == "pull_request": + mock_endpoint += "/files" + logger.debug("mock endpoint: %s", mock_endpoint) + for pg in (1, 2): + response_asset = f"{event_name}_files_pg{pg}.json" + mock.get( + mock_endpoint + ("" if pg == 1 else "?page=2"), + request_headers={ + "Authorization": "token 123456", + "Accept": "application/vnd.github.raw+json", + }, + headers={"link": f'<{mock_endpoint}?page=2>; rel="next"'} + if pg == 1 + else {}, + text=(TEST_ASSETS / response_asset).read_text(encoding="utf-8"), + ) + + files = gh_client.get_list_of_changed_files( + FileFilter(extensions=["cpp", "hpp"]), lines_changed_only=1 + ) + assert files + for file in files: + assert file.name in ("src/demo.cpp", "src/demo.hpp") diff --git a/tests/reviews/test_pr_review.py b/tests/reviews/test_pr_review.py index 2853b8ff..3f7de7aa 100644 --- a/tests/reviews/test_pr_review.py +++ b/tests/reviews/test_pr_review.py @@ -53,6 +53,7 @@ def mk_param_set(**kwargs) -> OrderedDict: tuple(mk_param_set(force_approved=True).values()), tuple(mk_param_set(force_approved=True, no_lgtm=True).values()), tuple(mk_param_set(tidy_review=True, format_review=False).values()), + tuple(mk_param_set(tidy_review=True, format_review=True).values()), tuple(mk_param_set(format_review=True).values()), tuple(mk_param_set(tidy_review=True, changes=1).values()), tuple(mk_param_set(tidy_review=True, changes=0).values()), @@ -66,6 +67,7 @@ def mk_param_set(**kwargs) -> OrderedDict: "approved", "no_lgtm", "tidy", # changes == diff_chunks only + "tidy+format", # changes == diff_chunks only "format", # changes == diff_chunks only "lines_added", "all_lines", @@ -123,7 +125,7 @@ def test_post_review( # load mock responses for pull_request event mock.get( base_url, - headers={"Accept": "application/vnd.github.diff"}, + request_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") diff --git a/tests/test_misc.py b/tests/test_misc.py index 128d338a..0f025757 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -8,7 +8,6 @@ from typing import List, cast import pytest -import requests_mock from cpp_linter.common_fs import get_line_cnt_from_cols, FileObj from cpp_linter.common_fs.file_filter import FileFilter @@ -19,8 +18,8 @@ start_log_group, end_log_group, ) -import cpp_linter.rest_api.github_api from cpp_linter.rest_api.github_api import GithubApiClient +from cpp_linter.clang_tools.clang_tidy import TidyNotification def test_exit_output(monkeypatch: pytest.MonkeyPatch, tmp_path: Path): @@ -82,70 +81,6 @@ def test_list_src_files( assert Path(file.name).suffix.lstrip(".") in extensions -@pytest.mark.parametrize( - "pseudo,expected_url,fake_runner", - [ - ( - dict( - repo="cpp-linter/test-cpp-linter-action", - sha="708a1371f3a966a479b77f1f94ec3b7911dffd77", - event_name="unknown", # let coverage include logged warning - ), - "{rest_api_url}/repos/{repo}/commits/{sha}", - True, - ), - ( - dict( - repo="cpp-linter/test-cpp-linter-action", - event_name="pull_request", - ), - "{rest_api_url}/repos/{repo}/pulls/{number}", - True, - ), - ({}, "", False), - ], - ids=["push", "pull_request", "local_dev"], -) -def test_get_changed_files( - caplog: pytest.LogCaptureFixture, - monkeypatch: pytest.MonkeyPatch, - pseudo: dict, - expected_url: str, - fake_runner: bool, -): - """test getting a list of changed files for an event. - - This is expected to fail if a github token not supplied as an env var. - We don't need to supply one for this test because the tested code will - execute anyway. - """ - caplog.set_level(logging.DEBUG, logger=logger.name) - # setup test to act as though executed in user's repo's CI - monkeypatch.setenv("CI", str(fake_runner).lower()) - gh_client = GithubApiClient() - for name, value in pseudo.items(): - setattr(gh_client, name, value) - if "event_name" in pseudo and pseudo["event_name"] == "pull_request": - gh_client.pull_request = 19 - if not fake_runner: - # getting a diff in CI (on a shallow checkout) fails - # monkey patch the .git.get_diff() to return nothing - monkeypatch.setattr( - cpp_linter.rest_api.github_api, "get_diff", lambda *args: "" - ) - monkeypatch.setenv("GITHUB_TOKEN", "123456") - - with requests_mock.Mocker() as mock: - mock.get( - expected_url.format(number=19, rest_api_url=gh_client.api_url, **pseudo), - request_headers={"Authorization": "token 123456"}, - text="", - ) - - files = gh_client.get_list_of_changed_files(FileFilter(), 0) - assert not files - - @pytest.mark.parametrize("line,cols,offset", [(13, 5, 144), (19, 1, 189)]) def test_file_offset_translation(line: int, cols: int, offset: int): """Validate output from ``get_line_cnt_from_cols()``""" @@ -184,3 +119,31 @@ def test_tool_exe_path(tool_name: str, version: str): exe_path = assemble_version_exec(tool_name, version) assert exe_path assert tool_name in exe_path + + +def test_clang_analyzer_link(): + """Ensures the hyper link for a diagnostic about clang-analyzer checks is + not malformed""" + file_name = "RF24.cpp" + line = "1504" + column = "9" + rationale = "Dereference of null pointer (loaded from variable 'pipe_num')" + severity = "warning" + diagnostic_name = "clang-analyzer-core.NullDereference" + note = TidyNotification( + ( + file_name, + line, + column, + severity, + rationale, + diagnostic_name, + ) + ) + assert note.diagnostic_link == ( + "[{}]({}/{}.html)".format( + diagnostic_name, + "https://clang.llvm.org/extra/clang-tidy/checks/clang-analyzer", + diagnostic_name.split("-", maxsplit=2)[2], + ) + ) diff --git a/tests/test_rate_limits.py b/tests/test_rate_limits.py index 7c08288f..9d24009c 100644 --- a/tests/test_rate_limits.py +++ b/tests/test_rate_limits.py @@ -41,5 +41,5 @@ def test_rate_limit(monkeypatch: pytest.MonkeyPatch, response_headers: Dict[str, # ensure function exits early with pytest.raises(SystemExit) as exc: gh_client.api_request(url) - assert exc.type == SystemExit + assert exc.type is SystemExit assert exc.value.code == 1