Closed Bug 1556844 Opened 5 years ago Closed 5 years ago

Enable more ESLint rules for netwerk/ (automatic changes)

Categories

(Core :: Networking, task, P3)

Product:
Core
Component:
Networking
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla69
Tracking Flags:
Tracking Status
firefox69 --- fixed

People

(Reporter: standard8, Assigned: avneesh1995, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [lang=js][necko-triaged])

Attachments

(1 file, 3 obsolete files)

Bug 1556844 - Enable more ESLint rules for netwerk, (auto & manual) fixes & format changes , r=standard8
47 bytes, text/x-phabricator-request
Details | Review

In bug 1554169 we enabled ESLint for netwerk/, however we also disabled various rules. Now that bug has landed, we should work to enable the rules that were initially disabled.

To help Mozilla out with this bug, here's the steps:

  1. Comment here on the bug that you want to volunteer to help. I (or someone else) will assign it to you.
  2. Download and build the Firefox source code: https://developer.mozilla.org/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build (an artifact build is sufficient).
  3. Start working on this bug.
    • Remove the relevant lines from the top-level .eslintrc.js
      • Note: Don't remove the lines below the prettier comment, there's separate work ongoing for that.
    • Run ./mach eslint --fix netwerk
    • Investigate any remaining issues raised and fix them.
    • Please check the indentation of the changes. Use hg diff or equivalent to check the indentation looks correct and check the surrounding code is fine (e.g. check that the second line of function arguments aligns correctly with the line above).
    • If you have direct questions about this bug, feel free to ask here or on irc.
  4. Build your change with mach build and test your change with:
    • ./mach xpcshell-test netwerk
    • ./mach mochitest netwerk
  5. Submit the patch for review. Mark me as a reviewer (r?standard8) so I'll get an email to come look at your code. I'll be doing an initial review of the patch, then passing it to image/ peers for final review.
  6. After a series of reviews and changes to your patch, I'll mark it for checkin or push it to autoland. Your code will soon be shipping to Firefox users worldwide!
  7. ...now you get to think about what kind of bug you'd like to work on next. Let me know what you're interested in and I can help you find your next contribution.
Keywords: good-first-bug

Hi Mark,
I am totally new to this (literally started 2 hours back), and seeing the level of detail you have provided to act upon, I think I can take this up and ease into the system as it goes. Will you be kind to assign it to me as I start working on it?

Hi Avneesh, sure, that's fine. I've just assigned it to you. Feel free to ask questions if you need to.

Assignee: nobody → avneesh1995
Status: NEW → ASSIGNED

Hi Mark,
Is there a possibility that the above mentioned tests may fail before any changes to the code yet?, because mine are failing just after compiling the build, no changes added. I can share with you the logs through which you might be able to tell if I am doing something incorrectly. Is this the right place to share these logs?

