1357557 - [meta] Enable eslint on more test files

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement, P2)

Description

[Geoff Brown [:gbrown]]

Linting of javascript files in the tree is provided by eslint, and that mechanism is working at this time. However, many test files are excluded from linting:

https://hg.mozilla.org/mozilla-central/file/tip/.eslintignore

Some existing eslint rules may catch risky or dangerous coding practices which lead to intermittent test failures. We may also be able to enable additional eslint rules or add new ones...but probably not until most tests are linted and error free.


The motivation for this work is similar to bug 1357513, but I think the implementation will be independent.

https://developer.mozilla.org/en-US/docs/ESLint
http://gecko.readthedocs.io/en/latest/tools/lint/linters/eslint-plugin-mozilla.html

Comment 1

[Mark Banner (:standard8)]

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.

Comment 2

[Geoff Brown [:gbrown]]

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?

Comment 3

[Mark Banner (:standard8)]

(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.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

I would like to see "use strict" in all of our test files!

Comment 5

[Geoff Brown [:gbrown]]

(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!

Comment 6

[Mark Banner (:standard8)]

We're done: https://www.thebanners.uk/standard8/2020/02/14/eslint-now-turned-on-for-all-of-the-firefox-gecko-codebase/

There's still more rules to enable in places, however bug 1596191 will handle those.