Closed Bug 1357557 Opened 4 years ago Closed 7 months ago
[meta] Enable eslint on more test files
Note: there is currently bug 1311338 and I also know DevTools has various bugs already filed to enabling ESLint in more places. I'd recommend treating this as a meta & splitting up the work. There may also be a two-step process (maybe two bugs per dir): 1) remove from the .eslintignore, 2) set "plugin:mozilla/recommended" as the default configuration. There will be some files we'll always whitelist (e.g. vendor, imported, generated). I'm quite happy to discuss more if you want.
Thanks :standard8. I do have a few questions... - It looks like it would be easy to enable eslinting of many more files: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e859bebe929ea9ce25637a1234a5f7b0b5043981. Other than ensuring a error-free run, is there more to do, do I need to be more cautious? - I see some "plugin:mozilla/xpcshell-test", "plugin:mozilla/mochitest-test", etc. Are these more appropriate for test directories than "recommended"? - I have seen a few cases like this: $ ./mach eslint testing/talos An error occurred running eslint. Please check the following error messages: getASTSource unsupported computed MemberExpression Error: getASTSource unsupported computed MemberExpression at Object.getASTSource (/home/gbrown/src/node_modules/eslint-plugin-mozilla/lib/helpers.js:87:17) at EventEmitter.CallExpression (/home/gbrown/src/node_modules/eslint-plugin-mozilla/lib/rules/avoid-removeChild.js:37:19) at emitOne (events.js:101:20) at EventEmitter.emit (events.js:188:7) at NodeEventGenerator.applySelector (/home/gbrown/src/node_modules/eslint/lib/util/node-event-generator.js:265:26) at NodeEventGenerator.applySelectors (/home/gbrown/src/node_modules/eslint/lib/util/node-event-generator.js:294:22) at NodeEventGenerator.enterNode (/home/gbrown/src/node_modules/eslint/lib/util/node-event-generator.js:308:14) at CodePathAnalyzer.enterNode (/home/gbrown/src/node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:602:23) at CommentEventGenerator.enterNode (/home/gbrown/src/node_modules/eslint/lib/util/comment-event-generator.js:98:23) at Traverser.enter (/home/gbrown/src/node_modules/eslint/lib/eslint.js:929:36) A failure occured in the eslint linter. ✖ 1 problem (0 errors, 0 warnings, 1 failure) Is there an easy way to determine what file/line is to blame?
(In reply to Geoff Brown [:gbrown] from comment #2) > Thanks :standard8. I do have a few questions... > > - It looks like it would be easy to enable eslinting of many more files: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=e859bebe929ea9ce25637a1234a5f7b0b5043981. Other than > ensuring a error-free run, is there more to do, do I need to be more > cautious? That's generally fine. We might want to make some of the owners / developers aware that we're enabling it for more directories. > - I see some "plugin:mozilla/xpcshell-test", > "plugin:mozilla/mochitest-test", etc. Are these more appropriate for test > directories than "recommended"? Ideally we should be specifying recommended in the top-level .eslintrc.js, and then all directories inherit from that. Due to historic reasons, we're currently only specifying it in the browser/ and toolkit/ directories (maybe one or two other places). I'm currently organising & mentoring bugs to move towards that direction. The various -test configs are designed to inherit from the recommended one. They typically specify additional globals that are available to tests and have a few more rules to help discover globals from other files. Hence, test directories do need the -test configurations, but somewhere in the tree above them we should also be specifying recommended as that's the bit that enables most of the rules. > $ ./mach eslint testing/talos > An error occurred running eslint. Please check the following error messages: > > getASTSource unsupported computed MemberExpression ... > Is there an easy way to determine what file/line is to blame? The only way currently is to go through the sub-directories, and then individual files to find out what is to blame. However, this should be simpler, so I've filed bug 1358418 to get this fixed with at least the filename that's failing (I have a patch already). In the testing/talos case it is testing/talos/talos/tests/canvasmark/scripts/jquery-1.4.2.min.js - which we can keep ignoring as it is a vendor file.
I would like to see "use strict" in all of our test files!
(In reply to Joel Maher ( :jmaher) from comment #4) > I would like to see "use strict" in all of our test files! eslint has a rule for that, but it looks like we don't currently enforce it...maybe one day!
Summary: Enable eslint on more test files → [meta] Enable eslint on more test files
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.