1325988 - Fix ESLint issues in devtools/server/tests/mochitest/

Status: RESOLVED FIXED

(DevTools :: General, defect, P3)

Description

[Tim Nguyen :ntim]

To view ESLint issues:

./mach eslint devtools/server/tests/mochitest/ --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

Comment 1

[Fabien Casters [:vaga]]

Hi,
I'd still like to work on the eslint issues.
Can I take this one ?

Comment 2

[Tim Nguyen :ntim]

(In reply to Fabien Casters from comment #1)
> Hi,
> I'd still like to work on the eslint issues.
> Can I take this one ?

Sure, assigned it to you.

Comment 3

[Fabien Casters [:vaga]]

Attachment: Bug 1325988 - Fix ESLint issues in devtools/server/tests/mochitest/

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

Comment 4

[Tim Nguyen :ntim]

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

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8846792 [details]
Bug 1325988 - Fix ESLint issues in devtools/server/tests/mochitest/

Redirecting to tromey

Comment 6

[Tom Tromey :tromey]

Comment on attachment 8846792 [details]
Bug 1325988 - Fix ESLint issues in devtools/server/tests/mochitest/

https://reviewboard.mozilla.org/r/119800/#review121984

Thank you for doing this.
This is looking good overall.  I had a few comments and nits but nothing too serious.

::: devtools/server/tests/mochitest/hello-actor.js:3
(Diff revision 1)
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
> +/* exported helloSpec, HelloActor */

I don't think HelloActor is really exported, since I don't think it's used anywhere else...

::: devtools/server/tests/mochitest/hello-actor.js:19
(Diff revision 1)
>        response: {count: protocol.RetVal("number")}
>      }
>    }
>  });
>  
> -var HelloActor = protocol.ActorClassWithSpec(helloSpec, {
> +let HelloActor = protocol.ActorClassWithSpec(helloSpec, {

There was a big sweep a couple years ago to avoid "let" at the top-level in favor of "var".  So I don't think this change is desirable.  Probably better is to just drop the name entirely, I think the side effects of the call are enough.

::: devtools/server/tests/mochitest/inspector-helpers.js:32
(Diff revision 1)
>    SimpleTest.registerCleanupFunction(function () {
>      DebuggerServer.destroy();
>    });
>  }
>  
> -var gAttachCleanups = [];
> +let gAttachCleanups = [];

Should use var.

::: devtools/server/tests/mochitest/inspector-helpers.js:296
(Diff revision 1)
>    });
>  
>    return deferred.promise;
>  }
>  
> -
> +let _tests = [];

I think this should remain "var"

