Merge pull request #8138 from edx/benp/add-jshint

Benp/add jshint

.jshintignore

+common/static/js/vendor
+lms/static/js/vendor

docs/en_us/internal/testing.rst

@@ -494,10 +494,10 @@ To view test coverage:
 3. Reports are located in the ``reports`` folder. The command generates
    HTML and XML (Cobertura format) reports.
 
-Code Style Quality
+Python Code Style Quality
 ------------------
 
-To view code style quality (including pep8 and pylint violations)::
+To view Python code style quality (including pep8 and pylint violations)::
 
     paver run_quality
 
@@ -531,6 +531,20 @@ More specific options are below.
    ``system`` is an optional argument here. It defaults to
    ``cms,lms,common``.
    
+
+JavaScript Code Style Quality
+------------------
+
+To view JavaScript code style quality::
+
+    paver run_jshint
+    
+-  This command also comes with a ``--limit`` switch, for example::
+
+	paver run_jshint --limit=700
+
+
+
 Testing using queue servers
 ---------------------------
 

package.json

@@ -4,5 +4,8 @@
   "dependencies": {
     "coffee-script": "1.6.1",
     "uglify-js": "2.4.15"
+  },
+  "devDependencies": {
+    "jshint": "^2.7.0"
   }
 }

pavelib/paver_tests/test_jshint.py

+"""
+Tests for paver quality tasks
+"""
+import unittest
+from mock import patch
+
+import pavelib.quality
+from paver.easy import BuildFailure
+
+
+class TestPaverJsHint(unittest.TestCase):
+    """
+    For testing run_jshint
+    """
+
+    def setUp(self):
+        super(TestPaverJsHint, self).setUp()
+
+        # Mock the paver @needs decorator
+        self._mock_paver_needs = patch.object(pavelib.quality.run_jshint, 'needs').start()
+        self._mock_paver_needs.return_value = 0
+
+        # Mock shell commands
+        patcher = patch('pavelib.quality.sh')
+        self._mock_paver_sh = patcher.start()
+
+        # Cleanup mocks
+        self.addCleanup(patcher.stop)
+        self.addCleanup(self._mock_paver_needs.stop)
+
+    @patch.object(pavelib.quality, '_write_metric')
+    @patch.object(pavelib.quality, '_prepare_report_dir')
+    @patch.object(pavelib.quality, '_get_count_from_last_line')
+    def test_jshint_violation_number_not_found(self, mock_count, mock_report_dir, mock_write_metric):  # pylint: disable=unused-argument
+        """
+        run_jshint encounters an error parsing the jshint output log
+        """
+        mock_count.return_value = None
+        with self.assertRaises(BuildFailure):
+            pavelib.quality.run_jshint("")
+
+    @patch.object(pavelib.quality, '_write_metric')
+    @patch.object(pavelib.quality, '_prepare_report_dir')
+    @patch.object(pavelib.quality, '_get_count_from_last_line')
+    def test_jshint_vanilla(self, mock_count, mock_report_dir, mock_write_metric):  # pylint: disable=unused-argument
+        """
+        jshint finds violations, but a limit was not set
+        """
+        mock_count.return_value = 1
+        pavelib.quality.run_jshint("")

pavelib/paver_tests/test_paver_quality.py

@@ -2,6 +2,7 @@
 Tests for paver quality tasks
 """
 import os
+from path import path  # pylint: disable=no-name-in-module
 import tempfile
 import unittest
 from mock import patch, MagicMock, mock_open
@@ -56,6 +57,70 @@ class TestPaverQualityViolations(unittest.TestCase):
         self.assertEqual(num, 2)
 
 
+class TestPaverJsHintViolationsCounts(unittest.TestCase):
+    """
+    For testing run_jshint
+    """
+
+    def setUp(self):
+        super(TestPaverJsHintViolationsCounts, self).setUp()
+
+        # Mock the paver @needs decorator
+        self._mock_paver_needs = patch.object(pavelib.quality.run_quality, 'needs').start()
+        self._mock_paver_needs.return_value = 0
+
+        # Temporary file infrastructure
+        self.f = tempfile.NamedTemporaryFile(delete=False)
+        self.f.close()
+
+        # Cleanup various mocks and tempfiles
+        self.addCleanup(self._mock_paver_needs.stop)
+        self.addCleanup(os.remove, self.f.name)
+
+    def test_get_violations_count(self):
+        with open(self.f.name, 'w') as f:
+            f.write("3000 violations found")
+        actual_count = pavelib.quality._get_count_from_last_line(self.f.name)  # pylint: disable=protected-access
+        self.assertEqual(actual_count, 3000)
+
+    def test_get_violations_no_number_found(self):
+        with open(self.f.name, 'w') as f:
+            f.write("Not expected string regex")
+        actual_count = pavelib.quality._get_count_from_last_line(self.f.name)  # pylint: disable=protected-access
+        self.assertEqual(actual_count, None)
+
+    def test_get_violations_count_truncated_report(self):
+        """
+        A truncated report (i.e. last line is just a violation)
+        """
+        with open(self.f.name, 'w') as f:
+            f.write("foo/bar/js/fizzbuzz.js: line 45, col 59, Missing semicolon.")
+        actual_count = pavelib.quality._get_count_from_last_line(self.f.name)  # pylint: disable=protected-access
+        self.assertEqual(actual_count, None)
+
+
+class TestPrepareReportDir(unittest.TestCase):
+    """
+    Tests the report directory preparation
+    """
+
+    def setUp(self):
+        super(TestPrepareReportDir, self).setUp()
+        self.test_dir = tempfile.mkdtemp()
+        self.test_file = tempfile.NamedTemporaryFile(delete=False, dir=self.test_dir)
+        self.addCleanup(os.removedirs, self.test_dir)
+
+    def test_report_dir_with_files(self):
+        self.assertTrue(os.path.exists(self.test_file.name))
+        pavelib.quality._prepare_report_dir(path(self.test_dir))  # pylint: disable=protected-access
+        self.assertFalse(os.path.exists(self.test_file.name))
+
+    def test_report_dir_without_files(self):
+        os.remove(self.test_file.name)
+        pavelib.quality._prepare_report_dir(path(self.test_dir))  # pylint: disable=protected-access
+        self.assertEqual(os.listdir(path(self.test_dir)), [])
+
+
 class TestPaverRunQuality(unittest.TestCase):
     """
     For testing the paver run_quality task

