Closed Bug 1280572 Opened 3 years ago Closed 3 years ago

Add layout/tools/reftest to flake8 linter

Categories

(Testing :: Reftest, defect)

defect
Not set

Tracking

(firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: ahal, Assigned: deepsrijit1105, Mentored)

Details

(Whiteboard: [good first bug][flake8])

Attachments

(1 file, 3 obsolete files)

There is a flake8 linter that can be run with:
mach lint -l flake8

We should add tools/layout/reftest to this linter here:
https://dxr.mozilla.org/mozilla-central/rev/5f95858f8ddf21ea2271a12810332efd09eff138/tools/lint/flake8.lint#124

After doing so we'll of course need to fix all the lint infractions before landing it.

Please ping ahal on irc.mozilla.org if you'd like to work on this.
Hi Andrew, as I had informed during the IRC chat, I would like to take this up. I am half way through the lint fixes. Can you please assign it to me?
Flags: needinfo?(ahalberstadt)
Attached patch patch1 (obsolete) — Splinter Review
This is an intermediate patch. The rest errors to be fixed are "line too long" ones. I have a couple of doubts in them. I could not find you online. Do give me some suggestions to resolve those.
Assignee: nobody → deepsrijit1105
Status: NEW → ASSIGNED
Flags: needinfo?(ahalberstadt)
Attachment #8835614 - Attachment is patch: true
Hi Deepa, thanks for the patch!

Line too long is sort of a case by case basis, and often comes down to personal preference. Here are some techniques I like to use.

For long strings:
func("pretend this is a long "
     "string")

For function arguments:
func(
    arg1,
    arg2,
    arg3,
    ...
)

Alternatively:
func(arg1, arg2, arg3,
     arg4, arg5, arg6, arg7)

Lists and dicts can be similar:
l = [
    'foo',
    'bar',
]

If you absolutely need to, you can use '\', though we usually like to avoid this if possible:
if condition1 and condition2 \
   and condifion3:
    print('foo')

Often the best way to break up lines is to do a small refactoring and use a few more variables. E.g instead of:
print("This is the return code: {}".format(func(arg1, arg2, arg3, arg4)))

Do:
ret = func(arg1, arg2, arg3, arg4)
print("This is the return code: {}".format(ret))

Hope that helps. Just try it out, and if the flake8 linter says it's ok, then it's probably fine. Don't be afraid to submit it even if you are unsure.. We will work through everything and if it isn't ok, I'll let you know. Cheers!
Attached patch patch2 (obsolete) — Splinter Review
I have made the changes as per your suggestions (Thank you). The only error remaining is in

mozilla-central/layout/tools/reftest/reftestcommandline.py

324:12  error  undefined name 'mozinfo'  F821 (flake8)

please take a look a let me know what has to be done for that.
Attachment #8835614 - Attachment is obsolete: true
Attachment #8836308 - Flags: review?(ahalberstadt)
Attachment #8836308 - Attachment is patch: true
Comment on attachment 8836308 [details] [diff] [review]
patch2

Review of attachment 8836308 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this is great! Almost there, please upload one more patch that fixes the following:

1. Fix the comment below
2. For the 'mozinfo' issue, you can add 'import mozinfo' to the top of the file, just above the 'import mozlog' line.
3. Please amend the commit message so it has the string "r=ahal" at the end of it. This is just mozilla policy so people know who the reviewer was.

Once you've uploaded the next patch, I'll do another quick review.. then if everything looks good I'll test it and push it to the repo.

::: layout/tools/reftest/output.py
@@ +53,5 @@
>              screenshots = extra["reftest_screenshots"]
> +
> +            # temporary variables to resolve flake8 errors.
> +            x = screenshots[0]["screenshot"]
> +            y = screenshots[2]["screenshot"]

No need to add this comment. Instead call the variables something more descriptive like "image1" and "image2".
Attachment #8836308 - Flags: review?(ahalberstadt) → feedback+
Attached patch updated_patch (obsolete) — Splinter Review
Hi Andrew, I have updated the patch.
Attachment #8836308 - Attachment is obsolete: true
Attachment #8836780 - Flags: review?(ahalberstadt)
Comment on attachment 8836780 [details] [diff] [review]
updated_patch

Review of attachment 8836780 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks good! Just one more error to fix up, and it should be good to land.

::: layout/tools/reftest/output.py
@@ +61,2 @@
>              elif len(screenshots) == 1:
> +                output_text += "\nREFTEST   IMAGE: data:image/png;base64,%(image1)s" % x

Looks like you forgot to update the 'x' to 'image_1' here.
Attachment #8836780 - Flags: review?(ahalberstadt) → review-
Attached patch final_patchSplinter Review
Sorry for that, I missed it out.
Attachment #8836780 - Attachment is obsolete: true
Attachment #8837283 - Flags: review?(ahalberstadt)
Comment on attachment 8837283 [details] [diff] [review]
final_patch

Review of attachment 8837283 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! Looks good, and verified it passes the lint locally. I also pushed this change to try (our testing server):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=296419332da16417cbcb624cda7c7c12a024650d

If the results all come back green, I'll land this for you.
Attachment #8837283 - Flags: review?(ahalberstadt) → review+
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/868a4dc22d7a
Add layout/tools/reftest to flake8 linter r=ahal
https://hg.mozilla.org/mozilla-central/rev/868a4dc22d7a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.