From c78ccf8fc1e506b87dffdb47f37937bfc2d4e13a Mon Sep 17 00:00:00 2001 From: John Eskew Date: Wed, 15 Nov 2017 20:36:10 -0500 Subject: [PATCH] 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 | 30 ++++++++++- pavelib/quality.py | 64 ++++++++++++++++------- scripts/all-tests.sh | 3 +- scripts/circle-ci-tests.sh | 2 +- scripts/generic-ci-tests.sh | 2 +- 5 files changed, 78 insertions(+), 23 deletions(-) diff --git a/pavelib/paver_tests/test_paver_quality.py b/pavelib/paver_tests/test_paver_quality.py index 2ced4b8cfe..8e35861ac5 100644 --- a/pavelib/paver_tests/test_paver_quality.py +++ b/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 diff --git a/pavelib/quality.py b/pavelib/quality.py index 7a954831fc..f533221518 100644 --- a/pavelib/quality.py +++ b/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 or :"), ]) @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" diff --git a/scripts/all-tests.sh b/scripts/all-tests.sh index b435d97a04..8de5bdf801 100755 --- a/scripts/all-tests.sh +++ b/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 diff --git a/scripts/circle-ci-tests.sh b/scripts/circle-ci-tests.sh index bcc201ff63..391ad2b981 100755 --- a/scripts/circle-ci-tests.sh +++ b/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 diff --git a/scripts/generic-ci-tests.sh b/scripts/generic-ci-tests.sh index abda8cec80..d835b0344f 100755 --- a/scripts/generic-ci-tests.sh +++ b/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