From 1f196523f3f410861ce383d817c8d5449656bd88 Mon Sep 17 00:00:00 2001 From: Ben Patterson Date: Tue, 9 Sep 2014 08:15:45 -0400 Subject: [PATCH] Paver: pep8/pylint violations limit switch. Fail if limit is breached. This adds the -l switch, wherein a violations limit is passed in. For example: paver run_pep8 -l 700 Will fail if more than 700 violations are found. By default, this limit is not enforced (i.e. if the -l switch is not passed into the command, then a limit will not be put into effect for that run) --- pavelib/paver_tests/pylint_test_list.json | 6 +++ pavelib/paver_tests/test_paver_quality.py | 45 ++++++++++++++++++++ pavelib/quality.py | 52 ++++++++++++++++++++++- scripts/all-tests.sh | 5 ++- 4 files changed, 104 insertions(+), 4 deletions(-) create mode 100644 pavelib/paver_tests/pylint_test_list.json create mode 100644 pavelib/paver_tests/test_paver_quality.py diff --git a/pavelib/paver_tests/pylint_test_list.json b/pavelib/paver_tests/pylint_test_list.json new file mode 100644 index 0000000000..d0e8b43aa9 --- /dev/null +++ b/pavelib/paver_tests/pylint_test_list.json @@ -0,0 +1,6 @@ +[ + "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 ^" +] diff --git a/pavelib/paver_tests/test_paver_quality.py b/pavelib/paver_tests/test_paver_quality.py new file mode 100644 index 0000000000..086aedfcee --- /dev/null +++ b/pavelib/paver_tests/test_paver_quality.py @@ -0,0 +1,45 @@ +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) diff --git a/pavelib/quality.py b/pavelib/quality.py index 4558db37f3..8780d2c951 100644 --- a/pavelib/quality.py +++ b/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') diff --git a/scripts/all-tests.sh b/scripts/all-tests.sh index 0d6b42f604..3aa5a97c4b 100755 --- a/scripts/all-tests.sh +++ b/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 END + exit $EXIT ;; "unit")