Merge pull request #13081 from edx/bjacobel/eslint

Replace JSHint with ESLint and the new JavaScript styleguide

.eslintignore

+# Vendor files and generated test artifacts
+**/vendor
+test_root/staticfiles
+
+
+# Vendor files living outside the /vendor/ dir
+*.min.js
+*-min.js
+*.nocache.js
+**/bootstrap*.js
+**/jquery*.js
+**/d3*.js
+
+
+# Translations files
+**/static/js/i18n
+
+
+# Gitignored xmodule stuff
+common/static/xmodule
+
+
+# Coffeescript directories (don't lint autogenerated files)
+cms/static/coffee
+lms/static/coffee
+common/static/coffee
+common/lib/capa/capa/tests/test_files/js
+
+
+# Symlinks into common/lib/xmodule/xmodule/js
+cms/static/xmodule_js
+lms/static/xmodule_js
+
+
+# This directory is about half Coffee and half JS, things get messy here so just ignore all existing coffee paths
+common/lib/xmodule/xmodule/js/spec/annotatable/display_spec.js
+common/lib/xmodule/xmodule/js/spec/capa/display_spec.js
+common/lib/xmodule/xmodule/js/spec/html/edit_spec.js
+common/lib/xmodule/xmodule/js/spec/problem/edit_spec.js
+common/lib/xmodule/xmodule/js/spec/problem/edit_spec_hint.js
+common/lib/xmodule/xmodule/js/spec/tabs/edit.js
+
+common/lib/xmodule/xmodule/js/src/annotatable/display.js
+common/lib/xmodule/xmodule/js/src/capa/display.js
+common/lib/xmodule/xmodule/js/src/conditional/display.js
+common/lib/xmodule/xmodule/js/src/discussion/display.js
+common/lib/xmodule/xmodule/js/src/html/display.js
+common/lib/xmodule/xmodule/js/src/html/edit.js
+common/lib/xmodule/xmodule/js/src/javascript_loader.js
+common/lib/xmodule/xmodule/js/src/problem/edit.js
+common/lib/xmodule/xmodule/js/src/raw/edit/json.js
+common/lib/xmodule/xmodule/js/src/raw/edit/metadata-only.js
+common/lib/xmodule/xmodule/js/src/raw/edit/xml.js
+common/lib/xmodule/xmodule/js/src/sequence/display.js
+common/lib/xmodule/xmodule/js/src/sequence/edit.js
+common/lib/xmodule/xmodule/js/src/tabs/tabs-aggregator.js
+common/lib/xmodule/xmodule/js/src/vertical/edit.js

.eslintrc.json

+{
+  "extends": "eslint-config-edx"
+}

package.json

@@ -18,8 +18,8 @@
   },
   "devDependencies": {
     "edx-custom-a11y-rules": "0.1.2",
-    "pa11y": "3.6.0",
-    "pa11y-reporter-1.0-json": "1.0.2",
+    "eslint": "^2.13.1",
+    "eslint-config-edx": "^1.2.0",
     "jasmine-core": "^2.4.1",
     "jasmine-jquery": "^2.1.1",
     "jquery": "^2.1.4",
@@ -31,8 +31,10 @@
     "karma-jasmine": "^0.3.8",
     "karma-jasmine-html-reporter": "^0.2.0",
     "karma-junit-reporter": "^0.4.1",
-    "karma-spec-reporter": "^0.0.20",
     "karma-requirejs": "^0.2.6",
+    "karma-spec-reporter": "^0.0.20",
+    "pa11y": "3.6.0",
+    "pa11y-reporter-1.0-json": "1.0.2",
     "plato": "1.2.2"
   }
 }

pavelib/paver_tests/test_eslint.py

