Merge pull request #5139 from edx/benp/enforce-pep8-pylint-threshold

Paver: fail if pylint/pep8 violations threshold is breached.

pavelib/paver_tests/pylint_test_list.json

+[
+    "foo/bar.py:192: [C0111(missing-docstring), Bliptv] Missing docstring",
+    "foo/bar/test.py:74: [C0322(no-space-before-operator)] Operator not preceded by a space",
+    "ugly/string/test.py:16: [C0103(invalid-name)] Invalid name \"whats up\" for type constant (should match (([A-Z_][A-Z0-9_]*)|(__.*__)|log|urlpatterns)$)",
+    "multiple/lines/test.py:72: [C0322(no-space-before-operator)] Operator not preceded by a space\nFOO_BAR='pipeline.storage.NonPackagingPipelineStorage'\n                   ^"
+]

pavelib/paver_tests/test_paver_quality.py

+import unittest
+import pavelib.quality
+import tempfile
+import os
+from ddt import ddt, file_data
+
+
+@ddt
+class TestPaverQualityViolations(unittest.TestCase):
+
+    def setUp(self):
+        self.f = tempfile.NamedTemporaryFile(delete=False)
+        self.f.close()
+
+    def test_pylint_parser_other_string(self):
+        with open(self.f.name, 'w') as f:
+            f.write("hello")
+        num = pavelib.quality._count_pylint_violations(f.name)
+        self.assertEqual(num, 0)
+
+    def test_pylint_parser_pep8(self):
+        # Pep8 violations should be ignored.
+        with open(self.f.name, 'w') as f:
+            f.write("foo/hello/test.py:304:15: E203 whitespace before ':'")
+        num = pavelib.quality._count_pylint_violations(f.name)
+        self.assertEqual(num, 0)
+
+    @file_data('pylint_test_list.json')
+    def test_pylint_parser_count_violations(self, value):
+    # Tests:
+    #     * Different types of violations
+    #     * One violation covering multiple lines
+        with open(self.f.name, 'w') as f:
+            f.write(value)
+        num = pavelib.quality._count_pylint_violations(f.name)
+        self.assertEqual(num, 1)
+
+    def test_pep8_parser(self):
+        with open(self.f.name, 'w') as f:
+            f.write("hello\nhithere")
+        num = pavelib.quality._count_pep8_violations(f.name)
+        self.assertEqual(num, 2)
+
+    def tearDown(self):
+        os.remove(self.f.name)

pavelib/quality.py

@@ -4,18 +4,26 @@ Check code quality using pep8, pylint, and diff_quality.
 from paver.easy import sh, task, cmdopts, needs
 import os
 import errno
+import re
+from optparse import make_option
+
 from .utils.envs import Env
 
+
 @task
 @needs('pavelib.prereqs.install_python_prereqs')
 @cmdopts([
     ("system=", "s", "System to act on"),
     ("errors", "e", "Check for errors only"),
+    ("limit=", "l", "limit for number of acceptable violations"),
 ])
 def run_pylint(options):
     """
-    Run pylint on system code
+    Run pylint on system code. When violations limit is passed in,
+    fail the task if too many violations are found.
     """
+    num_violations = 0
+    violations_limit = int(getattr(options, 'limit', -1))
     errors = getattr(options, 'errors', False)
     systems = getattr(options, 'system', 'lms,cms,common').split(',')
 
@@ -51,17 +59,46 @@ def run_pylint(options):
             )
         )
 
+        num_violations += _count_pylint_violations(
+            "{report_dir}/pylint.report".format(report_dir=report_dir))
+
+    print("Number of pylint violations: " + str(num_violations))
+    if num_violations > violations_limit > -1:
+        raise Exception("Failed. Too many pylint violations. "
+                        "The limit is {violations_limit}.".format(violations_limit=violations_limit))
+
+def _count_pylint_violations(report_file):
+    """
+    Parses a pylint report line-by-line and determines the number of violations reported
+    """
+    num_violations_report = 0
+    # An example string:
+    # common/lib/xmodule/xmodule/tests/test_conditional.py:21: [C0111(missing-docstring), DummySystem] Missing docstring
+    # More examples can be found in the unit tests for this method
+    pylint_pattern = re.compile(".(\d+):\ \[(\D\d+.+\]).")
+
+    for line in open(report_file):
+        violation_list_for_line = pylint_pattern.split(line)
+        # If the string is parsed into four parts, then we've found a violation. Example of split parts:
+        # test file, line number, violation name, violation details
+        if len(violation_list_for_line) == 4:
+            num_violations_report += 1
+    return num_violations_report
 
 @task
 @needs('pavelib.prereqs.install_python_prereqs')
 @cmdopts([
     ("system=", "s", "System to act on"),
+    ("limit=", "l", "limit for number of acceptable violations"),
 ])
 def run_pep8(options):
     """
-    Run pep8 on system code
+    Run pep8 on system code. When violations limit is passed in,
+    fail the task if too many violations are found.
     """
+    num_violations = 0
     systems = getattr(options, 'system', 'lms,cms,common').split(',')
+    violations_limit = int(getattr(options, 'limit', -1))
 
     for system in systems:
         # Directory to put the pep8 report in.
@@ -69,7 +106,18 @@ def run_pep8(options):
         report_dir = (Env.REPORT_DIR / system).makedirs_p()
 
         sh('pep8 {system} | tee {report_dir}/pep8.report'.format(system=system, report_dir=report_dir))
-
+        num_violations = num_violations + _count_pep8_violations(
+            "{report_dir}/pep8.report".format(report_dir=report_dir))
+
+    print("Number of pep8 violations: " + str(num_violations))
+    # Fail the task if the violations limit has been reached
+    if num_violations > violations_limit > -1:
+        raise Exception("Failed. Too many pep8 violations. "
+                        "The limit is {violations_limit}.".format(violations_limit=violations_limit))
+
+def _count_pep8_violations(report_file):
+    num_lines = sum(1 for line in open(report_file))
+    return num_lines
 
 @task
 @needs('pavelib.prereqs.install_python_prereqs')

scripts/all-tests.sh

@@ -103,8 +103,8 @@ SHARD=${SHARD:="all"}
 case "$TEST_SUITE" in
 
     "quality")
-        paver run_pep8 > pep8.log || { cat pep8.log ; exit 1; }
-        paver run_pylint > pylint.log || { cat pylint.log; exit 1; }
+        paver run_pep8 -l 725 > pep8.log || { cat pep8.log; EXIT=1; }
+        paver run_pylint -l 4800 > pylint.log || { cat pylint.log; EXIT=1; }
         paver run_quality
 
         # Need to create an empty test result so the post-build
@@ -116,6 +116,7 @@ case "$TEST_SUITE" in
 <testcase classname="quality" name="quality" time="0.604"></testcase>
 </testsuite>
 END
+        exit $EXIT
         ;;
 
     "unit")