Enable ESLint for dom/notification/test/unit/

RESOLVED FIXED in Firefox 64

Status

()

P3
normal
RESOLVED FIXED
a year ago
4 months ago

People

(Reporter: standard8, Assigned: spiro, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
mozilla64
Points:
---

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment, 1 obsolete attachment)

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 hidden (mozreview-request)
Assignee: standard8 → nicolaptv
(Reporter)

Comment 2

a year ago
mozreview-review
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?
Attachment #8938586 - Flags: review?(standard8)
Hi Nicola, are you planning to finish this?
Flags: needinfo?(nicolaptv)
Assignee: nicolaptv → nobody
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
Mentor: standard8
status-firefox59: affected → ---
Flags: needinfo?(nicolaptv)
Whiteboard: [lang=js]

Comment 5

6 months ago
Hey, I am beginner. Can I take this up?
@akshitsoni0000, sure please do. We'll assign once the first patch is posted.
(Assignee)

Comment 7

4 months ago
Created attachment 9015330 [details]
Bug 1423843 - Enabled ESLint for dom/notification/test/unit
(Assignee)

Comment 8

4 months ago
Mark

I have submitted both commits. Please look into them.

Thanks
Attachment #8938586 - Attachment is obsolete: true
Assignee: nobody → sharma.divyansh.501
Status: NEW → ASSIGNED
Attachment #9015330 - Attachment description: Bug - 1423843 Enabled ESLint for dom/notification/test/unit → Bug 1423843 - Enabled ESLint for dom/notification/test/unit

Comment 9

4 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd0480856174
Enabled ESLint for dom/notification/test/unit r=Standard8,mccr8

Comment 10

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dd0480856174
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.