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

RESOLVED FIXED in Firefox 65

Status

()

P5
normal
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: standard8, Assigned: jamesl33info, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
mozilla65
Points:
---

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: [necko-triaged][lang=js])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
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.
(Reporter)

Updated

a year ago
Blocks: 1357557
Priority: -- → P5
Whiteboard: [necko-triaged]
Comment hidden (mozreview-request)
(Reporter)

Comment 2

a year ago
mozreview-review
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.
Attachment #8938651 - Flags: review?(standard8)
(Reporter)

Updated

a year ago
Assignee: standard8 → nicolaptv
(Reporter)

Updated

10 months ago
Assignee: nicolaptv → nobody
(Reporter)

Comment 3

7 months ago
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
Mentor: standard8
status-firefox59: affected → ---
Whiteboard: [necko-triaged] → [necko-triaged][lang=js]
(Assignee)

Comment 4

5 months ago
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?
(Reporter)

Comment 5

5 months ago
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.
Assignee: nobody → jamesl33info
(Assignee)

Comment 6

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

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

Comment 7

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

Depends on D9293.
(Reporter)

Updated

5 months ago
Attachment #8938651 - Attachment is obsolete: true
Attachment #9018709 - Attachment description: Bug 1423839 - Part 1: ESLint automatic changes → Bug 1423839 - Part 1: Enable ESLint for NetUtil.jsm and netwerk/cookie/test/unit/ (automatic changes)
Attachment #9018714 - Attachment description: Bug 1423839 - Part 2: ESLint manual changes → Bug 1423839 - Part 2: Enable ESLint for NetUtil.jsm and netwerk/cookie/test/unit/ (manual changes)
(Reporter)

Comment 8

5 months ago
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

5 months ago
Pushed by mbanner@mozilla.com:
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
(Assignee)

Comment 10

5 months ago
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.
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
Flags: needinfo?(jamesl33info)
(Assignee)

Comment 12

5 months ago
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.
Flags: needinfo?(jamesl33info)
(Assignee)

Comment 13

5 months ago
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.
(Reporter)

Comment 14

5 months ago
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.
(Reporter)

Comment 15

5 months ago
You can just re-push it without any changes. Should work fine.
Flags: needinfo?(jamesl33info)
(Assignee)

Comment 16

5 months ago
Mark, I've re-pushed the first patch (automatic changes) without modifying it.
Flags: needinfo?(jamesl33info)

Comment 17

5 months ago
Pushed by mbanner@mozilla.com:
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

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eff50d923802
https://hg.mozilla.org/mozilla-central/rev/0dc37e5676af
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.