::: devtools/server/tests/mochitest/test_inspector-search-front.html:18
(Diff revision 1)
> -window.onload = function() {
> +"use strict";
> +
> +window.onload = function () {
>    const Cu = Components.utils;
>    const {require} = Cu.import("resource://devtools/shared/Loader.jsm", {});
> -  const promise = require("promise");
> +  const Promise = require("promise");

Almost universally in devtools we don't use "Promise" as the name when requiring "promise".  I don't remember the reason for this, but I think even if there isn't one it's a bit better to be consistent here.  So I think in this spot it would be preferable to just have an eslint disable.

::: devtools/server/tests/mochitest/test_inspector-search.html:18
(Diff revision 1)
> -window.onload = function() {
> +"use strict";
> +
> +window.onload = function () {
>    const Cu = Components.utils;
>    const {require} = Cu.import("resource://devtools/shared/Loader.jsm", {});
> -  const promise = require("promise");
> +  const Promise = require("promise");

Here too.

::: devtools/server/tests/mochitest/test_memory_allocations_01.html:38
(Diff revision 1)
>      (function outer() {
>        (function middle() {
>          (function inner() {
> -          alloc1 = {};                alloc1.line = Error().lineNumber;
> -          alloc2 = [];                alloc2.line = Error().lineNumber;
> -          alloc3 = new function() {}; alloc3.line = Error().lineNumber;
> +          alloc1 = {}; alloc1.line = Error().lineNumber;
> +          alloc2 = []; alloc2.line = Error().lineNumber;
> +          alloc3 = function () {}; alloc3.line = Error().lineNumber;

I wonder whether this is changing the meaning of the test.  I tend to think it does.

Comment 7

[Fabien Casters [:vaga]]

Comment on attachment 8846792 [details]
Bug 1325988 - Fix ESLint issues in devtools/server/tests/mochitest/

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119800/diff/1-2/

Comment 8

[Fabien Casters [:vaga]]

Comment on attachment 8846792 [details]
Bug 1325988 - Fix ESLint issues in devtools/server/tests/mochitest/

https://reviewboard.mozilla.org/r/119800/#review121984

> I don't think HelloActor is really exported, since I don't think it's used anywhere else...

This file is not used in any tests. I don't find "hello-actor" occurence in devtools/server/tests/mochitest folder. Can I remove it ?

> Almost universally in devtools we don't use "Promise" as the name when requiring "promise".  I don't remember the reason for this, but I think even if there isn't one it's a bit better to be consistent here.  So I think in this spot it would be preferable to just have an eslint disable.

I've changed test_inspector-resize.html too.

Why don't use the native Promise like in other files ? I can do that if it's true.

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8846792 [details]
Bug 1325988 - Fix ESLint issues in devtools/server/tests/mochitest/

https://reviewboard.mozilla.org/r/119800/#review121972

::: devtools/server/tests/mochitest/test_connectToChild.html:1
(Diff revision 1)
>  <SDOCTYPv HTM.>

I noticed there's a typo here (not caused by the patch), can you fix it ?

<!DOCTYPE html>

::: devtools/server/tests/mochitest/test_inspector-search-front.html:18
(Diff revisions 1 - 2)
>  "use strict";
>  
>  window.onload = function () {
>    const Cu = Components.utils;
>    const {require} = Cu.import("resource://devtools/shared/Loader.jsm", {});
> -  const Promise = require("promise");
> +  const promise = require("promise");

It should work if you remove this variable, and use `Promise` (with an uppercase `P`).

Promise is a native DOM API so it shouldn't need any require.

promise refers to the non-standard implementation gecko has, which we're trying to avoid.

::: devtools/server/tests/mochitest/test_inspector-search.html:18
(Diff revisions 1 - 2)
>  "use strict";
>  
>  window.onload = function () {
>    const Cu = Components.utils;
>    const {require} = Cu.import("resource://devtools/shared/Loader.jsm", {});
> -  const Promise = require("promise");
> +  const promise = require("promise");

Same comment here

Comment 10

[Tim Nguyen :ntim]

(In reply to Fabien Casters from comment #8)
> Comment on attachment 8846792 [details]
> Bug 1325988 - Fix ESLint issues in devtools/server/tests/mochitest/
> 
> https://reviewboard.mozilla.org/r/119800/#review121984
> 
> > I don't think HelloActor is really exported, since I don't think it's used anywhere else...
> 
> This file is not used in any tests. I don't find "hello-actor" occurence in
> devtools/server/tests/mochitest folder. Can I remove it ?
I suppose you can remove `var HelloActor =` if the tests still pass.

> > Almost universally in devtools we don't use "Promise" as the name when requiring "promise".  I don't remember the reason for this, but I think even if there isn't one it's a bit better to be consistent here.  So I think in this spot it would be preferable to just have an eslint disable.
> 
> I've changed test_inspector-resize.html too.
> 
> Why don't use the native Promise like in other files ? I can do that if it's
> true.

Our require("promise") predates the native Promise implementation.
We should use the native Promise wherever we can.

As long as the tests still pass it should be fine.

Comment 11

[Tim Nguyen :ntim]

Btw, you should change the commit message from r?ntim to r?tromey since Tom is now reviewing the patch.

Comment 12

[Tim Nguyen :ntim]

Comment on attachment 8846792 [details]
Bug 1325988 - Fix ESLint issues in devtools/server/tests/mochitest/

https://reviewboard.mozilla.org/r/119800/#review122706

::: commit-message-7b19a:1
(Diff revision 2)
> +Bug 1325988 - Fix ESLint issues in devtools/server/tests/mochitest/ r?ntim

Can you change this to r?tromey to reflect the new reviewer ?

Comment 13

[Tom Tromey :tromey]

(In reply to Fabien Casters from comment #8)
> Comment on attachment 8846792 [details]
> Bug 1325988 - Fix ESLint issues in devtools/server/tests/mochitest/
> 
> https://reviewboard.mozilla.org/r/119800/#review121984
> 
> > I don't think HelloActor is really exported, since I don't think it's used anywhere else...
> 
> This file is not used in any tests. I don't find "hello-actor" occurence in
> devtools/server/tests/mochitest folder. Can I remove it ?

I believe what happens here is that the call to protocol.ActorClassWithSpec
registers the hello actor.  Then later devtools/server/tests/browser/browser_register_actor.js
will try to send messages to this actor.  If it's not registered, that test will fail.

So, I think it's needed.  You can test removing it though, I may be mistaken.

> Why don't use the native Promise like in other files ? I can do that if it's
> true.

I think that is completely fine, if the tests still work.
In the past we've had the odd issue or two when switching promise implementations,
though I think that was mostly due to the semantic change from "deprecated-sync-thenables"
to more async promise implementations.

Comment 14

[Tom Tromey :tromey]

Comment on attachment 8846792 [details]
Bug 1325988 - Fix ESLint issues in devtools/server/tests/mochitest/

https://reviewboard.mozilla.org/r/119800/#review122902

Thanks.  One more nit but then I think it will be ready.

::: devtools/server/tests/mochitest/hello-actor.js:7
(Diff revisions 1 - 2)
> -/* exported helloSpec, HelloActor */
>  "use strict";
>  
>  const protocol = require("devtools/shared/protocol");
>  
> -const helloSpec = protocol.generateActorSpec({
> +protocol.generateActorSpec({

I think you still need the "const helloSpec" here because...

::: devtools/server/tests/mochitest/hello-actor.js:18
(Diff revisions 1 - 2)
>        response: {count: protocol.RetVal("number")}
>      }
>    }
>  });
>  
> -let HelloActor = protocol.ActorClassWithSpec(helloSpec, {
> +protocol.ActorClassWithSpec(helloSpec, {

...it's used here.

Comment 15

[Fabien Casters [:vaga]]

Comment on attachment 8846792 [details]
Bug 1325988 - Fix ESLint issues in devtools/server/tests/mochitest/

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119800/diff/2-3/

Comment 16

[Fabien Casters [:vaga]]

When I change promise by native Promise, mochitest has succeeded with less tests. ( < 2444)
I've prefered to keep promise instead of Promise (native).

Comment 17

[Tom Tromey :tromey]

Comment on attachment 8846792 [details]
Bug 1325988 - Fix ESLint issues in devtools/server/tests/mochitest/

https://reviewboard.mozilla.org/r/119800/#review123374

Thank you once again.  This looks good.

Comment 18

[Tom Tromey :tromey]

I can't seem to close the remaining issues (the ones opened by :ntim).
I think they are fixed though.
Fabien, can you push to try, or should I do that?

Comment 19

[Tim Nguyen :ntim]

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=22fe4f1ffeb4

Comment 20

[Tim Nguyen :ntim]

(In reply to Tom Tromey :tromey from comment #18)
> I can't seem to close the remaining issues (the ones opened by :ntim).
Closed!

Comment 21

[Fabien Casters [:vaga]]

> 06:54:30     INFO - TEST-PASS | devtools/server/tests/browser/browser_register_actor.js | Hello actor must exist - 
> 06:54:30     INFO - Console message: [JavaScript Error: "Error occurred while creating actor 'undefined: Error: Unable to load actor module 'undefined'.
> 06:54:30     INFO - You must provide a module name when calling require() from devtools/server/actors/common
> 06:54:30     INFO - require@resource://gre/modules/commonjs/toolkit/loader.js:750:13
> 06:54:30     INFO - RegisteredActorFactory/this._getConstructor@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/common.js:49:17
> 06:54:30     INFO - ObservedActorFactory.prototype.createActor@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/common.js:114:11
> 06:54:30     INFO - _getOrCreateActor@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1605:17
> 06:54:30     INFO - onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1755:17
> 06:54:30     INFO - receiveMessage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:761:7
> 06:54:30     INFO - 
> 06:54:30     INFO - 
> 06:54:30     INFO - Stack: RegisteredActorFactory/this._getConstructor@resource://gre/modules/commonjs/toolkit/loader.js -> resourc" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js" line: 1632}]
> 06:54:30     INFO - Buffered messages finished
> 06:54:30     INFO - TEST-UNEXPECTED-FAIL | devtools/server/tests/browser/browser_register_actor.js | A promise chain failed to handle a rejection:  - [object Object]
> 06:54:30     INFO - Stack trace:
> 06:54:30     INFO - JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: register :: line 199
> 06:54:30     INFO - JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: completePromise :: line 711
> 06:54:30     INFO - JS frame :: resource://gre/modules/Task.jsm :: TaskImpl_handleException :: line 455
> 06:54:30     INFO - JS frame :: resource://gre/modules/Task.jsm :: TaskImpl_run :: line 331
> 06:54:30     INFO - JS frame :: resource://devtools/shared/deprecated-sync-thenables.js :: reject :: line 50
> 06:54:30     INFO - JS frame :: resource://devtools/shared/deprecated-sync-thenables.js :: then :: line 26
> 06:54:30     INFO - JS frame :: resource://devtools/shared/deprecated-sync-thenables.js :: resolve :: line 74
> 06:54:30     INFO - JS frame :: resource://devtools/shared/deprecated-sync-thenables.js :: reject :: line 80
> 06:54:30     INFO - JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js :: listenerJson :: line 731
> 06:54:30     INFO - JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js :: emitOnObject :: line 112
> 06:54:30     INFO - JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js :: emit :: line 89
> 06:54:30     INFO - JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js :: emit :: line 1322
> 06:54:30     INFO - JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js :: emitReply :: line 1021
> 06:55:14     INFO - Not taking screenshot here: see the one that was previously logged
> 06:55:14     INFO - TEST-UNEXPECTED-FAIL | devtools/server/tests/browser/browser_register_actor.js | Test timed out -

I'm searching about test failure but I find nothing.
Can you help me ?

Comment 22

[Tim Nguyen :ntim]

(In reply to Fabien Casters from comment #21)
> I'm searching about test failure but I find nothing.
> Can you help me ?

Looking at the test file [0], it seems to be caused by the HelloActor change.

It should be fine if you restore `var HelloActor = ` then add a /* exported HelloActor */ on top of the file.

https://dxr.mozilla.org/mozilla-central/source/devtools/server/tests/browser/browser_register_actor.js#8

Comment 23

[Fabien Casters [:vaga]]

Comment on attachment 8846792 [details]
Bug 1325988 - Fix ESLint issues in devtools/server/tests/mochitest/

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119800/diff/3-4/

Comment 24

[Fabien Casters [:vaga]]

It works. Thanks. :)

I was trying without `--disable-e10s` option. (`mach mochitest devtools/server/tests/browser/browser_register_actor.js`)

In `browser.ini`, the test is skipped if e10s is enabled.
https://dxr.mozilla.org/mozilla-central/source/devtools/server/tests/browser/browser.ini#99

Why doesn't it work when I execute `mach mochitest devtools/server/tests/browser` ?

Comment 25

[Tim Nguyen :ntim]

(In reply to Fabien Casters from comment #24)
> It works. Thanks. :)
> 
> I was trying without `--disable-e10s` option. (`mach mochitest
> devtools/server/tests/browser/browser_register_actor.js`)
> 
> In `browser.ini`, the test is skipped if e10s is enabled.
> https://dxr.mozilla.org/mozilla-central/source/devtools/server/tests/browser/
> browser.ini#99
> 
> Why doesn't it work when I execute `mach mochitest
> devtools/server/tests/browser` ?

You have to include --disable-e10s, because e10s is the default test configuration.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f17eeabf363
Fix ESLint issues in devtools/server/tests/mochitest/ r=tromey

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1f17eeabf363