Merge pull request #5732 from edx/benp/optimize-run-quality

Optimize run_quality calls

pavelib/paver_tests/test_paver_quality.py

@@ -72,7 +72,7 @@ class TestPaverRunQuality(unittest.TestCase):
             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)
+        self.assertEqual(self._mock_paver_sh.call_count, 2)
 
     def test_failure_on_diffquality_pylint(self):
         """
@@ -85,22 +85,23 @@ class TestPaverRunQuality(unittest.TestCase):
             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)
+        self.assertEqual(self._mock_paver_sh.call_count, 2)
 
     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]
+        self._mock_paver_sh.side_effect = [Exception('unrecognized failure!'), 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)
+        self.assertEqual(self._mock_paver_sh.call_count, 1)
 
     def test_no_diff_quality_failures(self):
         # Assert nothing is raised
         pavelib.quality.run_quality("")
+        self.assertEqual(self._mock_paver_sh.call_count, 2)
 
 
 class CustomShMock(object):
@@ -114,8 +115,7 @@ class CustomShMock(object):
         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:
+        if "pep8" in arg:
             # Essentially mock diff-quality exiting with 1
             paver.easy.sh("exit 1")
         else:
@@ -126,8 +126,7 @@ class CustomShMock(object):
         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:
+        if "pylint" in arg:
             # Essentially mock diff-quality exiting with 1
             paver.easy.sh("exit 1")
         else:

pavelib/quality.py

@@ -154,16 +154,14 @@ def run_quality(options):
 
     pep8_reports = u' '.join(pep8_files)
 
-    sh(
-        "diff-quality --violations=pep8 --html-report {dquality_dir}/"
-        "diff_quality_pep8.html {pep8_reports}".format(
-            dquality_dir=dquality_dir, pep8_reports=pep8_reports)
-    )
-
     try:
         sh(
-            "diff-quality --violations=pep8 {pep8_reports} {percentage_string}".format(
-                pep8_reports=pep8_reports, percentage_string=percentage_string)
+            "diff-quality --violations=pep8 {pep8_reports} {percentage_string} "
+            "--html-report {dquality_dir}/diff_quality_pep8.html".format(
+                pep8_reports=pep8_reports,
+                percentage_string=percentage_string,
+                dquality_dir=dquality_dir
+            )
         )
     except BuildFailure, error_message:
         if is_percentage_failure(error_message):
@@ -188,21 +186,14 @@ def run_quality(options):
         "common:common/djangoapps:common/lib"
     )
 
-    sh(
-        "{pythonpath_prefix} diff-quality --violations=pylint --html-report "
-        "{dquality_dir}/diff_quality_pylint.html {pylint_reports}".format(
-            pythonpath_prefix=pythonpath_prefix,
-            dquality_dir=dquality_dir,
-            pylint_reports=pylint_reports
-        )
-    )
-
     try:
         sh(
-            "{pythonpath_prefix} diff-quality --violations=pylint {pylint_reports} {percentage_string}".format(
+            "{pythonpath_prefix} diff-quality --violations=pylint {pylint_reports} {percentage_string} "
+            "--html-report {dquality_dir}/diff_quality_pylint.html".format(
                 pythonpath_prefix=pythonpath_prefix,
                 pylint_reports=pylint_reports,
-                percentage_string=percentage_string
+                percentage_string=percentage_string,
+                dquality_dir=dquality_dir
             )
         )
     except BuildFailure, error_message:
@@ -211,7 +202,7 @@ def run_quality(options):
         else:
             raise BuildFailure(error_message)
 
-    # if one of the diff-quality runs failed, then paver quits with an error
+    # 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).")
 

requirements/edx/github.txt

@@ -22,7 +22,7 @@
 # Our libraries:
 -e git+https://github.com/edx/XBlock.git@246811773c67a84fdb17614a8e9f7ec7b1890574#egg=XBlock
 -e git+https://github.com/edx/codejail.git@66dd5a45e5072666ff9a70c768576e9ffd1daa4b#egg=codejail
--e git+https://github.com/edx/diff-cover.git@9a44ae21369662a7d06bfc5111875fc0d119e03b#egg=diff_cover
+-e git+https://github.com/edx/diff-cover.git@v0.7.1#egg=diff_cover
 -e git+https://github.com/edx/js-test-tool.git@v0.1.5#egg=js_test_tool
 -e git+https://github.com/edx/event-tracking.git@0.1.0#egg=event-tracking
 -e git+https://github.com/edx/edx-analytics-data-api-client.git@0.1.0#egg=edx-analytics-data-api-client