Closed Bug 1229859 Opened 9 years ago Closed 8 years ago

Make devtools pass mach eslint

Categories

(DevTools :: General, defect)

Product:
DevTools
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mossop, Unassigned)

References

Details

Attachments

(3 files, 4 obsolete files)

MozReview Request: Bug 1229859 - Massively reduce the number of eslint errors in devtools by ignoring lib files, adding missing .eslintrc files and making some rules be warnings; r=Mossop
40 bytes, text/x-review-board-request
pbro
: review+
Details
MozReview Request: Bug 1229859 - Introduce new import-globals-from eslint rule to import globals from other modules; r?Mossop
40 bytes, text/x-review-board-request
mossop
: review+
Details
MozReview Request: Bug 1229859 - Use import-globals-from eslint rule to fix the most common no-undef offenders in devtools; r=Mossop
40 bytes, text/x-review-board-request
pbro
: review+
Details 
As we get automation going it will be important to have the tree pass mach eslint. At the moment devtools fails a lot so we either need to reduce their ruleset or add more ignores.
Blocks: 1230070
The general consensus in the team was (I think) to disable rules to get eslint passing, and then re-enable them one by one.
I see that this bug blocks the meta bug 1229856. Shouldn't it block a more specific automation bug instead? Maybe bug 1229588?

I guess we need to do this only once we have a concrete plan to automate eslint. Once we do do it, we'll need a tracking bug to re-enable the rules: bug 1230070.
I was running eslint on the whole devtools directory and realized that one of our custom rules was broken: import-headjs-globals. This rule isn't really a rule as it doesn't report any errors, instead if looks at the head.js file in the same dir and import globals from there in scope.

So, with bug 1230093 and bug 1230095 fixed, we should have far less eslint errors.
Depends on: 1230093, 1230095
The other quick thing we can fix before disabling rules is that devtools/client/aboutdebugging/test doesn't have a proper .eslintrc file (which should inherit from devtools/.eslintrc.mochitests in order to have the proper test globals defined).
Ah, in fact there are other missing test .eslintrc files:
- devtools/shared/worker/tests/browser/
- devtools/shared/tests/browser/
and in other places for xpcshell tests.
I'll submit a new patch to add those.

I think we have about 32000 errors in the devtools code with those fixed.
Among these errors, I believe some could just be turned to warning instead of disabled entirely:

- indent: we have about 6000 of them, this makes no sense to me as an error
- comma-spacing: this is also a formatting-only issue, and we have 2000 of them
- brace-style: we have 2700 of them, this might also be a good one to change to a warning instead, at least for now
- camelcase: same, about 2700 of them
- ...

With this fixed locally, I was able to go down to about 18000 errors.

Another candidate for a quick fix is updating the .eslintignore which isn't correct today. We're not ignoring tern from the right folder.
Comment on attachment 8695220 [details] [diff] [review]
Bug_1229859_-_Add_the_expected_mochitest_.eslintrc.diff

Going to provide a bigger patch that includes this one.
Attachment #8695220 - Attachment is obsolete: true
Attachment #8695220 - Flags: review?(janx)
So I'm down to ~9000 errors now which is still a lot, but considering I had 32000 1 hour ago, it's not so bad. These were only low-hanging fruits however. The next ones are going to be harder.

So, as said previously, we might decide to disable the remaining failing rules for the sake of passing when we have eslint automated, and then re-enabling them one by one in bug 1230070.

I went quickly over the remaining failures are the large majority is from the no-undef rule. Here's what's happening:

We have many files that just assume global variables to be available. Let's take the xample of devtools/client/canvasdebugger/callslist.js
It uses, among other things, the following global vars: Heritage, WidgetMethods, SideMenuWidget, $, document, etc.
These things are deinfed in devtools/cient/canvasdebugger.js, and both these files are loaded via <script> tags in devtools/client/canvasdebugger/canvasdebugger.xul
So when the tool loads (in an iframe), the globals defined by canvasdebugger.js get added to the window and any other script loaded in the window after this can safely use them.

Only, ESLint doesn't know about this and therefore complains about the undefined variables.

The hard thing here is that *not* all our files are loaded this way, many are loaded via a module loader and so they specifically define the imports they need, no problem there. But for many of our tools, we at least have a couple of files that don't do this.

One solution is to manually maintain the list of globals in a comment inside the file:
/* globals: Heritage, WidgetMethods, SideMenuWidget, $, document */
But that's manual hence hard to maintain.

