Status
People
(Reporter: ntim, Assigned: micah)
Tracking
(Blocks 1 bug, {good-first-bug})
Firefox Tracking Flags
(firefox54 fixed)
Details
(Whiteboard: [lang=js])
Attachments
(2 attachments, 2 obsolete attachments)
Run `./mach eslint devtools/shared/webconsole/test --no-ignore --fix`
|
Reporter | |
Updated•3 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!
|
||
Updated•2 years ago
|
Assignee: nobody → tigleym
Status: NEW → ASSIGNED
|
|
||
Updated•2 years ago
|
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
|
||
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) |
| 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
Closed: 2 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
|
|
Reporter | |
Updated•2 years ago
|
Flags: needinfo?(tigleym)
|
||
Updated•Last year
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•