Closed Bug 1326408 Opened 9 years ago Closed 8 years ago

Fix ESLint errors in devtools/client/netmonitor/test and devtools/client/netmonitor/har/test

Categories

(DevTools :: Netmonitor, enhancement, P3)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(firefox54 fixed)

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

People

(Reporter: ntim, Assigned: vaga)

References

Details

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

Attachments

(1 file)

Bug 1326408 - Fix ESLint errors in devtools/client/netmonitor/test and devtools/client/netmonitor/har/test.
59 bytes, text/x-review-board-request
ntim
: review+
Details
No description provided.
All the .js files in devtools/client/netmonitor/test and devtools/client/netmonitor/har/test ?
Flags: needinfo?(ntim.bugs)
(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)
Severity: normal → enhancement
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)
(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)
> ./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)
(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)
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 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.
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.
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 ```
(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.
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
https://hg.mozilla.org/mozilla-central/rev/e4c79eb473ae
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: