Closed Bug 1423839 Opened 3 years ago Closed 2 years ago

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

Categories

(Core :: Networking, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: standard8, Assigned: jamesl33info, Mentored)

References

Details

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

Attachments

(2 files, 1 obsolete file)

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.
Blocks: 1357557
Priority: -- → P5
Whiteboard: [necko-triaged]
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)
Assignee: standard8 → nicolaptv
Assignee: nicolaptv → nobody
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
Whiteboard: [necko-triaged] → [necko-triaged][lang=js]
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?
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
Enabled ESLint for:
- netwerk/base/NetUtils.jsm
- netwerk/cookies/test/unit/**

Ran ESLint's automatic '--fix' option on the above files.
Manual ESLint changes for files enabled in previous [commit](https://phabricator.services.mozilla.com/D9293).

Depends on D9293.
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)
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.
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
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)
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)
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.
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.
You can just re-push it without any changes. Should work fine.
Flags: needinfo?(jamesl33info)
Mark, I've re-pushed the first patch (automatic changes) without modifying it.
Flags: needinfo?(jamesl33info)
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
https://hg.mozilla.org/mozilla-central/rev/eff50d923802
https://hg.mozilla.org/mozilla-central/rev/0dc37e5676af
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.