1325989 - Fix ESLint issues in devtools/server/tests/unit

Status: RESOLVED FIXED

(DevTools :: General, defect, P3)

Description

[Tim Nguyen :ntim]

+++ This bug was initially created as a clone of Bug #1325988 +++

To view ESLint issues:

./mach eslint devtools/server/tests/unit --no-ignore --fix


You'll need to fix them manually afterwards.

Recommendation: Adding a .eslintrc.js file in the directory will reduce the amount of errors.

https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/test/.eslintrc.js

You'll also need to remove the directory from .eslintignore once you're done.

Comment 1

[Tim Nguyen :ntim]

Here's a better example of eslintrc file: https://dxr.mozilla.org/mozilla-central/source/devtools/client/memory/test/unit/.eslintrc.js

Comment 2

[Michael Hoffmann]

Hi, I can start working on this bug.

Comment 3

[Michael Hoffmann]

Attachment: Fixes a bunch of easy issues

I've went through several test files and made obvious, easy to fix changes, saving the harder ones for later.  Given the sheer size of issues I probably won't have the energy to fix them all, but this is a start at least :-)

In order for the review to not get unwieldy I decided to stop and send a patch now, and I can continue after feedback/review.

I disabled the `no-debugger` since there are many tests containing the debugger statement.  Perhaps there are more options that could be disabled, like `no-inline-comments`.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8844229 [details] [diff] [review]
Fixes a bunch of easy issues

Review of attachment 8844229 [details] [diff] [review]:
-----------------------------------------------------------------

This seems like a good first pass, thanks for looking into this!

Where you able to run the tests to ensure they still work?

I think something like:

$ ./mach test devtools/server/tests/unit

would work.

Assuming you do fix all the errors here, we should also remove this directory from .eslintignore.