Maybe we could write a custom eslint rule just for these files. Maybe this rule could use a comment like:
/* use-globals-from: canvasdebugger.js */
if it sees this comment, then it loads that file, and adds all globals from there to the current scope.
Bug 1229859 - Massively reduce the number of eslint errors in devtools by ignoring lib files, adding missing .eslintrc files and making some rules be warnings; r?Mossop
Attachment #8695244 - Flags: review?(dtownsend)
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #8)
> Created attachment 8695244 [details]
> MozReview Request: Bug 1229859 - Massively reduce the number of eslint
> errors in devtools by ignoring lib files, adding missing .eslintrc files and
> making some rules be warnings; r?Mossop
> 
> Bug 1229859 - Massively reduce the number of eslint errors in devtools by
> ignoring lib files, adding missing .eslintrc files and making some rules be
> warnings; r?Mossop
Damn, I just realized I have committed an unrelated change to import-headjs-globals.js (I had done that change locally just so the rule would work correctly when ran from mach (see bug 1230095)). Please ignore that change.
https://reviewboard.mozilla.org/r/27031/#review24425

::: testing/eslint-plugin-mozilla/lib/rules/import-headjs-globals.js:94
(Diff revision 1)
> -      var testPath = this.getFilename();
> +      var fullTestPath = this.getFilename();

Please ignore this change. I will get it removed. This is unrelated.
No longer depends on: 1230095
Comment on attachment 8695244 [details]
MozReview Request: Bug 1229859 - Massively reduce the number of eslint errors in devtools by ignoring lib files, adding missing .eslintrc files and making some rules be warnings; r?Mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27031/diff/1-2/
Bug 1229859 - Introduce new import-globals-from eslint rule to import globals from other modules; r?Mossop
Attachment #8695290 - Flags: review?(dtownsend)
Bug 1229859 - Use import-globals-from eslint rule to fix the most common no-undef offenders in devtools; r?Mossop
Attachment #8695291 - Flags: review?(dtownsend)
The last 2 commits remove another 500 to 1000 errors thanks to the new rule I described in comment 7.
The goal was not to enable this rule *everywhere* yet but to fix the most common offenders.
There are some more tricky cases in devtools/client/debugger to deal with, and also, the rule should be made better incrementally (like add global variables from Object.define, Cu.import and lazyLoaders, ...)
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #5)
> Ah, in fact there are other missing test .eslintrc files:
> - devtools/shared/worker/tests/browser/
> - devtools/shared/tests/browser/
> and in other places for xpcshell tests.

I ran across this today and made a list.
See bug 1230195.
Blocks: 1230195
No longer blocks: 1230195
Comment on attachment 8695244 [details]
MozReview Request: Bug 1229859 - Massively reduce the number of eslint errors in devtools by ignoring lib files, adding missing .eslintrc files and making some rules be warnings; r?Mossop

https://reviewboard.mozilla.org/r/27031/#review24473

::: devtools/.eslintrc:39
(Diff revision 2)
> -    "brace-style": [2, "1tbs", {"allowSingleLine": false}],
> +    "brace-style": [1, "1tbs", {"allowSingleLine": false}],

You are of course welcome to do whatever you like here however I'll note that in browser and toolkit we're probably not going to do anything as warnings. If you don't care about the rule being followed then there isn't much point spamming developers with it.
Attachment #8695244 - Flags: review?(dtownsend) → review+
Attachment #8695291 - Flags: review?(dtownsend) → review+
Comment on attachment 8695291 [details]
MozReview Request: Bug 1229859 - Use import-globals-from eslint rule to fix the most common no-undef offenders in devtools; r?Mossop

https://reviewboard.mozilla.org/r/27053/#review24475
Comment on attachment 8695290 [details]
MozReview Request: Bug 1229859 - Introduce new import-globals-from eslint rule to import globals from other modules; r?Mossop

https://reviewboard.mozilla.org/r/27051/#review24477

::: testing/eslint-plugin-mozilla/docs/import-globals-from.rst:10
(Diff revision 1)
> +When the "import-globals-from: <path>" comment is found in a file, then all

This comment suggests you need a : for this to work

::: testing/eslint-plugin-mozilla/docs/index.rst:13
(Diff revision 1)
> +``import-globals-from`` When the "import-globals-from: <path>" comment is found

This comment suggests you need a : for it to work

::: testing/eslint-plugin-mozilla/lib/rules/import-globals-from.js:43
(Diff revision 1)
> +  }

This should be removed

