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

RESOLVED FIXED in Firefox 54

Status

enhancement
P3
normal
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: ntim, Assigned: fabien)

Tracking

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

unspecified
Firefox 54

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment)

Comment hidden (empty)

Comment 1

2 years ago
All the .js files in devtools/client/netmonitor/test and devtools/client/netmonitor/har/test ?
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 2

2 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)
(Reporter)

Updated

2 years ago
Severity: normal → enhancement
(Assignee)

Comment 3

2 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

2 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)
(Assignee)

Comment 7

2 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

2 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

2 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

2 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

2 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

2 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

2 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

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e4c79eb473ae
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54

Updated

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