::: devtools/server/tests/unit/babel_and_browserify_script_with_source_map.js
@@ +1,1 @@
> +(function e(t, n, r) {function s(o, u) {if (!n[o]) {if (!t[o]) {var a = typeof require == "function" && require; if (!u && a) return a(o, !0); if (i) return i(o, !0); var f = new Error("Cannot find module '" + o + "'"); throw f.code = "MODULE_NOT_FOUND", f;} var l = n[o] = {exports: {}}; t[o][0].call(l.exports, function (e) {var n = t[o][1][e]; return s(n ? n : e);}, l, l.exports, e, t, n, r);} return n[o].exports;} var i = typeof require == "function" && require; for (var o = 0; o < r.length; o++)s(r[o]); return s;})({1: [function (require, module, exports) {

I believe this is a minified / built script file.  Please add this path to the .eslintignore file and revert changes here.

::: devtools/server/tests/unit/setBreakpoint-on-column-with-no-offsets-in-gcd-script.js
@@ +3,4 @@
>  function f() {}
>  
>  (function () {
> +  var a = 1; var c = 3;

I believe these setBreakpoint files are formatted in a particular way that's needed for the test to work correctly, so we probably don't want to alter them.  I'd say add them to .eslintignore.

::: devtools/server/tests/unit/test_animation_name.js
@@ +1,1 @@
> +/* Any copyright is dedicated to the Public Domain.

Why does this entire file show as replaced with seemingly the same thing?  Line endings perhaps?

::: devtools/server/tests/unit/testactors.js
@@ +72,4 @@
>    return root;
>  }
>  
> +function TestTaxbActor(aConnection, aGlobal)

`TestTabActor`

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

For the next round, please update your patch workflow to add more context lines.  

If you are using hg, check the guide[1].  For example, add to ~/.hgrc:

```
[diff]
git = true
showfunc = true
unified = 8
```

If you are using git, pass something like `-U8` to `git show`.

[1]: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_customize_the_format_of_the_patches_Mercurial_creates.3F

Comment 6

[Michael Hoffmann]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> Comment on attachment 8844229 [details] [diff] [review]
> Fixes a bunch of easy issues
> 
> Review of attachment 8844229 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems like a good first pass, thanks for looking into this!
> 
> Where you able to run the tests to ensure they still work?
> 
> I think something like:
> 
> $ ./mach test devtools/server/tests/unit
> 
> would work.

Yes, all tests pass (now).

> Assuming you do fix all the errors here, we should also remove this
> directory from .eslintignore.

This patch brings down the number of problems from 3232 to 794.  So there's still a lot remaining.  I assume I should wait with removing the directory from .eslintignore until all have been resolved?

> ::: devtools/server/tests/unit/babel_and_browserify_script_with_source_map.js
> @@ +1,1 @@
> > +(function e(t, n, r) {function s(o, u) {if (!n[o]) {if (!t[o]) {var a = typeof require == "function" && require; if (!u && a) return a(o, !0); if (i) return i(o, !0); var f = new Error("Cannot find module '" + o + "'"); throw f.code = "MODULE_NOT_FOUND", f;} var l = n[o] = {exports: {}}; t[o][0].call(l.exports, function (e) {var n = t[o][1][e]; return s(n ? n : e);}, l, l.exports, e, t, n, r);} return n[o].exports;} var i = typeof require == "function" && require; for (var o = 0; o < r.length; o++)s(r[o]); return s;})({1: [function (require, module, exports) {
> 
> I believe this is a minified / built script file.  Please add this path to
> the .eslintignore file and revert changes here.
> 
> :::
> devtools/server/tests/unit/setBreakpoint-on-column-with-no-offsets-in-gcd-
> script.js
> @@ +3,4 @@
> >  function f() {}
> >  
> >  (function () {
> > +  var a = 1; var c = 3;
> 
> I believe these setBreakpoint files are formatted in a particular way that's
> needed for the test to work correctly, so we probably don't want to alter
> them.  I'd say add them to .eslintignore.

Great that you found this.  I realized a bit late that --fix had gone ahead and modified some files.  The changes above had been done by --fix, but I've reverted them now.  And, it turns out that these files were already in .eslintignore.  The --no-ignore flag had that masked.  I'll skip those flags in the future to have more control of what is happening.

> ::: devtools/server/tests/unit/test_animation_name.js
> @@ +1,1 @@
> > +/* Any copyright is dedicated to the Public Domain.
> 
> Why does this entire file show as replaced with seemingly the same thing? 
> Line endings perhaps?

Yes, those were line endings.

> ::: devtools/server/tests/unit/testactors.js
> @@ +72,4 @@
> >    return root;
> >  }
> >  
> > +function TestTaxbActor(aConnection, aGlobal)
> 
> `TestTabActor`

Thanks, I must have slipped on the keyboard right before creating the patch, as a lot of tests failed because of this.

I have addressed all your comments, and I will take a second pass through the eslint issues.  Do you prefer if I upload the current patch (for review?) and then add more patches later which can be reviewed separately.  Or should I squash things so that it all comes in one big patch?  (The first option sounds nicer from a reviewing perspective)

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Michael Brennan from comment #6)
> > Assuming you do fix all the errors here, we should also remove this
> > directory from .eslintignore.
> 
> This patch brings down the number of problems from 3232 to 794.  So there's
> still a lot remaining.  I assume I should wait with removing the directory
> from .eslintignore until all have been resolved?

Right, we'll need to fix them all before taking this step.  Are you planning to try for fixing them all here?

> > ::: devtools/server/tests/unit/test_animation_name.js
> > @@ +1,1 @@
> > > +/* Any copyright is dedicated to the Public Domain.
> > 
> > Why does this entire file show as replaced with seemingly the same thing? 
> > Line endings perhaps?
> 
> Yes, those were line endings.

Okay, well, it seems good to have the line endings fixed, so seems fine to keep this in the patch.  I'm hoping there are not actual content changes, or if so they are correct, since I can't really tell from the diff UI here. ;)

> > ::: devtools/server/tests/unit/testactors.js
> > @@ +72,4 @@
> > >    return root;
> > >  }
> > >  
> > > +function TestTaxbActor(aConnection, aGlobal)
> > 
> > `TestTabActor`
> 
> Thanks, I must have slipped on the keyboard right before creating the patch,
> as a lot of tests failed because of this.
> 
> I have addressed all your comments, and I will take a second pass through
> the eslint issues.  Do you prefer if I upload the current patch (for
> review?) and then add more patches later which can be reviewed separately. 
> Or should I squash things so that it all comes in one big patch?  (The first
> option sounds nicer from a reviewing perspective)

For reviewing purposes, sure, it sounds fine to update the current one with those things fixed, and then add additional new patches as you continue.  We may want to squash them all together at the end before landing, but it should ease reviewing to keep them separate.

Thanks!

Comment 8

[Michael Hoffmann]

Attachment: bug1325989.first_pass.patch

New version of the first pass with earlier comments addressed.

Comment 9

[Michael Hoffmann]

Attachment: bug1325989.second_pass.patch

Comment 10

[Michael Hoffmann]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> Right, we'll need to fix them all before taking this step.  Are you planning
> to try for fixing them all here?

I am not sure yet.  The two patches I've submitted now fixes everything except the rules `max-nested-callbacks` and `no-shadow`, which make up a total of 312 errors.  I suppose the right solution would be to rewrite the affected tests to use Task.js.  Renaming callback arguments and breaking out callbacks to named functions just to please the linter might not necessarily improve readability in this case.  What do you think?

Rewriting with Task.js is quite some work since there are so many tests.  That's why I'm not sure how much I'll be willing to do.  Had some thoughts about writing some automated tool to help me modify the AST, but I don't know how viable that is.

> Okay, well, it seems good to have the line endings fixed, so seems fine to
> keep this in the patch.  I'm hoping there are not actual content changes, or
> if so they are correct, since I can't really tell from the diff UI here. ;)

Adding the -w flag to `hg diff` on those two files told me nothing changed.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8847582 [details] [diff] [review]
bug1325989.first_pass.patch

Review of attachment 8847582 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good to me!  However, could you update the commit message to be more specific (such as "Bug 1325989 - First pass at ESLint issues in devtools/server/tests/unit. r=jryans")?  Feel free to copy over the r+ to the new attachment after updating the message.

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8847583 [details] [diff] [review]
bug1325989.second_pass.patch

Review of attachment 8847583 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks like another good set of improvements!

I've noted a few things that would be good to tweak.  Also, please make the commit message a bit more specific as mentioned for the previous pass.

::: devtools/server/tests/unit/head_dbg.js
@@ +43,5 @@
>  
>  const { addDebuggerToGlobal } = Cu.import("resource://gre/modules/jsdebugger.jsm", {});
>  
> +const systemPrincipal = Cc[
> +  "@mozilla.org/systemprincipal;1"

Hmm, this is not really the typical formatting for these, let's go with:

const systemPrincipal = Cc["@mozilla.org/systemprincipal;1"]
                        .createInstance(Ci.nsIPrincipal);

The `loadSubScript` below should probably be changed to match and use destructuring:

var { loadSubScript } = Cc["@mozilla.org/moz/jssubscript-loader;1"]
                        .getService(...)

@@ +329,5 @@
>  }
>  
> +function testGlobal(name) {
> +  let sandbox = Cu.Sandbox(
> +    Cc["@mozilla.org/systemprincipal;1"].createInstance(Ci.nsIPrincipal)

Reformat as described above.

@@ +336,4 @@
>    return sandbox;
>  }
>  
> +function addTestGlobal(name, aServer = DebuggerServer) {

`aServer` should be renamed too.

@@ +397,5 @@
>  
>  /**
>   * Initialize the testing debugger server.
>   */
> +function initTestDebuggerServer(aServer = DebuggerServer) {

`aServer` should be renamed too.

@@ +451,5 @@
>  
>  /**
>   * Takes a relative file path and returns the absolute file url for it.
>   */
> +function getFileUrl(name, aAllowMissing = false) {

Looks like all the default args were skipped.

::: devtools/server/tests/unit/post_init_global_actors.js
@@ +7,4 @@
>  
>  PostInitGlobalActor.prototype = {
>    actorPrefix: "postInitGlobal",
> +  onPing: function onPing(request) {

I believe we also disallow function names, so this could be `onPing(request)`

::: devtools/server/tests/unit/test_profiler_bufferstatus.js
@@ +7,5 @@
>   * Tests if the profiler actor returns its buffer status via getBufferInfo.
>   */
>  
>  const Profiler = Cc["@mozilla.org/tools/profiler;1"].getService(Ci.nsIProfiler);
> +// ms

I really dislike this rule... :S Anyway, that's for another day. :)

::: devtools/server/tests/unit/xpcshell_debugging_script.js
@@ +1,3 @@
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
> +/* eslint-disable */

What was wrong with this file?

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Michael Brennan from comment #10)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> > Right, we'll need to fix them all before taking this step.  Are you planning
> > to try for fixing them all here?
> 
> I am not sure yet.  The two patches I've submitted now fixes everything
> except the rules `max-nested-callbacks` and `no-shadow`, which make up a
> total of 312 errors.  I suppose the right solution would be to rewrite the
> affected tests to use Task.js.  Renaming callback arguments and breaking out
> callbacks to named functions just to please the linter might not necessarily
> improve readability in this case.  What do you think?
> 
> Rewriting with Task.js is quite some work since there are so many tests. 
> That's why I'm not sure how much I'll be willing to do.  Had some thoughts
> about writing some automated tool to help me modify the AST, but I don't
> know how viable that is.

Ah, yes, it does seem like converting to Task.js would the best way to improve tests, but I agree it's a lot more complex than the other changes.

I'd say let's do this:

* In this bug, update files with those problems to disable no-shadow and / or max-nested-callbacks as needed (so that we can at least remove the test directory from the ignore list to ensure new files follow the rules)
* File a new bug for someone to convert the tests to Task.js and remove the ESLint rule comments

> > Okay, well, it seems good to have the line endings fixed, so seems fine to
> > keep this in the patch.  I'm hoping there are not actual content changes, or
> > if so they are correct, since I can't really tell from the diff UI here. ;)
> 
> Adding the -w flag to `hg diff` on those two files told me nothing changed.

Okay, sounds good!

Comment 14

[Michael Hoffmann]

Attachment: bug1325989.first_pass.patch

Changed commit message

Comment 15

[Michael Hoffmann]

Thanks for the feedback!

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #12)
> ::: devtools/server/tests/unit/post_init_global_actors.js
> @@ +7,4 @@
> >  
> >  PostInitGlobalActor.prototype = {
> >    actorPrefix: "postInitGlobal",
> > +  onPing: function onPing(request) {
> 
> I believe we also disallow function names, so this could be `onPing(request)`
> 

Neat, I wasn't aware this syntax is also valid outside a class.

> >  
> >  const Profiler = Cc["@mozilla.org/tools/profiler;1"].getService(Ci.nsIProfiler);
> > +// ms
> 
> I really dislike this rule... :S Anyway, that's for another day. :)

Then we are two :)

> ::: devtools/server/tests/unit/xpcshell_debugging_script.js
> @@ +1,3 @@
> >  /* Any copyright is dedicated to the Public Domain.
> >     http://creativecommons.org/publicdomain/zero/1.0/ */
> > +/* eslint-disable */
> 
> What was wrong with this file?

`strict` and `no-inline-comments`, but I didn't want to mess with test subject code.  But in the new patch I'm about to attach I added this file to .eslintignore instead.

Comment 16

[Michael Hoffmann]

Attachment: bug1325989.second_pass.patch

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8848596 [details] [diff] [review]
bug1325989.second_pass.patch

Review of attachment 8848596 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this part looks good to me!

Comment 18

[Michael Hoffmann]

Attachment: bug1325989.third_pass.patch

Third patch and final patch.  Removes the test directory from .eslintignore and disables the rules no-shadow and/or max-nested-callbacks on a per file basis.

New Bug 1348530 has been written to re-enable them.

As of this patch there are no more problems reported by ./mach eslint devtools/server/tests/unit/

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8848732 [details] [diff] [review]
bug1325989.third_pass.patch

Review of attachment 8848732 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.  Thanks for working on this!

If you need help running this on try before landing, set needinfo? to me at the bottom of the bug to request this.

Comment 20

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Michael Brennan from comment #15)
> > >  
> > >  const Profiler = Cc["@mozilla.org/tools/profiler;1"].getService(Ci.nsIProfiler);
> > > +// ms
> > 
> > I really dislike this rule... :S Anyway, that's for another day. :)
> 
> Then we are two :)

In bug 1326100, I just turned off the "no-inline-comments" rule.  If you wanted, you could undo these, though you'd likely need to wait until that bug is merged / marked FIXED, so that you have the same rule set locally.

Just an idea, so proceed however you wish. :)

Comment 21

[Michael Hoffmann]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #20)
> (In reply to Michael Brennan from comment #15)
> > > >  
> > > >  const Profiler = Cc["@mozilla.org/tools/profiler;1"].getService(Ci.nsIProfiler);
> > > > +// ms
> > > 
> > > I really dislike this rule... :S Anyway, that's for another day. :)
> > 
> > Then we are two :)
> 
> In bug 1326100, I just turned off the "no-inline-comments" rule.  If you
> wanted, you could undo these, though you'd likely need to wait until that
> bug is merged / marked FIXED, so that you have the same rule set locally.
> 
> Just an idea, so proceed however you wish. :)

Thanks for the heads up!  Sure, I'll wait until merge and then undo those changes, I have no hurry.  In the same time I'll fold everything up into one commit for try and landing.

Comment 22

[Michael Hoffmann]

Attachment: bug1325989.patch

I undid the changes to the '// ms'-comments so that they are inline again.  Then I folded the three patches into one.

Push to try (hoping I got the flags right): https://treeherder.mozilla.org/#/jobs?repo=try&revision=5be9e7c6827f315971a5e276e5389fee0f0ab6a7

Comment 23

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8850196 [details] [diff] [review]
bug1325989.patch

Review of attachment 8850196 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks again for working on this!

Assuming it's green on try, please add the "checkin-needed" keyword to the bug, and a sheriff will land it for you.

Comment 24

[Michael Hoffmann]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #23)
> Comment on attachment 8850196 [details] [diff] [review]
> bug1325989.patch
> 
> Review of attachment 8850196 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks again for working on this!
> 
> Assuming it's green on try, please add the "checkin-needed" keyword to the
> bug, and a sheriff will land it for you.

Sure! I'm happy to help out! :)

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/060667a1f5ae
Resolve ESLint issues in devtools/server/tests/unit. r=jryans

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/060667a1f5ae