1504323 - Enable ESLint for netwerk/test/httpserver/

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P2)

Description

[Mark Banner (:standard8)]

As part of rolling out ESLint across the tree, we should enable it for the netwerk/test/httpserver/ directory.

I'm happy to mentor this bug. There's background on our eslint setups here:

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

Please note: We want to end up with two separate commits. One with the automatic changes, the second with the manual changes. It helps with reviewing if these appear as a commit series in phabricator.

Here's some approximate steps:

- In .eslintignore, remove the `netwerk/test/httpserver/**` line.

- Run eslint:

./mach eslint --fix netwerk

This should fix most 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, e.g.

$ hg commit -m "Bug nnn - Enable ESLint for netwerk/test/httpserver/ (automatic changes)" netwerk/

- For the remaining issues, you'll need to fix them by hand. Most of those should be reasonably easy to understand, but there's more information on some specific bits (especially no-undef) here:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ESLint#Common_issues_and_how_to_solve_them
http://eslint.org/docs/rules/

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

If you're not sure, please ask.

- Create a second commit of the manual changes, e.g.

$ hg commit -m "Bug nnn - Enable ESLint for netwerk/test/httpserver/ (manual changes)"

- Post the two commits via phabricator: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html
When you post, please request initial review from myself (`standard8`), I'll then redirect to the owners of the code if the changes look good.

Comment 1

[Junior [inactive]]

Attachment: Bug 1504323 - P1 Enable ESLint for netwerk/test/httpserver/ (automatic changes)

Comment 2

[Junior [inactive]]

Attachment: Bug 1504323 - P2 Enable ESLint for netwerk/test/httpserver/ (manual changes)

Comment 3

[Junior [inactive]]

Do you agree to add |quit| to tools/lint/eslint/eslint-plugin-mozilla/lib/configs/xpcshell-test.js

Comment 4

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99b0049f04e4266abd2417507541edf046b9b33e

Comment 5

[Mark Banner (:standard8)]

(In reply to Junior Hsu from comment #3)
> Do you agree to add |quit| to
> tools/lint/eslint/eslint-plugin-mozilla/lib/configs/xpcshell-test.js

I think in this case, quit() is a special xpcshell function that we're generally not going to be calling (and shouldn't be?) and there's probably only two cases in the tree. Hence for this case I think we should whitelist it in the file, not in xpcshell-test.js.

Comment 6

[Mark Banner (:standard8)]

Dragana: Thank you for reviewing the manual patch. Did you see the other patch here as well (https://phabricator.services.mozilla.com/D10985)?

Comment 7

[Dragana Damjanovic [:dragana]]

(In reply to Mark Banner (:standard8) from comment #6)
> Dragana: Thank you for reviewing the manual patch. Did you see the other
> patch here as well (https://phabricator.services.mozilla.com/D10985)?

done.
I am still not use to phabricator, thanks.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2a943fb3c3ac
P1 Enable ESLint for netwerk/test/httpserver/ (automatic changes) r=Standard8,dragana
https://hg.mozilla.org/integration/autoland/rev/c52a3f7f06ad
P2 Enable ESLint for netwerk/test/httpserver/ (manual changes) r=Standard8,dragana

Comment 9

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/2a943fb3c3ac
https://hg.mozilla.org/mozilla-central/rev/c52a3f7f06ad