Fix eslint issues in devtools/shared/webconsole/test/

RESOLVED FIXED in Firefox 54

Status

Product:
DevTools
Component:
General
Importance:
P3
normal
Status:
RESOLVED FIXED
Reported:
2 years ago
Modified:
9 months ago

People

(Reporter: ntim, Assigned: micah)

Tracking

(Blocks: 1 bug, {good-first-bug})

Version:
unspecified
Target:
Firefox 54
Keywords:
good-first-bug

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(2 attachments, 2 obsolete attachments)

Bug 1326412 - Fix eslint issues in devtools/shared/webconsole/test/
59 bytes, text/x-review-board-request
ntim
: review+
Details
Bug 1326412 - Fix ESLint issues in devtools/shared/webconsole/test/common.js.
59 bytes, text/x-review-board-request
jryans
: review+
Details
(Reporter)

Description

•
2 years ago 
Run `./mach eslint devtools/shared/webconsole/test --no-ignore --fix`
 (Reporter)

Updated

•
2 years ago
Priority: -- → P3
 (Assignee)

Comment 1

•
2 years ago 
Hi, I'd like to take a shot at this one!
 (Reporter)

Comment 2

•
2 years ago 
(In reply to tigleym from comment #1)
> Hi, I'd like to take a shot at this one!

Hi,

Thanks for your interest in this bug! If this is your first contribution, have a look at our contribution guides:
- https://developer.mozilla.org/en-US/docs/Tools/Contributing
- https://wiki.mozilla.org/DevTools/Hacking

You can reach us on IRC in the #devtools channel if you have any question.
Or simply comment on this bug, and if you have a question don't forget to set a "Need information" flag using the form below. This way you will be sure we get a notification about your comment.

To fix this bug, you need to run `./mach eslint devtools/shared/webconsole/test --no-ignore --fix` and fix all related errors.

You'll want to add a .eslintrc file in devtools/shared/webconsole/test similar to this one: https://dxr.mozilla.org/mozilla-central/source/devtools/client/performance/test/unit/.eslintrc.js

Once you're ready, please post a patch so I can look at it.
 (Assignee)

Comment 3

•
2 years ago 
(In reply to Tim Nguyen :ntim from comment #2)
> (In reply to tigleym from comment #1)
> > Hi, I'd like to take a shot at this one!
> 
> Hi,
> 
> Thanks for your interest in this bug! If this is your first contribution,
> have a look at our contribution guides:
> - https://developer.mozilla.org/en-US/docs/Tools/Contributing
> - https://wiki.mozilla.org/DevTools/Hacking
> 
> You can reach us on IRC in the #devtools channel if you have any question.
> Or simply comment on this bug, and if you have a question don't forget to
> set a "Need information" flag using the form below. This way you will be
> sure we get a notification about your comment.
> 
> To fix this bug, you need to run `./mach eslint
> devtools/shared/webconsole/test --no-ignore --fix` and fix all related
> errors.
> 
> You'll want to add a .eslintrc file in devtools/shared/webconsole/test
> similar to this one:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/performance/
> test/unit/.eslintrc.js
> 
> Once you're ready, please post a patch so I can look at it.

Thanks! I'll take a look!
Assignee: nobody → tigleym
Status: NEW → ASSIGNED
 (Assignee)

Comment 4

•
2 years ago
Posted patch Bug1326412.patch (obsolete) — — Splinter Review 
r? gl@mozilla.com
Attachment #8828640 - Flags: review+
Attachment #8828640 - Flags: review+ → review?(ntim.bugs)

Comment 5

•
2 years ago 
Comment on attachment 8828640 [details] [diff] [review]
Bug1326412.patch

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

::: devtools/shared/webconsole/test/.eslintrc.js
@@ +2,5 @@
> +
> +module.exports = {
> +  // Extend from the shared list of defined globals for mochitests.
> +  "extends": "../../../.eslintrc.xpcshell.js",
> +  "rules": {

I don't think we need to add these rules beyond extending .eslintrc.xpcshell.js

Comment 6

•
2 years ago 
The patch otherwise looks good to me. Leaving the review to be completed by ntim.
 (Reporter)

Comment 7

•
2 years ago 
Comment on attachment 8828640 [details] [diff] [review]
Bug1326412.patch

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

The changes look fine to me. I would take the time to fix the other rules rather than disabling them.

::: devtools/shared/webconsole/test/.eslintrc.js
@@ +10,5 @@
> +    "no-unused-vars": [1, {"vars": "local", "args": "none"}],
> +    // Sets the maximum number nested callbacks to 10
> +    "max-nested-callbacks": [2, 10],
> +    // Allows for the identifier name "response" to be used
> +    "no-shadow": [2, {"allow": ["response"]}],

I agree with :gl.
I would be fine with keeping "no-unused-vars": [2, {"vars": "local", "args": "none"}], since we've been using that for tests, but the rest should be removed.

About no-return-assign: it's usually something like:

return foo = bar;

which can be changed to:

foo = bar;
return foo;

For the other rules, I've provided some suggestions below.

::: devtools/shared/webconsole/test/common.js
@@ +37,5 @@
>    }
>    DebuggerServer.allowChromeProcess = true;
>  }
>  
> +function connectToDebugger(callback) {

I would recommend changing this function to be promise based instead of callback based:

function connectToDebugger() {
  initCommon();
  initDebuggerServer();

  let transport = DebuggerServer.connectPipe();
  let client = new DebuggerClient(transport);
  
  let dbgState = {dbgClient: client}

  return new Promise(resolve => {
    client.connect().then(response => resolve([dbgState, response]));
  });

}

@@ +72,3 @@
>    }
>  
> +  connectToDebugger(function _onConnect(state, response) {

You can use `Task.async` and the promise based APIs to reduce the number of callbacks. It should help with the no-shadow rule as well.

var _attachConsole = Task.async(function* (listener, callback, attachToTab, attachToWorker) {
  function _onAttachConsole(...) {
    ...
  }
  let [state, response] = yield connectToDebugger();
   ...
  if (attachToTab) {
    let { tabs } = yield state.dbgClient.listTabs();
    if (response.error) {
      ...
    }
    ...

    let [tabResponse, tabClient] = yield state.dbgClient.attachTab();
    ...
    if (attachToWorker) {
      worker.addEventListener("message", function() {
        let { workers } = yield tabClient.listWorkers();
        worker = workers.filter(w => w.url == workerName)[0];
        ...
        let [workerResponse, workerClient] = yield tabClient.attachWorker(worker.actor);
        ...
        yield workerClient.attachThread({});
        ...
      }, {once: true});
    } else {
      ...
    }
  } else {
    let processResponse = yield state.dbgClient.getProcess();
    yield attachTab(processResponse.form.actor);
    let { consoleActor } = processResponse.form;
    ...
  }
});
Attachment #8828640 - Flags: review?(ntim.bugs) → feedback+
 (Reporter)

Comment 8

•
2 years ago 
After discussion with :pbro, you can keep devtools/shared/webconsole/test/common.js in .eslintignore to silence the max-nested-callbacks rule. A new bug can be filed to fix that file.

Also, I forgot to mention it, but you'll want to remove devtools/shared/webconsole/test/** from .eslintignore and replace it with devtools/shared/webconsole/test/common.js.
Flags: needinfo?(tigleym)
 (Assignee)

Comment 9

•
2 years ago
Posted patch Bug1326412v2.patch (obsolete) — — Splinter Review 
Ran into some problems with the devtools/shared/webconsole/test/unit/test_throttle.js file . I fixed the eslint 'no-return-assign' error, but it resulted in another eslint error: 'no-sequences'. This eslint error is being thrown within the activitySeen assignment inside the callback. Here was my solution to resolve the error: 

+  listener.addActivityCallback(() => {
+    activitySeen = (true, null, null, null,
                                activityDistributor
                                .ACTIVITY_SUBTYPE_RESPONSE_COMPLETE,
                                null, TEST_INPUT.length, null);
+    return activitySeen;
+  });

This passes the eslint tests, but fails the unit tests. I'm not totally
 sure what the use of the comma operator within the callback is supposed to achieve when reassigning the activitySeen variable, so I was hoping you could give me some hints on how to resolve this problem. Thanks!
Attachment #8830115 - Flags: feedback?(ntim.bugs)
 (Reporter)

Comment 10

•
2 years ago 
Comment on attachment 8830115 [details] [diff] [review]
Bug1326412v2.patch

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

::: devtools/shared/webconsole/test/unit/test_throttle.js
@@ -120,3 @@
>                                 activityDistributor
>                                 .ACTIVITY_SUBTYPE_RESPONSE_COMPLETE,
>                                 null, TEST_INPUT.length, null);

Seems like this is a bug from ESLint.

I would try:

listener.addActivityCallback(
  () => { activitySeen = true },
  null, null, null,
  activityDistributor.ACTIVITY_SUBTYPE_RESPONSE_COMPLETE,
  null,
  TEST_INPUT.length,
  null
);

or:

listener.addActivityCallback(
  (() => { activitySeen = true }),
  null, null, null,
  activityDistributor.ACTIVITY_SUBTYPE_RESPONSE_COMPLETE,
  null,
  TEST_INPUT.length,
  null
);
Attachment #8830115 - Flags: feedback?(ntim.bugs)
 (Reporter)

Updated

•
2 years ago
Attachment #8828640 - Attachment is obsolete: true
 (Reporter)

Updated

•
2 years ago
Attachment #8830115 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 19

•
2 years ago
mozreview-review 
Comment on attachment 8836380 [details]
Bug 1326412 - Fix eslint issues in devtools/shared/webconsole/test/

https://reviewboard.mozilla.org/r/111814/#review113100
Attachment #8836380 - Flags: review?(ntim.bugs) → review+

Comment 20

•
2 years ago
mozreview-review 
Comment on attachment 8836381 [details]
Bug 1326412 - Fix ESLint issues in devtools/shared/webconsole/test/common.js.

https://reviewboard.mozilla.org/r/111816/#review113380

Looks good, thanks for the cleanup!

::: devtools/shared/webconsole/test/common.js:20
(Diff revision 4)
> -
> -var ConsoleAPIStorage = Cc["@mozilla.org/consoleAPI-storage;1"]
> -                          .getService(Ci.nsIConsoleAPIStorage);
>  var {DebuggerServer} = require("devtools/server/main");
>  var {DebuggerClient, ObjectClient} = require("devtools/shared/client/main");
> +const Services = require("Services");

Either use var or change the others to const so they all match here.

::: devtools/shared/webconsole/test/common.js:132
(Diff revision 4)
> -              });
> -            }, {once: true});
> -          } else {
> +  } else {
> -            aState.actor = tab.consoleActor;
> -            aState.dbgClient.attachConsole(tab.consoleActor, aListeners,
> -                                           _onAttachConsole.bind(null, aState));
> +    state.actor = tab.consoleActor;
> +    state.dbgClient.attachConsole(tab.consoleActor, listeners,
> +                                   _onAttachConsole.bind(null, state));

Nit: off by one space
Attachment #8836381 - Flags: review?(jryans) → review+
Comment hidden (mozreview-request)

Comment 22

•
2 years ago 
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3ca657b5dcd9
Fix eslint issues in devtools/shared/webconsole/test/ r=ntim
https://hg.mozilla.org/integration/autoland/rev/4b70610f52e2
Fix ESLint issues in devtools/shared/webconsole/test/common.js. r=jryans

Comment 23

•
2 years ago
bugherder 
https://hg.mozilla.org/mozilla-central/rev/3ca657b5dcd9
https://hg.mozilla.org/mozilla-central/rev/4b70610f52e2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
 (Reporter)

Updated

•
2 years ago
Flags: needinfo?(tigleym)

Updated

•
9 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.