Merge pull request #5650 from edx/benp/TE-555

run_quality should run both quality reports in the case of a threshold breach

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

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