Merge pull request #5810 from edx/benp/improve-run-quality

Reduce complexity in run_quality.

pavelib/paver_tests/test_paver_get_quality_reports.py

@@ -0,0 +1,48 @@
+import os
+import tempfile
+import unittest
+from mock import patch, Mock
+from ddt import ddt, file_data
+
+import pavelib.quality
+import paver.easy
+from paver.easy import BuildFailure
+
+
+
+class TestGetReportFiles(unittest.TestCase):
+    """
+    Ensure only the report files we want are returned as part of run_quality.
+    """
+
+    @patch('os.walk')
+    def test_get_pylint_reports(self, my_mock):
+
+        my_mock.return_value = iter([
+            ('/foo', ('',), ('pylint.report',)),
+            ('/bar', ('/baz',), ('pylint.report',))
+        ])
+        reports = pavelib.quality.get_violations_reports("pylint")
+        self.assertEqual(len(reports), 2)
+
+    @patch('os.walk')
+    def test_get_pep8_reports(self, my_mock):
+        my_mock.return_value = iter([
+            ('/foo', ('',), ('pep8.report',)),
+            ('/bar', ('/baz',), ('pep8.report',))
+        ])
+        reports = pavelib.quality.get_violations_reports("pep8")
+        self.assertEqual(len(reports), 2)
+
+    @patch('os.walk')
+    def test_get_pep8_reports_noisy(self, my_mock):
+        """ Several conditions: different report types, different files, multiple files """
+        my_mock.return_value = iter([
+            ('/foo', ('',), ('pep8.report',)),
+            ('/fooz', ('/ball',), ('pylint.report',)),
+            ('/fooz', ('/ball',), ('non.report',)),
+            ('/fooz', ('/ball',), ('lms.xml',)),
+            ('/bar', ('/baz',), ('pep8.report',))
+        ])
+        reports = pavelib.quality.get_violations_reports("pep8")
+        self.assertEqual(len(reports), 2)

pavelib/quality.py

@@ -146,12 +146,7 @@ def run_quality(options):
     # If pep8 reports exist, use those
     # Otherwise, `diff-quality` will call pep8 itself
 
-    pep8_files = []
-    for subdir, _dirs, files in os.walk(os.path.join(Env.REPORT_DIR)):
-        for f in files:
-            if f == "pep8.report":
-                pep8_files.append(os.path.join(subdir, f))
-
+    pep8_files = get_violations_reports("pep8")
     pep8_reports = u' '.join(pep8_files)
 
     try:
@@ -173,12 +168,7 @@ def run_quality(options):
     # If pylint reports exist, use those
     # Otherwise, `diff-quality` will call pylint itself
 
-    pylint_files = []
-    for subdir, _dirs, files in os.walk(os.path.join(Env.REPORT_DIR)):
-        for f in files:
-            if f == "pylint.report":
-                pylint_files.append(os.path.join(subdir, f))
-
+    pylint_files = get_violations_reports("pylint")
     pylint_reports = u' '.join(pylint_files)
 
     pythonpath_prefix = (
@@ -217,3 +207,15 @@ def is_percentage_failure(error_message):
         return False
     else:
         return True
+
+
+def get_violations_reports(violations_type):
+    """
+    Finds violations reports files by naming convention (e.g., all "pep8.report" files)
+    """
+    violations_files = []
+    for subdir, _dirs, files in os.walk(os.path.join(Env.REPORT_DIR)):
+        for f in files:
+            if f == "{violations_type}.report".format(violations_type=violations_type):
+                violations_files.append(os.path.join(subdir, f))
+    return violations_files