Add jshint diff-quality.

The platform includes jshint as a development tool, and our
builds are enforcing a limit on total number of jshint violations.
This commit will enforce no new jshint violations on a per-change
basis, much like pylint and pep8 are enforced. So with this change,
we'll be enforcing our linting requirements consistently, regardless
of type of violations.

Also on Jenkins, runs quality task after installing jshint.

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 once (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 once (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/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