Merge pull request #14674 from cpennington/cale/latest-edx-lint

Cale/latest edx lint

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)

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

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

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,
+	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-few-public-methods,
+	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

pylintrc_tweaks

 # 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}$

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

requirements/edx/paver.txt

 # 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

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
 

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.