Fix the systems parsing code for pylint.

Add lower pylint error threshold to guard against unsuccessful runs.
Increase upper pylint threshold. Add tests for pylint option parsing.

pavelib/paver_tests/test_paver_quality.py

@@ -8,7 +8,7 @@ import unittest
 
 import paver.easy
 import paver.tasks
-from ddt import ddt, file_data
+from ddt import ddt, file_data, data, unpack
 from mock import MagicMock, mock_open, patch
 from path import Path as path
 from paver.easy import BuildFailure
@@ -60,6 +60,34 @@ class TestPaverQualityViolations(unittest.TestCase):
         self.assertEqual(num, 2)
 
 
+@ddt
+class TestPaverQualityOptions(unittest.TestCase):
+    """
+    Tests the paver pylint command-line options parsing.
+    """
+    @data(
+        ({'limit': '5500'}, (-1, 5500, False, pavelib.quality.ALL_SYSTEMS.split(','))),
+        ({'limit': '1000:5500'}, (1000, 5500, False, pavelib.quality.ALL_SYSTEMS.split(','))),
+        ({'limit': '1:2:3:4:5'}, (1, 2, False, pavelib.quality.ALL_SYSTEMS.split(','))),
+        ({'system': 'lms,cms'}, (-1, -1, False, ['lms', 'cms'])),
+        (
+            {'limit': '2000:5000', 'errors': True, 'system': 'lms,cms,openedx'},
+            (2000, 5000, True, ['lms', 'cms', 'openedx'])
+        ),
+    )
+    @unpack
+    def test_pylint_parser_other_string(self, options, expected_values):
+        class PaverOptions(object):
+            """
+            Simple options class to mimick paver's Namespace object.
+            """
+            def __init__(self, d):
+                self.__dict__ = d
+        paver_options = PaverOptions(options)
+        returned_values = pavelib.quality._parse_pylint_options(paver_options)  # pylint: disable=protected-access
+        self.assertEqual(returned_values, expected_values)
+
+
 class TestPaverReportViolationsCounts(unittest.TestCase):
     """
     For testing utility functions for getting counts from reports for

pavelib/quality.py

@@ -6,7 +6,6 @@ Check code quality using pep8, pylint, and diff_quality.
 import json
 import os
 import re
-from string import join
 
 from paver.easy import BuildFailure, call_task, cmdopts, needs, sh, task
 
@@ -15,13 +14,7 @@ from openedx.core.djangolib.markup import HTML
 from .utils.envs import Env
 from .utils.timer import timed
 
-ALL_SYSTEMS = [
-    'cms',
-    'common',
-    'lms',
-    'openedx',
-    'pavelib',
-]
+ALL_SYSTEMS = 'lms,cms,common,openedx,pavelib'
 
 
 def top_python_dirs(dirname):
@@ -55,7 +48,7 @@ def find_fixme(options):
     Run pylint on system code, only looking for fixme items.
     """
     num_fixme = 0
-    systems = getattr(options, 'system', '').split(',') or ALL_SYSTEMS
+    systems = getattr(options, 'system', ALL_SYSTEMS).split(',')
 
     for system in systems:
         # Directory to put the pylint report in.
@@ -92,7 +85,7 @@ def find_fixme(options):
 @cmdopts([
     ("system=", "s", "System to act on"),
     ("errors", "e", "Check for errors only"),
-    ("limit=", "l", "limit for number of acceptable violations"),
+    ("limit=", "l", "Limits for number of acceptable violations - either <upper> or <lower>:<upper>"),
 ])
 @timed
 def run_pylint(options):
@@ -100,14 +93,12 @@ def run_pylint(options):
     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', '').split(',') or ALL_SYSTEMS
+    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
     for system in systems:
         # Directory to put the pylint report in.
         # This makes the folder if it doesn't already exist.
