Run pylint like pep8, rather than through diff-lint

pavelib/paver_tests/test_paver_quality.py

@@ -56,7 +56,7 @@ class TestPaverQualityViolations(unittest.TestCase):
     def test_pep8_parser(self):
         with open(self.f.name, 'w') as f:
             f.write("hello\nhithere")
-        num, _violations = pavelib.quality._pep8_violations(f.name)  # pylint: disable=protected-access
+        num = len(pavelib.quality._pep8_violations(f.name))  # pylint: disable=protected-access
         self.assertEqual(num, 2)
 
 
@@ -97,16 +97,11 @@ class TestPaverReportViolationsCounts(unittest.TestCase):
     def setUp(self):
         super(TestPaverReportViolationsCounts, self).setUp()
 
-        # Mock the paver @needs decorator
-        self._mock_paver_needs = patch.object(pavelib.quality.run_quality, 'needs').start()
-        self._mock_paver_needs.return_value = 0
-
         # Temporary file infrastructure
         self.f = tempfile.NamedTemporaryFile(delete=False)
         self.f.close()
 
         # Cleanup various mocks and tempfiles
-        self.addCleanup(self._mock_paver_needs.stop)
         self.addCleanup(os.remove, self.f.name)
 
     def test_get_eslint_violations_count(self):
@@ -294,12 +289,9 @@ class TestPaverRunQuality(unittest.TestCase):
         paver.tasks.environment = paver.tasks.Environment()
 
         # mock the @needs decorator to skip it
-        self._mock_paver_needs = patch.object(pavelib.quality.run_quality, 'needs').start()
-        self._mock_paver_needs.return_value = 0
         patcher = patch('pavelib.quality.sh')
         self._mock_paver_sh = patcher.start()
         self.addCleanup(patcher.stop)
-        self.addCleanup(self._mock_paver_needs.stop)
 
     @patch('__builtin__.open', mock_open())
     def test_failure_on_diffquality_pep8(self):
@@ -326,9 +318,8 @@ class TestPaverRunQuality(unittest.TestCase):
         """
 
         # Underlying sh call must fail when it is running the pylint diff-quality task
-        self._mock_paver_sh.side_effect = fail_on_pylint
-        _mock_pep8_violations = MagicMock(return_value=(0, []))
-        with patch('pavelib.quality._get_pep8_violations', _mock_pep8_violations):
+        _mock_pylint_violations = MagicMock(return_value=(10000, ['some error']))
+        with patch('pavelib.quality._get_pylint_violations', _mock_pylint_violations):
             with self.assertRaises(SystemExit):
                 pavelib.quality.run_quality("")
 

pavelib/quality.py

@@ -74,32 +74,25 @@ def find_fixme(options):
     print "Number of pylint fixmes: " + str(num_fixme)
 
 
-@task
-@needs('pavelib.prereqs.install_python_prereqs')
-@cmdopts([
-    ("system=", "s", "System to act on"),
-    ("errors", "e", "Check for errors only"),
-    ("limit=", "l", "Limits for number of acceptable violations - either <upper> or <lower>:<upper>"),
-])
-@timed
-def run_pylint(options):
+def _get_pylint_violations(systems=ALL_SYSTEMS.split(','), errors_only=False):
     """
-    Run pylint on system code. When violations limit is passed in,
-    fail the task if too many violations are found.
+    Runs pylint. Returns a tuple of (number_of_violations, list_of_violations)
+    where list_of_violations is a list of all pylint violations found, separated
+    by new lines.
     """
-    lower_violations_limit, upper_violations_limit, errors, systems = _parse_pylint_options(options)
-
     # Make sure the metrics subdirectory exists
     Env.METRICS_DIR.makedirs_p()
 
     num_violations = 0
+    violations_list = []
+
     for system in systems:
         # Directory to put the pylint report in.
         # This makes the folder if it doesn't already exist.
         report_dir = (Env.REPORT_DIR / system).makedirs_p()
 
         flags = []
-        if errors:
+        if errors_only:
             flags.append("--errors-only")
 
         apps_list = ' '.join(top_python_dirs(system))
@@ -114,8 +107,33 @@ def run_pylint(options):
             ignore_error=True,
         )
 
-        num_violations += _count_pylint_violations(
-            "{report_dir}/pylint.report".format(report_dir=report_dir))
+        report = "{report_dir}/pylint.report".format(report_dir=report_dir)
+        num_violations += _count_pylint_violations(report)
+        with open(report) as report_contents:
+            violations_list.extend(report_contents)
+
+    # Print number of violations to log
+    return num_violations, violations_list
+
+
+@task
+@needs('pavelib.prereqs.install_python_prereqs')
+@cmdopts([
+    ("system=", "s", "System to act on"),
+    ("errors", "e", "Check for errors only"),
+    ("limit=", "l", "Limits for number of acceptable violations - either <upper> or <lower>:<upper>"),
+])
+@timed
+def run_pylint(options):
+    """
+    Run pylint on system code. When violations limit is passed in,
+    fail the task if too many violations are found.
+    """
+    lower_violations_limit, upper_violations_limit, errors, systems = _parse_pylint_options(options)
+    errors = getattr(options, 'errors', False)
+    systems = getattr(options, 'system', ALL_SYSTEMS).split(',')
+
+    num_violations, violations_list = _get_pylint_violations(systems, errors)
 
     # Print number of violations to log
     violations_count_str = "Number of pylint violations: " + str(num_violations)
@@ -200,22 +218,19 @@ def _get_pep8_violations():
 
     sh('pep8 . | tee {report_dir}/pep8.report -a'.format(report_dir=report_dir))
 
-    count, violations_list = _pep8_violations(
+    violations_list = _pep8_violations(
         "{report_dir}/pep8.report".format(report_dir=report_dir)
     )
 
-    return count, violations_list
+    return (len(violations_list), violations_list)
 
 
 def _pep8_violations(report_file):
     """
