Closed
Bug 1326408
Opened 7 years ago
Closed 7 years ago
Fix ESLint errors in devtools/client/netmonitor/test and devtools/client/netmonitor/har/test
Categories
(DevTools :: Netmonitor, enhancement, P3)
DevTools
Netmonitor
Tracking
(firefox54 fixed)
RESOLVED
FIXED
Firefox 54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: ntim, Assigned: vaga)
References
Details
(Keywords: good-first-bug, Whiteboard: [lang=js])
Attachments
(1 file)
No description provided.
Comment 1•7 years ago
|
||
All the .js files in devtools/client/netmonitor/test and devtools/client/netmonitor/har/test ?
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 2•7 years ago
|
||
(In reply to [:Towkir] Ahmed from comment #1) > All the .js files in devtools/client/netmonitor/test and > devtools/client/netmonitor/har/test ? You'll need to run these 2 commands: `./mach eslint devtools/client/netmonitor/test --no-ignore` and `./mach eslint devtools/client/netmonitor/har/test --no-ignore` and fix the errors shown.
Flags: needinfo?(ntim.bugs)
Updated•7 years ago
|
Priority: -- → P3
Reporter | ||
Updated•7 years ago
|
Severity: normal → enhancement
Assignee | ||
Comment 3•7 years ago
|
||
Hello, I'd like to work on this bug. I ran these commands and I already tried to fix the errors of devtools/client/netmonitor/har/test
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 4•7 years ago
|
||
(In reply to Fabien Casters from comment #3) > Hello, > I'd like to work on this bug. > > I ran these commands and I already tried to fix the errors of > devtools/client/netmonitor/har/test Great! I've assigned this bug to you. The next step is to attach a patch of your changes for review :)
Assignee: nobody → fabien
Flags: needinfo?(ntim.bugs)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
> ./mach mochitest devtools/client/netmonitor/test It works but when I run: > ./mach mochitest devtools/client/netmonitor/test/browser_net_simple-request-details.js It fails: > Uncaught exception - at chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_simple-request-details.js:66 - TypeError: tabEl is undefined I've tried without my patch and it didn't work too.
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 8•7 years ago
|
||
(In reply to Fabien Casters from comment #7) > > ./mach mochitest devtools/client/netmonitor/test > It works but when I run: > > ./mach mochitest devtools/client/netmonitor/test/browser_net_simple-request-details.js > It fails: > > Uncaught exception - at chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_simple-request-details.js:66 - TypeError: tabEl is undefined > I've tried without my patch and it didn't work too. According to the tests config file (devtools/client/netmonitor/test/browser.ini), the test is disabled (see bug 1258809), so you don't need to worry about it.
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8841351 [details] Bug 1326408 - Fix ESLint errors in devtools/client/netmonitor/test and devtools/client/netmonitor/har/test. https://reviewboard.mozilla.org/r/115592/#review117002 Thanks for the cleanup! Looks mostly good, can you remove the 2 folders from .eslintignore ? ::: devtools/client/netmonitor/har/test/html_har_post-data-test-page.html:19 (Diff revision 2) > > <body> > <p>HAR POST data test</p> > > <script type="text/javascript"> > - function post(aAddress, aData) { > + /* exported executeTest executeTest2 */ nit: we usually use commas to separate globals: /* exported executeTest, executeTest2 */ ::: devtools/client/netmonitor/test/browser_net_header-docs.js:42 (Diff revision 2) > - document.querySelectorAll(".properties-view .treeRow.stringRow").forEach((rowEl, index) => { > - let headerName = rowEl.querySelectorAll(".treeLabelCell .treeLabel")[0].textContent; > + document.querySelectorAll(".properties-view .treeRow.stringRow").forEach( > + (rowEl, index) => { > + let headerName = rowEl.querySelectorAll(".treeLabelCell .treeLabel")[0] > + .textContent; > - let headerDocURL = getHeadersURL(headerName); > + let headerDocURL = getHeadersURL(headerName); > - let learnMoreEl = rowEl.querySelectorAll(".treeValueCell .learn-more-link"); > + let learnMoreEl = rowEl.querySelectorAll(".treeValueCell .learn-more-link"); > > - if (headerDocURL === null) { > + if (headerDocURL === null) { > - ok(learnMoreEl.length === 0, > + ok(learnMoreEl.length === 0, > - "undocumented header does not include a \"Learn More\" button"); > + "undocumented header does not include a \"Learn More\" button"); > - } else { > + } else { > - ok(learnMoreEl[0].getAttribute("title") === headerDocURL, > + ok(learnMoreEl[0].getAttribute("title") === headerDocURL, > - "documented header includes a \"Learn More\" button with a link to MDN"); > + "documented header includes a \"Learn More\" button with a link to MDN"); > - } > + } > - }); > - } > + } > + ); > + } To avoid changing the indentation of the whole block, I would rather do: let selector = ".properties-view .treeRow.stringRow"; document.querySelectorAll(selector).forEach((rowEl, index) => { ::: devtools/client/netmonitor/test/browser_net_image-tooltip.js:19 (Diff revision 2) > let { ACTIVITY_TYPE, EVENTS } = windowRequire("devtools/client/netmonitor/constants"); > - let { > - getDisplayedRequests, > - getSortedRequests, > - } = windowRequire("devtools/client/netmonitor/selectors/index"); > let toolboxDoc = monitor.toolbox.doc; Can you move the toolboxDoc variable definition right before the 3 helper functions using it ? It would help seeing where the variable is used. ::: devtools/client/netmonitor/test/browser_net_security-redirect.js:27 (Diff revision 2) > - let initial = getSortedRequests(gStore.getState()).get(0); > - let redirect = getSortedRequests(gStore.getState()).get(1); > - > - let initialSecurityIcon = document.querySelectorAll(".requests-security-state-icon")[0]; > + let [ > + initialSecurityIcon, > + redirectSecurityIcon, > + ] = document.querySelectorAll(".requests-security-state-icon"); Nice! ::: devtools/client/netmonitor/test/browser_net_simple-request-data.js:226 (Diff revision 2) > monitor.panelWin.once(EVENTS.RECEIVED_RESPONSE_CONTENT, () => { > let requestItem = getSortedRequests(gStore.getState()).get(0); > > ok(requestItem.responseContent, > "There should be a responseContent data available."); > + // eslint-disable-next-line mozilla/no-cpows-in-tests Hmm, looks like a bug, this isn't a CPOW at all. I filed bug 1342778. You can leave the comment the way it is for now. ::: devtools/client/netmonitor/test/head.js:7 (Diff revision 2) > http://creativecommons.org/publicdomain/zero/1.0/ */ > > /* import-globals-from ../../framework/test/shared-head.js */ > +/* exported Toolbox restartNetMonitor teardown waitForExplicitFinish > + verifyRequestItemTarget waitFor testFilterButtons loadCommonFrameScript > + performRequestsInContent waitForNetworkEvents */ nit: separate the globals with commas ::: devtools/client/netmonitor/test/head.js:25 (Diff revision 2) > getUrlBaseName, > getUrlQuery, > getUrlHost, > } = require("devtools/client/netmonitor/utils/request-utils"); > > +/* eslint-disable */ Can you be more specific about the rules you're disabling ? /* eslint-disable max-len, ... */ ::: devtools/client/netmonitor/test/require-helper.js:35 (Diff revision 2) > return `module.exports = require("devtools/client/netmonitor/test/fixtures/localization-helper")`; > case "devtools/client/shared/redux/create-store": > return `module.exports = require("devtools/client/netmonitor/test/fixtures/create-store")`; > } > + > + return ``; Web console uses `return null` in a file similar to this one. Can you use that? ::: devtools/client/netmonitor/test/service-workers/status-codes.html:19 (Diff revision 2) > > <body> > <p>Status codes test</p> > > <script type="text/javascript"> > + /* exported registerServiceWorker unregisterServiceWorker performRequests */ nit: separate with commas
Attachment #8841351 -
Flags: review?(ntim.bugs) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8841351 [details] Bug 1326408 - Fix ESLint errors in devtools/client/netmonitor/test and devtools/client/netmonitor/har/test. https://reviewboard.mozilla.org/r/115592/#review117002 > Can you move the toolboxDoc variable definition right before the 3 helper functions using it ? It would help seeing where the variable is used. I can't move the variable definition. showTooltipAndVerify is called line 30.
Reporter | ||
Comment 12•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8841351 [details] Bug 1326408 - Fix ESLint errors in devtools/client/netmonitor/test and devtools/client/netmonitor/har/test. https://reviewboard.mozilla.org/r/115592/#review117002 > I can't move the variable definition. showTooltipAndVerify is called line 30. If I understand correctly, this is no longer the case with your changes. toolboxDoc is only used by the 3 helper functions. So, unless I'm missing something, toolboxDoc can be defined just before those 3 helper functions.
Assignee | ||
Comment 13•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8841351 [details] Bug 1326408 - Fix ESLint errors in devtools/client/netmonitor/test and devtools/client/netmonitor/har/test. https://reviewboard.mozilla.org/r/115592/#review117002 > If I understand correctly, this is no longer the case with your changes. toolboxDoc is only used by the 3 helper functions. So, unless I'm missing something, toolboxDoc can be defined just before those 3 helper functions. If I define `toolboxDoc` just before those 3 helper functions, the variable doesn't exist when the first function call (showTooltipAndVerify) is executed. I don't know if my explanations are clear and if it's true so I print the error: ``` ReferenceError: can't access lexical declaration `toolboxDoc' before initialization ```
Reporter | ||
Comment 14•7 years ago
|
||
(In reply to Fabien Casters from comment #13) > Comment on attachment 8841351 [details] > Bug 1326408 - Fix ESLint errors in devtools/client/netmonitor/test and > devtools/client/netmonitor/har/test. > > https://reviewboard.mozilla.org/r/115592/#review117002 > > > If I understand correctly, this is no longer the case with your changes. toolboxDoc is only used by the 3 helper functions. So, unless I'm missing something, toolboxDoc can be defined just before those 3 helper functions. > > If I define `toolboxDoc` just before those 3 helper functions, the variable > doesn't exist when the first function call (showTooltipAndVerify) is > executed. > > I don't know if my explanations are clear and if it's true so I print the > error: > ``` > ReferenceError: can't access lexical declaration `toolboxDoc' before > initialization > ``` Seems fine then, dropping the issue.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e4c79eb473ae Fix ESLint errors in devtools/client/netmonitor/test and devtools/client/netmonitor/har/test. r=ntim
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e4c79eb473ae
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•