@@ -147,10 +138,45 @@ def run_pylint(options):
     with open(Env.METRICS_DIR / "pylint", "w") as f:
         f.write(violations_count_str)
 
-    # Fail number of violations is greater than the limit
-    if num_violations > violations_limit > -1:
-        raise BuildFailure("Failed. Too many pylint violations. "
-                           "The limit is {violations_limit}.".format(violations_limit=violations_limit))
+    # Fail when number of violations is less than the lower limit,
+    # which likely means that pylint did not run successfully.
+    # If pylint *did* run successfully, then great! Modify the lower limit.
+    if num_violations < lower_violations_limit > -1:
+        raise BuildFailure(
+            "Failed. Too few pylint violations. "
+            "Expected to see at least {lower_limit} pylint violations. "
+            "Either pylint is not running correctly -or- "
+            "the limits should be lowered and/or the lower limit should be removed.".format(
+                lower_limit=lower_violations_limit
+            )
+        )
+
+    # Fail when number of violations is greater than the upper limit.
+    if num_violations > upper_violations_limit > -1:
+        raise BuildFailure(
+            "Failed. Too many pylint violations. "
+            "The limit is {upper_limit}.".format(upper_limit=upper_violations_limit)
+        )
+
+
+def _parse_pylint_options(options):
+    """
+    Parse the options passed to run_pylint.
+    """
+    lower_violations_limit = upper_violations_limit = -1
+    violations_limit = getattr(options, 'limit', '').split(':')
+    if violations_limit[0]:
+        # Limit was specified.
+        if len(violations_limit) == 1:
+            # Only upper limit was specified.
+            upper_violations_limit = int(violations_limit[0])
+        else:
+            # Upper and lower limits were both specified.
+            lower_violations_limit = int(violations_limit[0])
+            upper_violations_limit = int(violations_limit[1])
+    errors = getattr(options, 'errors', False)
+    systems = getattr(options, 'system', ALL_SYSTEMS).split(',')
+    return lower_violations_limit, upper_violations_limit, errors, systems
 
 
 def _count_pylint_violations(report_file):
@@ -244,7 +270,7 @@ def run_complexity():
     Uses radon to examine cyclomatic complexity.
     For additional details on radon, see http://radon.readthedocs.org/
     """
-    system_string = join(ALL_SYSTEMS, '/ ') + '/'
+    system_string = '/ '.join(ALL_SYSTEMS.split(',')) + '/'
     complexity_report_dir = (Env.REPORT_DIR / "complexity")
     complexity_report = complexity_report_dir / "python_complexity.log"
 

scripts/all-tests.sh

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

scripts/circle-ci-tests.sh

@@ -57,7 +57,7 @@ else
             echo "Finding pylint violations and storing in report..."
             # HACK: we need to print something to the console, otherwise circleci
             # fails and aborts the job because nothing is displayed for > 10 minutes.
-            paver run_pylint -l $PYLINT_THRESHOLD | tee pylint.log || EXIT=1
+            paver run_pylint -l $LOWER_PYLINT_THRESHOLD:$UPPER_PYLINT_THRESHOLD | tee pylint.log || EXIT=1
 
             mkdir -p reports
             PATH=$PATH:node_modules/.bin

scripts/generic-ci-tests.sh

@@ -84,7 +84,7 @@ case "$TEST_SUITE" in
         echo "Finding pep8 violations and storing report..."
         paver run_pep8 > pep8.log || { cat pep8.log; EXIT=1; }
         echo "Finding pylint violations and storing in report..."
-        paver run_pylint -l $PYLINT_THRESHOLD > pylint.log || { echo 'Too many pylint violations. You can view them in pylint.log'; EXIT=1; }
+        paver run_pylint -l $LOWER_PYLINT_THRESHOLD:$UPPER_PYLINT_THRESHOLD > pylint.log || { echo 'Too many pylint violations. You can view them in pylint.log'; EXIT=1; }
 
         mkdir -p reports