Merge pull request #9515 from edx/benp/add-jshint-diff-quality

Add jshint to run_quality.

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

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):
     """

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

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."
 

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 <<END