diff --git a/pavelib/paver_tests/test_paver_quality.py b/pavelib/paver_tests/test_paver_quality.py index 8e35861ac5..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 @@ -56,7 +57,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 +98,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 +290,15 @@ 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) + + 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): @@ -315,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): @@ -326,17 +325,17 @@ 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): - with self.assertRaises(SystemExit): - pavelib.quality.run_quality("") + _mock_pylint_violations = MagicMock(return_value=(10000, ['some error'])) + with patch('pavelib.quality._get_pylint_violations', _mock_pylint_violations): + 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()) @@ -348,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): @@ -374,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 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 diff --git a/pavelib/quality.py b/pavelib/quality.py index c1e5f98ef6..ef0a4ed1b8 100644 --- a/pavelib/quality.py +++ b/pavelib/quality.py @@ -57,29 +57,66 @@ 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 R,C,W,E --enable=fixme " - "--msg-template={msg_template} {apps} " - "| tee {report_dir}/pylint_fixme.report".format( - pythonpath_prefix=pythonpath_prefix, - msg_template='"{path}:{line}: [{msg_id}({symbol}), {obj}] {msg}"', + cmd = ( + "pylint --disable all --enable=fixme " + "--output-format=parseable {apps} " + "> {report_dir}/pylint_fixme.report".format( 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)) print "Number of pylint fixmes: " + str(num_fixme) +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 + by new lines. + """ + # 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_only: + flags.append("--errors-only") + + apps_list = ' '.join(top_python_dirs(system)) + + 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, + ) + + 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 + return num_violations, violations_list + + @task @needs('pavelib.prereqs.install_python_prereqs') @cmdopts([ @@ -94,41 +131,10 @@ def run_pylint(options): 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(',') - # 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. - report_dir = (Env.REPORT_DIR / system).makedirs_p() - - flags = [] - if errors: - flags.append("--errors-only") - - apps_list = ' '.join(top_python_dirs(system)) - - pythonpath_prefix = ( - "PYTHONPATH={system}/djangoapps:common/djangoapps:common/lib".format( - system=system - ) - ) - - sh( - "{pythonpath_prefix} pylint {flags} --msg-template={msg_template} {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 - ) - ) - - num_violations += _count_pylint_violations( - "{report_dir}/pylint.report".format(report_dir=report_dir)) + 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) @@ -198,37 +204,35 @@ 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)) - count, violations_list = _pep8_violations( - "{report_dir}/pep8.report".format(report_dir=report_dir) - ) + violations_list = _pep8_violations(report) - 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 @@ -680,6 +684,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): @@ -698,7 +703,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. @@ -706,26 +711,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 > -1: lines.append(fail_line) lines.append(sep + '\n') if is_html: @@ -734,18 +739,36 @@ 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 _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(clean=False) + + _, upper_violations_limit, _, _ = _parse_pylint_options(options) + + # Print number of violations to log + 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=upper_violations_limit)) + + if count > upper_violations_limit > -1: + 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) @@ -759,36 +782,12 @@ 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) - 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, - dquality_dir=dquality_dir - ): - diff_quality_percentage_pass = False - # 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, @@ -803,7 +802,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). @@ -812,11 +811,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 3658f1f647..30e67dcb30 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 @@ -56,31 +56,296 @@ 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 = + # 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 +368,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 +445,4 @@ int-import-graph = [EXCEPTIONS] overgeneral-exceptions = Exception -# 5c46ef5d76dd14aadf0311325da7f519a2241646 +# 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}$ diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 6c527840cc..9e00743e20 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -41,10 +41,12 @@ edx-ccx-keys==0.2.1 edx-celeryutils==0.2.7 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 + +# 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 edx-enterprise==0.55.1 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 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 diff --git a/scripts/generic-ci-tests.sh b/scripts/generic-ci-tests.sh index d835b0344f..9d0a568c85 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 (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; +} + 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 -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.