(In reply to Avneesh Singhal from comment #3)

Hi Mark,
Is there a possibility that the above mentioned tests may fail before any changes to the code yet?, because mine are failing just after compiling the build, no changes added. I can share with you the logs through which you might be able to tell if I am doing something incorrectly. Is this the right place to share these logs?

I ran the eslint test after removing the lines and there are no issues in that.
Only xpcshell-test is currently failing
I also updated the mozilla clone and rebuilt to be sure

(In reply to Avneesh Singhal from comment #4)

(In reply to Avneesh Singhal from comment #3)

Hi Mark,
Is there a possibility that the above mentioned tests may fail before any changes to the code yet?, because mine are failing just after compiling the build, no changes added. I can share with you the logs through which you might be able to tell if I am doing something incorrectly. Is this the right place to share these logs?

I see errors in test_protocolproxyservice.js and test_udp_multicast.js. I would guess it is most likely that they're to do with how our machines are setup and what it expects locally.

If they're failing before and after, I wouldn't worry about them too much, although you could file bugs on them if there aren't already any.

I ran the eslint test after removing the lines and there are no issues in that.

I'm sorry about this, I just realised I got the link to the lines to remove wrong. The link was for the image/ lines (which will be covered elsewhere eventually).

Here's the correct link: https://searchfox.org/mozilla-central/rev/9eb2f739c165b4e294929f7b99fbb4d90f8a396b/.eslintrc.js#204,209-212,217,219,221,232

There's multiple rules there that are capable of automatically being fixed, and these are in the correct netwerk/ section (see the "files" array just above).

I've updated that link in comment 0 as well in case anyone looks at it in future.

If they're failing before and after, I wouldn't worry about them too much, although you could file bugs on them if there aren't already any.
That's a relief, I will search and file if not there after this change.

I'm sorry about this, I just realised I got the link to the lines to remove wrong. The link was for the image/ lines (which will be covered elsewhere eventually).

Here's the correct link: https://searchfox.org/mozilla-central/rev/9eb2f739c165b4e294929f7b99fbb4d90f8a396b/.eslintrc.js#204,209-212,217,219,221,232

There's multiple rules there that are capable of automatically being fixed, and these are in the correct netwerk/ section (see the "files" array just above).

I've updated that link in comment 0 as well in case anyone looks at it in future.

No worries, I will start right away

Removing the new lines in eslint has bring out linting issue at 2 places, I am disabling those errors, because changes will cause other tests to fail.
I am adding eslint-disable-next-line before each. We can later raise a bug to correct those methods to follow eslint.
Do I need to do anything else before raising the patch?
Also, point if I could do something better to solve this.

Thanks
Avneesh

iirc it should be reasonably easy to fix those errors - https://eslint.org/docs/rules/ has information about the rules, and examples of good/bad cases.

Don't forget to check the indentation in the diffs, and run the main tests again.

If you want to put up a wip patch that's fine - just skip "r=Standard8" on the commit message.

(In reply to Mark Banner (:standard8) from comment #8)

iirc it should be reasonably easy to fix those errors - https://eslint.org/docs/rules/ has information about the rules, and examples of good/bad cases.

Don't forget to check the indentation in the diffs, and run the main tests again.

If you want to put up a wip patch that's fine - just skip "r=Standard8" on the commit message.
I went through each line again to see where tests might be failing.
I found the turning off of https://searchfox.org/mozilla-central/rev/9eb2f739c165b4e294929f7b99fbb4d90f8a396b/.eslintrc.js#211 as the culprit of failing tests. Need to handle it seperately.
Adding a WIP patch soon. Tests and lint all are running fine as of now.
No need of eslint-disable anymore

(In reply to Mark Banner (:standard8) from comment #8)

iirc it should be reasonably easy to fix those errors - https://eslint.org/docs/rules/ has information about the rules, and examples of good/bad cases.

Don't forget to check the indentation in the diffs, and run the main tests again.

If you want to put up a wip patch that's fine - just skip "r=Standard8" on the commit message.

I went through each line again to see where tests might be failing.
I found the turning off of https://searchfox.org/mozilla-central/rev/9eb2f739c165b4e294929f7b99fbb4d90f8a396b/.eslintrc.js#211 as the culprit of failing tests. Need to handle it seperately.
Adding a WIP patch soon. Tests and lint all are running fine as of now.
No need of eslint-disable anymore

Hi Mark,
I was successfully able to pinpoint the error which is stopping me, it is related to rule: mozilla/use-chromeutils-generateqi which changes implementation of all QueryInterface methods to chromeutils based methods. At https://searchfox.org/mozilla-central/rev/9eb2f739c165b4e294929f7b99fbb4d90f8a396b/netwerk/test/unit/head_cache2.js#413 , it fails because of out-of-format implementation.
What do you suggest?, we can let this particular line (only implementation in this format) remain as an exception or take any other approach. I was not able to find the rule's implementation for changing the code or I would have figured something. Let me know what you think.
Everything else related to the bug is complete and is passing all current tests.

(In reply to Avneesh Singhal from comment #12)

I was successfully able to pinpoint the error which is stopping me, it is related to rule: mozilla/use-chromeutils-generateqi which changes implementation of all QueryInterface methods to chromeutils based methods. At https://searchfox.org/mozilla-central/rev/9eb2f739c165b4e294929f7b99fbb4d90f8a396b/netwerk/test/unit/head_cache2.js#413 , it fails because of out-of-format implementation.
What do you suggest?, we can let this particular line (only implementation in this format) remain as an exception or take any other approach. I was not able to find the rule's implementation for changing the code or I would have figured something. Let me know what you think.

I think the easiest thing to do is to just disable it:

// eslint-disable-next-line mozilla/use-chromeutils-generateqi
QueryInterface() { return this; }

Everything else related to the bug is complete and is passing all current tests.

Cool, I'll take a quick look a it.

Priority: -- → P3
Whiteboard: [lang=js] → [lang=js][necko-triaged]

Depends on D33992

Avneesh, please could fold the changes in D34917 into the original D33992 please? We normally include the review changes in the same commit.

Flags: needinfo?(avneesh1995)

(In reply to Mark Banner (:standard8) from comment #16)

Avneesh, please could fold the changes in D34917 into the original D33992 please? We normally include the review changes in the same commit.
I am really sorry, couldn't respond earlier, was a bit busy. I don't know how to ammend my pushed commit in hg, so wasn't able to push right away.

Flags: needinfo?(avneesh1995)

Thanks for waiting, I have added the said changes into the same commit. i wasn't able to find anything specific regarding the flow for folding my commits and pushing onto D33992 directly without having to redo the changes, so I have added both commits in the earlier Bug.
Please let me know if there is anything I need to do, or can do better. I am grateful to you for helping me through this.
Avneesh Singhal

Attachment #9070029 - Attachment is obsolete: true
Attachment #9071968 - Attachment is obsolete: true
Attachment #9072469 - Attachment is obsolete: true

(In reply to Avneesh Singhal from comment #19)

Thanks for waiting, I have added the said changes into the same commit. i wasn't able to find anything specific regarding the flow for folding my commits and pushing onto D33992 directly without having to redo the changes, so I have added both commits in the earlier Bug.
Please let me know if there is anything I need to do, or can do better. I am grateful to you for helping me through this.

Generally if you're using moz-phab you should be able to hg commit --amend and then run moz-phab submit again. If you want to fold commits, then you can generally use hg histedit.

If you've done something which means you've lost the commit header information, then you need to make sure there's a line in the header along the lines of Differential Revision: https://phabricator.services.mozilla.com/D33992 - that's what moz-phab uses to detect which revision to upgrade.

In any case, I've taken your most recent patch and pushed that up to the original revision, so we're all good to go here now.

Thank you for all your work on this.

Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7163f0d58a23
Enable more ESLint rules for netwerk, (auto & manual) fixes & format changes , r=dragana

(In reply to Mark Banner (:standard8) from comment #20)

(In reply to Avneesh Singhal from comment #19)

Thanks for waiting, I have added the said changes into the same commit. i wasn't able to find anything specific regarding the flow for folding my commits and pushing onto D33992 directly without having to redo the changes, so I have added both commits in the earlier Bug.
Please let me know if there is anything I need to do, or can do better. I am grateful to you for helping me through this.

Generally if you're using moz-phab you should be able to hg commit --amend and then run moz-phab submit again. If you want to fold commits, then you can generally use hg histedit.

If you've done something which means you've lost the commit header information, then you need to make sure there's a line in the header along the lines of Differential Revision: https://phabricator.services.mozilla.com/D33992 - that's what moz-phab uses to detect which revision to upgrade.
In any case, I've taken your most recent patch and pushed that up to the original revision, so we're all good to go here now.

I searched how to fold commits and was getting redirected to uncommit etc and was skeptical of losing commit. Anyway, now I got the correct way and will follow the same in future.
Thank you for your help on my first-bug. Couldn't have done anything without you.

Regards
Avneesh

https://hg.mozilla.org/mozilla-central/rev/7163f0d58a23

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox69: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: