From bb2c921462b57891acbc1b4a592215bb122d87a7 Mon Sep 17 00:00:00 2001 From: Peter Galantha Date: Thu, 7 Dec 2017 18:26:01 -0800 Subject: [PATCH 1/7] support java tools: checkstyle and clover --- diff_cover/tool.py | 17 +- .../java_violations_reporter.py | 176 ++++++++++++++++++ 2 files changed, 191 insertions(+), 2 deletions(-) create mode 100644 diff_cover/violationsreporters/java_violations_reporter.py diff --git a/diff_cover/tool.py b/diff_cover/tool.py index 5828706b..77313ed3 100644 --- a/diff_cover/tool.py +++ b/diff_cover/tool.py @@ -25,6 +25,9 @@ flake8_driver, pyflakes_driver, PylintDriver, jshint_driver, eslint_driver, pydocstyle_driver, pycodestyle_driver) +from diff_cover.violationsreporters.java_violations_reporter import ( + CloverXmlCoverageReporter, + CheckstyleXmlDriver, checkstyle_driver) QUALITY_DRIVERS = { 'pycodestyle': pycodestyle_driver, @@ -33,7 +36,9 @@ 'flake8': flake8_driver, 'jshint': jshint_driver, 'eslint': eslint_driver, - 'pydocstyle': pydocstyle_driver + 'pydocstyle': pydocstyle_driver, + 'checkstyle': checkstyle_driver, + 'checkstylexml': CheckstyleXmlDriver() } COVERAGE_XML_HELP = "XML coverage report" @@ -243,7 +248,15 @@ def generate_coverage_report(coverage_xml, compare_branch, ignore_unstaged=ignore_unstaged, exclude=exclude) xml_roots = [cElementTree.parse(xml_root) for xml_root in coverage_xml] - coverage = XmlCoverageReporter(xml_roots) + clover_xml_roots = [clover_xml for clover_xml in xml_roots if clover_xml.findall('.[@clover]')] + cobertura_xml_roots = [cobertura_xml for cobertura_xml in xml_roots if cobertura_xml.findall('.[@line-rate]')] + if clover_xml_roots and cobertura_xml_roots: + raise TypeError("Can't handle mixed coverage reports") + elif clover_xml_roots: + coverage = CloverXmlCoverageReporter(clover_xml_roots) + elif cobertura_xml_roots: + coverage = XmlCoverageReporter(cobertura_xml_roots) + # Build a report generator if html_report is not None: diff --git a/diff_cover/violationsreporters/java_violations_reporter.py b/diff_cover/violationsreporters/java_violations_reporter.py new file mode 100644 index 00000000..49f808b4 --- /dev/null +++ b/diff_cover/violationsreporters/java_violations_reporter.py @@ -0,0 +1,176 @@ +""" +Classes for querying the information in a test coverage report. +""" +from __future__ import unicode_literals + +import re +from collections import defaultdict + +import os +import itertools +import posixpath +from xml.etree import cElementTree +from diff_cover.command_runner import run_command_for_code +from diff_cover.git_path import GitPathTool +from diff_cover.violationsreporters.base import BaseViolationReporter, Violation, RegexBasedDriver, QualityDriver + + +class CloverXmlCoverageReporter(BaseViolationReporter): + """ + Query information from a Clover XML coverage report. + """ + + def __init__(self, xml_roots): + """ + Load the Clover XML coverage report represented + by the cElementTree with root element `xml_root`. + """ + super(CloverXmlCoverageReporter, self).__init__("XML") + self._xml_roots = xml_roots + + # Create a dict to cache violations dict results + # Keys are source file paths, values are output of `violations()` + self._info_cache = defaultdict(list) + + @staticmethod + def _get_src_path_line_nodes(xml_document): + """ + Returns a list of nodes containing line information for `src_path` + in `xml_document`. + + If file is not present in `xml_document`, return None + """ + files = [file_tree + for file_tree in xml_document.findall(".//file") + or []] + if not files: + return None + else: + lines = [file.findall('./line[@type="stmt"]') for file in files] + return [elem for elem in itertools.chain(*lines)] + + def _cache_file(self, src_path): + """ + Load the data from `self._xml_roots` + for `src_path`, if it hasn't been already. + """ + # If we have not yet loaded this source file + if src_path not in self._info_cache: + # We only want to keep violations that show up in each xml source. + # Thus, each time, we take the intersection. However, to do this + # we must treat the first time as a special case and just add all + # the violations from the first xml report. + violations = None + + # A line is measured if it is measured in any of the reports, so + # we take set union each time and can just start with the empty set + measured = set() + + # Loop through the files that contain the xml roots + for xml_document in self._xml_roots: + line_nodes = self._get_src_path_line_nodes(xml_document) + + if line_nodes is None: + continue + + # First case, need to define violations initially + if violations is None: + violations = set( + Violation(int(line.get('num')), None) + for line in line_nodes + if int(line.get('count', 0)) == 0) + + # If we already have a violations set, + # take the intersection of the new + # violations set and its old self + else: + violations = violations & set( + Violation(int(line.get('num')), None) + for line in line_nodes + if int(line.get('count', 0)) == 0 + ) + + # Measured is the union of itself and the new measured + measured = measured | set( + int(line.get('num')) for line in line_nodes + ) + + # If we don't have any information about the source file, + # don't report any violations + if violations is None: + violations = set() + self._info_cache[src_path] = (violations, measured) + + def violations(self, src_path): + """ + See base class comments. + """ + + self._cache_file(src_path) + + # Yield all lines not covered + return self._info_cache[src_path][0] + + def measured_lines(self, src_path): + """ + See base class docstring. + """ + self._cache_file(src_path) + return self._info_cache[src_path][1] + + +""" + Report checkstyle violations. + + http://checkstyle.sourceforge.net/apidocs/com/puppycrawl/tools/checkstyle/DefaultLogger.html + https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/AuditEventDefaultFormatter.java +""" +checkstyle_driver = RegexBasedDriver( + name='checkstyle', + supported_extensions=['java'], + command=['checkstyle'], + expression=r'^\[\w+\]\s+([^:]+):(\d+):(?:\d+:)? (.*)$', + command_to_check_install=['java', '-cp', 'checkstyle-8.5-all.jar', 'com.puppycrawl.tools.checkstyle.Main', '-version'] +) + + +class CheckstyleXmlDriver(QualityDriver): + def __init__(self): + """ + args: + expression: regex used to parse report + See super for other args + """ + super(CheckstyleXmlDriver, self).__init__( + 'checkstyle', + ['java'], + ['java', '-jar', 'checkstyle-8.5-all.jar', '-c', '/google_checks.xml' ] + ) + self.command_to_check_install = ['java', '-cp', 'checkstyle-8.5-all.jar', 'com.puppycrawl.tools.checkstyle.Main', '-version'] + + def parse_reports(self, reports): + """ + Args: + reports: list[str] - output from the report + Return: + A dict[Str:Violation] + Violation is a simple named tuple Defined above + """ + xml_document = cElementTree.fromstring("".join(reports)) + files = xml_document.findall(".//file") + violations_dict = defaultdict(list) + for file in files: + for error in file.findall('error'): + line_number = error.get('line') + error_str = u"{0}: {1}".format(error.get('severity'), error.get('message')) + violation = Violation(int(line_number), error_str) + filename = GitPathTool.relative_path(file.get('name')) + violations_dict[filename].append(violation) + return violations_dict + + def installed(self): + """ + Method checks if the provided tool is installed. + Returns: boolean True if installed + """ + return run_command_for_code(self.command_to_check_install) == 0 From 1666c1821821c0add1ba362926b4569aed7b49ef Mon Sep 17 00:00:00 2001 From: Peter Galantha Date: Fri, 8 Dec 2017 04:24:55 -0800 Subject: [PATCH 2/7] added tests --- .../tests/test_java_violations_reporter.py | 487 ++++++++++++++++++ .../java_violations_reporter.py | 44 +- 2 files changed, 510 insertions(+), 21 deletions(-) create mode 100644 diff_cover/tests/test_java_violations_reporter.py diff --git a/diff_cover/tests/test_java_violations_reporter.py b/diff_cover/tests/test_java_violations_reporter.py new file mode 100644 index 00000000..9d00543d --- /dev/null +++ b/diff_cover/tests/test_java_violations_reporter.py @@ -0,0 +1,487 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +import os +import subprocess +import xml.etree.cElementTree as etree +from xml.etree.ElementTree import tostring +from subprocess import Popen +from textwrap import dedent +import mock + +import six +from diff_cover.violationsreporters import base + +from diff_cover.command_runner import CommandError, run_command_for_code +import unittest +from diff_cover.violationsreporters.base import QualityReporter +from diff_cover.violationsreporters.java_violations_reporter import ( + CloverXmlCoverageReporter, Violation, checkstyle_driver, + CheckstyleXmlDriver) +from mock import Mock, patch, MagicMock +from six import BytesIO, StringIO + + +def _patch_so_all_files_exist(): + _mock_exists = patch.object(base.os.path, 'exists').start() + _mock_exists.returnvalue = True + +def _setup_patch(return_value, status_code=0): + mocked_process = mock.Mock() + mocked_process.returncode = status_code + mocked_process.communicate.return_value = return_value + mocked_subprocess = mock.patch('diff_cover.command_runner.subprocess').start() + mocked_subprocess.Popen.return_value = mocked_process + return mocked_process + + +class CloverXmlCoverageReporterTest(unittest.TestCase): + + MANY_VIOLATIONS = set([Violation(3, None), Violation(7, None), + Violation(11, None), Violation(13, None)]) + FEW_MEASURED = set([2, 3, 5, 7, 11, 13]) + + FEW_VIOLATIONS = set([Violation(3, None), Violation(11, None)]) + MANY_MEASURED = set([2, 3, 5, 7, 11, 13, 17]) + + ONE_VIOLATION = set([Violation(11, None)]) + VERY_MANY_MEASURED = set([2, 3, 5, 7, 11, 13, 17, 23, 24, 25, 26, 26, 27]) + + def setUp(self): + # Paths generated by git_path are always the given argument + _git_path_mock = patch('diff_cover.violationsreporters.violations_reporter.GitPathTool').start() + _git_path_mock.relative_path = lambda path: path + _git_path_mock.absolute_path = lambda path: path + + def tearDown(self): + patch.stopall() + + def test_violations(self): + + # Construct the XML report + file_paths = ['file1.java', 'subdir/file2.java'] + violations = self.MANY_VIOLATIONS + measured = self.FEW_MEASURED + xml = self._coverage_xml(file_paths, violations, measured) + + # Parse the report + coverage = CloverXmlCoverageReporter(xml) + + # Expect that the name is set + self.assertEqual(coverage.name(), "XML") + + # By construction, each file has the same set + # of covered/uncovered lines + self.assertEqual(violations, coverage.violations('file1.java')) + self.assertEqual(measured, coverage.measured_lines('file1.java')) + + # Try getting a smaller range + result = coverage.violations('subdir/file2.java') + self.assertEqual(result, violations) + + # Once more on the first file (for caching) + result = coverage.violations('file1.java') + self.assertEqual(result, violations) + + def test_two_inputs_first_violate(self): + + # Construct the XML report + file_paths = ['file1.java'] + + violations1 = self.MANY_VIOLATIONS + violations2 = self.FEW_VIOLATIONS + + measured1 = self.FEW_MEASURED + measured2 = self.MANY_MEASURED + + xml = self._coverage_xml(file_paths, violations1, measured1) + xml2 = self._coverage_xml(file_paths, violations2, measured2) + + # Parse the report + coverage = CloverXmlCoverageReporter([xml, xml2]) + + # By construction, each file has the same set + # of covered/uncovered lines + self.assertEqual( + violations1 & violations2, + coverage.violations('file1.java') + ) + + self.assertEqual( + measured1 | measured2, + coverage.measured_lines('file1.java') + ) + + def test_two_inputs_second_violate(self): + + # Construct the XML report + file_paths = ['file1.java'] + + violations1 = self.MANY_VIOLATIONS + violations2 = self.FEW_VIOLATIONS + + measured1 = self.FEW_MEASURED + measured2 = self.MANY_MEASURED + + xml = self._coverage_xml(file_paths, violations1, measured1) + xml2 = self._coverage_xml(file_paths, violations2, measured2) + + # Parse the report + coverage = CloverXmlCoverageReporter([xml2, xml]) + + # By construction, each file has the same set + # of covered/uncovered lines + self.assertEqual( + violations1 & violations2, + coverage.violations('file1.java') + ) + + self.assertEqual( + measured1 | measured2, + coverage.measured_lines('file1.java') + ) + + def test_three_inputs(self): + + # Construct the XML report + file_paths = ['file1.java'] + + violations1 = self.MANY_VIOLATIONS + violations2 = self.FEW_VIOLATIONS + violations3 = self.ONE_VIOLATION + + measured1 = self.FEW_MEASURED + measured2 = self.MANY_MEASURED + measured3 = self.VERY_MANY_MEASURED + + xml = self._coverage_xml(file_paths, violations1, measured1) + xml2 = self._coverage_xml(file_paths, violations2, measured2) + xml3 = self._coverage_xml(file_paths, violations3, measured3) + + # Parse the report + coverage = CloverXmlCoverageReporter([xml2, xml, xml3]) + + # By construction, each file has the same set + # of covered/uncovered lines + self.assertEqual( + violations1 & violations2 & violations3, + coverage.violations('file1.java') + ) + + self.assertEqual( + measured1 | measured2 | measured3, + coverage.measured_lines('file1.java') + ) + + def test_different_files_in_inputs(self): + + # Construct the XML report + xml_roots = [ + self._coverage_xml(['file.java'], self.MANY_VIOLATIONS, self.FEW_MEASURED), + self._coverage_xml(['other_file.java'], self.FEW_VIOLATIONS, self.MANY_MEASURED) + ] + + # Parse the report + coverage = CloverXmlCoverageReporter(xml_roots) + + v = coverage.violations('file.java') + m = coverage.measured_lines('file.java') + self.assertEqual(self.MANY_VIOLATIONS, v) + self.assertEqual(self.FEW_VIOLATIONS, coverage.violations('other_file.java')) + + def test_empty_violations(self): + """ + Test that an empty violations report is handled properly + """ + + # Construct the XML report + file_paths = ['file1.java'] + + violations1 = self.MANY_VIOLATIONS + violations2 = set() + + measured1 = self.FEW_MEASURED + measured2 = self.MANY_MEASURED + + xml = self._coverage_xml(file_paths, violations1, measured1) + xml2 = self._coverage_xml(file_paths, violations2, measured2) + + # Parse the report + coverage = CloverXmlCoverageReporter([xml2, xml]) + + # By construction, each file has the same set + # of covered/uncovered lines + self.assertEqual( + violations1 & violations2, + coverage.violations('file1.java') + ) + + self.assertEqual( + measured1 | measured2, + coverage.measured_lines('file1.java') + ) + + def test_no_such_file(self): + + # Construct the XML report with no source files + xml = self._coverage_xml([], [], []) + + # Parse the report + coverage = CloverXmlCoverageReporter(xml) + + # Expect that we get no results + result = coverage.violations('file.java') + self.assertEqual(result, set([])) + + def _coverage_xml( + self, + file_paths, + violations, + measured + ): + """ + Build an XML tree with source files specified by `file_paths`. + Each source fill will have the same set of covered and + uncovered lines. + + `file_paths` is a list of path strings + `line_dict` is a dictionary with keys that are line numbers + and values that are True/False indicating whether the line + is covered + + This leaves out some attributes of the Cobertura format, + but includes all the elements. + """ + root = etree.Element('coverage') + root.set('clover', '4.2.0') + project = etree.SubElement(root, 'project') + package = etree.SubElement(project, 'package') + + violation_lines = set(violation.line for violation in violations) + + for path in file_paths: + + src_node = etree.SubElement(package, 'file') + src_node.set('path', path) + + # Create a node for each line in measured + for line_num in measured: + is_covered = line_num not in violation_lines + line = etree.SubElement(src_node, 'line') + + hits = 1 if is_covered else 0 + line.set('count', str(hits)) + line.set('num', str(line_num)) + line.set('type', 'stmt') + return root + + +class CheckstyleQualityReporterTest(unittest.TestCase): + """Tests for checkstyle quality violations.""" + + def setUp(self): + """Set up required files.""" + _patch_so_all_files_exist() + + def tearDown(self): + """Undo all patches.""" + patch.stopall() + + def test_no_such_file(self): + """Expect that we get no results.""" + quality = QualityReporter(checkstyle_driver) + + result = quality.violations('') + self.assertEqual(result, []) + + def test_no_java_file(self): + """Expect that we get no results because no Python files.""" + quality = QualityReporter(checkstyle_driver) + file_paths = ['file1.coffee', 'subdir/file2.js'] + for path in file_paths: + result = quality.violations(path) + self.assertEqual(result, []) + + def test_quality(self): + """Integration test.""" + # Patch the output of `checkstyle` + _setup_patch(( + dedent(""" + [WARN] ../new_file.java:1:1: Line contains a tab character. + [WARN] ../new_file.java:13: 'if' construct must use '{}'s. + """).strip().encode('ascii'), '' + )) + + expected_violations = [ + Violation(1, 'Line contains a tab character.'), + Violation(13, "'if' construct must use '{}'s."), + ] + + # Parse the report + quality = QualityReporter(checkstyle_driver) + + # Expect that the name is set + self.assertEqual(quality.name(), 'checkstyle') + + # Measured_lines is undefined for a + # quality reporter since all lines are measured + self.assertEqual(quality.measured_lines('../new_file.java'), None) + + # Expect that we get violations for file1.java only + # We're not guaranteed that the violations are returned + # in any particular order. + actual_violations = quality.violations('../new_file.java') + self.assertEqual(len(actual_violations), len(expected_violations)) + for expected in expected_violations: + self.assertIn(expected, actual_violations) + +class CheckstyleXmlQualityReporterTest(unittest.TestCase): + + def setUp(self): + _patch_so_all_files_exist() + # Paths generated by git_path are always the given argument + _git_path_mock = patch('diff_cover.violationsreporters.java_violations_reporter.GitPathTool').start() + _git_path_mock.relative_path = lambda path: path + _git_path_mock.absolute_path = lambda path: path + + def tearDown(self): + """ + Undo all patches. + """ + patch.stopall() + + def test_no_such_file(self): + quality = QualityReporter(CheckstyleXmlDriver()) + + # Expect that we get no results + result = quality.violations('') + self.assertEqual(result, []) + + def test_no_java_file(self): + quality = QualityReporter(CheckstyleXmlDriver()) + file_paths = ['file1.coffee', 'subdir/file2.js'] + # Expect that we get no results because no Java files + for path in file_paths: + result = quality.violations(path) + self.assertEqual(result, []) + + def test_quality(self): + # Patch the output of `checkstyle` + _setup_patch(( + dedent(""" + + + + + + + + + + + + + + + + """).strip().encode('ascii'), '' + )) + + expected_violations = [ + Violation(1, 'error: Missing docstring'), + Violation(2, "error: Unused variable 'd'"), + Violation(2, "warning: TODO: Not the real way we'll store usages!"), + Violation(579, "error: Unable to import 'rooted_paths'"), + Violation(150, "error: error while code parsing ([Errno 2] No such file or directory)"), + Violation(149, "error: Comma not followed by a space"), + Violation(113, "error: Unused argument 'cls'") + ] + + # Parse the report + quality = QualityReporter(CheckstyleXmlDriver()) + + # Expect that the name is set + self.assertEqual(quality.name(), 'checkstyle') + + # Measured_lines is undefined for a + # quality reporter since all lines are measured + self.assertEqual(quality.measured_lines('file1.java'), None) + + # Expect that we get violations for file1.java only + # We're not guaranteed that the violations are returned + # in any particular order. + actual_violations = quality.violations('file1.java') + self.assertEqual(len(actual_violations), len(expected_violations)) + for expected in expected_violations: + self.assertIn(expected, actual_violations) + + def test_quality_error(self): + # Patch the output stderr/stdout and returncode of `checkstyle` + _setup_patch(( + dedent(""" + + + + + + + """), b'oops'), status_code=1) + + # Parse the report + with patch('diff_cover.violationsreporters.java_violations_reporter.run_command_for_code') as code: + code.return_value = 0 + quality = QualityReporter(CheckstyleXmlDriver()) + + # Expect an error + self.assertRaises(CommandError, quality.violations, 'file1.java') + + def test_quality_pregenerated_report(self): + + # When the user provides us with a pre-generated pylint report + # then use that instead of calling pylint directly. + checkstyle_reports = [ + BytesIO(dedent(u""" + + + + + + + + + + + + + """).strip().encode('utf-8')), + + BytesIO(dedent(u""" + + + + + + + + + + """).strip().encode('utf-8')) + ] + + # Generate the violation report + quality = QualityReporter(CheckstyleXmlDriver(), reports=checkstyle_reports) + + # Expect that we get the right violations + expected_violations = [ + Violation(1, u'error: Missing docstring'), + Violation(57, u'warning: TODO the name of this method is a little bit confusing'), + Violation(183, u"error: Invalid name '' for type argument (should match [a-z_][a-z0-9_]{2,30}$)") + ] + + # We're not guaranteed that the violations are returned + # in any particular order. + actual_violations = quality.violations('path/to/file.java') + self.assertEqual(len(actual_violations), len(expected_violations)) + for expected in expected_violations: + self.assertIn(expected, actual_violations) + diff --git a/diff_cover/violationsreporters/java_violations_reporter.py b/diff_cover/violationsreporters/java_violations_reporter.py index 49f808b4..d933d5be 100644 --- a/diff_cover/violationsreporters/java_violations_reporter.py +++ b/diff_cover/violationsreporters/java_violations_reporter.py @@ -3,12 +3,9 @@ """ from __future__ import unicode_literals -import re from collections import defaultdict -import os import itertools -import posixpath from xml.etree import cElementTree from diff_cover.command_runner import run_command_for_code from diff_cover.git_path import GitPathTool @@ -33,21 +30,22 @@ def __init__(self, xml_roots): self._info_cache = defaultdict(list) @staticmethod - def _get_src_path_line_nodes(xml_document): + def _get_src_path_line_nodes(xml_document, src_path): """ - Returns a list of nodes containing line information for `src_path` + Return a list of nodes containing line information for `src_path` in `xml_document`. If file is not present in `xml_document`, return None """ files = [file_tree for file_tree in xml_document.findall(".//file") + if file_tree.get('path') == src_path or []] if not files: return None - else: - lines = [file.findall('./line[@type="stmt"]') for file in files] - return [elem for elem in itertools.chain(*lines)] + lines = [file_tree.findall('./line[@type="stmt"]') + for file_tree in files] + return [elem for elem in itertools.chain(*lines)] def _cache_file(self, src_path): """ @@ -68,7 +66,8 @@ def _cache_file(self, src_path): # Loop through the files that contain the xml roots for xml_document in self._xml_roots: - line_nodes = self._get_src_path_line_nodes(xml_document) + line_nodes = self._get_src_path_line_nodes(xml_document, + src_path) if line_nodes is None: continue @@ -142,9 +141,10 @@ def __init__(self): See super for other args """ super(CheckstyleXmlDriver, self).__init__( - 'checkstyle', - ['java'], - ['java', '-jar', 'checkstyle-8.5-all.jar', '-c', '/google_checks.xml' ] + 'checkstyle', + ['java'], + ['java', '-jar', 'checkstyle-8.5-all.jar', '-c', + '/google_checks.xml'] ) self.command_to_check_install = ['java', '-cp', 'checkstyle-8.5-all.jar', 'com.puppycrawl.tools.checkstyle.Main', '-version'] @@ -156,16 +156,18 @@ def parse_reports(self, reports): A dict[Str:Violation] Violation is a simple named tuple Defined above """ - xml_document = cElementTree.fromstring("".join(reports)) - files = xml_document.findall(".//file") violations_dict = defaultdict(list) - for file in files: - for error in file.findall('error'): - line_number = error.get('line') - error_str = u"{0}: {1}".format(error.get('severity'), error.get('message')) - violation = Violation(int(line_number), error_str) - filename = GitPathTool.relative_path(file.get('name')) - violations_dict[filename].append(violation) + for report in reports: + xml_document = cElementTree.fromstring("".join(report)) + files = xml_document.findall(".//file") + for file_tree in files: + for error in file_tree.findall('error'): + line_number = error.get('line') + error_str = u"{0}: {1}".format(error.get('severity'), + error.get('message')) + violation = Violation(int(line_number), error_str) + filename = GitPathTool.relative_path(file_tree.get('name')) + violations_dict[filename].append(violation) return violations_dict def installed(self): From dd1b2ee06ec3a38300e1b36e69fac1af61318ee8 Mon Sep 17 00:00:00 2001 From: Peter Galantha Date: Fri, 8 Dec 2017 04:45:41 -0800 Subject: [PATCH 3/7] address violations --- .../tests/test_java_violations_reporter.py | 22 +++++++------------ 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/diff_cover/tests/test_java_violations_reporter.py b/diff_cover/tests/test_java_violations_reporter.py index 9d00543d..3e8f9d16 100644 --- a/diff_cover/tests/test_java_violations_reporter.py +++ b/diff_cover/tests/test_java_violations_reporter.py @@ -1,31 +1,27 @@ # -*- coding: utf-8 -*- from __future__ import unicode_literals -import os -import subprocess import xml.etree.cElementTree as etree -from xml.etree.ElementTree import tostring -from subprocess import Popen from textwrap import dedent +import unittest import mock +from mock import patch +from six import BytesIO -import six from diff_cover.violationsreporters import base -from diff_cover.command_runner import CommandError, run_command_for_code -import unittest +from diff_cover.command_runner import CommandError from diff_cover.violationsreporters.base import QualityReporter from diff_cover.violationsreporters.java_violations_reporter import ( CloverXmlCoverageReporter, Violation, checkstyle_driver, CheckstyleXmlDriver) -from mock import Mock, patch, MagicMock -from six import BytesIO, StringIO def _patch_so_all_files_exist(): _mock_exists = patch.object(base.os.path, 'exists').start() _mock_exists.returnvalue = True + def _setup_patch(return_value, status_code=0): mocked_process = mock.Mock() mocked_process.returncode = status_code @@ -184,9 +180,7 @@ def test_different_files_in_inputs(self): # Parse the report coverage = CloverXmlCoverageReporter(xml_roots) - v = coverage.violations('file.java') - m = coverage.measured_lines('file.java') - self.assertEqual(self.MANY_VIOLATIONS, v) + self.assertEqual(self.MANY_VIOLATIONS, coverage.violations('file.java')) self.assertEqual(self.FEW_VIOLATIONS, coverage.violations('other_file.java')) def test_empty_violations(self): @@ -335,6 +329,7 @@ def test_quality(self): for expected in expected_violations: self.assertIn(expected, actual_violations) + class CheckstyleXmlQualityReporterTest(unittest.TestCase): def setUp(self): @@ -460,7 +455,7 @@ def test_quality_pregenerated_report(self): - + @@ -484,4 +479,3 @@ def test_quality_pregenerated_report(self): self.assertEqual(len(actual_violations), len(expected_violations)) for expected in expected_violations: self.assertIn(expected, actual_violations) - From 4f98f672c3cdaa8cdc629dc6ae3e846163354810 Mon Sep 17 00:00:00 2001 From: Peter Galantha Date: Fri, 8 Dec 2017 04:51:10 -0800 Subject: [PATCH 4/7] add changelog --- CHANGELOG | 4 ++++ diff_cover/__init__.py | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index 6cedd458..1b38c148 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,7 @@ +12/08/2017 v1.0.1 + + * Add Support for clover and checkstyle + 11/14/2017 v1.0.0 * Drop support for python 3.3 and 2.6 diff --git a/diff_cover/__init__.py b/diff_cover/__init__.py index 55be2162..687e6fdb 100644 --- a/diff_cover/__init__.py +++ b/diff_cover/__init__.py @@ -1,3 +1,3 @@ -VERSION = '1.0.0' +VERSION = '1.0.1' DESCRIPTION = 'Automatically find diff lines that need test coverage.' QUALITY_DESCRIPTION = 'Automatically find diff lines with quality violations.' From b12407037e84cdd97e1b4217453c291648a4499c Mon Sep 17 00:00:00 2001 From: Peter Galantha Date: Fri, 8 Dec 2017 14:29:19 -0800 Subject: [PATCH 5/7] add findbugs support --- .../tests/test_java_violations_reporter.py | 109 +++++++++++++++++- diff_cover/tool.py | 5 +- .../java_violations_reporter.py | 64 +++++++++- 3 files changed, 170 insertions(+), 8 deletions(-) diff --git a/diff_cover/tests/test_java_violations_reporter.py b/diff_cover/tests/test_java_violations_reporter.py index 3e8f9d16..e39cb972 100644 --- a/diff_cover/tests/test_java_violations_reporter.py +++ b/diff_cover/tests/test_java_violations_reporter.py @@ -14,7 +14,7 @@ from diff_cover.violationsreporters.base import QualityReporter from diff_cover.violationsreporters.java_violations_reporter import ( CloverXmlCoverageReporter, Violation, checkstyle_driver, - CheckstyleXmlDriver) + CheckstyleXmlDriver, FindbugsXmlDriver) def _patch_so_all_files_exist(): @@ -432,8 +432,8 @@ def test_quality_error(self): def test_quality_pregenerated_report(self): - # When the user provides us with a pre-generated pylint report - # then use that instead of calling pylint directly. + # When the user provides us with a pre-generated checkstyle report + # then use that instead of calling checkstyle directly. checkstyle_reports = [ BytesIO(dedent(u""" @@ -479,3 +479,106 @@ def test_quality_pregenerated_report(self): self.assertEqual(len(actual_violations), len(expected_violations)) for expected in expected_violations: self.assertIn(expected, actual_violations) + +class FindbugsQualityReporterTest(unittest.TestCase): + + def setUp(self): + _patch_so_all_files_exist() + # Paths generated by git_path are always the given argument + _git_path_mock = patch('diff_cover.violationsreporters.java_violations_reporter.GitPathTool').start() + _git_path_mock.relative_path = lambda path: path + _git_path_mock.absolute_path = lambda path: path + + def tearDown(self): + """ + Undo all patches. + """ + patch.stopall() + + def test_no_such_file(self): + quality = QualityReporter(FindbugsXmlDriver()) + + # Expect that we get no results + result = quality.violations('') + self.assertEqual(result, []) + + def test_no_java_file(self): + quality = QualityReporter(FindbugsXmlDriver()) + file_paths = ['file1.coffee', 'subdir/file2.js'] + # Expect that we get no results because no Java files + for path in file_paths: + result = quality.violations(path) + self.assertEqual(result, []) + + def test_quality_pregenerated_report(self): + # When the user provides us with a pre-generated findbugs report + # then use that instead of calling findbugs directly. + findbugs_reports = [ + BytesIO(dedent(u""" + + + + Consider using Locale parameterized version of invoked method + Use of non-localized String.toUpperCase() or String.toLowerCase() in org.opensource.sample.file$1.isMultipart(HttpServletRequest) + + + At file.java:[lines 94-103] + + In class org.opensource.sample.file$1 + + + + In method org.opensource.sample.file$1.isMultipart(HttpServletRequest) + + + At file.java:[line 97] + + + Another occurrence at file.java:[line 103, 104] + + + + """).strip().encode('utf-8')), + + BytesIO(dedent(u""" + + + + Consider using Locale parameterized version of invoked method + Use of non-localized String.toUpperCase() or String.toLowerCase() in org.opensource.sample.file$1.isMultipart(HttpServletRequest) + + + At file.java:[lines 94-103] + + In class org.opensource.sample.file$1 + + + + In method org.opensource.sample.file$1.isMultipart(HttpServletRequest) + + + At file.java:[line 97] + + + Another occurrence at file.java:[line 183] + + + + """).strip().encode('utf-8')) + ] + + # Generate the violation report + quality = QualityReporter(FindbugsXmlDriver(), reports=findbugs_reports) + + # Expect that we get the right violations + expected_violations = [ + Violation(97, u'I18N: Consider using Locale parameterized version of invoked method'), + Violation(183, u'I18N: Consider using Locale parameterized version of invoked method') + ] + + # We're not guaranteed that the violations are returned + # in any particular order. + actual_violations = quality.violations('path/to/file.java') + self.assertEqual(len(actual_violations), len(expected_violations)) + for expected in expected_violations: + self.assertIn(expected, actual_violations) diff --git a/diff_cover/tool.py b/diff_cover/tool.py index 77313ed3..a862beec 100644 --- a/diff_cover/tool.py +++ b/diff_cover/tool.py @@ -27,7 +27,7 @@ pycodestyle_driver) from diff_cover.violationsreporters.java_violations_reporter import ( CloverXmlCoverageReporter, - CheckstyleXmlDriver, checkstyle_driver) + CheckstyleXmlDriver, checkstyle_driver, FindbugsXmlDriver) QUALITY_DRIVERS = { 'pycodestyle': pycodestyle_driver, @@ -38,7 +38,8 @@ 'eslint': eslint_driver, 'pydocstyle': pydocstyle_driver, 'checkstyle': checkstyle_driver, - 'checkstylexml': CheckstyleXmlDriver() + 'checkstylexml': CheckstyleXmlDriver(), + 'findbugs': FindbugsXmlDriver() } COVERAGE_XML_HELP = "XML coverage report" diff --git a/diff_cover/violationsreporters/java_violations_reporter.py b/diff_cover/violationsreporters/java_violations_reporter.py index d933d5be..73232364 100644 --- a/diff_cover/violationsreporters/java_violations_reporter.py +++ b/diff_cover/violationsreporters/java_violations_reporter.py @@ -136,9 +136,7 @@ def measured_lines(self, src_path): class CheckstyleXmlDriver(QualityDriver): def __init__(self): """ - args: - expression: regex used to parse report - See super for other args + See super for args """ super(CheckstyleXmlDriver, self).__init__( 'checkstyle', @@ -176,3 +174,63 @@ def installed(self): Returns: boolean True if installed """ return run_command_for_code(self.command_to_check_install) == 0 + + +""" + Report checkstyle violations. + + http://checkstyle.sourceforge.net/apidocs/com/puppycrawl/tools/checkstyle/DefaultLogger.html + https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/AuditEventDefaultFormatter.java +""" +checkstyle_driver = RegexBasedDriver( + name='checkstyle', + supported_extensions=['java'], + command=['checkstyle'], + expression=r'^\[\w+\]\s+([^:]+):(\d+):(?:\d+:)? (.*)$', + command_to_check_install=['java', '-cp', 'checkstyle-8.5-all.jar', 'com.puppycrawl.tools.checkstyle.Main', '-version'] +) + + +class FindbugsXmlDriver(QualityDriver): + def __init__(self): + """ + See super for args + """ + super(FindbugsXmlDriver, self).__init__( + 'findbugs', + ['java'], + ['false'] + ) + + def parse_reports(self, reports): + """ + Args: + reports: list[str] - output from the report + Return: + A dict[Str:Violation] + Violation is a simple named tuple Defined above + """ + violations_dict = defaultdict(list) + for report in reports: + xml_document = cElementTree.fromstring("".join(report)) + bugs = xml_document.findall(".//BugInstance") + for bug in bugs: + category = bug.get('category') + short_message = bug.find('ShortMessage').text + line = bug.find('SourceLine') + start = int(line.get('start')) + end = int(line.get('end')) + for line_number in range(start, end+1): + error_str = u"{0}: {1}".format(category, short_message) + violation = Violation(line_number, error_str) + filename = GitPathTool.relative_path(line.get('sourcepath')) + violations_dict[filename].append(violation) + + return violations_dict + + def installed(self): + """ + Method checks if the provided tool is installed. + Returns: boolean False: As findbugs analyses bytecode, it would be hard to run it from outside the build framework. + """ + return False From d16b6836462beed7f9d4e3d9702e7beb0f8cb1c6 Mon Sep 17 00:00:00 2001 From: Peter Galantha Date: Fri, 8 Dec 2017 14:35:19 -0800 Subject: [PATCH 6/7] removed duplicated code --- .../tests/test_java_violations_reporter.py | 1 + .../java_violations_reporter.py | 18 ++---------------- 2 files changed, 3 insertions(+), 16 deletions(-) diff --git a/diff_cover/tests/test_java_violations_reporter.py b/diff_cover/tests/test_java_violations_reporter.py index e39cb972..ab29174b 100644 --- a/diff_cover/tests/test_java_violations_reporter.py +++ b/diff_cover/tests/test_java_violations_reporter.py @@ -480,6 +480,7 @@ def test_quality_pregenerated_report(self): for expected in expected_violations: self.assertIn(expected, actual_violations) + class FindbugsQualityReporterTest(unittest.TestCase): def setUp(self): diff --git a/diff_cover/violationsreporters/java_violations_reporter.py b/diff_cover/violationsreporters/java_violations_reporter.py index 73232364..154cf8a8 100644 --- a/diff_cover/violationsreporters/java_violations_reporter.py +++ b/diff_cover/violationsreporters/java_violations_reporter.py @@ -176,21 +176,6 @@ def installed(self): return run_command_for_code(self.command_to_check_install) == 0 -""" - Report checkstyle violations. - - http://checkstyle.sourceforge.net/apidocs/com/puppycrawl/tools/checkstyle/DefaultLogger.html - https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/AuditEventDefaultFormatter.java -""" -checkstyle_driver = RegexBasedDriver( - name='checkstyle', - supported_extensions=['java'], - command=['checkstyle'], - expression=r'^\[\w+\]\s+([^:]+):(\d+):(?:\d+:)? (.*)$', - command_to_check_install=['java', '-cp', 'checkstyle-8.5-all.jar', 'com.puppycrawl.tools.checkstyle.Main', '-version'] -) - - class FindbugsXmlDriver(QualityDriver): def __init__(self): """ @@ -223,7 +208,8 @@ def parse_reports(self, reports): for line_number in range(start, end+1): error_str = u"{0}: {1}".format(category, short_message) violation = Violation(line_number, error_str) - filename = GitPathTool.relative_path(line.get('sourcepath')) + filename = GitPathTool.relative_path( + line.get('sourcepath')) violations_dict[filename].append(violation) return violations_dict From 00237b3a43f42879215da6a38cfaceb33dc717d7 Mon Sep 17 00:00:00 2001 From: Peter Galantha Date: Fri, 8 Dec 2017 14:53:11 -0800 Subject: [PATCH 7/7] exclude findbugs violations when line information is missing --- diff_cover/violationsreporters/java_violations_reporter.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/diff_cover/violationsreporters/java_violations_reporter.py b/diff_cover/violationsreporters/java_violations_reporter.py index 154cf8a8..4454f53d 100644 --- a/diff_cover/violationsreporters/java_violations_reporter.py +++ b/diff_cover/violationsreporters/java_violations_reporter.py @@ -203,6 +203,8 @@ def parse_reports(self, reports): category = bug.get('category') short_message = bug.find('ShortMessage').text line = bug.find('SourceLine') + if line.get('start') is None or line.get('end') is None: + continue start = int(line.get('start')) end = int(line.get('end')) for line_number in range(start, end+1):