diff --git a/pavelib/paver_tests/test_paver_quality.py b/pavelib/paver_tests/test_paver_quality.py index 81dd096046..db08061eee 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 twice (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 twice (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/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." 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 <