From 3c35fbb56b0dc72de25bdcce840c64f9f7ca00ec Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 10 Mar 2017 15:45:23 -0500 Subject: [PATCH 01/12] Switch to the parseable output-format for pylint --- pavelib/quality.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/pavelib/quality.py b/pavelib/quality.py index 18ce99e890..4e534d7e81 100644 --- a/pavelib/quality.py +++ b/pavelib/quality.py @@ -64,11 +64,10 @@ def find_fixme(options): ) sh( - "{pythonpath_prefix} pylint --disable R,C,W,E --enable=fixme " - "--msg-template={msg_template} {apps} " + "{pythonpath_prefix} pylint --disable all --enable=fixme " + "--output-format=parseable {apps} " "| tee {report_dir}/pylint_fixme.report".format( pythonpath_prefix=pythonpath_prefix, - msg_template='"{path}:{line}: [{msg_id}({symbol}), {obj}] {msg}"', apps=apps_list, report_dir=report_dir ) @@ -117,11 +116,10 @@ def run_pylint(options): ) sh( - "{pythonpath_prefix} pylint {flags} --msg-template={msg_template} {apps} | " + "{pythonpath_prefix} pylint {flags} --output-format=parseable {apps} | " "tee {report_dir}/pylint.report".format( pythonpath_prefix=pythonpath_prefix, flags=" ".join(flags), - msg_template='"{path}:{line}: [{msg_id}({symbol}), {obj}] {msg}"', apps=apps_list, report_dir=report_dir ) From 898dd6be49478cb00844a5f126e10af904ff46bc Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 10 Mar 2017 15:46:49 -0500 Subject: [PATCH 02/12] Use the latest version of edx-lint --- pylintrc | 300 +++++++++++++++++++++++++++++++++++--- requirements/edx/base.txt | 6 +- 2 files changed, 284 insertions(+), 22 deletions(-) diff --git a/pylintrc b/pylintrc index 3658f1f647..5f657dc134 100644 --- a/pylintrc +++ b/pylintrc @@ -2,7 +2,7 @@ # ** DO NOT EDIT THIS FILE ** # *************************** # -# This file was generated by edx-lint: http://github.com/edx.edx-lint +# This file was generated by edx-lint: http://github.com/edx/edx-lint # # If you want to change this file, you have two choices, depending on whether # you want to make a local change that applies only to this repo, or whether @@ -58,29 +58,293 @@ persistent = yes load-plugins = edx_lint.pylint,pylint_django,pylint_celery [MESSAGES CONTROL] +enable = + # These are controlled by explicit choices in the pylintrc files + blacklisted-name, + line-too-long, + # These affect the correctness of the code + syntax-error, + init-is-generator, + return-in-init, + function-redefined, + not-in-loop, + return-outside-function, + yield-outside-function, + return-arg-in-generator, + nonexistent-operator, + duplicate-argument-name, + abstract-class-instantiated, + bad-reversed-sequence, + continue-in-finally, + method-hidden, + access-member-before-definition, + no-method-argument, + no-self-argument, + invalid-slots-object, + assigning-non-slot, + invalid-slots, + inherit-non-class, + inconsistent-mro, + duplicate-bases, + non-iterator-returned, + unexpected-special-method-signature, + invalid-length-returned, + import-error, + used-before-assignment, + undefined-variable, + undefined-all-variable, + invalid-all-object, + no-name-in-module, + unbalance-tuple-unpacking, + unpacking-non-sequence, + bad-except-order, + raising-bad-type, + misplaced-bare-raise, + raising-non-exception, + nonimplemented-raised, + catching-non-exception, + slots-on-old-class, + super-on-old-class, + bad-super-call, + missing-super-argument, + no-member, + not-callable, + assignment-from-no-return, + no-value-for-parameter, + too-many-function-args, + unexpected-keyword-arg, + redundant-keyword-arg, + invalid-sequence-index, + invalid-slice-index, + assignment-from-none, + not-context-manager, + invalid-unary-operand-type, + unsupported-binary-operation, + repeated-keyword, + not-an-iterable, + not-a-mapping, + unsupported-membership-test, + unsubscriptable-object, + logging-unsupported-format, + logging-too-many-args, + logging-too-few-args, + bad-format-character, + truncated-format-string, + mixed-fomat-string, + format-needs-mapping, + missing-format-string-key, + too-many-format-args, + too-few-format-args, + bad-str-strip-call, + model-unicode-not-callable, + super-method-not-called, + non-parent-method-called, + test-inherits-tests, + translation-of-non-string, + redefined-variable-type, + cyclical-import, + unreachable, + dangerous-default-value, + pointless-statement, + pointless-string-statement, + expression-not-assigned, + duplicate-key, + confusing-with-statement, + using-constant-test, + lost-exception, + assert-on-tuple, + attribute-defined-outside-init, + bad-staticmethod-argument, + arguments-differ, + signature-differs, + abstract-method, + super-init-not-called, + relative-import, + import-self, + misplaced-future, + invalid-encoded-data, + global-variable-undefined, + redefined-outer-name, + redefined-builtin, + redefined-in-handler, + undefined-loop-variable, + cell-var-from-loop, + duplicate-except, + nonstandard-exception, + binary-op-exception, + property-on-old-class, + bad-format-string-key, + unused-format-string-key, + bad-format-string, + missing-format-argument-key, + unused-format-string-argument, + format-combined-specification, + missing-format-attribute, + invalid-format-index, + anomalous-backslash-in-string, + anomalous-unicode-escape-in-string, + bad-open-mode, + boolean-datetime, + # Checking failed for some reason + fatal, + astroid-error, + parse-error, + method-check-failed, + django-not-available, + raw-checker-failed, + django-not-available-placeholder, + # Documentation is important + empty-docstring, + invalid-characters-in-docstring, + missing-docstring, + wrong-spelling-in-comment, + wrong-spelling-in-docstring, + # Unused code should be deleted + unused-import, + unused-variable, + unused-argument, + # These are dangerous! + exec-used, + eval-used, + # These represent idiomatic python. Not adhering to them + # will raise red flags with future readers. + bad-classmethod-argument, + bad-mcs-classmethod-argument, + bad-mcs-method-argument, + bad-whitespace, + consider-iterating-dictionary, + consider-using-enumerate, + literal-used-as-attribute, + multiple-imports, + multiple-statements, + old-style-class, + simplifiable-range, + singleton-comparison, + superfluous-parens, + unidiomatic-typecheck, + unneeded-not, + wrong-assert-type, + simplifiable-if-statement, + no-classmethod-decorator, + no-staticmethod-decorator, + unnecessary-pass, + unnecessary-lambda, + useless-else-on-loop, + unnecessary-semicolon, + reimported, + global-variable-not-assigned, + global-at-module-level, + bare-except, + broad-except, + logging-not-lazy, + redundant-unittest-assert, + model-missing-unicode, + model-has-unicode, + model-no-explicit-unicode, + protected-access, + # Don't use things that are deprecated + deprecated-module, + deprecated-method, + # These help manage code complexity + too-many-nested-blocks, + too-many-statements, + too-many-boolean-expressions, + # Consistent import order makes finding where code is + # imported from easier + ungrouped-imports, + wrong-import-order, + wrong-import-position, + wildcard-import, + # These should be auto-fixed by any competent editor + missing-final-newline, + mixed-line-endings, + trailing-newlines, + trailing-whitespace, + unexpected-line-ending-format, + mixed-indentation, + # These attempt to limit pylint line-noise + bad-option-value, + unrecognized-inline-option, + useless-suppression, + bad-inline-option, + deprecated-pragma, disable = + # These should be left to the discretion of the reviewer + bad-continuation, + invalid-name, + misplaced-comparison-constant, + file-ignored, + bad-indentation, + lowercase-l-suffix, + unused-wildcard-import, + global-statement, + no-else-return, + # These are disabled by pylint by default + apply-builtin, + backtick, + basestring-builtin, + buffer-builtin, + cmp-builtin, + cmp-method, + coerce-builtin, + coerce-method, + delslice-method, + dict-iter-method, + dict-view-method, + duplicate-code, + execfile-builtin, + file-builtin, + filter-builtin-not-iterating, + fixme, + getslice-method, + hex-method, + import-star-module-level, + indexing-exception, + input-builtin, + intern-builtin, locally-disabled, locally-enabled, - too-few-public-methods, - bad-builtin, - star-args, - abstract-class-not-used, - abstract-class-little-used, - no-init, - fixme, logging-format-interpolation, - too-many-lines, + long-builtin, + long-suffix, + map-builtin-not-iterating, + metaclass-assignment, + next-method-called, + no-absolute-import, + no-init, no-self-use, - too-many-ancestors, - too-many-instance-attributes, + nonzero-method, + oct-method, + old-division, + old-ne-operator, + old-octal-literal, + old-raise-syntax, + parameter-unpacking, + print-statement, + raising-string, + range-builtin-not-iterating, + raw_input-builtin, + reduce-builtin, + reload-builtin, + round-builtin, + setslice-method, + standarderror-builtin, + suppressed-message, too-few-public-methods, + too-many-ancestors, + too-many-arguments, + too-many-branches, + too-many-instance-attributes, + too-many-lines, + too-many-locals, too-many-public-methods, too-many-return-statements, - too-many-branches, - too-many-arguments, - too-many-locals, - unused-wildcard-import, - duplicate-code + unichr-builtin, + unicode-builtin, + unpacking-in-except, + using-cmp-argument, + xrange-builtin, + zip-builtin-not-iterating, [REPORTS] output-format = text @@ -103,7 +367,7 @@ inlinevar-rgx = [A-Za-z_][A-Za-z0-9_]*$ good-names = f,i,j,k,db,ex,Run,_,__ bad-names = foo,bar,baz,toto,tutu,tata no-docstring-rgx = __.*__$|test_.+|setUp$|setUpClass$|tearDown$|tearDownClass$|Meta$ -docstring-min-length = -1 +docstring-min-length = 5 [FORMAT] max-line-length = 120 @@ -180,4 +444,4 @@ int-import-graph = [EXCEPTIONS] overgeneral-exceptions = Exception -# 5c46ef5d76dd14aadf0311325da7f519a2241646 +# ebf3c66ab89931381f66d2fbf20aa581696156c2 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 59abf69e7a..0e555e2f9e 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -41,10 +41,8 @@ edx-ccx-keys==0.2.1 edx-celeryutils==0.2.6 edx-drf-extensions==1.2.3 edx-i18n-tools==0.3.10 -edx-lint==0.4.3 -# astroid 1.3.8 is an unmet dependency of the version of pylint used in edx-lint 0.4.3 -# when upgrading edx-lint, we should try to remove this from the platform -astroid==1.3.8 +edx-lint==0.5.4 +enum34==1.1.6 edx-django-oauth2-provider==1.2.5 edx-django-sites-extensions==2.3.0 edx-enterprise==0.55.1 From 653cc22580d193c2ee8dc075e50c98b35ed1d002 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 15 Mar 2017 16:29:53 -0400 Subject: [PATCH 03/12] Run pip freeze after installing python prereqs --- pavelib/prereqs.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pavelib/prereqs.py b/pavelib/prereqs.py index 8d123d857f..9bdaacb11a 100644 --- a/pavelib/prereqs.py +++ b/pavelib/prereqs.py @@ -305,6 +305,8 @@ def install_python_prereqs(): prereq_cache("Python prereqs", files_to_fingerprint, python_prereqs_installation) + sh("pip freeze") + @task @timed From 840eb0be4ee45388327045b7949fed507974bdfd Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 16 Mar 2017 10:38:45 -0400 Subject: [PATCH 04/12] Capture paver quality logs into the artifacts directory Capture paver quality build stdout and stderr into separate log files Send pylint results directly to a report file, so that we don't overwhelm jenkins --- pavelib/quality.py | 6 +++--- scripts/generic-ci-tests.sh | 36 ++++++++++++++++++++++++++---------- 2 files changed, 29 insertions(+), 13 deletions(-) diff --git a/pavelib/quality.py b/pavelib/quality.py index 4e534d7e81..77dcca9b34 100644 --- a/pavelib/quality.py +++ b/pavelib/quality.py @@ -66,7 +66,7 @@ def find_fixme(options): sh( "{pythonpath_prefix} pylint --disable all --enable=fixme " "--output-format=parseable {apps} " - "| tee {report_dir}/pylint_fixme.report".format( + "> {report_dir}/pylint_fixme.report".format( pythonpath_prefix=pythonpath_prefix, apps=apps_list, report_dir=report_dir @@ -116,8 +116,8 @@ def run_pylint(options): ) sh( - "{pythonpath_prefix} pylint {flags} --output-format=parseable {apps} | " - "tee {report_dir}/pylint.report".format( + "{pythonpath_prefix} pylint {flags} --output-format=parseable {apps} " + "> {report_dir}/pylint.report".format( pythonpath_prefix=pythonpath_prefix, flags=" ".join(flags), apps=apps_list, diff --git a/scripts/generic-ci-tests.sh b/scripts/generic-ci-tests.sh index d835b0344f..e0b6111702 100755 --- a/scripts/generic-ci-tests.sh +++ b/scripts/generic-ci-tests.sh @@ -76,31 +76,47 @@ else fi PARALLEL="--processes=-1" export SUBSET_JOB=$JOB_NAME + +function run_paver_quality { + QUALITY_TASK=$1 + shift + mkdir -p test_root/log/ + LOG_PREFIX=test_root/log/$QUALITY_TASK + paver $QUALITY_TASK $* 2> $LOG_PREFIX.err.log > $LOG_PREFIX.out.log || { + echo "STDOUT:"; + cat $LOG_PREFIX.out.log; + echo "STDERR:"; + cat $LOG_PREFIX.err.log; + return 1; + } + return 0; +} + case "$TEST_SUITE" in "quality") echo "Finding fixme's and storing report..." - paver find_fixme > fixme.log || { cat fixme.log; EXIT=1; } + run_paver_quality find_fixme || EXIT=1 echo "Finding pep8 violations and storing report..." - paver run_pep8 > pep8.log || { cat pep8.log; EXIT=1; } + run_paver_quality run_pep8 || EXIT=1 echo "Finding pylint violations and storing in report..." - 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; } + run_paver_quality run_pylint -l $LOWER_PYLINT_THRESHOLD:$UPPER_PYLINT_THRESHOLD || EXIT=1 mkdir -p reports echo "Finding ESLint violations and storing report..." - paver run_eslint -l $ESLINT_THRESHOLD > eslint.log || { echo 'Too many eslint violations. You can view them in eslint.log'; EXIT=1; } + run_paver_quality run_eslint -l $ESLINT_THRESHOLD || EXIT=1 echo "Finding Stylelint violations and storing report..." - paver run_stylelint -l $STYLELINT_THRESHOLD > stylelint.log || { echo 'Too many stylelint violations. You can view them in stylelint.log'; EXIT=1; } + run_paver_quality run_stylelint -l $STYLELINT_THRESHOLD || EXIT=1 echo "Running code complexity report (python)." - paver run_complexity || echo "Unable to calculate code complexity. Ignoring error." + run_paver_quality run_complexity || echo "Unable to calculate code complexity. Ignoring error." echo "Running xss linter report." - paver run_xsslint -t $XSSLINT_THRESHOLDS > xsslint.log || { echo 'Too many xsslint violations. You can view them in xsslint.log'; EXIT=1; } - echo "Running xss commit linter report." - paver run_xsscommitlint > xsscommitlint.log || { cat xsscommitlint.log; EXIT=1; } + run_paver_quality run_xsslint -t $XSSLINT_THRESHOLDS || EXIT=1 + echo "Running safe commit linter report." + run_paver_quality run_xsscommitlint || EXIT=1 # Run quality task. Pass in the 'fail-under' percentage to diff-quality echo "Running diff quality." - paver run_quality -p 100 || EXIT=1 + run_paver_quality run_quality -p 100 || EXIT=1 # Need to create an empty test result so the post-build # action doesn't fail the build. From 6d8ce9bcd26abf95cd1e3564fe7a768bb99011c8 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 20 Apr 2017 09:53:14 -0400 Subject: [PATCH 05/12] Move pylint sys.path hackery into pylintrc so that any use of pylint gets it --- pavelib/quality.py | 37 +++++++++---------------------------- pylintrc | 3 ++- pylintrc_tweaks | 1 + 3 files changed, 12 insertions(+), 29 deletions(-) diff --git a/pavelib/quality.py b/pavelib/quality.py index 77dcca9b34..1c5de5cfca 100644 --- a/pavelib/quality.py +++ b/pavelib/quality.py @@ -57,22 +57,17 @@ def find_fixme(options): apps_list = ' '.join(top_python_dirs(system)) - pythonpath_prefix = ( - "PYTHONPATH={system}/djangoapps:common/djangoapps:common/lib".format( - system=system - ) - ) - - sh( - "{pythonpath_prefix} pylint --disable all --enable=fixme " + cmd = ( + "pylint --disable all --enable=fixme " "--output-format=parseable {apps} " "> {report_dir}/pylint_fixme.report".format( - pythonpath_prefix=pythonpath_prefix, apps=apps_list, report_dir=report_dir ) ) + sh(cmd, ignore_error=True) + num_fixme += _count_pylint_violations( "{report_dir}/pylint_fixme.report".format(report_dir=report_dir)) @@ -109,20 +104,14 @@ def run_pylint(options): apps_list = ' '.join(top_python_dirs(system)) - pythonpath_prefix = ( - "PYTHONPATH={system}/djangoapps:common/djangoapps:common/lib".format( - system=system - ) - ) - sh( - "{pythonpath_prefix} pylint {flags} --output-format=parseable {apps} " + "pylint {flags} --output-format=parseable {apps} " "> {report_dir}/pylint.report".format( - pythonpath_prefix=pythonpath_prefix, flags=" ".join(flags), apps=apps_list, report_dir=report_dir - ) + ), + ignore_error=True, ) num_violations += _count_pylint_violations( @@ -768,15 +757,9 @@ def run_quality(options): eslint_files = get_violations_reports("eslint") eslint_reports = u' '.join(eslint_files) - pythonpath_prefix = ( - "PYTHONPATH=$PYTHONPATH:lms:lms/djangoapps:cms:cms/djangoapps:" - "common:common/djangoapps:common/lib" - ) - # run diff-quality for pylint. if not run_diff_quality( violations_type="pylint", - prefix=pythonpath_prefix, reports=pylint_reports, percentage_string=percentage_string, branch_string=compare_branch_string, @@ -787,7 +770,6 @@ def run_quality(options): # run diff-quality for eslint. if not run_diff_quality( violations_type="eslint", - prefix=pythonpath_prefix, reports=eslint_reports, percentage_string=percentage_string, branch_string=compare_branch_string, @@ -801,7 +783,7 @@ def run_quality(options): def run_diff_quality( - violations_type=None, prefix=None, reports=None, percentage_string=None, branch_string=None, dquality_dir=None + violations_type=None, reports=None, percentage_string=None, branch_string=None, dquality_dir=None ): """ This executes the diff-quality commandline tool for the given violation type (e.g., pylint, eslint). @@ -810,11 +792,10 @@ def run_diff_quality( """ try: sh( - "{pythonpath_prefix} diff-quality --violations={type} " + "diff-quality --violations={type} " "{reports} {percentage_string} {compare_branch_string} " "--html-report {dquality_dir}/diff_quality_{type}.html ".format( type=violations_type, - pythonpath_prefix=prefix, reports=reports, percentage_string=percentage_string, compare_branch_string=branch_string, diff --git a/pylintrc b/pylintrc index 5f657dc134..30e67dcb30 100644 --- a/pylintrc +++ b/pylintrc @@ -56,6 +56,7 @@ ignore = ,.git,.tox,migrations,node_modules,.pycharm_helpers persistent = yes load-plugins = edx_lint.pylint,pylint_django,pylint_celery +init-hook = "import sys; sys.path.extend(['lms/djangoapps', 'cms/djangoapps', 'common/djangoapps'])" [MESSAGES CONTROL] enable = @@ -444,4 +445,4 @@ int-import-graph = [EXCEPTIONS] overgeneral-exceptions = Exception -# ebf3c66ab89931381f66d2fbf20aa581696156c2 +# cb770bb6272f6fe1edfd74aa1fb912be5541481c diff --git a/pylintrc_tweaks b/pylintrc_tweaks index 6d479b623d..1fdd6e1b25 100644 --- a/pylintrc_tweaks +++ b/pylintrc_tweaks @@ -1,6 +1,7 @@ # pylintrc tweaks for use with edx_lint. [MASTER] ignore+ = ,.git,.tox,migrations,node_modules,.pycharm_helpers +init-hook="import sys; sys.path.extend(['lms/djangoapps', 'cms/djangoapps', 'common/djangoapps'])" [BASIC] attr-rgx = [a-z_][a-z0-9_]{2,40}$ From 39128d3e4ef7880d2493bd98508f3ff7733e29d7 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 20 Apr 2017 12:50:23 -0400 Subject: [PATCH 06/12] Add missing requirements needed to run paver --- requirements/edx/paver.txt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/requirements/edx/paver.txt b/requirements/edx/paver.txt index cebbb5f375..31b82fbf85 100644 --- a/requirements/edx/paver.txt +++ b/requirements/edx/paver.txt @@ -1,3 +1,8 @@ # Requirements to run and test Paver Paver==1.2.4 libsass==0.10.0 +wrapt==1.10.5 +markupsafe +edx-opaque-keys +requests +mock \ No newline at end of file From d92e6ae34ba1b6562c8d4f35f78d57d7db21b256 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 21 Apr 2017 14:16:36 -0400 Subject: [PATCH 07/12] Fix pylint-django --- requirements/edx/base.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 0e555e2f9e..72ded10ec2 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -42,6 +42,10 @@ edx-celeryutils==0.2.6 edx-drf-extensions==1.2.3 edx-i18n-tools==0.3.10 edx-lint==0.5.4 + +# Pin this to test a fix to pylint-django +git+https://github.com/cpennington/pylint-django@fix-field-inference-during-monkey-patching#egg=pylint-django==0.0 + enum34==1.1.6 edx-django-oauth2-provider==1.2.5 edx-django-sites-extensions==2.3.0 From 8c35e3d82724b4c78fcdd4bc7c87226d87cea595 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 3 May 2017 13:54:28 -0400 Subject: [PATCH 08/12] Run pylint like pep8, rather than through diff-lint --- pavelib/paver_tests/test_paver_quality.py | 15 +-- pavelib/quality.py | 112 ++++++++++++---------- scripts/all-tests.sh | 2 +- 3 files changed, 67 insertions(+), 62 deletions(-) diff --git a/pavelib/paver_tests/test_paver_quality.py b/pavelib/paver_tests/test_paver_quality.py index 8e35861ac5..b9bfccd25c 100644 --- a/pavelib/paver_tests/test_paver_quality.py +++ b/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("") diff --git a/pavelib/quality.py b/pavelib/quality.py index 1c5de5cfca..e8b7bc64c8 100644 --- a/pavelib/quality.py +++ b/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 or :"), -]) -@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 or :"), +]) +@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 = ['\n'] sep = '-------------
\n' - title = "

Quality Report: pep8

\n" + title = HTML("

Quality Report: {}

\n").format(linter) violations_bullets = ''.join( [HTML('
  • {violation}

    • \n').format(violation=violation) for violation in violations_list] ) violations_str = HTML('
      \n{bullets}
    \n').format(bullets=HTML(violations_bullets)) - violations_count_str = "Violations: {count}
    \n" - fail_line = "FAILURE: pep8 count should be 0
    \n" + violations_count_str = HTML("Violations: {count}
    \n") + fail_line = HTML("FAILURE: {} count should be 0
    \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", diff --git a/scripts/all-tests.sh b/scripts/all-tests.sh index 8de5bdf801..88c0b7df2a 100755 --- a/scripts/all-tests.sh +++ b/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 From 7376b1142ad9d89c464f4abe521bb99704ceec12 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 3 May 2017 14:26:44 -0400 Subject: [PATCH 09/12] Don't re-run pylint or pep8 when generating diffquality reports --- pavelib/quality.py | 44 +++++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/pavelib/quality.py b/pavelib/quality.py index e8b7bc64c8..58c00b3811 100644 --- a/pavelib/quality.py +++ b/pavelib/quality.py @@ -74,7 +74,7 @@ def find_fixme(options): print "Number of pylint fixmes: " + str(num_fixme) -def _get_pylint_violations(systems=ALL_SYSTEMS.split(','), errors_only=False): +def _get_pylint_violations(systems=ALL_SYSTEMS.split(','), errors_only=False, clean=True): """ 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 @@ -97,19 +97,20 @@ def _get_pylint_violations(systems=ALL_SYSTEMS.split(','), errors_only=False): apps_list = ' '.join(top_python_dirs(system)) - sh( - "pylint {flags} --output-format=parseable {apps} " - "> {report_dir}/pylint.report".format( - flags=" ".join(flags), - apps=apps_list, - report_dir=report_dir - ), - ignore_error=True, - ) + system_report = report_dir / 'pylint.report' + if clean or not system_report.exists(): + sh( + "pylint {flags} --output-format=parseable {apps} " + "> {report_dir}/pylint.report".format( + flags=" ".join(flags), + apps=apps_list, + report_dir=report_dir + ), + ignore_error=True, + ) - report = "{report_dir}/pylint.report".format(report_dir=report_dir) - num_violations += _count_pylint_violations(report) - with open(report) as report_contents: + num_violations += _count_pylint_violations(system_report) + with open(system_report) as report_contents: violations_list.extend(report_contents) # Print number of violations to log @@ -203,24 +204,25 @@ def _count_pylint_violations(report_file): return num_violations_report -def _get_pep8_violations(): +def _get_pep8_violations(clean=True): """ Runs pep8. Returns a tuple of (number_of_violations, violations_string) where violations_string is a string of all pep8 violations found, separated by new lines. """ report_dir = (Env.REPORT_DIR / 'pep8') - report_dir.rmtree(ignore_errors=True) + if clean: + report_dir.rmtree(ignore_errors=True) report_dir.makedirs_p() + report = report_dir / 'pep8.report' # Make sure the metrics subdirectory exists Env.METRICS_DIR.makedirs_p() - sh('pep8 . | tee {report_dir}/pep8.report -a'.format(report_dir=report_dir)) + if not report.exists(): + sh('pep8 . | tee {} -a'.format(report)) - violations_list = _pep8_violations( - "{report_dir}/pep8.report".format(report_dir=report_dir) - ) + violations_list = _pep8_violations(report) return (len(violations_list), violations_list) @@ -737,7 +739,7 @@ def run_quality(options): return ''.join(lines) # Run pep8 directly since we have 0 violations on master - (count, violations_list) = _get_pep8_violations() + (count, violations_list) = _get_pep8_violations(clean=False) # Print number of violations to log print _lint_output('pep8', count, violations_list) @@ -753,7 +755,7 @@ def run_quality(options): # If pylint reports exist, use those # Otherwise, `diff-quality` will call pylint itself - (count, violations_list) = _get_pylint_violations() + (count, violations_list) = _get_pylint_violations(clean=False) # Print number of violations to log print _lint_output('pylint', count, violations_list, limit=6100) From 83211fa87d9522c520a68cf6394fb134066973db Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 4 May 2017 13:51:11 -0400 Subject: [PATCH 10/12] Only dump last 100 lines of stdout and stderr to the jenkins console --- scripts/generic-ci-tests.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/generic-ci-tests.sh b/scripts/generic-ci-tests.sh index e0b6111702..dd310feecc 100755 --- a/scripts/generic-ci-tests.sh +++ b/scripts/generic-ci-tests.sh @@ -83,10 +83,10 @@ function run_paver_quality { mkdir -p test_root/log/ LOG_PREFIX=test_root/log/$QUALITY_TASK paver $QUALITY_TASK $* 2> $LOG_PREFIX.err.log > $LOG_PREFIX.out.log || { - echo "STDOUT:"; - cat $LOG_PREFIX.out.log; - echo "STDERR:"; - cat $LOG_PREFIX.err.log; + echo "STDOUT (last 100 lines of $LOG_PREFIX.out.log):"; + tail -n 100 $LOG_PREFIX.out.log; + echo "STDERR (last 100 lines of $LOG_PREFIX.err.log):"; + tail -n 100 $LOG_PREFIX.err.log; return 1; } return 0; From 97f12ff2969ba4ff21b37dae6c0137e897798939 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 10 May 2017 12:30:35 -0400 Subject: [PATCH 11/12] Fix tests that validate how the linting code runs --- pavelib/paver_tests/test_paver_quality.py | 51 +++++++++++++---------- pavelib/paver_tests/utils.py | 16 +++---- 2 files changed, 38 insertions(+), 29 deletions(-) diff --git a/pavelib/paver_tests/test_paver_quality.py b/pavelib/paver_tests/test_paver_quality.py index b9bfccd25c..d02fa3bdd1 100644 --- a/pavelib/paver_tests/test_paver_quality.py +++ b/pavelib/paver_tests/test_paver_quality.py @@ -2,6 +2,7 @@ Tests for paver quality tasks """ import os +import shutil import tempfile import textwrap import unittest @@ -293,6 +294,12 @@ class TestPaverRunQuality(unittest.TestCase): self._mock_paver_sh = patcher.start() self.addCleanup(patcher.stop) + self.report_dir = tempfile.mkdtemp() + report_dir_patcher = patch('pavelib.utils.envs.Env.REPORT_DIR', path(self.report_dir)) + report_dir_patcher.start() + self.addCleanup(shutil.rmtree, self.report_dir) + self.addCleanup(report_dir_patcher.stop) + @patch('__builtin__.open', mock_open()) def test_failure_on_diffquality_pep8(self): """ @@ -307,9 +314,9 @@ class TestPaverRunQuality(unittest.TestCase): pavelib.quality.run_quality("") # Test that pep8, pylint and eslint were called by counting the calls to - # _get_pep8_violations (for pep8) and sh (for diff-quality pylint & eslint) + # _get_pep8_violations (for pep8) and sh (5 for pylint & 1 for eslint) self.assertEqual(_mock_pep8_violations.call_count, 1) - self.assertEqual(self._mock_paver_sh.call_count, 2) + self.assertEqual(self._mock_paver_sh.call_count, 6) @patch('__builtin__.open', mock_open()) def test_failure_on_diffquality_pylint(self): @@ -320,14 +327,15 @@ class TestPaverRunQuality(unittest.TestCase): # Underlying sh call must fail when it is running the pylint diff-quality task _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("") + with patch('pavelib.quality._parse_pylint_options', return_value=(0, 1000, 0, 0)): + with self.assertRaises(SystemExit): + pavelib.quality.run_quality("") # Test that both pep8 and pylint were called by counting the calls - # Assert that _get_pep8_violations (which calls "pep8") is called once - self.assertEqual(_mock_pep8_violations.call_count, 1) - # And assert that sh was called twice (for the calls to pylint & eslint). - # This means that even in the event of a diff-quality pylint failure, eslint is still called. + # Assert that _get_pylint_violations (which calls "pylint") is called once + self.assertEqual(_mock_pylint_violations.call_count, 1) + # And assert that sh was called 6 times (1 for pep8 & 1 for eslint). + # This means that even in the event of a diff-quality pylint failure, eslint and pep8 are still called. self.assertEqual(self._mock_paver_sh.call_count, 2) @patch('__builtin__.open', mock_open()) @@ -339,15 +347,20 @@ class TestPaverRunQuality(unittest.TestCase): # Underlying sh call must fail when it is running the eslint diff-quality task self._mock_paver_sh.side_effect = fail_on_eslint _mock_pep8_violations = MagicMock(return_value=(0, [])) + _mock_pylint_violations = MagicMock(return_value=(0, [])) with patch('pavelib.quality._get_pep8_violations', _mock_pep8_violations): - with self.assertRaises(SystemExit): - pavelib.quality.run_quality("") - self.assertRaises(BuildFailure) + with patch('pavelib.quality._get_pylint_violations', _mock_pylint_violations): + with self.assertRaises(SystemExit): + pavelib.quality.run_quality("") + self.assertRaises(BuildFailure) + print self._mock_paver_sh.mock_calls + # Test that both pep8 and pylint were called by counting the calls # Assert that _get_pep8_violations (which calls "pep8") is called once - self.assertEqual(_mock_pep8_violations.call_count, 1) - # And assert that sh was called twice (for the calls to pep8 and pylint) - self.assertEqual(self._mock_paver_sh.call_count, 2) + _mock_pep8_violations.assert_called_once_with(clean=False) + _mock_pylint_violations.assert_called_once_with(clean=False) + # And assert that sh was called once (the call to eslint) + self.assertEqual(self._mock_paver_sh.call_count, 1) @patch('__builtin__.open', mock_open()) def test_other_exception(self): @@ -365,10 +378,6 @@ class TestPaverRunQuality(unittest.TestCase): @patch('__builtin__.open', mock_open()) def test_no_diff_quality_failures(self): # Assert nothing is raised - _mock_pep8_violations = MagicMock(return_value=(0, [])) - with patch('pavelib.quality._get_pep8_violations', _mock_pep8_violations): - pavelib.quality.run_quality("") - # Assert that _get_pep8_violations (which calls "pep8") is called once - self.assertEqual(_mock_pep8_violations.call_count, 1) - # And assert that sh was called twice (for the call to "pylint" & "eslint") - self.assertEqual(self._mock_paver_sh.call_count, 2) + pavelib.quality.run_quality("") + # And assert that sh was called 7 times (1 for pep8, 5 for pylint, and 1 for eslint) + self.assertEqual(self._mock_paver_sh.call_count, 7) diff --git a/pavelib/paver_tests/utils.py b/pavelib/paver_tests/utils.py index 36819c23a1..cce3af0404 100644 --- a/pavelib/paver_tests/utils.py +++ b/pavelib/paver_tests/utils.py @@ -63,47 +63,47 @@ class MockEnvironment(tasks.Environment): self.messages.append(unicode(output)) -def fail_on_eslint(arg): +def fail_on_eslint(*args, **kwargs): """ For our tests, we need the call for diff-quality running pep8 reports to fail, since that is what is going to fail when we pass in a percentage ("p") requirement. """ - if "eslint" in arg: + if "eslint" in args[0]: # Essentially mock diff-quality exiting with 1 paver.easy.sh("exit 1") else: return -def fail_on_pylint(arg): +def fail_on_pylint(*args, **kwargs): """ For our tests, we need the call for diff-quality running pep8 reports to fail, since that is what is going to fail when we pass in a percentage ("p") requirement. """ - if "pylint" in arg: + if "pylint" in args[0]: # Essentially mock diff-quality exiting with 1 paver.easy.sh("exit 1") else: return -def fail_on_npm_install(arg): +def fail_on_npm_install(*args, **kwargs): """ For our tests, we need the call for diff-quality running pep8 reports to fail, since that is what is going to fail when we pass in a percentage ("p") requirement. """ - if "npm install" in arg: + if "npm install" in args[0]: raise BuildFailure('Subprocess return code: 1') else: return -def unexpected_fail_on_npm_install(arg): +def unexpected_fail_on_npm_install(*args, **kwargs): """ For our tests, we need the call for diff-quality running pep8 reports to fail, since that is what is going to fail when we pass in a percentage ("p") requirement. """ - if "npm install" in arg: + if "npm install" in args[0]: raise BuildFailure('Subprocess return code: 50') else: return From 5ed3e5edba9161d19d234dbc7a1bf63de878dcbf Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 22 Nov 2017 15:07:28 -0500 Subject: [PATCH 12/12] Make run_quality use the same upper pylint limit as run_pylint --- pavelib/quality.py | 11 +++++++---- scripts/generic-ci-tests.sh | 2 +- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/pavelib/quality.py b/pavelib/quality.py index 58c00b3811..1f03e17374 100644 --- a/pavelib/quality.py +++ b/pavelib/quality.py @@ -685,6 +685,7 @@ def _get_xsscommitlint_count(filename): @cmdopts([ ("compare-branch=", "b", "Branch to compare against, defaults to origin/master"), ("percentage=", "p", "fail if diff-quality is below this percentage"), + ("limit=", "l", "Limits for number of acceptable violations - either or :"), ]) @timed def run_quality(options): @@ -730,7 +731,7 @@ def run_quality(options): lines.extend([sep, title, sep, violations_str, sep, violations_count_str]) - if count > limit: + if count > limit > -1: lines.append(fail_line) lines.append(sep + '\n') if is_html: @@ -757,14 +758,16 @@ def run_quality(options): (count, violations_list) = _get_pylint_violations(clean=False) + _, upper_violations_limit, _, _ = _parse_pylint_options(options) + # Print number of violations to log - print _lint_output('pylint', count, violations_list, limit=6100) + print _lint_output('pylint', count, violations_list, limit=upper_violations_limit) # 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)) + f.write(_lint_output('pylint', count, violations_list, is_html=True, limit=upper_violations_limit)) - if count > 6100: + if count > upper_violations_limit > -1: diff_quality_percentage_pass = False # ----- Set up for diff-quality pylint call ----- diff --git a/scripts/generic-ci-tests.sh b/scripts/generic-ci-tests.sh index dd310feecc..9d0a568c85 100755 --- a/scripts/generic-ci-tests.sh +++ b/scripts/generic-ci-tests.sh @@ -116,7 +116,7 @@ case "$TEST_SUITE" in run_paver_quality run_xsscommitlint || EXIT=1 # Run quality task. Pass in the 'fail-under' percentage to diff-quality echo "Running diff quality." - run_paver_quality run_quality -p 100 || EXIT=1 + run_paver_quality run_quality -p 100 -l $LOWER_PYLINT_THRESHOLD:$UPPER_PYLINT_THRESHOLD || EXIT=1 # Need to create an empty test result so the post-build # action doesn't fail the build.