+"""
+Tests for paver quality tasks
+"""
+import unittest
+from mock import patch
+
+import pavelib.quality
+from paver.easy import BuildFailure
+
+
+class TestPaverESLint(unittest.TestCase):
+    """
+    For testing run_eslint
+    """
+
+    def setUp(self):
+        super(TestPaverESLint, self).setUp()
+
+        # Mock the paver @needs decorator
+        self._mock_paver_needs = patch.object(pavelib.quality.run_eslint, '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_eslint_violation_number_not_found(self, mock_count, mock_report_dir, mock_write_metric):  # pylint: disable=unused-argument
+        """
+        run_eslint encounters an error parsing the eslint output log
+        """
+        mock_count.return_value = None
+        with self.assertRaises(BuildFailure):
+            pavelib.quality.run_eslint("")
+
+    @patch.object(pavelib.quality, '_write_metric')
+    @patch.object(pavelib.quality, '_prepare_report_dir')
+    @patch.object(pavelib.quality, '_get_count_from_last_line')
+    def test_eslint_vanilla(self, mock_count, mock_report_dir, mock_write_metric):  # pylint: disable=unused-argument
+        """
+        eslint finds violations, but a limit was not set
+        """
+        mock_count.return_value = 1
+        pavelib.quality.run_eslint("")

pavelib/paver_tests/test_jshint.py

@@ -8,6 +8,7 @@ import pavelib.quality
 from paver.easy import BuildFailure
 
 
+# @TODO Deprecated, remove in favor of ESLint
 class TestPaverJsHint(unittest.TestCase):
     """
     For testing run_jshint

pavelib/paver_tests/test_paver_quality.py

@@ -61,7 +61,7 @@ class TestPaverQualityViolations(unittest.TestCase):
 class TestPaverReportViolationsCounts(unittest.TestCase):
     """
     For testing utility functions for getting counts from reports for
-    run_jshint, run_complexity, run_safelint, and run_safecommit_report.
+    run_jshint, run_eslint, run_complexity, run_safelint, and run_safecommit_report.
     """
 
     def setUp(self):
@@ -79,19 +79,20 @@ class TestPaverReportViolationsCounts(unittest.TestCase):
         self.addCleanup(self._mock_paver_needs.stop)
         self.addCleanup(os.remove, self.f.name)
 
+    # @TODO: Deprecated, remove in favor of ESLint
     def test_get_jshint_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, "jshint")  # pylint: disable=protected-access
         self.assertEqual(actual_count, 3000)
 
-    def test_get_violations_no_number_found(self):
+    def test_get_jshint_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, "jshint")  # pylint: disable=protected-access
         self.assertEqual(actual_count, None)
 
-    def test_get_violations_count_truncated_report(self):
+    def test_get_jshint_violations_count_truncated_report(self):
         """
         A truncated report (i.e. last line is just a violation)
         """
@@ -100,6 +101,27 @@ class TestPaverReportViolationsCounts(unittest.TestCase):
         actual_count = pavelib.quality._get_count_from_last_line(self.f.name, "jshint")  # pylint: disable=protected-access
         self.assertEqual(actual_count, None)
 
+    def test_get_eslint_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, "eslint")  # pylint: disable=protected-access
+        self.assertEqual(actual_count, 3000)
+
+    def test_get_eslint_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, "eslint")  # pylint: disable=protected-access
+        self.assertEqual(actual_count, None)
+
+    def test_get_eslint_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, "eslint")  # pylint: disable=protected-access
+        self.assertEqual(actual_count, None)
+
     def test_complexity_value(self):
         with open(self.f.name, 'w') as f:
             f.write("Average complexity: A (1.93953443446)")
@@ -274,7 +296,7 @@ class TestPaverRunQuality(unittest.TestCase):
     @patch('__builtin__.open', mock_open())
     def test_failure_on_diffquality_pep8(self):
         """
-        If pep8 finds errors, pylint and jshint should still be run
+        If pep8 finds errors, pylint and eslint should still be run
         """
         # Mock _get_pep8_violations to return a violation
         _mock_pep8_violations = MagicMock(
@@ -284,10 +306,10 @@ class TestPaverRunQuality(unittest.TestCase):
             with self.assertRaises(SystemExit):
                 pavelib.quality.run_quality("")
 
-        # 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)
+        # Test that pep8, pylint, jshint and eslint were called  (@TODO remove JSHint) by counting the calls to
+        # _get_pep8_violations (for pep8) and sh (for diff-quality pylint & eslint)
         self.assertEqual(_mock_pep8_violations.call_count, 1)
-        self.assertEqual(self._mock_paver_sh.call_count, 2)
+        self.assertEqual(self._mock_paver_sh.call_count, 3)
 
     @patch('__builtin__.open', mock_open())
     def test_failure_on_diffquality_pylint(self):
@@ -305,10 +327,11 @@ 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 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)
+        # And assert that sh was called 3x (for the calls to pylint & jshint & eslint). (@TODO remove JSHint)
+        # This means that even in the event of a diff-quality pylint failure, eslint is still called.
+        self.assertEqual(self._mock_paver_sh.call_count, 3)
 
+    # @TODO: Deprecated, remove in favor of ESLint
     @patch('__builtin__.open', mock_open())
     def test_failure_on_diffquality_jshint(self):
         """
@@ -325,8 +348,27 @@ 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 twice (for the calls to pep8 and pylint)
-        self.assertEqual(self._mock_paver_sh.call_count, 2)
+        # And assert that sh was called 3x (for the calls to pep8 , jshint and pylint) @TODO remove this test
+        self.assertEqual(self._mock_paver_sh.call_count, 3)
+
+    @patch('__builtin__.open', mock_open())
+    def test_failure_on_diffquality_eslint(self):
+        """
+        If diff-quality fails on eslint, the paver task should also fail
+        """
+
+        # Underlying sh call must fail when it is running the eslint diff-quality task
+        self._mock_paver_sh.side_effect = CustomShMock().fail_on_eslint
+        _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 3x (for the calls to pep8, jshint and pylint) @TODO, remove jshint and decrement
+        self.assertEqual(self._mock_paver_sh.call_count, 3)
 
     @patch('__builtin__.open', mock_open())
     def test_other_exception(self):
@@ -349,8 +391,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 twice (for the call to "pylint" & "jshint")
-        self.assertEqual(self._mock_paver_sh.call_count, 2)
+        # And assert that sh was called 3x (for the call to "pylint" & "jshint" & "eslint") (@TODO, remove jshint)
+        self.assertEqual(self._mock_paver_sh.call_count, 3)
 
 
 class CustomShMock(object):
@@ -370,6 +412,7 @@ class CustomShMock(object):
         else:
             return
 
+    # @TODO: Deprecated, remove in favor of ESLint
     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
@@ -380,3 +423,14 @@ class CustomShMock(object):
             paver.easy.sh("exit 1")
         else:
             return
+
+    def fail_on_eslint(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 "eslint" in arg:
+            # Essentially mock diff-quality exiting with 1
+            paver.easy.sh("exit 1")
+        else:
+            return

pavelib/quality.py

@@ -257,6 +257,7 @@ def run_complexity():
         print "ERROR: Unable to calculate python-only code-complexity."
 
 
+# @TODO: Remove, deprecated in favor of ESLint
 @task
 @needs('pavelib.prereqs.install_node_prereqs')
 @cmdopts([
@@ -302,6 +303,50 @@ def run_jshint(options):
 
 
 @task
+@needs('pavelib.prereqs.install_node_prereqs')
+@cmdopts([
+    ("limit=", "l", "limit for number of acceptable violations"),
+])
+def run_eslint(options):
+    """
+    Runs eslint on static asset directories.
+    If limit option is passed, fails build if more violations than the limit are found.
+    """
+
+    eslint_report_dir = (Env.REPORT_DIR / "eslint")
+    eslint_report = eslint_report_dir / "eslint.report"
+    _prepare_report_dir(eslint_report_dir)
+    violations_limit = int(getattr(options, 'limit', -1))
+
+    sh(
+        "eslint --format=compact . | tee {eslint_report}".format(
+            eslint_report=eslint_report
+        ),
+        ignore_error=True
+    )
+
+    try:
+        num_violations = int(_get_count_from_last_line(eslint_report, "eslint"))
+    except TypeError:
+        raise BuildFailure(
+            "Error. Number of eslint violations could not be found in {eslint_report}".format(
+                eslint_report=eslint_report
+            )
+        )
+
+    # Record the metric
+    _write_metric(num_violations, (Env.METRICS_DIR / "eslint"))
+
+    # Fail if number of violations is greater than the limit
+    if num_violations > violations_limit > -1:
+        raise BuildFailure(
+            "ESLint Failed. Too many violations ({count}).\nThe limit is {violations_limit}.".format(
+                count=num_violations, violations_limit=violations_limit
+            )
+        )
+
+
+@task
 @needs('pavelib.prereqs.install_python_prereqs')
 @cmdopts([
     ("thresholds=", "t", "json containing limit for number of acceptable violations per rule"),
@@ -451,9 +496,11 @@ def run_safecommit_report():
 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
+    Used for things like reports/metrics/eslint, which will simply tell you the number of
+    eslint violations found
     """
+    Env.METRICS_DIR.makedirs_p()
+
     with open(filename, "w") as metric_file:
         metric_file.write(str(metric))
 
@@ -485,7 +532,9 @@ def _get_report_contents(filename, last_line_only=False):
         with open(filename, 'r') as report_file:
             if last_line_only:
                 lines = report_file.readlines()
-                return lines[len(lines) - 1]
+                for line in reversed(lines):
+                    if line != '\n':
+                        return line
             else:
                 return report_file.read()
     else:
@@ -503,7 +552,7 @@ def _get_count_from_last_line(filename, file_type):
         # Example of the last line of a complexity report: "Average complexity: A (1.93953443446)"
         regex = r'\d+.\d+'
     else:
-        # Example of the last line of a jshint report (for example): "3482 errors"
+        # Example of the last line of a compact-formatted eslint report (for example): "62829 problems"
         regex = r'^\d+'
 
     try:
@@ -663,9 +712,14 @@ def run_quality(options):
 
     pylint_files = get_violations_reports("pylint")
     pylint_reports = u' '.join(pylint_files)
+
+    # @TODO: Remove, deprecated in favor of ESLint
     jshint_files = get_violations_reports("jshint")
     jshint_reports = u' '.join(jshint_files)
 
+    eslint_files = get_violations_reports("eslint")
+    eslint_reports = u' '.join(eslint_files)
+
     pythonpath_prefix = (
         "PYTHONPATH=$PYTHONPATH:lms:lms/djangoapps:cms:cms/djangoapps:"
         "common:common/djangoapps:common/lib"
@@ -682,6 +736,7 @@ def run_quality(options):
     ):
         diff_quality_percentage_pass = False
 
+    # @TODO: Remove, deprecated in favor of ESLint
     # run diff-quality for jshint.
     if not run_diff_quality(
             violations_type="jshint",
@@ -693,6 +748,17 @@ def run_quality(options):
     ):
         diff_quality_percentage_pass = False
 
+    # run diff-quality for eslint.
+    if not run_diff_quality(
+            violations_type="eslint",
+            prefix=pythonpath_prefix,
+            reports=eslint_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).")
@@ -702,7 +768,7 @@ 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).
+    This executes the diff-quality commandline tool for the given violation type (e.g., pylint, eslint).
     If diff-quality fails due to quality issues, this method returns False.
 
     """

requirements/edx/base.txt

@@ -146,7 +146,7 @@ bok-choy==0.5.3
 chrono==1.0.2
 coverage==4.0.2
 ddt==0.8.0
-diff-cover==0.9.0
+diff-cover==0.9.8
 django-crum==0.5
 django_nose==1.4.1
 factory_boy==2.5.1

scripts/all-tests.sh

@@ -12,7 +12,8 @@ set -e
 
 # Violations thresholds for failing the build
 export PYLINT_THRESHOLD=4175
-export JSHINT_THRESHOLD=7550
+export JSHINT_THRESHOLD=7550  # @TODO Remove, deprecated in favor of ESLint
+export ESLINT_THRESHOLD=49019
 
 SAFELINT_THRESHOLDS=`cat scripts/safelint_thresholds.json`
 export SAFELINT_THRESHOLDS=${SAFELINT_THRESHOLDS//[[:space:]]/}

scripts/circle-ci-tests.sh

@@ -60,10 +60,15 @@ else
             paver run_pylint -l $PYLINT_THRESHOLD | tee pylint.log || EXIT=1
 
             mkdir -p reports
-            echo "Finding jshint violations and storing report..."
             PATH=$PATH:node_modules/.bin
+
+            # @TODO: Remove, deprecated in favor of ESLint
+            echo "Finding JSHint violations and storing report..."
             paver run_jshint -l $JSHINT_THRESHOLD > jshint.log || { cat jshint.log; EXIT=1; }
 
+            echo "Finding ESLint violations and storing report..."
+            paver run_eslint -l $ESLINT_THRESHOLD > eslint.log || { cat eslint.log; EXIT=1; }
+
             # Run quality task. Pass in the 'fail-under' percentage to diff-quality
             paver run_quality -p 100 || EXIT=1
 

scripts/generic-ci-tests.sh

@@ -80,8 +80,14 @@ case "$TEST_SUITE" in
         paver run_pylint -l $PYLINT_THRESHOLD > pylint.log || { cat pylint.log; EXIT=1; }
 
         mkdir -p reports
-        echo "Finding jshint violations and storing report..."
+
+        # @TODO: Remove, deprecated in favor of ESLint
+        echo "Finding JSHint violations and storing report..."
         paver run_jshint -l $JSHINT_THRESHOLD > jshint.log || { cat jshint.log; EXIT=1; }
+
+        echo "Finding ESLint violations and storing report..."
+        paver run_eslint -l $ESLINT_THRESHOLD > eslint.log || { cat eslint.log; EXIT=1; }
+
         echo "Running code complexity report (python)."
         paver run_complexity > reports/code_complexity.log || echo "Unable to calculate code complexity. Ignoring error."
         echo "Running safe template linter report."

scripts/safe_template_linter.py

@@ -543,7 +543,7 @@ class SummaryResults(object):
                     print("{}: {}{} violations".format(rule_id, padding, self.totals_by_rule[rule_id]), file=out)
                 print("", file=out)
 
-            # matches output of jshint for simplicity
+            # matches output of eslint for simplicity
             print("", file=out)
             print("{} violations total".format(self.total_violations), file=out)