pavelib/quality.py

@@ -241,6 +241,86 @@ def run_complexity():
 
 
 @task
+@needs('pavelib.prereqs.install_node_prereqs')
+@cmdopts([
+    ("limit=", "l", "limit for number of acceptable violations"),
+])
+def run_jshint(options):
+    """
+    Runs jshint on static asset directories
+    """
+
+    violations_limit = int(getattr(options, 'limit', -1))
+
+    jshint_report_dir = (Env.REPORT_DIR / "jshint")
+    jshint_report = jshint_report_dir / "jshint.report"
+    _prepare_report_dir(jshint_report_dir)
+
+    jshint_directories = ["common/static/js", "cms/static/js", "lms/static/js"]
+
+    sh(
+        "jshint {list} --config .jshintrc >> {jshint_report}".format(
+            list=(" ".join(jshint_directories)), jshint_report=jshint_report
+        ),
+        ignore_error=True
+    )
+    num_violations = _get_count_from_last_line(jshint_report)
+
+    if not num_violations:
+        raise BuildFailure("Error in calculating total number of violations.")
+
+    # Record the metric
+    _write_metric(str(num_violations), (Env.METRICS_DIR / "jshint"))
+
+    # Fail if number of violations is greater than the limit
+    if num_violations > violations_limit > -1:
+        raise Exception(
+            "JSHint Failed. Too many violations ({count}).\nThe limit is {violations_limit}.".format(
+                count=num_violations, violations_limit=violations_limit
+            )
+        )
+
+
+def _write_metric(metric, filename):
+    """
+    Write a given metric to a given file
+    Used for things like reports/metrics/jshint, which will simply tell you the number of
+    jshint violations found
+    """
+    with open(filename, "w") as metric_file:
+        metric_file.write(metric)
+
+
+def _prepare_report_dir(dir_name):
+    """
+    Sets a given directory to a created, but empty state
+    """
+    dir_name.rmtree_p()
+    dir_name.mkdir_p()
+
+
+def _get_last_report_line(filename):
+    """
+    Returns the last line of a given file. Used for getting output from quality output files.
+    """
+    with open(filename, 'r') as report_file:
+        lines = report_file.readlines()
+        return lines[len(lines) - 1]
+
+
+def _get_count_from_last_line(filename):
+    """
+    This will return the number in a line that looks something like "3000 errors found". It is returning
+    the digits only (as an integer).
+    """
+    last_line = _get_last_report_line(filename)
+    try:
+        return int(re.search(r'^\d+', last_line).group(0))
+    except AttributeError:
+        return None
+
+
+@task
 @needs('pavelib.prereqs.install_python_prereqs')
 @cmdopts([
     ("compare-branch=", "b", "Branch to compare against, defaults to origin/master"),

scripts/all-tests.sh

@@ -57,6 +57,7 @@ source scripts/jenkins-common.sh
 
 # Violations thresholds for failing the build
 PYLINT_THRESHOLD=7350
+JSHINT_THRESHOLD=3700
 
 # If the environment variable 'SHARD' is not set, default to 'all'.
 # This could happen if you are trying to use this script from
@@ -78,6 +79,10 @@ case "$TEST_SUITE" in
         paver run_quality -p 100
 
         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; }
+        echo "Running code complexity report (python)."
         paver run_complexity > reports/code_complexity.log || echo "Unable to calculate code complexity. Ignoring error."
         # Need to create an empty test result so the post-build
         # action doesn't fail the build.