1556854 - Enable ESLint for dom/media/test/

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, task, P3)

Description

[Mark Banner (:standard8)]

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

Note: I've not put this as a good-first-bug as there's quite a lot of errors (~700) listed after the automatic fixes. I think that number will probably go down rapidly as the main issues are fixed (sometimes multiple errors are reported due to one issue), but will likely take more time than I'd expect for a good-first-bug. If you want to take this on, and it starts taking too long, feel free to submit partial patches.

To help Mozilla out with this bug, here's the steps:

1. Comment here on the bug that you want to volunteer to help. I (or someone else) will assign it to you.
2. Download and build the Firefox source code, see the docs for details. An artifact build is all you need.
   • If you have any problems, please ask on IRC in the #introduction channel. They're there to help you get started.
   • You can also read the Developer Guide, which has answers to most development questions.
3. Start working on this bug
   • Please note:
     • We want to end up with two separate commits. One with the automatic changes, the second with the manual changes.
     • Although you'll change .eslintignore at the start, only the second commit should have the .eslintignore changes. Please follow the suggested commands closely.
   • Here's what to do:
     1. In .eslintignore, remove the dom/media/test/** and !dom/media/test/marionette/yttest/*.js lines.
     2. Run eslint ./mach eslint --fix dom/media/test/
        • This should fix some of the issues.
     3. Inspect the diff to make sure that the indentation of the lines surrounding the changes look ok.
     4. Create a commit of the work so far. Note the extra dom/media/test/ at the end (this avoids committing .eslintignore at this stage)
        • $ hg commit -m "Bug nnn - Enable ESLint for dom/media/test/ (automatic changes). r?Standard8" dom/media/test/
     5. For the remaining issues, you'll need to fix them by hand. To find them, run ./mach eslint dom/media/test/.
        • Most of those should be reasonably easy to understand, but there's more information on some specific bits (especially no-undef) here and here.
     6. Create a second commit of the manual changes, note, there's no directory specifier this time, so .eslintignore will be included.
        • $ hg commit -m "Bug nnn - Enable ESLint for dom/media/test/ (manual changes). r?Standard8
     7. Post the two commits via phabricator. Please use moz-phab to submit them.
4. Once the patches are submitted, I'll take a look. If there's any changes necessary I'll comment in Phabricator, so be prepared to update the patches. If there's no changes, I'll request review from a dom peer, so there may still be more to go.
5. Once we're happy with the changes, I'll push it to autoland - our integration branch. Your code will then soon be shipping to Firefox users worldwide!
6. Now you get to think about what kind of bug you'd like to work on next. Let me know what you're interested in and I can help you find your next contribution.

Comment 1

[Karan Sapolia]

May I work on this next Mark?

Comment 2

[Mark Banner (:standard8)]

Hi Karan, sure, this seems like a good next one for you to work on.

Comment 3

[Ruchika Gosain]

HI Karan it looks like you are not working on it, can i take this up?

Thanks
Ruchika

Comment 4

[Karan Sapolia]

Hey Ruchika, I am working on it currently. Will inform you if I am unable to complete it. Thanks.

Comment 5

[Karan Sapolia]

Attachment: Bug 1556854 - Enable ESLint for dom/media/test/ - Enable prettier and ESLint rules curly and mozilla/consistent-if-bracing. r?jya

Comment 6

[Mark Banner (:standard8)]

Hi Karan, are you still working on this?

Comment 7

[Karan Sapolia]

Hey Mark, sorry I won't be working on this anymore. Feel free to reassign.

Comment 8

[Mark Banner (:standard8)]

Ok, no problem thank you for letting us know.

Comment 9

[Mark Banner (:standard8)]

Attachment: Bug 1556854 - Enable ESLint for dom/media/test/ - Manually enable remaining rules. r?jya

Depends on D37752

Comment 10

[Mark Banner (:standard8)]

Attachment: Bug 1556854 - Enable ESLint for dom/media/test/, disabling failing rules, fixing instances of no-shadow failures. r?jya

Depends on D49798

Comment 11

[Mark Banner (:standard8)]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be03f1f7d406c24c37e7705b3b24a069a244a8a1

Comment 12

[Mark Banner (:standard8)]

Attachment: Bug 1556854 - Enable ESLint for dom/media/test/ - Enable ESLint rules curly and mozilla/consistent-if-bracing. r?jya

Depends on D37752

Comment 13

[Mark Banner (:standard8)]

Attachment: Bug 1556854 - Enable ESLint for dom/media/test/ - Enable automatically fixable rules. r?jya.

This enables the following rules:

• dot-notation
• mozilla/no-useless-parameters
• no-else-return
• no-useless-return
• object-shorthand

Depends on D50130

Comment 14

[Mark Banner (:standard8)]

Attachment: Bug 1556854 - Enable ESLint for dom/media/test/ - Enable ESLint rules no-undef and no-unused-vars. r?jya

Depends on D50131

Comment 15

[Mark Banner (:standard8)]

Note: The list on this page is in phabricator ID order. It is probably best to follow the stack in the "Revision Contents" section of the phabricator pages.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=147bcea10fd22691fcfee75ec28596b614d3831d

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8719996c65b3
Enable ESLint for dom/media/test/, disabling failing rules, fixing instances of no-shadow failures. r=jya
https://hg.mozilla.org/integration/autoland/rev/0f02ff8a5c67
Enable ESLint for dom/media/test/ - Enable prettier and ESLint rules curly and mozilla/consistent-if-bracing. r=jya
https://hg.mozilla.org/integration/autoland/rev/057cc9878594
Enable ESLint for dom/media/test/ - Enable automatically fixable rules. r=jya.
https://hg.mozilla.org/integration/autoland/rev/cfd809c63877
Enable ESLint for dom/media/test/ - Enable ESLint rules no-undef and no-unused-vars. r=jya
https://hg.mozilla.org/integration/autoland/rev/61a4022333fa
Enable ESLint for dom/media/test/ - Manually enable remaining rules. r=jya

Comment 17

[Narcis Beleuzu [:NarcisB]]

Backed out for mda failures on /test_media_selection.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/132bd541dbef4de71c008f2b182fcb162651084a
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=272724891&repo=autoland&lineNumber=2783

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77b66c3539ec
Enable ESLint for dom/media/test/, disabling failing rules, fixing instances of no-shadow failures. r=jya
https://hg.mozilla.org/integration/autoland/rev/f10e6e88331a
Enable ESLint for dom/media/test/ - Enable prettier and ESLint rules curly and mozilla/consistent-if-bracing. r=jya
https://hg.mozilla.org/integration/autoland/rev/6368dec38ad6
Enable ESLint for dom/media/test/ - Enable automatically fixable rules. r=jya.
https://hg.mozilla.org/integration/autoland/rev/68d0e52b6dd1
Enable ESLint for dom/media/test/ - Enable ESLint rules no-undef and no-unused-vars. r=jya
https://hg.mozilla.org/integration/autoland/rev/f72e51defe97
Enable ESLint for dom/media/test/ - Manually enable remaining rules. r=jya

Comment 19

[Mark Banner (:standard8)]

I'd missed a couple of changes in the no-shadow patch. All fixed up now.

Comment 20

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/77b66c3539ec
https://hg.mozilla.org/mozilla-central/rev/f10e6e88331a
https://hg.mozilla.org/mozilla-central/rev/6368dec38ad6
https://hg.mozilla.org/mozilla-central/rev/68d0e52b6dd1
https://hg.mozilla.org/mozilla-central/rev/f72e51defe97