::: testing/eslint-plugin-mozilla/lib/rules/import-globals-from.js:55
(Diff revision 1)
> +        var match = /^import-globals-from[\s:]*(.*)$/.exec(value);

This rule matches "import-globals-fromfoobar" which it probably shouldn't.

I'd just do /^import-globals-from\s+(.*)$/
Attachment #8695290 - Flags: review?(dtownsend)
(In reply to Dave Townsend [:mossop] from comment #17)
> Comment on attachment 8695244 [details]
> MozReview Request: Bug 1229859 - Massively reduce the number of eslint
> errors in devtools by ignoring lib files, adding missing .eslintrc files and
> making some rules be warnings; r?Mossop
> 
> https://reviewboard.mozilla.org/r/27031/#review24473
> 
> ::: devtools/.eslintrc:39
> (Diff revision 2)
> > -    "brace-style": [2, "1tbs", {"allowSingleLine": false}],
> > +    "brace-style": [1, "1tbs", {"allowSingleLine": false}],
> 
> You are of course welcome to do whatever you like here however I'll note
> that in browser and toolkit we're probably not going to do anything as
> warnings. If you don't care about the rule being followed then there isn't
> much point spamming developers with it.
Yup, I agree.
I'd say let's keep 1 for rules we want to keep as errors in the future but are temporarily disabling for the sake of automating.
Bug 1229859 - Massively reduce the number of eslint errors in devtools by ignoring lib files, adding missing .eslintrc files and making some rules be warnings; r=Mossop
Attachment #8695811 - Flags: review?(dtownsend)
Bug 1229859 - Introduce new import-globals-from eslint rule to import globals from other modules; r?Mossop
Attachment #8695812 - Flags: review?(dtownsend)
Bug 1229859 - Use import-globals-from eslint rule to fix the most common no-undef offenders in devtools; r=Mossop
Attachment #8695813 - Flags: review?(dtownsend)
Attachment #8695811 - Flags: review+
Comment on attachment 8695811 [details]
MozReview Request: Bug 1229859 - Massively reduce the number of eslint errors in devtools by ignoring lib files, adding missing .eslintrc files and making some rules be warnings; r=Mossop

https://reviewboard.mozilla.org/r/27195/#review24587

This was already R+
Attachment #8695813 - Flags: review+
Comment on attachment 8695813 [details]
MozReview Request: Bug 1229859 - Use import-globals-from eslint rule to fix the most common no-undef offenders in devtools; r=Mossop

https://reviewboard.mozilla.org/r/27199/#review24589

This was already R+
Attachment #8695244 - Attachment is obsolete: true
Attachment #8695290 - Attachment is obsolete: true
Attachment #8695291 - Attachment is obsolete: true
Attachment #8695811 - Flags: review?(dtownsend)
Comment on attachment 8695813 [details]
MozReview Request: Bug 1229859 - Use import-globals-from eslint rule to fix the most common no-undef offenders in devtools; r=Mossop

Really sorry about the noise on this bug.
I pushed to mozreview from a different computer, and it created a new review entirely, didn't update the commits in the same review.
Also, even if setting "r=Mossop" in the message, it didn't mark those 2 commits as R+. I guess I should have pushed only the one changeset that needed re-review only.
Attachment #8695813 - Flags: review?(dtownsend)
Comment on attachment 8695812 [details]
MozReview Request: Bug 1229859 - Introduce new import-globals-from eslint rule to import globals from other modules; r?Mossop

https://reviewboard.mozilla.org/r/27197/#review24805
Attachment #8695812 - Flags: review?(dtownsend) → review+
Keywords: leave-open
The next step for this bug would be to add devtools files to .eslintignore so that `./mach eslint` at root level passes.
Then we can re-enable eslint file by file, which is an easier unit to deal with than rule by rule on the whole devtools code base.
Also re-enabling eslint for a single file is really nice good-first-bug.
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #31)
> The next step for this bug would be to add devtools files to .eslintignore
> so that `./mach eslint` at root level passes.
> Then we can re-enable eslint file by file, which is an easier unit to deal
> with than rule by rule on the whole devtools code base.
> Also re-enabling eslint for a single file is really nice good-first-bug.

Bug 1231956 and bug 1231957 logged.
Depends on: 1231956
We discussed this on irc and agreed that this bug is fixed, and that
bug 1231957 is the new meta-bug for fixing eslint warnings in devtools.
Blocks: devtools-eslint
Keywords: leave-open
Actually close it.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: