1423843 - Enable ESLint for dom/notification/test/unit/

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, enhancement, P3)

Description

[Mark Banner (:standard8)]

As part of rolling out ESLint across the tree, we should enable it for dom/notification/test/unit/

There's background on our eslint setups here:

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

Here's some approximate steps:

- In .eslintignore, remove the `dom/notification/test/unit/**` line.
- Run eslint:

./mach eslint --fix dom/notification/test/unit/

This should fix some 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. The best way to fix those will be to do a repository move for common_test_notificationdb.js to head_notificationdb.js, and change the name in the xpcshell.ini file.

This will allow ESLint to automatically pick up the symbols that the file is declaring and use them in the test files.

(you can just run `./mach eslint dom/notification/test/unit/` 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.

Comment 1

[Nicola Webb]

Attachment: Bug 1423843 - Enable ESLint for dom/notification/test/unit/

Review commit: https://reviewboard.mozilla.org/r/209228/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209228/

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 8938586 [details]
Bug 1423843 - Enable ESLint for dom/notification/test/unit/

https://reviewboard.mozilla.org/r/209228/#review215108

Thank you for the patch, it is a great start, and fixes the ESLint issues!

One minor thing, is that I'm not seeing the move of dom/notification/test/unit/common_test_notificationdb.js to dom/notification/test/unit/head_notificationdb.js in the patch. Did you `hg mv dom/notification/test/unit/common_test_notificationdb.js dom/notification/test/unit/head_notificationdb.js` or something like that?

There's also a couple more comments to address as well.

Once you've addressed them and push the new patch, if you can also come into mozreview and mark them as fixed, that'd be useful in tidying up.

Also, please can you add mccr8 to the review request as well please, as its his area (change `r?standard8` to `r?standard8,r?mccr8`).

Any questions, please ask.

Thanks

::: .eslintignore
(Diff revision 1)
>  # generated or library files in pocket
>  browser/extensions/pocket/content/panels/js/tmpl.js
>  browser/extensions/pocket/content/panels/js/vendor/**
>  # generated or library files in activity-stream
>  browser/extensions/activity-stream/data/content/activity-stream.bundle.js
> -browser/extensions/activity-stream/test/**

Please undo this change, we still need it (I suspect it is a merge/rebase that went bad).

::: .eslintignore:411
(Diff revision 1)
>  toolkit/mozapps/update/tests/data/xpcshellConstantsPP.js
>  
>  # Third party
>  toolkit/modules/third_party/**
>  third_party/**
> +

All the files seemed to have gained an extra newline at the end. This is unusual, maybe your editor is misconfigured or something?

Comment 3

[Matthew N. [:MattN]]

Hi Nicola, are you planning to finish this?

Comment 4

[Mark Banner (:standard8)]

Opening up as mentored bug.

As there's a few things that have changed, I'd suggest following comment 0 from scratch against the latest tree, rather than trying to reuse the previous patch.

However, note that the reviewers should be ":standard8" and ":mccr8".

Also note that we now use phabricator rather than mozreview, see here for info: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

Comment 5

[akshitsoni0000]

Hey, I am beginner. Can I take this up?

Comment 6

[Mark Banner (:standard8)]

@akshitsoni0000, sure please do. We'll assign once the first patch is posted.

Comment 7

[Divyansh Sharma [:spiro]]

Attachment: Bug 1423843 - Enabled ESLint for dom/notification/test/unit

Comment 8

[Divyansh Sharma [:spiro]]

Mark

I have submitted both commits. Please look into them.

Thanks

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dd0480856174
Enabled ESLint for dom/notification/test/unit r=Standard8,mccr8

Comment 10

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/dd0480856174