[meta] Enable eslint on more test files

NEW
Unassigned

Status

enhancement
P2
normal
2 years ago
20 days ago

People

(Reporter: gbrown, Unassigned)

Tracking

(Depends on 5 bugs, Blocks 1 bug, {meta})

Firefox Tracking Flags

(Not tracked)

Details

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
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.
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!
Depends on: 1359541
(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
Depends on: 1361859
Depends on: 1367235
Depends on: 1367780
Depends on: 1375678
Depends on: 1375903
Priority: -- → P2
Depends on: 1403956
Depends on: 1403959
Depends on: 1403961

Updated

2 years ago
Depends on: devtools-eslint
Assignee: gbrown → nobody
Depends on: 1417383
Depends on: 1421458
Depends on: 1423841
Depends on: 1423839
Depends on: 1423843
Depends on: 1423844

Updated

a year ago
Product: Testing → Firefox Build System
Depends on: 1457835
Depends on: 1475004
Depends on: 1489980
Depends on: 1496082
Depends on: 1497604
Depends on: 1501445
Depends on: 1501621
Depends on: 1501662
Depends on: 1501931
Depends on: 1501932
Depends on: 1504323
Depends on: 1506042
Depends on: 1506559
Depends on: 1508817
Depends on: 1508819
Depends on: 1508823
Depends on: 1508825
Depends on: 1508980
Depends on: 1508983
Depends on: 1508984
Depends on: 1508985
Depends on: 1508987
Depends on: 1508988
Depends on: 1508989
Depends on: 1508990
Depends on: 1508991
Depends on: 1508992
Depends on: 1509042
Depends on: 1515605
Depends on: 1515942
Depends on: 1515949
Depends on: 1521928
Depends on: 1532933
Depends on: 1532934
Depends on: 1532935
Depends on: 1532937
Depends on: 1532941
Depends on: 1541888
You need to log in before you can comment on or make changes to this bug.