-    Returns a tuple of (num_violations, violations_list) for all
-    pep8 violations in the given report_file.
+    Returns the list of all pep8 violations in the given report_file.
     """
     with open(report_file) as f:
-        violations_list = f.readlines()
-    num_lines = len(violations_list)
-    return num_lines, violations_list
+        return f.readlines()
 
 
 @task
@@ -686,7 +701,7 @@ def run_quality(options):
     # Save the pass variable. It will be set to false later if failures are detected.
     diff_quality_percentage_pass = True
 
-    def _pep8_output(count, violations_list, is_html=False):
+    def _lint_output(linter, count, violations_list, is_html=False, limit=0):
         """
         Given a count & list of pep8 violations, pretty-print the pep8 output.
         If `is_html`, will print out with HTML markup.
@@ -694,26 +709,26 @@ def run_quality(options):
         if is_html:
             lines = ['<body>\n']
             sep = '-------------<br/>\n'
-            title = "<h1>Quality Report: pep8</h1>\n"
+            title = HTML("<h1>Quality Report: {}</h1>\n").format(linter)
             violations_bullets = ''.join(
                 [HTML('<li>{violation}</li><br/>\n').format(violation=violation) for violation in violations_list]
             )
             violations_str = HTML('<ul>\n{bullets}</ul>\n').format(bullets=HTML(violations_bullets))
-            violations_count_str = "<b>Violations</b>: {count}<br/>\n"
-            fail_line = "<b>FAILURE</b>: pep8 count should be 0<br/>\n"
+            violations_count_str = HTML("<b>Violations</b>: {count}<br/>\n")
+            fail_line = HTML("<b>FAILURE</b>: {} count should be 0<br/>\n").format(linter)
         else:
             lines = []
             sep = '-------------\n'
-            title = "Quality Report: pep8\n"
+            title = "Quality Report: {}\n".format(linter)
             violations_str = ''.join(violations_list)
             violations_count_str = "Violations: {count}\n"
-            fail_line = "FAILURE: pep8 count should be 0\n"
+            fail_line = "FAILURE: {} count should be {}\n".format(linter, limit)
 
         violations_count_str = violations_count_str.format(count=count)
 
         lines.extend([sep, title, sep, violations_str, sep, violations_count_str])
 
-        if count > 0:
+        if count > limit:
             lines.append(fail_line)
         lines.append(sep + '\n')
         if is_html:
@@ -725,15 +740,31 @@ def run_quality(options):
     (count, violations_list) = _get_pep8_violations()
 
     # Print number of violations to log
-    print _pep8_output(count, violations_list)
+    print _lint_output('pep8', count, violations_list)
 
     # Also write the number of violations to a file
     with open(dquality_dir / "diff_quality_pep8.html", "w") as f:
-        f.write(_pep8_output(count, violations_list, is_html=True))
+        f.write(_lint_output('pep8', count, violations_list, is_html=True))
 
     if count > 0:
         diff_quality_percentage_pass = False
 
+    # Generate diff-quality html report for pylint, and print to console
+    # If pylint reports exist, use those
+    # Otherwise, `diff-quality` will call pylint itself
+
+    (count, violations_list) = _get_pylint_violations()
+
+    # Print number of violations to log
+    print _lint_output('pylint', count, violations_list, limit=6100)
+
+    # Also write the number of violations to a file
+    with open(dquality_dir / "diff_quality_pylint.html", "w") as f:
+        f.write(_lint_output('pylint', count, violations_list, is_html=True, limit=6100))
+
+    if count > 6100:
+        diff_quality_percentage_pass = False
+
     # ----- Set up for diff-quality pylint call -----
     # Set the string, if needed, to be used for the diff-quality --compare-branch switch.
     compare_branch = getattr(options, 'compare_branch', None)
@@ -747,26 +778,9 @@ def run_quality(options):
     if diff_threshold > -1:
         percentage_string = u'--fail-under={0}'.format(diff_threshold)
 
-    # Generate diff-quality html report for pylint, and print to console
-    # If pylint reports exist, use those
-    # Otherwise, `diff-quality` will call pylint itself
-
-    pylint_files = get_violations_reports("pylint")
-    pylint_reports = u' '.join(pylint_files)
-
     eslint_files = get_violations_reports("eslint")
     eslint_reports = u' '.join(eslint_files)
 
-    # run diff-quality for pylint.
-    if not run_diff_quality(
-            violations_type="pylint",
-            reports=pylint_reports,
-            percentage_string=percentage_string,
-            branch_string=compare_branch_string,
-            dquality_dir=dquality_dir
-    ):
-        diff_quality_percentage_pass = False
-
     # run diff-quality for eslint.
     if not run_diff_quality(
             violations_type="eslint",

scripts/all-tests.sh

@@ -12,7 +12,7 @@ set -e
 
 # Violations thresholds for failing the build
 export LOWER_PYLINT_THRESHOLD=1000
-export UPPER_PYLINT_THRESHOLD=5335
+export UPPER_PYLINT_THRESHOLD=6200
 export ESLINT_THRESHOLD=9134
 export STYLELINT_THRESHOLD=973