Closed Bug 1325988 Opened 3 years ago Closed 3 years ago

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

Categories

(DevTools :: General, defect, P3)

defect

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: ntim, Assigned: fabien)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [lang=js])

Attachments

(1 file)

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
Summary: Fix ESLint isues in devtools/server/tests/mochitest/ → Fix ESLint issues in devtools/server/tests/mochitest/
Priority: -- → P3
Hi,
I'd still like to work on the eslint issues.
Can I take this one ?
Flags: needinfo?(ntim.bugs)
(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.
Assignee: nobody → fabien
Flags: needinfo?(ntim.bugs)
Comment on attachment 8846792 [details]
Bug 1325988 - Fix ESLint issues in devtools/server/tests/mochitest/

Redirecting to tromey
Attachment #8846792 - Flags: review?(ntim.bugs) → review?(ttromey)
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.
Attachment #8846792 - Flags: review?(ttromey)
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 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
(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.
Btw, you should change the commit message from r?ntim to r?tromey since Tom is now reviewing the patch.
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 ?
Attachment #8846792 - Flags: review?(ntim.bugs) → review?(ttromey)
(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 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.
Attachment #8846792 - Flags: review?(ttromey)
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 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.
Attachment #8846792 - Flags: review?(ttromey) → review+
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?
(In reply to Tom Tromey :tromey from comment #18)
> I can't seem to close the remaining issues (the ones opened by :ntim).
Closed!
> 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 ?
Flags: needinfo?(ttromey)
(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
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` ?
Flags: needinfo?(ttromey) → needinfo?(ntim.bugs)
(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.
Flags: needinfo?(ntim.bugs)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1f17eeabf363
Fix ESLint issues in devtools/server/tests/mochitest/ r=tromey
https://hg.mozilla.org/mozilla-central/rev/1f17eeabf363
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.