1423839 - Enable ESLint for NetUtil.jsm and netwerk/cookie/test/unit/

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P5)

Description

[Mark Banner (:standard8)]

As part of rolling out ESLint across the tree, we should enable it for netwerk/base/NetUtil.jsm and netwerk/cookie/test/unit/

There's background on our eslint setups here:

https://developer.mozilla.org/docs/ESLint

Here's some approximate steps:

- In .eslintignore, remove the `netwerk/base/NetUtils.jsm` and `netwerk/cookies/test/unit/**` lines.
- Run eslint:

./mach eslint --fix netwerk

This should fix most/all of the issues.

- Inspect the diff to make sure that the indentation of the lines surrounding the changes look ok.

- Create a commit of the work so far.

- For the remaining issues, you'll need to fix them by hand. I think most of these will be no-undef, there are hints on how to fix those here:

https://developer.mozilla.org/en-US/docs/ESLint#no-undef

(you can just run `./mach eslint netwerk` to check you've fixed them).

- Create a second commit of the manual changes and push it to mozreview:

http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview.html

If you're not sure of how to fix something, please look at the rules documentation http://eslint.org/docs/rules/ or just ask here.

Comment 1

[Nicola Webb]

Attachment: Bug 1423839 Enable ESLint for NetUtil.jsm and netwerk/cookie/test/unit/

Review commit: https://reviewboard.mozilla.org/r/209256/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209256/

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 8938651 [details]
Bug 1423839 Enable ESLint for NetUtil.jsm and netwerk/cookie/test/unit/

https://reviewboard.mozilla.org/r/209256/#review215114

Thank you for the patch. Looking good. There's a few small updates to do, once you've updated it, please can you add `jdm` (Josh Matthews) to the reviewers as well as me.

::: netwerk/base/NetUtil.jsm:485
(Diff revision 1)
>      /**
>       * Returns a reference to nsIIOService.
>       *
>       * @return a reference to nsIIOService.
>       */
> -    get ioService()
> +    get ioService() {

This getter was intended to avoid getting the ioService each time, since Services.io now does the caching for us, we can drop the ioService getter and just use `Services.io` rather than `this.ioService`

::: netwerk/cookie/test/unit/test_bug1155169.js:46
(Diff revision 1)
>      type: "changed", isSession: true, isSecure: false, isHttpOnly: false
>    });
>  }
>  
> -function setCookie(value, expected) {
> -  function setCookieInternal(value, expected = null) {
> +function setCookie(value, expected = null) {
> +  function setCookieInternal() {

Unfortunately this isn't quite going to work, as the second call to setCookieInternal (line 72) won't get the default value.

It is probably best to keep the arguments for setCookieInternal, but rename them something like `valueInternal` and `expectedInternal = null`.

::: netwerk/cookie/test/unit/test_bug1321912.js:6
(Diff revision 1)
>  const {utils: Cu, interfaces: Ci, classes: Cc} = Components;
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  
>  do_get_profile();
> -const dirSvc = Cc["@mozilla.org/file/directory_service;1"].
> +const dirSvc = Services.dirsvc;

Again, I think we can drop the intermediate variables for dirSvc / storage.

::: netwerk/cookie/test/unit/test_eviction.js:106
(Diff revision 1)
>      const BASE_URI = Services.io.newURI("http://" + base_host);
>      const PUB_FOO_PATH = Services.io.newURI("http://" + subdomain_host + "/foo/");
>      const WWW_BAR_PATH = Services.io.newURI("http://" + other_subdomain_host + "/bar/");
>      const OTHER_BAR_PATH = Services.io.newURI("http://" + another_subdomain_host + "/bar/");
>      const PUB_BAR_PATH = Services.io.newURI("http://" + subdomain_host + "/bar/");
> -    const WWW_FOO_PATH = Services.io.newURI("http://" + other_subdomain_host + "/foo/");
> +    // const WWW_FOO_PATH = Services.io.newURI("http://" + other_subdomain_host + "/foo/");

You can just delete this line.

Comment 3

[Mark Banner (:standard8)]

Opening as a mentored bug.

There's a previous patch here: https://hg.mozilla.org/mozreview/gecko/rev/008fd795c1c9

However, that is probably a bit out of date now.

What is probably easiest to do is to generally follow comment 0:

- Run the automatic fix option.
- Create a commit for the automatic fixes.
- Create a second commit for any manual fixes that are needed. Note: see also the issues discussed in comment 2.
- Posting a patch now should be performed via phabricator: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

Comment 4

[James Lee]

Hi, this looks like it will be the ideal first bug for me; something simple to learn the process. Could I please have this assigned to me?

Comment 5

[Mark Banner (:standard8)]

Hi James, welcome. Sure, please take it. Generally with bugs you can just attach once you've generated it if no-one's already working on it.

Comment 6

[James Lee]

Attachment: Bug 1423839 - Part 1: Enable ESLint for NetUtil.jsm and netwerk/cookie/test/unit/ (automatic changes)

Enabled ESLint for:
- netwerk/base/NetUtils.jsm
- netwerk/cookies/test/unit/**

Ran ESLint's automatic '--fix' option on the above files.

Comment 7

[James Lee]

Attachment: Bug 1423839 - Part 2: Enable ESLint for NetUtil.jsm and netwerk/cookie/test/unit/ (manual changes)

Manual ESLint changes for files enabled in previous [commit](https://phabricator.services.mozilla.com/D9293).

Depends on D9293.

Comment 8

[Mark Banner (:standard8)]

James, thank you for working on these.

FYI I've just updated the titles of the changesets so that the first lines are a bit more descriptive of what is actually being changed (a lot of people just look at the first line, unless they need more info).

I have trigged the landing process for these, they'll start landing sometime in the next few hours, and the bug will be updated with progress, resulting in the resolution automatically changing to "fixed" when it lands on our main branch (mozilla-central).

In future bugs, if you get review and it is ready for someone to land it for you, you can also add "checkin-needed" in the keywords field and then someone will come along to land it.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/406ca9722ffa
Part 1: Enable ESLint for NetUtil.jsm and netwerk/cookie/test/unit/ (automatic changes) r=Standard8,jdm
https://hg.mozilla.org/integration/autoland/rev/501fffbf872d
Part 2: Enable ESLint for NetUtil.jsm and netwerk/cookie/test/unit/ (manual changes) r=Standard8,jdm

Comment 10

[James Lee]

Mark, thank you for your help.

I will make sure that I use more descriptive *at a glance* titles in the future; I see where you are coming from.

I didn't know about the "checkin-needed" keyword thanks for letting me know about that. I will almost definitely end up using it.

Comment 11

[Dorel Luca [:dluca]]

Backed out 2 changesets (bug 1423839) for xpcshell failures on netwerk/test/unit/test_NetUtil.js

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=207105361&repo=autoland&lineNumber=2080

INFO -  TEST-PASS | netwerk/test/unit/test_immutable.js | took 2657ms
[task 2018-10-22T21:45:09.931Z] 21:45:09     INFO -  Retrying tests that failed when run in parallel.
[task 2018-10-22T21:45:09.940Z] 21:45:09     INFO -  TEST-START | netwerk/test/unit/test_NetUtil.js
[task 2018-10-22T21:45:10.320Z] 21:45:10  WARNING -  TEST-UNEXPECTED-FAIL | netwerk/test/unit/test_NetUtil.js | xpcshell return code: 0
[task 2018-10-22T21:45:10.321Z] 21:45:10     INFO -  TEST-INFO took 377ms
[task 2018-10-22T21:45:10.321Z] 21:45:10     INFO -  >>>>>>>
[task 2018-10-22T21:45:10.322Z] 21:45:10     INFO -  PID 8774 | [8774, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2751
[task 2018-10-22T21:45:10.322Z] 21:45:10     INFO -  PID 8774 | Couldn't convert chrome URL: chrome://branding/locale/brand.properties
[task 2018-10-22T21:45:10.323Z] 21:45:10     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2018-10-22T21:45:10.324Z] 21:45:10     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2018-10-22T21:45:10.324Z] 21:45:10     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2018-10-22T21:45:10.325Z] 21:45:10     INFO -  running event loop
[task 2018-10-22T21:45:10.326Z] 21:45:10     INFO -  PID 8774 | [8774, Main Thread] WARNING: Could not get the program name for a cubeb stream.: 'NS_SUCCEEDED(rv)', file /builds/worker/workspace/build/src/dom/media/CubebUtils.cpp, line 381
[task 2018-10-22T21:45:10.327Z] 21:45:10     INFO -  "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
[task 2018-10-22T21:45:10.328Z] 21:45:10     INFO -  netwerk/test/unit/test_NetUtil.js | Starting test_async_copy
[task 2018-10-22T21:45:10.330Z] 21:45:10     INFO -  (xpcshell/head.js) | test test_async_copy pending (2)
[task 2018-10-22T21:45:10.330Z] 21:45:10     INFO -  (xpcshell/head.js) | test pending (3)
[task 2018-10-22T21:45:10.333Z] 21:45:10     INFO -  "test_async_copy with buffered input, buffered output"
[task 2018-10-22T21:45:10.333Z] 21:45:10     INFO -  (xpcshell/head.js) | test run_next_test 0 finished (3)
[task 2018-10-22T21:45:10.341Z] 21:45:10     INFO -  TEST-PASS | netwerk/test/unit/test_NetUtil.js | test_async_copy - [test_async_copy : 182] "[test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output]" == "[test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output,test_async_copy with buffered input, buffered output]"
[task 2018-10-22T21:45:10.343Z] 21:45:10     INFO -  "test_async_copy with buffered input, unbuffered output"

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=501fffbf872d0a320069448010aa0706d3b81409

Backout:
https://hg.mozilla.org/integration/autoland/rev/bb628db881452e7a667b1961af9eef2fcb5b4fd2

Comment 12

[James Lee]

As it turns out I missed the testing file for 'netwerk/base/NetUtil.jsm' which relied on the 'ioService' function
which was removed in the second commit (manual changes). I have now fixed this issue and will update the differential.

Comment 13

[James Lee]

I've updated the second differential (manual changes) to fix the issue; the commits should be good to land again. Apologies for any inconvenience caused.

Comment 14

[Mark Banner (:standard8)]

James, thanks for the update. Could you re-push the first patch as well (automatic changes). This is to work around a current Phabricator/Lando issue.

Comment 15

[Mark Banner (:standard8)]

You can just re-push it without any changes. Should work fine.

Comment 16

[James Lee]

Mark, I've re-pushed the first patch (automatic changes) without modifying it.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eff50d923802
Part 1: Enable ESLint for NetUtil.jsm and netwerk/cookie/test/unit/ (automatic changes) r=Standard8,jdm
https://hg.mozilla.org/integration/autoland/rev/0dc37e5676af
Part 2: Enable ESLint for NetUtil.jsm and netwerk/cookie/test/unit/ (manual changes) r=Standard8,jdm

Comment 18

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/eff50d923802
https://hg.mozilla.org/mozilla-central/rev/0dc37e5676af