[meta] Enable eslint on more test files

NEW
Unassigned

Status

Testing
Lint
P2
normal
8 months ago
9 days ago

People

(Reporter: gbrown, Unassigned)

Tracking

(Depends on: 6 bugs, Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

8 months ago
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
(Reporter)

Updated

8 months ago
Component: General → Lint
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.
(Reporter)

Comment 2

8 months ago
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?
Flags: needinfo?(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.
Flags: needinfo?(standard8)
I would like to see "use strict" in all of our test files!
(Reporter)

Updated

8 months ago
Depends on: 1359541
(Reporter)

Comment 5

8 months ago
(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
(Reporter)

Updated

8 months ago
Depends on: 1361859
(Reporter)

Updated

7 months ago
Depends on: 1367235
(Reporter)

Updated

7 months ago
Depends on: 1367780
(Reporter)

Updated

6 months ago
Depends on: 1375678
(Reporter)

Updated

6 months ago
Depends on: 1375903
Priority: -- → P2
Depends on: 1403956
Depends on: 1403959
Depends on: 1403961

Updated

a month ago
Depends on: 1231957
(Reporter)

Updated

a month ago
Assignee: gbrown → nobody
Depends on: 1417383
Depends on: 1421458
Depends on: 1423841
Depends on: 1423839
Depends on: 1423843
Depends on: 1423844
You need to log in before you can comment on or make changes to this bug.