Closed Bug 1326412 Opened 4 years ago Closed 3 years ago

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

Categories

(DevTools :: General, defect, P3)

Product:
DevTools
Component:
General
Type:
defect

Tracking

(firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: ntim, Assigned: micah)

References

Details

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

Attachments

(2 files, 2 obsolete files)

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 
Run `./mach eslint devtools/shared/webconsole/test --no-ignore --fix`
Priority: -- → P3
Hi, I'd like to take a shot at this one!
(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.
(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
Attached patch Bug1326412.patch (obsolete) — — Splinter Review 
r? gl@mozilla.com
Attachment #8828640 - Flags: review+
Attachment #8828640 - Flags: review+ → review?(ntim.bugs)
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
The patch otherwise looks good to me. Leaving the review to be completed by ntim.
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+
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)
Attached 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)
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)
Attachment #8828640 - Attachment is obsolete: true
Attachment #8830115 - Attachment is obsolete: true
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/3ca657b5dcd9
https://hg.mozilla.org/mozilla-central/rev/4b70610f52e2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Flags: needinfo?(tigleym)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.