From 0683e5ff3f5ec393f7d1b8e284d5a9c7cdd72916 Mon Sep 17 00:00:00 2001 From: Ben Patterson Date: Sat, 27 Jun 2015 08:30:11 -0400 Subject: [PATCH 1/2] Add jshint diff-quality. The platform includes jshint as a development tool, and our builds are enforcing a limit on total number of jshint violations. This commit will enforce no new jshint violations on a per-change basis, much like pylint and pep8 are enforced. So with this change, we'll be enforcing our linting requirements consistently, regardless of type of violations. Also on Jenkins, runs quality task after installing jshint. --- pavelib/paver_tests/test_paver_quality.py | 47 ++++++++++++--- pavelib/quality.py | 69 +++++++++++++++++------ requirements/edx/base.txt | 2 +- scripts/generic-ci-tests.sh | 5 +- 4 files changed, 95 insertions(+), 28 deletions(-) diff --git a/pavelib/paver_tests/test_paver_quality.py b/pavelib/paver_tests/test_paver_quality.py index 81dd096046..46f23ace3c 100644 --- a/pavelib/paver_tests/test_paver_quality.py +++ b/pavelib/paver_tests/test_paver_quality.py @@ -187,7 +187,7 @@ class TestPaverRunQuality(unittest.TestCase): @patch('__builtin__.open', mock_open()) def test_failure_on_diffquality_pep8(self): """ - If pep8 finds errors, pylint should still be run + If pep8 finds errors, pylint and jshint should still be run """ # Mock _get_pep8_violations to return a violation _mock_pep8_violations = MagicMock( @@ -198,10 +198,10 @@ class TestPaverRunQuality(unittest.TestCase): pavelib.quality.run_quality("") self.assertRaises(BuildFailure) - # Test that both pep8 and pylint were called by counting the calls to _get_pep8_violations - # (for pep8) and sh (for diff-quality pylint) + # Test that pep8, pylint, and jshint were called by counting the calls to + # _get_pep8_violations (for pep8) and sh (for diff-quality pylint & jshint) self.assertEqual(_mock_pep8_violations.call_count, 1) - self.assertEqual(self._mock_paver_sh.call_count, 1) + self.assertEqual(self._mock_paver_sh.call_count, 2) @patch('__builtin__.open', mock_open()) def test_failure_on_diffquality_pylint(self): @@ -219,8 +219,28 @@ class TestPaverRunQuality(unittest.TestCase): # 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 once (for the call to "pylint") - self.assertEqual(self._mock_paver_sh.call_count, 1) + # And assert that sh was called twice (for the calls to pylint & jshint). This means that even in + # the event of a diff-quality pylint failure, jshint is still called. + self.assertEqual(self._mock_paver_sh.call_count, 2) + + @patch('__builtin__.open', mock_open()) + def test_failure_on_diffquality_jshint(self): + """ + If diff-quality fails on jshint, the paver task should also fail + """ + + # Underlying sh call must fail when it is running the jshint diff-quality task + self._mock_paver_sh.side_effect = CustomShMock().fail_on_jshint + _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("") + self.assertRaises(BuildFailure) + # 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 once (for the calls to pep8 and pylint) + self.assertEqual(self._mock_paver_sh.call_count, 2) @patch('__builtin__.open', mock_open()) def test_other_exception(self): @@ -243,8 +263,8 @@ class TestPaverRunQuality(unittest.TestCase): 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 once (for the call to "pylint") - self.assertEqual(self._mock_paver_sh.call_count, 1) + # And assert that sh was called once (for the call to "pylint" & "jshint") + self.assertEqual(self._mock_paver_sh.call_count, 2) class CustomShMock(object): @@ -263,3 +283,14 @@ class CustomShMock(object): paver.easy.sh("exit 1") else: return + + def fail_on_jshint(self, arg): + """ + 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 "jshint" in arg: + # Essentially mock diff-quality exiting with 1 + paver.easy.sh("exit 1") + else: + return diff --git a/pavelib/quality.py b/pavelib/quality.py index c0260df408..d05242620e 100644 --- a/pavelib/quality.py +++ b/pavelib/quality.py @@ -373,7 +373,9 @@ def run_quality(options): # Directory to put the diff reports in. # This makes the folder if it doesn't already exist. dquality_dir = (Env.REPORT_DIR / "diff_quality").makedirs_p() - diff_quality_percentage_failure = False + + # 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): """ @@ -421,20 +423,20 @@ def run_quality(options): f.write(_pep8_output(count, violations_list, is_html=True)) if count > 0: - diff_quality_percentage_failure = True + 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) - compare_branch_string = '' + compare_branch_string = u'' if compare_branch: - compare_branch_string = '--compare-branch={0}'.format(compare_branch) + compare_branch_string = u'--compare-branch={0}'.format(compare_branch) # Set the string, if needed, to be used for the diff-quality --fail-under switch. diff_threshold = int(getattr(options, 'percentage', -1)) - percentage_string = '' + percentage_string = u'' if diff_threshold > -1: - percentage_string = '--fail-under={0}'.format(diff_threshold) + 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 @@ -448,28 +450,61 @@ def run_quality(options): "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 jshint. + if not run_diff_quality( + violations_type="jshint", + prefix=pythonpath_prefix, + reports=pylint_reports, + percentage_string=percentage_string, + branch_string=compare_branch_string, + dquality_dir=dquality_dir + ): + diff_quality_percentage_pass = False + + # If one of the quality runs fails, then paver exits with an error when it is finished + if not diff_quality_percentage_pass: + raise BuildFailure("Diff-quality failure(s).") + + +def run_diff_quality( + violations_type=None, prefix=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, jshint). + If diff-quality fails due to quality issues, this method returns False. + + """ try: sh( - "{pythonpath_prefix} diff-quality --violations=pylint " - "{pylint_reports} {percentage_string} {compare_branch_string} " - "--html-report {dquality_dir}/diff_quality_pylint.html ".format( - pythonpath_prefix=pythonpath_prefix, - pylint_reports=pylint_reports, + "{pythonpath_prefix} 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=compare_branch_string, + compare_branch_string=branch_string, dquality_dir=dquality_dir, ) ) + return True except BuildFailure, error_message: if is_percentage_failure(error_message): - diff_quality_percentage_failure = True + return False else: raise BuildFailure(error_message) - # If one of the diff-quality runs fails, then paver exits with an error when it is finished - if diff_quality_percentage_failure: - raise BuildFailure("Diff-quality failure(s).") - def is_percentage_failure(error_message): """ diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 25e85d6b03..80e2bb2fcf 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -127,7 +127,7 @@ bok-choy==0.4.3 chrono==1.0.2 coverage==3.7.1 ddt==0.8.0 -diff-cover==0.7.3 +diff-cover==0.8.0 django-crum==0.5 django_nose==1.3 factory_boy==2.2.1 diff --git a/scripts/generic-ci-tests.sh b/scripts/generic-ci-tests.sh index c2ab5056e2..f072c48fd6 100755 --- a/scripts/generic-ci-tests.sh +++ b/scripts/generic-ci-tests.sh @@ -67,8 +67,6 @@ case "$TEST_SUITE" in paver run_pep8 > pep8.log || { cat pep8.log; EXIT=1; } echo "Finding pylint violations and storing in report..." paver run_pylint -l $PYLINT_THRESHOLD || { cat pylint.log; EXIT=1; } - # Run quality task. Pass in the 'fail-under' percentage to diff-quality - paver run_quality -p 100 || EXIT=1 mkdir -p reports echo "Finding jshint violations and storing report..." @@ -76,6 +74,9 @@ case "$TEST_SUITE" in paver run_jshint -l $JSHINT_THRESHOLD > jshint.log || { cat jshint.log; EXIT=1; } echo "Running code complexity report (python)." paver run_complexity > reports/code_complexity.log || echo "Unable to calculate code complexity. Ignoring error." + # Run quality task. Pass in the 'fail-under' percentage to diff-quality + paver run_quality -p 100 || EXIT=1 + # Need to create an empty test result so the post-build # action doesn't fail the build. cat > reports/quality.xml < Date: Mon, 31 Aug 2015 10:58:09 -0400 Subject: [PATCH 2/2] Update circle-ci to run quality after jshint has been added to PATH. [Also correct some comments.] --- pavelib/paver_tests/test_paver_quality.py | 4 ++-- scripts/circle-ci-tests.sh | 7 ++++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/pavelib/paver_tests/test_paver_quality.py b/pavelib/paver_tests/test_paver_quality.py index 46f23ace3c..db08061eee 100644 --- a/pavelib/paver_tests/test_paver_quality.py +++ b/pavelib/paver_tests/test_paver_quality.py @@ -239,7 +239,7 @@ class TestPaverRunQuality(unittest.TestCase): # 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 once (for the calls to pep8 and pylint) + # And assert that sh was called twice (for the calls to pep8 and pylint) self.assertEqual(self._mock_paver_sh.call_count, 2) @patch('__builtin__.open', mock_open()) @@ -263,7 +263,7 @@ class TestPaverRunQuality(unittest.TestCase): 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 once (for the call to "pylint" & "jshint") + # And assert that sh was called twice (for the call to "pylint" & "jshint") self.assertEqual(self._mock_paver_sh.call_count, 2) diff --git a/scripts/circle-ci-tests.sh b/scripts/circle-ci-tests.sh index 8840c1485f..f8cb422b4a 100755 --- a/scripts/circle-ci-tests.sh +++ b/scripts/circle-ci-tests.sh @@ -44,13 +44,14 @@ case $CIRCLE_NODE_INDEX in # fails and aborts the job because nothing is displayed for > 10 minutes. paver run_pylint -l $PYLINT_THRESHOLD | tee pylint.log || EXIT=1 - # Run quality task. Pass in the 'fail-under' percentage to diff-quality - paver run_quality -p 100 || EXIT=1 - mkdir -p reports echo "Finding jshint violations and storing report..." PATH=$PATH:node_modules/.bin paver run_jshint -l $JSHINT_THRESHOLD > jshint.log || { cat jshint.log; EXIT=1; } + + # Run quality task. Pass in the 'fail-under' percentage to diff-quality + paver run_quality -p 100 || EXIT=1 + echo "Running code complexity report (python)." paver run_complexity > reports/code_complexity.log || echo "Unable to calculate code complexity. Ignoring error."