1280572 - Add layout/tools/reftest to flake8 linter

Status: RESOLVED FIXED

(Testing :: Reftest, defect)

Description

[Andrew Halberstadt [:ahal]]

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.

Comment 1

[Deepa]

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?

Comment 2

[Deepa]

Attachment: patch1

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.

Comment 3

[Andrew Halberstadt [:ahal]]

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!

Comment 4

[Deepa]

Attachment: patch2

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.

Comment 5

[Andrew Halberstadt [:ahal]]

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".

Comment 6

[Deepa]

Attachment: updated_patch

Hi Andrew, I have updated the patch.

Comment 7

[Andrew Halberstadt [:ahal]]

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.

Comment 8

[Deepa]

Attachment: final_patch

Sorry for that, I missed it out.

Comment 9

[Andrew Halberstadt [:ahal]]

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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/868a4dc22d7a
Add layout/tools/reftest to flake8 linter r=ahal

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/868a4dc22d7a