diff --git a/pavelib/paver_tests/test_paver_quality.py b/pavelib/paver_tests/test_paver_quality.py index 086aedfcee..7c2bf3254a 100644 --- a/pavelib/paver_tests/test_paver_quality.py +++ b/pavelib/paver_tests/test_paver_quality.py @@ -1,9 +1,12 @@ -import unittest -import pavelib.quality -import tempfile import os +import tempfile +import unittest +from mock import patch from ddt import ddt, file_data +import pavelib.quality +import paver.easy +from paver.easy import BuildFailure @ddt class TestPaverQualityViolations(unittest.TestCase): @@ -43,3 +46,89 @@ class TestPaverQualityViolations(unittest.TestCase): def tearDown(self): os.remove(self.f.name) + + +class TestPaverRunQuality(unittest.TestCase): + """ + For testing the paver run_quality task + """ + + def setUp(self): + + # 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 + self._mock_paver_sh = patch('pavelib.quality.sh').start() + + def test_failure_on_diffquality_pep8(self): + """ + If pep8 diff-quality fails due to the percentage threshold, pylint + should still be run + """ + + # Underlying sh call must fail when it is running the pep8 diff-quality task + self._mock_paver_sh.side_effect = CustomShMock().fail_on_pep8 + with self.assertRaises(SystemExit): + pavelib.quality.run_quality("") + self.assertRaises(BuildFailure) + # Test that both pep8 and pylint were called by counting the calls + self.assertEqual(self._mock_paver_sh.call_count, 4) + + def test_failure_on_diffquality_pylint(self): + """ + If diff-quality fails on pylint, the paver task should also fail + """ + + # Underlying sh call must fail when it is running the pylint diff-quality task + self._mock_paver_sh.side_effect = CustomShMock().fail_on_pylint + with self.assertRaises(SystemExit): + pavelib.quality.run_quality("") + self.assertRaises(BuildFailure) + # Test that both pep8 and pylint were called by counting the calls + self.assertEqual(self._mock_paver_sh.call_count, 4) + + def test_other_exception(self): + """ + If diff-quality fails for an unknown reason on the first run (pep8), then + pylint should not be run + """ + self._mock_paver_sh.side_effect = [0, Exception('unrecognized failure!'), 0, 0] + with self.assertRaises(Exception): + pavelib.quality.run_quality("") + # Test that pylint is NOT called by counting calls + self.assertEqual(self._mock_paver_sh.call_count, 2) + + def test_no_diff_quality_failures(self): + # Assert nothing is raised + pavelib.quality.run_quality("") + + +class CustomShMock(object): + """ + Diff-quality makes a number of sh calls. None of those calls should be made during tests; however, some + of them need to have certain responses. + """ + + def fail_on_pep8(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. + """ + # Condition is for the sh calls that contain both 'pep8' and 'html' + if "pep8" in arg and "html" not in arg: + # Essentially mock diff-quality exiting with 1 + paver.easy.sh("exit 1") + else: + return + + def fail_on_pylint(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. + """ + # Condition is for the sh calls that contain both 'pep8' and 'html' + if "pylint" in arg and "html" not 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 f233428d90..280a472f2c 100644 --- a/pavelib/quality.py +++ b/pavelib/quality.py @@ -1,11 +1,9 @@ """ Check code quality using pep8, pylint, and diff_quality. """ -from paver.easy import sh, task, cmdopts, needs +from paver.easy import sh, task, cmdopts, needs, BuildFailure import os -import errno import re -from optparse import make_option from .utils.envs import Env @@ -136,12 +134,13 @@ 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 # Set the string, if needed, to be used for the diff-quality --fail-under switch. diff_threshold = int(getattr(options, 'percentage', -1)) - threshold_string = '' + percentage_string = '' if diff_threshold > -1: - threshold_string = '--fail-under={0}'.format(diff_threshold) + percentage_string = '--fail-under={0}'.format(diff_threshold) # Generate diff-quality html report for pep8, and print to console # If pep8 reports exist, use those @@ -161,12 +160,18 @@ def run_quality(options): dquality_dir=dquality_dir, pep8_reports=pep8_reports) ) - sh( - "diff-quality --violations=pep8 {pep8_reports} {threshold_string}".format( - pep8_reports=pep8_reports, threshold_string=threshold_string) - ) + try: + sh( + "diff-quality --violations=pep8 {pep8_reports} {percentage_string}".format( + pep8_reports=pep8_reports, percentage_string=percentage_string) + ) + except BuildFailure, error_message: + if is_percentage_failure(error_message): + diff_quality_percentage_failure = True + else: + raise BuildFailure(error_message) - # Generage diff-quality html report for pylint, and print to console + # Generate diff-quality html report for pylint, and print to console # If pylint reports exist, use those # Otherwise, `diff-quality` will call pylint itself @@ -192,10 +197,32 @@ def run_quality(options): ) ) - sh( - "{pythonpath_prefix} diff-quality --violations=pylint {pylint_reports} {threshold_string}".format( - pythonpath_prefix=pythonpath_prefix, - pylint_reports=pylint_reports, - threshold_string=threshold_string + try: + sh( + "{pythonpath_prefix} diff-quality --violations=pylint {pylint_reports} {percentage_string}".format( + pythonpath_prefix=pythonpath_prefix, + pylint_reports=pylint_reports, + percentage_string=percentage_string + ) ) - ) + except BuildFailure, error_message: + if is_percentage_failure(error_message): + diff_quality_percentage_failure = True + else: + raise BuildFailure(error_message) + + # if one of the diff-quality runs failed, then paver quits with an error + if diff_quality_percentage_failure: + raise BuildFailure("Diff-quality failure(s).") + + +def is_percentage_failure(error_message): + """ + When diff-quality is run with a threshold percentage, it ends with an exit code of 1. This bubbles up to + paver with a subprocess return code error. If the subprocess exits with anything other than 1, raise + a paver exception. + """ + if "Subprocess return code: 1" not in error_message: + return False + else: + return True