Enable ESLint for chrome/

RESOLVED FIXED in Firefox 58

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: standard8, Assigned: mithilan91, Mentored)

Tracking

(Blocks 1 bug, {good-first-bug})

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

2 years ago
As part of rolling out ESLint across the tree, we should enable it for the chrome/ directory.

I'm happy to mentor this bug. There's background on our eslint setups here:

https://developer.mozilla.org/docs/ESLint

Here's some approximate steps:

- In .eslintignore, remove the `chrome/**` line.
- Run eslint:

./mach eslint --fix chrome

This should fix most/all of the issues.

- Inspect the diff to make sure that the indentation of the lines surrounding the changes look ok.

- Create a commit of the work so far.

- For the remaining issues, you'll need to fix them by hand. I think most of these will be no-undef, there are hints on how to fix those here:

https://developer.mozilla.org/en-US/docs/ESLint#no-undef

(you can just run `./mach eslint chrome` to check you've fixed them).

- Create a second commit of the manual changes and push it to mozreview:

http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview.html

If you're not sure of how to fix something, please look at the rules documentation http://eslint.org/docs/rules/ or just ask here.
Reporter

Updated

2 years ago
Keywords: good-first-bug
Assignee

Comment 1

2 years ago
Hey! I'am a new contributor and I've been looking for a good first bug. This issue looks like a good place to get started. Can I be assigned this bug?
Reporter

Comment 2

2 years ago
Thank you for picking it up, I've assigned it to you.
Assignee: nobody → mithilan91
Assignee

Comment 3

2 years ago
Hi!
I just pushed the changes to mozreview! It took me a while completely understand what was going on.I was a little overwhelmed when I saw there were 163 errors once I enabled eslint for the chrome directory but it wasn't difficult at all once I got down to it.
Assignee

Comment 4

2 years ago
I just realized that the push to mozreview was aborted because I "cannot submit reviews referencing multiple bugs'. Do you know why I am getting this error?
Comment hidden (mozreview-request)
Assignee

Comment 7

2 years ago
Never mind I figured it out,I needed to reference my Bug Number in my commit messages! On MozReview it says that my request lacks reviewers because I wasn't sure if I was supposed to specify Mark as a reviewer when making the push.
Flags: needinfo?(standard8)
Reporter

Comment 8

2 years ago
(In reply to Mithilan Sivanesan from comment #3)
> I just pushed the changes to mozreview! It took me a while completely
> understand what was going on.I was a little overwhelmed when I saw there
> were 163 errors once I enabled eslint for the chrome directory but it wasn't
> difficult at all once I got down to it.

Thanks, I'll take a look in a minute. Did you use --fix? That should have resolved most of those errors automatically, unless I'd misjudged it.
Flags: needinfo?(standard8)
Reporter

Comment 9

2 years ago
mozreview-review
Comment on attachment 8921337 [details]
Bug 1403956 - Enable ESLint from chrome/ directory.

https://reviewboard.mozilla.org/r/192352/#review197618

::: commit-message-19b32:1
(Diff revision 1)
> +Bug 1403956 removed chrome directory from .eslintignore file

This changeset can be merged into the main one, no need to keep these ones separate.
Reporter

Comment 10

2 years ago
mozreview-review
Comment on attachment 8921338 [details]
Bug 1403956 resolved all no-undef, no-redeclare and no-unused-vars eslint errors

https://reviewboard.mozilla.org/r/192354/#review197622

Thank you very much for working on the patch. There are a few issues that were my fault as I'd forgotten to mention what to do when I first submitted the bug. It shouldn't be hard to change them though, and most of the hard work appears to be done already.

I'm ok to review this, since it appears to be classed as part of toolkit and is test-only anyway.

Next time you push a patch, just add " r?Standard" in the commit message and it will request review from me automatically.

::: commit-message-3f306:1
(Diff revision 1)
> +Bug 1403956 resolved all no-undef, no-redeclare and no-unused-vars eslint errors

For the commit message, I'd go with something like:

Bug 1403956 - Enable ESLint from chrome/ directory. r?Standard8

That's descriptive enough to tell people what is going on in the patch and where it is targeted to apply to.

::: .eslintrc.js:10
(Diff revision 1)
>    // tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js to
>    // allow external repositories that use the plugin to pick them up as well.
>    "extends": [
> -    "plugin:mozilla/recommended"
> +    "plugin:mozilla/recommended",
> +    "plugin:mozilla/chrome-test",
> +    "plugin:mozilla/xpcshell-test"

Sorry, I forgot to mention this when I filed the bug - you should only need the xpcshell-test entry, but it should also go into a new .eslintrc.js file in the chrome/test/ sub-directory, here's an existing example:

http://searchfox.org/mozilla-central/source/browser/components/places/tests/unit/.eslintrc.js

::: .eslintrc.js:23
(Diff revision 1)
>    // turn off processing of the html plugin for .xml files.
>    "settings": {
>      "html/xml-extensions": [ ".xhtml" ]
>    },
> +  "rules": {
> +        "no-native-reassign": ["error", {"exceptions": ["run_test"]}]

This shouldn't be necessary. I suspect if you relocate the extends definitions I suggested it will go away.

::: browser/components/about/test/unit/test_getURIFlags.js:10
(Diff revision 1)
>  const contract = "@mozilla.org/network/protocol/about;1?what=newtab";
>  const am = Cc[contract].getService(Ci.nsIAboutModule);
>  const uri = Services.io.newURI("about:newtab");
>  
> +/*eslint no-native-reassign: "error"*/
> +/*eslint-env browser*/

As above, neither of these comments should need to be here, it should be getting everything inherited in the right places.

::: chrome/test/unit/test_bug564667.js:50
(Diff revision 1)
>   * the manifest files
>   *
>   * @param type The type of overlay: overlay|style
>   */
>  function test_no_overlays(chromeURL, target, type) {
> -  var type = type || "overlay";
> +  type = type || "overlay";

We can improve on this here. You can drop this `type = type || "overlay";` line and change the function definition to:

function test_no_overlays(chromeURL, target, type = "overlay") {

::: chrome/test/unit/test_data_protocol_registration.js:15
(Diff revision 1)
>  
> -function run_test()
> +function run_test() {
> -{
>    const uuidGenerator = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
>  
>    Components.utils.import("resource://testing-common/AppInfo.jsm", this);

Rather than adding the `globals newAppInfo` line above, please can you change the import line to:

let newAppInfo = Components.utils.import("resource://testing-common/AppInfo.jsm", {}).newAppInfo;

That's then an explicit description of what is being imported.

Please also do this in the other files where you're currently specifying newAppInfo as a global.

::: chrome/test/unit_ipc/test_resolve_uris_ipc.js:4
(Diff revision 1)
>  //
>  // Run test script in content process instead of chrome (xpcshell's default)
>  //
> -
> +/* globals do_run_test */

Can you move the comment to just inside the run_test() please - just before the load() line. That is where the global is imported from in this case, so I prefer to keep the comment & loading line close together.
Assignee

Comment 11

2 years ago
Once the issues are resolved, Do I make another commit and push to MozReview again?
Reporter

Comment 12

2 years ago
(In reply to Mithilan Sivanesan from comment #11)
> Once the issues are resolved, Do I make another commit and push to MozReview
> again?

Please fold them all into one commit with the commit message I suggested. Then push to MozReview again :-)
Assignee

Comment 13

2 years ago
I've resolved all the errors except one. After I've relocated the extends definition to a new .eslintrc.js file in the chrome/test subdirectory, I'am still getting a no-native-reassign error for the run_test() function. Adding a rule exception resolves the error but I'am not sure if this is the best way to go about it.Any ideas?
Reporter

Comment 14

2 years ago
(In reply to Mithilan Sivanesan from comment #13)
> I've resolved all the errors except one. After I've relocated the extends
> definition to a new .eslintrc.js file in the chrome/test subdirectory, I'am
> still getting a no-native-reassign error for the run_test() function. Adding
> a rule exception resolves the error but I'am not sure if this is the best
> way to go about it.Any ideas?

I just took a look, it seems this is a one-off special case due to how chrome/test/unit_ipc/test_resolve_uris_ipc.js works.

I think you can just add:

// eslint-disable-next-line no-native-reassign

just above where it assigns to run_test in test_resolve_uris.js.
Comment hidden (mozreview-request)
Reporter

Comment 16

2 years ago
mozreview-review
Comment on attachment 8921910 [details]
Bug 1403956 - Enable ESLint from chrome/ directory.

https://reviewboard.mozilla.org/r/192942/#review198496

Thank you for the new update. For some reason, it is showing as multiple patches still. You might need to check what you're pushing with `hg outgoing`. If you need to merge the commits, there's some hints here for how to do it: https://stackoverflow.com/posts/1559922/revisions

::: browser/.eslintrc.js:5
(Diff revision 1)
>  "use strict";
>  
>  module.exports = {
> +  "extends": [
> +    "plugin:mozilla/xpcshell-test",

This change shouldn't be here. I'm also not seeing the new chrome/test/.eslintrc.js file, did you forget to `hg add` it?
Attachment #8921910 - Flags: review?(standard8)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8921338 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8921910 - Attachment is obsolete: true
Reporter

Comment 18

2 years ago
mozreview-review
Comment on attachment 8921337 [details]
Bug 1403956 - Enable ESLint from chrome/ directory.

https://reviewboard.mozilla.org/r/192352/#review198728

Thank you, this is much better.

There's a few small tweaks just to finish this off.

Could you also rebase on top of the latest mozilla-central? There's been a small change to .eslintignore and we may or may not be able to get mozreview to land the patch without updating - so might as well rebase to be safe and avoid another update cycle.

Just rebase & add the changes and keep the one changeset please.

::: .eslintrc.js:8
(Diff revision 2)
>  module.exports = {
>    // New rules and configurations should generally be added in
>    // tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js to
>    // allow external repositories that use the plugin to pick them up as well.
>    "extends": [
> -    "plugin:mozilla/recommended"
> +    "plugin:mozilla/recommended",

Can you drop the comma you added to this line please? We don't need it for this change and prefer to keep the blame minimal.

::: .eslintrc.js:13
(Diff revision 2)
> -    "plugin:mozilla/recommended"
> +    "plugin:mozilla/recommended",
>    ],
>    "plugins": [
>      "mozilla"
>    ],
> +

Also undo the addition of this line.

::: browser/components/about/test/unit/.eslintrc.js:5
(Diff revision 2)
>  "use strict";
>  
>  module.exports = {
>    "extends": [
> -    "plugin:mozilla/browser-test"
> +    "plugin:mozilla/browser-test",

Please also drop the comma here.
Attachment #8921337 - Flags: review?(standard8)
Assignee

Comment 19

2 years ago
Will do! I really appreciate your patience and help through all this :-)
Comment hidden (mozreview-request)
Reporter

Comment 21

2 years ago
mozreview-review
Comment on attachment 8921337 [details]
Bug 1403956 - Enable ESLint from chrome/ directory.

https://reviewboard.mozilla.org/r/192352/#review198964

Great, many thanks for the updates. This looks good to go.
Attachment #8921337 - Flags: review?(standard8) → review+

Comment 22

2 years ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3c2982568a78
Enable ESLint from chrome/ directory. r=standard8
Reporter

Comment 23

2 years ago
(In reply to Pulsebot from comment #22)
> Pushed by mbanner@mozilla.com:
> https://hg.mozilla.org/integration/autoland/rev/3c2982568a78
> Enable ESLint from chrome/ directory. r=standard8

Mithilan: as you'll see from above, this has been landed on our integration branch. If the tests all pass (which they should do), then this will probably be merged to mozilla-central (the main branch) sometime in the next 24 hours, at which point the bug will be marked as fixed.

Thank you for you work on this.
https://hg.mozilla.org/mozilla-central/rev/3c2982568a78
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.