From c2181e031c658653bca7392f8c159ce9e1ee1aca Mon Sep 17 00:00:00 2001 From: Brian Jacobel Date: Tue, 2 Aug 2016 10:02:41 -0400 Subject: [PATCH 1/3] Remove JSHint dependency and configuration --- .jshintignore | 9 --- .jshintrc | 154 -------------------------------------------------- package.json | 1 - 3 files changed, 164 deletions(-) delete mode 100644 .jshintignore delete mode 100644 .jshintrc diff --git a/.jshintignore b/.jshintignore deleted file mode 100644 index 0065f2da20..0000000000 --- a/.jshintignore +++ /dev/null @@ -1,9 +0,0 @@ -**/vendor -cms/static/cms/js/build.js -cms/static/cms/js/spec/main.js -cms/static/js/i18n/**/*.js -lms/static/js/i18n/**/*.js -lms/static/lms/js/build.js -lms/static/lms/js/spec/main.js -node_modules -venv diff --git a/.jshintrc b/.jshintrc deleted file mode 100644 index 85f5d2013a..0000000000 --- a/.jshintrc +++ /dev/null @@ -1,154 +0,0 @@ -// -------------------------------------------------------------------- -// JSHint Configuration -// -------------------------------------------------------------------- -// -// http://www.jshint.com/ -// http://jshint.com/docs/options/ -{ - // == Enforcing Options =============================================== - "bitwise" : false, // Prohibits the use of bitwise operators such as ^ (XOR), | (OR) and others. Bitwise operators are very rare in JavaScript programs and quite often & is simply a mistyped &&. - "camelcase" : false, // Allows you to force all variable names to use either camelCase style or UPPER_CASE with underscores. - "curly" : true, // Requires you to always put curly braces around blocks in loops and conditionals. JavaScript allows you to omit curly braces when the block consists of only one statement - "eqeqeq" : true, // Prohibits the use of == and != in favor of === and !==. - "es3" : false, // Tells JSHint that your code needs to adhere to ECMAScript 3 specification. Use this option if you need your program to be executable in older browsers—such as Internet Explorer 6/7/8/9. - "forin" : true, // Requires all for in loops to filter object's items. - "freeze" : true, // Prohibits overwriting prototypes of native objects such as Array, Date and so on. - "immed" : true, // Prohibits the use of immediate function invocations without wrapping them in parentheses. - // "indent" : 4, // Enforces specific tab width for your code. Has no effect when "white" option is not used. - "latedef" : "nofunc", // Prohibits the use of a variable before it was defined. Setting this option to "nofunc" will allow function declarations to be ignored. - "newcap" : false, // Requires you to capitalize names of constructor functions. - "noarg" : true, // Prohibits the use of arguments.caller and arguments.callee. - "noempty" : true, // Warns when you have an empty block in your code. - "nonbsp" : true, // Warns about "non-breaking whitespace" characters. - "nonew" : true, // Prohibits the use of constructor functions for side-effects. - "plusplus" : false, // Prohibits the use of unary increment and decrement operators. - "quotmark" : false, // Enforces the consistency of quotation marks used throughout your code. It accepts three values: true, "single", and "double". - "undef" : true, // Prohibits the use of explicitly undeclared variables. - "unused" : true, // Warns when you define and never use your variables. - "strict" : true, // Requires all functions to run in ECMAScript 5's strict mode. - "trailing" : true, // Makes it an error to leave a trailing whitespace in your code. - "maxlen" : 120, // Lets you set the maximum length of a line. - //"maxparams" : 4, // Lets you set the max number of formal parameters allowed per function. - //"maxdepth" : 4, // Lets you control how nested do you want your blocks to be. - //"maxstatements" : 4, // Lets you set the max number of statements allowed per function. - //"maxcomplexity" : 4, // Lets you control cyclomatic complexity throughout your code. - - - // == Relaxing Options ================================================ - "asi" : false, // Suppresses warnings about missing semicolons. - "boss" : false, // Suppresses warnings about the use of assignments in cases where comparisons are expected. - "debug" : false, // Suppresses warnings about the debugger statements in your code. - "eqnull" : false, // Suppresses warnings about == null comparisons. - "esnext" : false, // Tells JSHint that your code uses ECMAScript 6 specific syntax. - "evil" : false, // Suppresses warnings about the use of eval. - "expr" : false, // Suppresses warnings about the use of expressions where normally you would expect to see assignments or function calls. - "funcscope" : false, // Suppresses warnings about declaring variables inside of control structures while accessing them later from the outside. - "gcl" : false, // Makes JSHint compatible with Google Closure Compiler. - "globalstrict" : false, // Suppresses warnings about the use of global strict mode. - "iterator" : false, // Suppresses warnings about the __iterator__ property. - "lastsemic" : false, // Suppresses warnings about missing semicolons, but only when the semicolon is omitted for the last statement in a one-line block. - "laxbreak" : false, // Suppresses most of the warnings about possibly unsafe line breaks in your code. - "laxcomma" : false, // Suppresses warnings about comma-first coding style. - "loopfunc" : false, // Suppresses warnings about functions inside of loops. - "maxerr" : 100, // Set the maximum amount of warnings JSHint will produce before giving up. - "moz" : false, // Tells JSHint that your code uses Mozilla JavaScript extensions. - "notypeof" : false, // Suppresses warnings about invalid typeof operator values. - "proto" : false, // Suppresses warnings about the __proto__ property. - "scripturl" : false, // Suppresses warnings about the use of script-targeted URLs—such as javascript:... - "smarttabs" : false, // Suppresses warnings about mixed tabs and spaces when the latter are used for alignment only. - "shadow" : false, // Suppresses warnings about variable shadowing i.e. declaring a variable that had been already declared somewhere in the outer scope. - "sub" : false, // Suppresses warnings about using [] notation when it can be expressed in dot notation. - "supernew" : false, // Suppresses warnings about "weird" constructions like new function () { ... } and new Object;. - "validthis" : true, // Suppresses warnings about possible strict violations when the code is running in strict mode and you use this in a non-constructor function. - "noyield" : false, // Suppresses warnings about generator functions with no yield statement in them. - - - // == Environments ==================================================== - // - // These options pre-define global variables that are exposed by - // popular JavaScript libraries and runtime environments—such as - // browser or node.js. - "browser" : true, // Defines globals exposed by modern browsers: all the way from good old document and navigator to the HTML5 FileReader and other new developments in the browser world. - "devel" : true, // Defines globals that are usually used for logging poor-man's debugging: console, alert, etc. - // The rest should remain `false`. Please see explanation for the "predef" parameter below. - "couch" : false, // Defines globals exposed by CouchDB. - "dojo" : false, // Defines globals exposed by the Dojo Toolkit - "jquery" : false, // Defines globals exposed by the jQuery JavaScript library. - "mootools" : false, // Defines globals exposed by the MooTools JavaScript framework. - "node" : false, // Defines globals available when your code is running inside of the Node runtime environment. - "nonstandard" : false, // Defines non-standard but widely adopted globals such as escape and unescape. - "phantom" : false, // Defines globals available when your core is running inside of the PhantomJS runtime environment. - "prototypejs" : false, // Defines globals exposed by the Prototype JavaScript framework. - "rhino" : false, // Defines globals available when your code is running inside of the Rhino runtime environment. - "worker" : false, // Defines globals available when your code is running inside of a Web Worker. - "wsh" : false, // Defines globals available when your code is running as a script for the Windows Script Host. - "yui" : false, // Defines globals exposed by the YUI JavaScript framework. - - - // == JSLint Legacy =================================================== - // - // These options are legacy from JSLint. Aside from bug fixes they will - // not be improved in any way and might be removed at any point. - // "nomen" : false, // Disallows the use of dangling _ in variables. - // "onevar" : false, // Allows only one var statement per function. - // "passfail" : false, // Makes JSHint stop on the first error or warning. - // "white" : false, // make JSHint check your source code against Douglas Crockford's JavaScript coding style. - - - // == Undocumented Options ============================================ - // - // If you are using some global variable, for example `define`, or `$`, please - // make it available within your JS file by passing it to the wrapper anonymous - // function like so: - // - // (function (define, $) { - // 'use strict'; - // // Your content goes here which uses `define`, and `$`. - // }).call(this, window.define, window.jQuery); - // - // The parameter "predef" should remain empty for this configuration file - // to remain as general as possible. - "predef": [ - // JavaScript global libraries - "Backbone", - "jQuery", - "$", - "_", - - // RequireJS globals - "define", - "require", - "RequireJS", - - // Jasmine globals - "jasmine", - "describe", "xdescribe", - "it", "xit", - "spyOn", - "beforeEach", - "afterEach", - "expect", - "waitsFor", - "runs", - - // jQuery-Jasmine globals - "loadFixtures", - "appendLoadFixtures", - "readFixtures", - "setFixtures", - "appendSetFixtures", - "spyOnEvent", - - // Django i18n catalog globals - "interpolate", - "gettext", - "ngettext", - - // Miscellaneous globals - "JSON", - - // edX globals - "edx", - "XBlock" - ] -} diff --git a/package.json b/package.json index 03b5ba12fa..71cc87fca5 100644 --- a/package.json +++ b/package.json @@ -23,7 +23,6 @@ "jasmine-core": "^2.4.1", "jasmine-jquery": "^2.1.1", "jquery": "^2.1.4", - "jshint": "^2.7.0", "karma": "^0.13.22", "karma-chrome-launcher": "^0.2.3", "karma-coverage": "^0.5.5", From 017dbffd5effdfc6a39a4239a8dad66827ef4657 Mon Sep 17 00:00:00 2001 From: Brian Jacobel Date: Tue, 2 Aug 2016 10:09:03 -0400 Subject: [PATCH 2/3] Remove ESLint from build scripts and script tests --- pavelib/paver_tests/test_jshint.py | 51 ---------------- pavelib/paver_tests/test_paver_quality.py | 72 +++-------------------- pavelib/quality.py | 61 ------------------- scripts/all-tests.sh | 1 - scripts/circle-ci-tests.sh | 4 -- scripts/generic-ci-tests.sh | 5 -- 6 files changed, 9 insertions(+), 185 deletions(-) delete mode 100644 pavelib/paver_tests/test_jshint.py diff --git a/pavelib/paver_tests/test_jshint.py b/pavelib/paver_tests/test_jshint.py deleted file mode 100644 index 36f78fa801..0000000000 --- a/pavelib/paver_tests/test_jshint.py +++ /dev/null @@ -1,51 +0,0 @@ -""" -Tests for paver quality tasks -""" -import unittest -from mock import patch - -import pavelib.quality -from paver.easy import BuildFailure - - -# @TODO Deprecated, remove in favor of ESLint -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("") diff --git a/pavelib/paver_tests/test_paver_quality.py b/pavelib/paver_tests/test_paver_quality.py index 8bddfd5e1c..7c2810c621 100644 --- a/pavelib/paver_tests/test_paver_quality.py +++ b/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_eslint, run_complexity, run_safelint, and run_safecommit_report. + run_eslint, run_complexity, run_safelint, and run_safecommit_report. """ def setUp(self): @@ -79,28 +79,6 @@ 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_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_jshint_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, "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") @@ -306,10 +284,10 @@ class TestPaverRunQuality(unittest.TestCase): with self.assertRaises(SystemExit): pavelib.quality.run_quality("") - # Test that pep8, pylint, jshint and eslint were called (@TODO remove JSHint) by counting the calls to + # Test that pep8, pylint and eslint were called 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, 3) + self.assertEqual(self._mock_paver_sh.call_count, 2) @patch('__builtin__.open', mock_open()) def test_failure_on_diffquality_pylint(self): @@ -327,29 +305,9 @@ 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 3x (for the calls to pylint & jshint & eslint). (@TODO remove JSHint) + # And assert that sh was called twice (for the calls to pylint & eslint). # 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): - """ - 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 3x (for the calls to pep8 , jshint and pylint) @TODO remove this test - self.assertEqual(self._mock_paver_sh.call_count, 3) + self.assertEqual(self._mock_paver_sh.call_count, 2) @patch('__builtin__.open', mock_open()) def test_failure_on_diffquality_eslint(self): @@ -367,8 +325,8 @@ 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 3x (for the calls to pep8, jshint and pylint) @TODO, remove jshint and decrement - self.assertEqual(self._mock_paver_sh.call_count, 3) + # 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): @@ -391,8 +349,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 3x (for the call to "pylint" & "jshint" & "eslint") (@TODO, remove jshint) - self.assertEqual(self._mock_paver_sh.call_count, 3) + # And assert that sh was called twice (for the call to "pylint" & "eslint") + self.assertEqual(self._mock_paver_sh.call_count, 2) class CustomShMock(object): @@ -412,18 +370,6 @@ 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 - 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 - 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 diff --git a/pavelib/quality.py b/pavelib/quality.py index 59eb5db9c7..4b98c88a6b 100644 --- a/pavelib/quality.py +++ b/pavelib/quality.py @@ -257,51 +257,6 @@ 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([ - ("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) - - sh( - "jshint . --config .jshintrc >> {jshint_report}".format( - jshint_report=jshint_report - ), - ignore_error=True - ) - - try: - num_violations = int(_get_count_from_last_line(jshint_report, "jshint")) - except TypeError: - raise BuildFailure( - "Error. Number of jshint violations could not be found in {jshint_report}".format( - jshint_report=jshint_report - ) - ) - - # Record the metric - _write_metric(num_violations, (Env.METRICS_DIR / "jshint")) - - # Fail if number of violations is greater than the limit - if num_violations > violations_limit > -1: - raise BuildFailure( - "JSHint Failed. Too many violations ({count}).\nThe limit is {violations_limit}.".format( - count=num_violations, violations_limit=violations_limit - ) - ) - - @task @needs('pavelib.prereqs.install_node_prereqs') @cmdopts([ @@ -713,10 +668,6 @@ 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) @@ -736,18 +687,6 @@ 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", - prefix=pythonpath_prefix, - reports=jshint_reports, - percentage_string=percentage_string, - branch_string=compare_branch_string, - dquality_dir=dquality_dir - ): - diff_quality_percentage_pass = False - # run diff-quality for eslint. if not run_diff_quality( violations_type="eslint", diff --git a/scripts/all-tests.sh b/scripts/all-tests.sh index 68cfe355dd..03284fbecc 100755 --- a/scripts/all-tests.sh +++ b/scripts/all-tests.sh @@ -12,7 +12,6 @@ set -e # Violations thresholds for failing the build export PYLINT_THRESHOLD=4175 -export JSHINT_THRESHOLD=7550 # @TODO Remove, deprecated in favor of ESLint export ESLINT_THRESHOLD=49019 SAFELINT_THRESHOLDS=`cat scripts/safelint_thresholds.json` diff --git a/scripts/circle-ci-tests.sh b/scripts/circle-ci-tests.sh index e9738a4455..10a6e2502a 100755 --- a/scripts/circle-ci-tests.sh +++ b/scripts/circle-ci-tests.sh @@ -62,10 +62,6 @@ else mkdir -p reports 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; } diff --git a/scripts/generic-ci-tests.sh b/scripts/generic-ci-tests.sh index 24a289da37..1d5fc916ce 100755 --- a/scripts/generic-ci-tests.sh +++ b/scripts/generic-ci-tests.sh @@ -81,13 +81,8 @@ case "$TEST_SUITE" in mkdir -p reports - # @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." From 9e67f15306ca47b3fd23d0eaeef75693284ff682 Mon Sep 17 00:00:00 2001 From: Brian Jacobel Date: Tue, 2 Aug 2016 13:59:30 -0400 Subject: [PATCH 3/3] Update docs --- docs/en_us/internal/testing.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/en_us/internal/testing.rst b/docs/en_us/internal/testing.rst index 51e2783c29..b92fe01d78 100644 --- a/docs/en_us/internal/testing.rst +++ b/docs/en_us/internal/testing.rst @@ -802,13 +802,13 @@ To view JavaScript code style quality run this command. :: - paver run_jshint + paver run_eslint - This command also comes with a ``--limit`` switch, this is an example of that switch. :: - paver run_jshint --limit=700 + paver run_eslint --limit=50000 @@ -829,7 +829,7 @@ Two tools are available for evaluating complexity of edx-platform code: :: - plato -q -x common/static/js/vendor/ -t common -l .jshintrc -r -d jscomplexity common/static/js/ + plato -q -x common/static/js/vendor/ -t common -e .eslintrc.json -r -d jscomplexity common/static/js/