diff --git a/src/python/review/inspectors/checkstyle/checkstyle.py b/src/python/review/inspectors/checkstyle/checkstyle.py index e16d3373..57f809ec 100644 --- a/src/python/review/inspectors/checkstyle/checkstyle.py +++ b/src/python/review/inspectors/checkstyle/checkstyle.py @@ -55,6 +55,11 @@ def inspect(self, path: Path, config: dict) -> List[BaseIssue]: @classmethod def choose_issue_type(cls, check_class: str) -> IssueType: + """ + Defines IssueType by Checkstyle check class using config. + """ + + # Example: com.puppycrawl.tools.checkstyle.checks.sizes.LineLengthCheck -> LineLengthCheck check_class_name = check_class.split('.')[-1] issue_type = CHECK_CLASS_NAME_TO_ISSUE_TYPE.get(check_class_name) if not issue_type: diff --git a/src/python/review/inspectors/parsers/checkstyle_parser.py b/src/python/review/inspectors/parsers/checkstyle_parser.py index 1db9874c..482340c4 100644 --- a/src/python/review/inspectors/parsers/checkstyle_parser.py +++ b/src/python/review/inspectors/parsers/checkstyle_parser.py @@ -26,8 +26,11 @@ logger = logging.getLogger(__name__) -# Check if the result of the inspectors is correct: it exists and it is not empty def __is_result_file_correct(file_path: Path, inspector_type: InspectorType) -> bool: + """ + Check if the result of the inspectors is correct: it exists and it is not empty. + """ + if not file_path.is_file(): logger.error('%s: error - no output file' % inspector_type.value) return False @@ -36,10 +39,16 @@ def __is_result_file_correct(file_path: Path, inspector_type: InspectorType) -> if not file_content: logger.error('%s: error - empty file' % inspector_type.value) return False + return True def __parse_error_message(element: ElementTree) -> str: + """ + Extracts the error message from the tree element and + removes the substring '(max allowed is \\d+)' from it, if possible. + """ + message = element.attrib['message'] return re.sub(r'\(max allowed is \d+\). ', '', message) @@ -48,6 +57,10 @@ def __parse_error_message(element: ElementTree) -> str: # e.g. BoolExprLenIssue, CyclomaticComplexityIssue and so on def __parse_measurable_issue(issue_data: Dict[str, Any], issue_type: IssueType, measure_value: int) -> Optional[BaseIssue]: + """ + Depending on the issue_type creates a corresponding measurable issue. + """ + if issue_type == IssueType.CYCLOMATIC_COMPLEXITY: issue_data[IssueData.CYCLOMATIC_COMPLEXITY.value] = measure_value issue_data[IssueData.DESCRIPTION.value] = get_cyclomatic_complexity_tip() @@ -68,19 +81,35 @@ def __parse_measurable_issue(issue_data: Dict[str, Any], issue_type: IssueType, def __should_handle_element(element: ElementTree) -> bool: + """ + Checks if a tree element is a file. + """ + return element.tag == 'file' def __is_error(element: ElementTree) -> bool: + """ + Checks if a tree element is an error. + """ + return element.tag == 'error' -# TODO Needs testing +# TODO: Needs refactoring def parse_checkstyle_file_result( file_path: Path, inspector_type: InspectorType, issue_type_selector: Callable[[str], IssueType], origin_class_to_description: Dict[str, str]) -> List[BaseIssue]: + """ + Parses the output, which is a xml file, and returns a list of the issues found there. + issue_type_selector determines the IssueType of an inspection. + origin_class_to_description allows to parse measurable errors. + + If the passed path is not a correct file, an empty list is returned. + """ + if not __is_result_file_correct(file_path, inspector_type): return [] @@ -98,6 +127,7 @@ def parse_checkstyle_file_result( continue message = __parse_error_message(inner_element) + # Example: com.puppycrawl.tools.checkstyle.checks.sizes.LineLengthCheck -> LineLengthCheck origin_class = inner_element.attrib['source'].split('.')[-1] issue_data = IssueData.get_base_issue_data_dict(code_file_path, inspector_type, line_number=int(inner_element.attrib['line']), diff --git a/src/python/review/inspectors/pmd/pmd.py b/src/python/review/inspectors/pmd/pmd.py index 345931c8..eb4878dd 100644 --- a/src/python/review/inspectors/pmd/pmd.py +++ b/src/python/review/inspectors/pmd/pmd.py @@ -58,6 +58,11 @@ def inspect(self, path: Path, config: dict) -> List[BaseIssue]: return self.parse_output(output_path) def parse_output(self, output_path: Path) -> List[BaseIssue]: + """ + Parses the PMD output, which is a csv file, and returns a list of the issues found there. + + If the passed path is not a file, an empty list is returned. + """ if not output_path.is_file(): logger.error('%s: error - no output file' % self.inspector_type.value) return [] @@ -77,6 +82,9 @@ def parse_output(self, output_path: Path) -> List[BaseIssue]: @classmethod def choose_issue_type(cls, rule: str) -> IssueType: + """ + Defines IssueType by PMD rule name using config. + """ issue_type = PMD_RULE_TO_ISSUE_TYPE.get(rule) if not issue_type: logger.warning('%s: %s - unknown rule' % diff --git a/test/python/inspectors/__init__.py b/test/python/inspectors/__init__.py index 7efbda31..88e24f37 100644 --- a/test/python/inspectors/__init__.py +++ b/test/python/inspectors/__init__.py @@ -13,3 +13,7 @@ JS_DATA_FOLDER = CURRENT_TEST_DATA_FOLDER / 'js' PYTHON_AST_DATA_FOLDER = CURRENT_TEST_DATA_FOLDER / 'python_ast' + +CHECKSTYLE_DATA_FOLDER = CURRENT_TEST_DATA_FOLDER / 'checkstyle' + +PMD_DATA_FOLDER = CURRENT_TEST_DATA_FOLDER / 'pmd' diff --git a/test/python/inspectors/test_checkstyle_inspector.py b/test/python/inspectors/test_checkstyle_inspector.py index f13062aa..d5069a5c 100644 --- a/test/python/inspectors/test_checkstyle_inspector.py +++ b/test/python/inspectors/test_checkstyle_inspector.py @@ -1,11 +1,154 @@ -from test.python.inspectors import JAVA_DATA_FOLDER +from pathlib import Path +from test.python.inspectors import CHECKSTYLE_DATA_FOLDER, JAVA_DATA_FOLDER from test.python.inspectors.conftest import gather_issues_test_info, IssuesTestInfo, use_file_metadata +from typing import List import pytest from src.python.review.common.language import Language from src.python.review.inspectors.checkstyle.checkstyle import CheckstyleInspector +from src.python.review.inspectors.inspector_type import InspectorType +from src.python.review.inspectors.issue import ( + BoolExprLenIssue, + CodeIssue, + CyclomaticComplexityIssue, + FuncLenIssue, + IssueType, + LineLenIssue, +) +from src.python.review.inspectors.parsers.checkstyle_parser import parse_checkstyle_file_result +from src.python.review.inspectors.tips import ( + get_bool_expr_len_tip, + get_cyclomatic_complexity_tip, + get_func_len_tip, + get_line_len_tip, +) from src.python.review.reviewers.utils.issues_filter import filter_low_measure_issues +FILE_NAME_AND_ISSUES = [ + ('empty_file.xml', []), + ('single_file_project_without_issues.xml', []), + ('multi_file_project_without_issues.xml', []), + ( + 'single_file_project_without_metric_issues.xml', + [ + CodeIssue( + origin_class='MultipleStringLiteralsCheck', type=IssueType.BEST_PRACTICES, + description='The String "Howdy" appears 4 times in the file.', + file_path=Path('/home/user/Desktop/some_project/main.java'), + line_no=6, column_no=13, inspector_type=InspectorType.CHECKSTYLE, + ), + CodeIssue( + origin_class='WhitespaceAroundCheck', type=IssueType.CODE_STYLE, + description="'{' is not followed by whitespace.", + file_path=Path('/home/user/Desktop/some_project/main.java'), + line_no=12, column_no=32, inspector_type=InspectorType.CHECKSTYLE, + ), + CodeIssue( + origin_class='StringLiteralEqualityCheck', type=IssueType.ERROR_PRONE, + description="Literal Strings should be compared using equals(), not '=='.", + file_path=Path('/home/user/Desktop/some_project/main.java'), + line_no=37, column_no=33, inspector_type=InspectorType.CHECKSTYLE, + ), + ], + ), + ( + 'single_file_project_with_metric_issues.xml', + [ + FuncLenIssue( + origin_class='JavaNCSSCheck', type=IssueType.FUNC_LEN, + description=get_func_len_tip(), + file_path=Path('/home/user/Desktop/some_project/main.java'), + line_no=5, column_no=5, inspector_type=InspectorType.CHECKSTYLE, + func_len=42, + ), + CyclomaticComplexityIssue( + origin_class='CyclomaticComplexityCheck', type=IssueType.CYCLOMATIC_COMPLEXITY, + description=get_cyclomatic_complexity_tip(), + file_path=Path('/home/user/Desktop/some_project/main.java'), + line_no=5, column_no=5, inspector_type=InspectorType.CHECKSTYLE, + cc_value=69, + ), + CodeIssue( + origin_class='WhitespaceAroundCheck', type=IssueType.CODE_STYLE, + description="'switch' is not followed by whitespace.", + file_path=Path('/home/user/Desktop/some_project/main.java'), + line_no=31, column_no=25, inspector_type=InspectorType.CHECKSTYLE, + ), + ], + ), + ( + 'multi_file_project_without_metric_issues.xml', + [ + CodeIssue( + origin_class='EmptyBlockCheck', type=IssueType.BEST_PRACTICES, + description='Must have at least one statement.', + file_path=Path('/home/user/Desktop/some_project/main1.java'), + line_no=62, column_no=38, inspector_type=InspectorType.CHECKSTYLE, + ), + CodeIssue( + origin_class='CovariantEqualsCheck', type=IssueType.ERROR_PRONE, + description='covariant equals without overriding equals(java.lang.Object).', + file_path=Path('/home/user/Desktop/some_project/main1.java'), + line_no=68, column_no=20, inspector_type=InspectorType.CHECKSTYLE, + ), + CodeIssue( + origin_class='IndentationCheck', type=IssueType.CODE_STYLE, + description="'if' child has incorrect indentation level 2, expected level should be 12.", + file_path=Path('/home/user/Desktop/some_project/main2.java'), + line_no=116, column_no=3, inspector_type=InspectorType.CHECKSTYLE, + ), + CodeIssue( + origin_class='UpperEllCheck', type=IssueType.BEST_PRACTICES, + description="Should use uppercase 'L'.", + file_path=Path('/home/user/Desktop/some_project/main2.java'), + line_no=123, column_no=15, inspector_type=InspectorType.CHECKSTYLE, + ), + ], + ), + ( + 'multi_file_project_with_metric_issues.xml', + [ + CodeIssue( + origin_class='OneStatementPerLineCheck', type=IssueType.CODE_STYLE, + description='Only one statement per line allowed.', + file_path=Path('/home/user/Desktop/some_project/main1.java'), + line_no=69, column_no=31, inspector_type=InspectorType.CHECKSTYLE, + ), + BoolExprLenIssue( + origin_class='BooleanExpressionComplexityCheck', type=IssueType.BOOL_EXPR_LEN, + description=get_bool_expr_len_tip(), + file_path=Path('/home/user/Desktop/some_project/main1.java'), + line_no=112, column_no=9, inspector_type=InspectorType.CHECKSTYLE, bool_expr_len=77, + ), + LineLenIssue( + origin_class='LineLengthCheck', type=IssueType.LINE_LEN, + description=get_line_len_tip(), + file_path=Path('/home/user/Desktop/some_project/main2.java'), + line_no=62, column_no=1, inspector_type=InspectorType.CHECKSTYLE, line_len=228, + ), + CodeIssue( + origin_class='WhitespaceAfterCheck', type=IssueType.CODE_STYLE, + description="',' is not followed by whitespace.", + file_path=Path('/home/user/Desktop/some_project/main2.java'), + line_no=136, column_no=19, inspector_type=InspectorType.CHECKSTYLE, + ), + ], + ), +] + + +@pytest.mark.parametrize(('file_name', 'expected_issues'), FILE_NAME_AND_ISSUES) +def test_output_parsing(file_name: str, expected_issues: List[CodeIssue]): + path_to_file = CHECKSTYLE_DATA_FOLDER / file_name + issues = parse_checkstyle_file_result( + path_to_file, + InspectorType.CHECKSTYLE, + CheckstyleInspector.choose_issue_type, + CheckstyleInspector.origin_class_to_pattern, + ) + assert issues == expected_issues + + FILE_NAMES_AND_N_ISSUES = [ ('test_simple_valid_program.java', 0), ('test_spaces.java', 14), diff --git a/test/python/inspectors/test_pmd_inspector.py b/test/python/inspectors/test_pmd_inspector.py index 71ae077e..03957f20 100644 --- a/test/python/inspectors/test_pmd_inspector.py +++ b/test/python/inspectors/test_pmd_inspector.py @@ -1,10 +1,86 @@ -from test.python.inspectors import JAVA_DATA_FOLDER +from pathlib import Path +from test.python.inspectors import JAVA_DATA_FOLDER, PMD_DATA_FOLDER +from typing import List import pytest +from src.python.review.inspectors.inspector_type import InspectorType +from src.python.review.inspectors.issue import CodeIssue, IssueType from src.python.review.inspectors.pmd.pmd import PMDInspector from .conftest import use_file_metadata +FILE_NAME_AND_ISSUES = [ + ('empty_file.csv', []), + ('project_without_issues.csv', []), + ( + 'single_file_project.csv', + [ + CodeIssue( + origin_class='AvoidDuplicateLiterals', type=IssueType.BEST_PRACTICES, + description="The String literal 'Howdy' appears 4 times in this file; " + "the first occurrence is on line 6", + file_path=Path('/home/user/Desktop/some_project/main.java'), + line_no=6, column_no=1, inspector_type=InspectorType.PMD, + ), + CodeIssue( + origin_class='UncommentedEmptyMethodBody', type=IssueType.BEST_PRACTICES, + description='Document empty method body', + file_path=Path('/home/user/Desktop/some_project/main.java'), + line_no=12, column_no=1, inspector_type=InspectorType.PMD, + ), + CodeIssue( + origin_class='UnusedLocalVariable', type=IssueType.BEST_PRACTICES, + description="Avoid unused local variables such as 'result'.", + file_path=Path('/home/user/Desktop/some_project/main.java'), + line_no=31, column_no=1, inspector_type=InspectorType.PMD, + ), + CodeIssue( + origin_class='UnusedPrivateMethod', type=IssueType.BEST_PRACTICES, + description="Avoid unused private methods such as 'emptyLoop()'.", + file_path=Path('/home/user/Desktop/some_project/main.java'), + line_no=61, column_no=1, inspector_type=InspectorType.PMD, + ), + ], + ), + ( + 'multi_file_project.csv', + [ + CodeIssue( + origin_class='CompareObjectsWithEquals', type=IssueType.ERROR_PRONE, + description='Use equals() to compare object references.', + file_path=Path('/home/user/Desktop/some_project/main1.java'), + line_no=37, column_no=1, inspector_type=InspectorType.PMD, + ), + CodeIssue( + origin_class='SuspiciousEqualsMethodName', type=IssueType.ERROR_PRONE, + description='The method name and parameter number are suspiciously close to equals(Object)', + file_path=Path('/home/user/Desktop/some_project/main1.java'), + line_no=68, column_no=1, inspector_type=InspectorType.PMD, + ), + CodeIssue( + origin_class='UselessParentheses', type=IssueType.CODE_STYLE, + description='Useless parentheses.', + file_path=Path('/home/user/Desktop/some_project/main2.java'), + line_no=113, column_no=1, inspector_type=InspectorType.PMD, + ), + CodeIssue( + origin_class='EmptyIfStmt', type=IssueType.BEST_PRACTICES, + description='Avoid empty if statements', + file_path=Path('/home/user/Desktop/some_project/main2.java'), + line_no=131, column_no=1, inspector_type=InspectorType.PMD, + ), + ], + ), +] + + +@pytest.mark.parametrize(('file_name', 'expected_issues'), FILE_NAME_AND_ISSUES) +def test_output_parsing(file_name: str, expected_issues: List[CodeIssue]): + path_to_file = PMD_DATA_FOLDER / file_name + issues = PMDInspector().parse_output(path_to_file) + assert issues == expected_issues + + FILE_NAMES_AND_N_ISSUES = [ ('test_algorithm_with_scanner.java', 0), ('test_simple_valid_program.java', 0), diff --git a/test/resources/inspectors/checkstyle/empty_file.xml b/test/resources/inspectors/checkstyle/empty_file.xml new file mode 100644 index 00000000..e69de29b diff --git a/test/resources/inspectors/checkstyle/multi_file_project_with_metric_issues.xml b/test/resources/inspectors/checkstyle/multi_file_project_with_metric_issues.xml new file mode 100644 index 00000000..df3e577d --- /dev/null +++ b/test/resources/inspectors/checkstyle/multi_file_project_with_metric_issues.xml @@ -0,0 +1,15 @@ + + + + + + + + + + + diff --git a/test/resources/inspectors/checkstyle/multi_file_project_without_issues.xml b/test/resources/inspectors/checkstyle/multi_file_project_without_issues.xml new file mode 100644 index 00000000..4af12849 --- /dev/null +++ b/test/resources/inspectors/checkstyle/multi_file_project_without_issues.xml @@ -0,0 +1,9 @@ + + + + + + + + + diff --git a/test/resources/inspectors/checkstyle/multi_file_project_without_metric_issues.xml b/test/resources/inspectors/checkstyle/multi_file_project_without_metric_issues.xml new file mode 100644 index 00000000..ff8f3ed4 --- /dev/null +++ b/test/resources/inspectors/checkstyle/multi_file_project_without_metric_issues.xml @@ -0,0 +1,17 @@ + + + + + + + + + + + diff --git a/test/resources/inspectors/checkstyle/single_file_project_with_metric_issues.xml b/test/resources/inspectors/checkstyle/single_file_project_with_metric_issues.xml new file mode 100644 index 00000000..598cf159 --- /dev/null +++ b/test/resources/inspectors/checkstyle/single_file_project_with_metric_issues.xml @@ -0,0 +1,11 @@ + + + + + + + + diff --git a/test/resources/inspectors/checkstyle/single_file_project_without_issues.xml b/test/resources/inspectors/checkstyle/single_file_project_without_issues.xml new file mode 100644 index 00000000..60219b74 --- /dev/null +++ b/test/resources/inspectors/checkstyle/single_file_project_without_issues.xml @@ -0,0 +1,4 @@ + + + + diff --git a/test/resources/inspectors/checkstyle/single_file_project_without_metric_issues.xml b/test/resources/inspectors/checkstyle/single_file_project_without_metric_issues.xml new file mode 100644 index 00000000..121bbd22 --- /dev/null +++ b/test/resources/inspectors/checkstyle/single_file_project_without_metric_issues.xml @@ -0,0 +1,12 @@ + + + + + + + + diff --git a/test/resources/inspectors/pmd/empty_file.csv b/test/resources/inspectors/pmd/empty_file.csv new file mode 100644 index 00000000..e69de29b diff --git a/test/resources/inspectors/pmd/multi_file_project.csv b/test/resources/inspectors/pmd/multi_file_project.csv new file mode 100644 index 00000000..bba8268c --- /dev/null +++ b/test/resources/inspectors/pmd/multi_file_project.csv @@ -0,0 +1,5 @@ +"Problem","Package","File","Priority","Line","Description","Rule set","Rule" +"1","","/home/user/Desktop/some_project/main1.java","3","37","Use equals() to compare object references.","Error Prone","CompareObjectsWithEquals" +"2","","/home/user/Desktop/some_project/main1.java","2","68","The method name and parameter number are suspiciously close to equals(Object)","Error Prone","SuspiciousEqualsMethodName" +"3","","/home/user/Desktop/some_project/main2.java","4","113","Useless parentheses.","Code Style","UselessParentheses" +"4","","/home/user/Desktop/some_project/main2.java","3","131","Avoid empty if statements","Error Prone","EmptyIfStmt" diff --git a/test/resources/inspectors/pmd/project_without_issues.csv b/test/resources/inspectors/pmd/project_without_issues.csv new file mode 100644 index 00000000..0a5f43af --- /dev/null +++ b/test/resources/inspectors/pmd/project_without_issues.csv @@ -0,0 +1 @@ +"Problem","Package","File","Priority","Line","Description","Rule set","Rule" diff --git a/test/resources/inspectors/pmd/single_file_project.csv b/test/resources/inspectors/pmd/single_file_project.csv new file mode 100644 index 00000000..78d15d72 --- /dev/null +++ b/test/resources/inspectors/pmd/single_file_project.csv @@ -0,0 +1,5 @@ +"Problem","Package","File","Priority","Line","Description","Rule set","Rule" +"1","","/home/user/Desktop/some_project/main.java","3","6","The String literal 'Howdy' appears 4 times in this file; the first occurrence is on line 6","Error Prone","AvoidDuplicateLiterals" +"2","","/home/user/Desktop/some_project/main.java","3","12","Document empty method body","Documentation","UncommentedEmptyMethodBody" +"3","","/home/user/Desktop/some_project/main.java","3","31","Avoid unused local variables such as 'result'.","Best Practices","UnusedLocalVariable" +"4","","/home/user/Desktop/some_project/main.java","3","61","Avoid unused private methods such as 'emptyLoop()'.","Best Practices","UnusedPrivateMethod" \ No newline at end of file diff --git a/whitelist.txt b/whitelist.txt index 5e10335d..b505f492 100644 --- a/whitelist.txt +++ b/whitelist.txt @@ -182,3 +182,4 @@ Singleline Xpath Ctor Atclause +puppycrawl