Closed Bug 1280571 Opened 8 years ago Closed 8 years ago

Add testing/mochitest to flake8 linter

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: ahal, Assigned: psd, Mentored)

Details

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

Attachments

(2 files, 4 obsolete files)

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

We should add testing/mochitest 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 
I am trying to fix this issue. I am in the process of fixing all lint infractions as of now.

For fixing infractions like [1] (import should be at top of file), What do you guys recommend? 
[2] is one hack that I came across


[1]: https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runtestsremote.py#9
[2]: http://stackoverflow.com/questions/36827962/pep8-import-not-at-top-of-file-with-sys-path
Flags: needinfo?(jmaher)
great question :psd.  This is not necessarily an easy one to do.  I sort of like the idea presented in your link to stackoverflow where they moved the sys.insert to a different file and then imported that first.

i.e.:
pathfixer.py:
import os
import sys
sys.path.insert(
    0, os.path.abspath(
        os.path.realpath(
            os.path.dirname(__file__))))


then the first line is:
import pathfixer


:ahal, do you have different thoughts?
Flags: needinfo?(jmaher) → needinfo?(ahalberstadt)
Assignee: nobody → prabhjyotsingh95
Hm, maybe this is a rule we should just ignore in the global config [1]? I think people are generally pretty good at putting imports at the top, and when they don't there's usually a good reason like this. To clarify, this rule only flags module level imports (function level imports aren't flagged).

[1] https://dxr.mozilla.org/mozilla-central/source/.flake8
Flags: needinfo?(ahalberstadt)
I like what :ahal says- we can always solve the imports later- lets ignore them for now.
Thanks for suggesting the fix, I have a couple of more doubts though.

1) I have included testing/mochitest dir to the list of files that are being checked, But do we want to check the pywebsocket dir? 
2) Ignoring the E402 (module level import error not at top of file error) in the global config, leads to more lint infractions being exposed in the some testing/talos, tools/lint and taskcluster files. I think adding the "ignore = E402" in the config file is overriding some config setting that we are adding from somewhere else, but I'm not able to figure this out.

Thanks again!
Flags: needinfo?(jmaher)
Great questions!

1) Yes we should ignore the pywebsocket subdir. You can add 'testing/mochitest/pywebsocket' to the exclude list to accomplish this.

2) Huh that's really weird, I wonder why that happens. We could create a new .flake8 file in testing/mochitest and only ignore it there I guess. Or we could figure out why the extra errors are happening and fix them too.

Thanks for looking into this btw!
Flags: needinfo?(jmaher)
Ah, so from the pycodestyle documentation:
http://pep8.readthedocs.io/en/latest/intro.html#configuration

The following checks are ignored by default:
E121, E123, E126, E133, E226, E241, E242, E704 and W503 

I guess when we specify ignore ourselves, then the default ignore list gets overwritten by our specified one. So in addition to E403, we should also ignore all of the above. Please copy paste the sample message from the docs into the config as a comment so it's easy to tell what each one does.

Maybe as a follow up we should decide if we want to enable any of those disabled by default checks.
(In reply to Andrew Halberstadt [:ahal] from comment #8)
> The following checks are ignored by default:
> E121, E123, E126, E133, E226, E241, E242, E704 and W503 
This helped a lot, thanks! 

> I guess when we specify ignore ourselves, then the default ignore list gets
> overwritten by our specified one.
This makes complete sense. I have added all of the above to the global ignore list as well. (as of now)

Uploading a patch in the next comment
Attached patch 1280571.patch (obsolete) — Splinter Review
Does the following,
1) Adds testing/mochitest to the files being checked
2) excludes the dir pywebsocket in testing/mochitest
3) ignore the default ignore list and E402 (top level module import not at top of file)
4) fixes lint infractions for the included files in testing/mochitest
Attachment #8791448 - Flags: review?(ahalberstadt)
Comment on attachment 8791448 [details] [diff] [review]
1280571.patch

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

Hi :psd, thank you for submitting a patch so fast!  This looks like a great start, I just have a few small details and questions to add- I will leave the official review to :ahal :)

::: .flake8
@@ +1,2 @@
>  [flake8]
> +ignore = E121, E123, E126, E133, E226, E241, E242, E704, W503, E402

We might be ignoring these errors for all flake8 directories, not just testing/mochitest.  Maybe that is ok, but it seems as though it would be much better if we could find a way to ignore these errors just for testing/mochitest/*

::: testing/mochitest/runrobocop.py
@@ +446,5 @@
>              # that starts Fennec and then waits indefinitely, since cat
>              # never returns.
> +            browserArgs = ["start", "-n",
> +                           ("org.mozilla.roboexample.test/org.mozilla."
> +                            "gecko.LaunchFennecWithConfigurationActivity"), "&&", "cat"]

similar question to the mochitest, does adding () around a previous string allow this to run as expected?

::: testing/mochitest/runtests.py
@@ +154,5 @@
>          # Failures reporting, after the end of the tests execution
>          self.errors = []
>      def valid_message(self, obj):
> +        """True if the given object is a valid structured message
> +        (only does a superficial validation)"""

if we are using """ to make a commment, it would be consistent with other python tools to do:
"""
    msg...
    msg...
"""

of course since this is a single sentence, possible we could just do:
# msg...
# msg...

@@ +416,5 @@
>                  "httpd.js"),
>              "-e",
> +            ("const _PROFILE_PATH = '%(profile)s'; const _SERVER_PORT = '%(port)s'; "
> +             "const _SERVER_ADDR = '%(server)s'; const _TEST_PREFIX = %(testPrefix)s; "
> +             "const _DISPLAY_RESULTS = %(displayResults)s;") % {

I assume this allows us to run mochitests properly still?  My biggest question is with adding () around the previous string.

@@ +1138,5 @@
>              manifestFileAbs = os.path.abspath(options.manifestFile)
>              assert manifestFileAbs.startswith(SCRIPT_DIR)
>              manifest = TestManifest([options.manifestFile], strict=False)
> +        elif options.manifestFile and os.path.isfile(os.path.join(SCRIPT_DIR,
> +                                                                  options.manifestFile)):

could you do the newline split after the and instead?
elif options.manifestFile and
     os.path.isfile(os.path.join(SCRIPT_DIR, options.manifestFile)):
(In reply to Joel Maher ( :jmaher) from comment #11)
> ::: .flake8
> @@ +1,2 @@
> >  [flake8]
> > +ignore = , E402
> 
> We might be ignoring these errors for all flake8 directories, not just
> testing/mochitest.  Maybe that is ok, but it seems as though it would be
> much better if we could find a way to ignore these errors just for
> testing/mochitest/*
Be default, every flake dir ignores E121, E123, E126, E133, E226, E241, E242, E704, W503. I've just added E402 to all dirs. But I see your point. I will create a new flake8 config file for testing/mochitest/* and add the E402 just for that dir.


> if we are using """ to make a commment, it would be consistent with other
> python tools to do:
> """
>     msg...
>     msg...
> """
I'll make this change.

> I assume this allows us to run mochitests properly still?  My biggest
> question is with adding () around the previous string.

I had looked up online and had come across http://stackoverflow.com/questions/10660435/pythonic-way-to-create-a-long-multi-line-string

> could you do the newline split after the and instead?
> elif options.manifestFile and
>      os.path.isfile(os.path.join(SCRIPT_DIR, options.manifestFile)):
I'll make this change.
(In reply to Joel Maher ( :jmaher) from comment #11)
> @@ +1,2 @@
> >  [flake8]
> > +ignore = E121, E123, E126, E133, E226, E241, E242, E704, W503, E402
> 
> We might be ignoring these errors for all flake8 directories, not just
> testing/mochitest.  Maybe that is ok, but it seems as though it would be
> much better if we could find a way to ignore these errors just for
> testing/mochitest/*

Aside from E402, these were being ignored globally anyway. See comment 8 for the explanation. As for E402, I think personally ignoring it globally would be a good thing as we often put logic before an import all over the tree. I feel this is a case where pep8 is being too strict, though we could always discuss it on the mailing list if you disagree.

There's actually a bug in the flake8 mozlint implementation (bug 1277851) where subdir .flake8 files don't work super well. I'd prefer to avoid creating a new .flake8 file unless we really need to.

> 
> ::: testing/mochitest/runrobocop.py
> @@ +446,5 @@
> >              # that starts Fennec and then waits indefinitely, since cat
> >              # never returns.
> > +            browserArgs = ["start", "-n",
> > +                           ("org.mozilla.roboexample.test/org.mozilla."
> > +                            "gecko.LaunchFennecWithConfigurationActivity"), "&&", "cat"]
> 
> similar question to the mochitest, does adding () around a previous string
> allow this to run as expected?
> 
> ::: testing/mochitest/runtests.py
> @@ +154,5 @@
> >          # Failures reporting, after the end of the tests execution
> >          self.errors = []
> >      def valid_message(self, obj):
> > +        """True if the given object is a valid structured message
> > +        (only does a superficial validation)"""
> 
> if we are using """ to make a commment, it would be consistent with other
> python tools to do:
> """
>     msg...
>     msg...
> """

This is a docstring, not a comment. So the way it is is actually what pep 257 (https://www.python.org/dev/peps/pep-0257/) recommends doing.
Comment on attachment 8791448 [details] [diff] [review]
1280571.patch

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

Thanks very much, this looks great! Please upload a new patch with the comments fixed and flag me for review again.

::: testing/mochitest/bisection.py
@@ +224,1 @@
>                          self.contents['testsToRun'][0])

You actually don't need to use separate brackets here to span multiple lines because the string is already in a set of brackets from the append call. E.g:

self.summary.append(
    "TEST-UNEXPECTED-FAIL | %s | Bleedthrough detected, this test is the "
    "root cause for many of the above failures" % self.contents['testsToRun'][0])

I'll just leave this one comment, but please fix this in all the other places as well.

::: testing/mochitest/runtestsb2g.py
@@ +218,5 @@
>                  Services.io.offline = false;
>                  """)
>              self.marionette.execute_script("""
> +                let SECURITY_PREF = ("security.turn_off_all_security_")
> +                                     "so_that_viruses_can_take_over_this_computer");

I think this is a syntax error (keep in mind this is actually JavaScript here)

@@ +311,5 @@
>              try:
>                  os.remove(options.pidFile)
>                  os.remove(options.pidFile + ".xpcshell.pid")
>              except:
> +                print ("Warning: cleaning up pidfile '%s' was unsuccessful "

I know you were attempting to use the bracket to span multiple lines, but this is actually using the print() function instead. Which is fine, print function is actually preferred over plain 'print'. Just remove the space in-between please.

::: testing/mochitest/runtestsremote.py
@@ +192,5 @@
>          # Runtime (webapp).
>          if options.flavor == 'chrome':
>              # append overlay to chrome.manifest
> +            chrome = ("overlay chrome://browser/content/browser.xul "
> +                      "chrome://mochikit/content/browser-test-overlay.xul")

When concatenating simple strings over multiple lines, I prefer to use +. I find it a bit more readable:

chrome = "overlay chrome://browser/content/browser.xul " +
         "chrome://mochikit/content/browser-test-overlay.xul"
Attachment #8791448 - Flags: review?(ahalberstadt)
Comment on attachment 8791448 [details] [diff] [review]
1280571.patch

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

::: .flake8
@@ +1,2 @@
>  [flake8]
> +ignore = E121, E123, E126, E133, E226, E241, E242, E704, W503, E402

Would you mind adding:
# See http://pep8.readthedocs.io/en/latest/intro.html#configuration

just above?
Attached patch 1280571-2.patch (obsolete) — Splinter Review
Changes made as per :jmaher's and :ahal's reviews
Attachment #8791448 - Attachment is obsolete: true
Attachment #8792031 - Flags: review?(ahalberstadt)
Comment on attachment 8792031 [details] [diff] [review]
1280571-2.patch

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

Thanks, this looks great! r+ with comments means please upload a new patch that addresses the comments and 'carry forward the r+' (set it to +). Do you have level 1 commit access? If not I'll test this patch on try and get it landed for you.

::: testing/mochitest/leaks.py
@@ +62,5 @@
>                                   (test["fileName"], test["leakedWindowsString"]))
>  
>              if test["leakedDocShells"]:
> +                self.logger.warning(("TEST-UNEXPECTED-FAIL | %s | leaked %d docShell(s) until "
> +                                     "shutdown") %

nit: unnecessary brackets

::: testing/mochitest/runtests.py
@@ +954,5 @@
>  
>          # Write userChrome.css.
> +        chrome = ("""
> +@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");"""
> +                  """ /* set default namespace to XUL */

Rather than this weird double """ thing, just move the /* ... */ comment to the line above. Also might as well convert it to a // style comment. Then you can also remove the outer brackets as well.
Attachment #8792031 - Flags: review?(ahalberstadt) → review+
Attached patch 1280571-3.patch (obsolete) — Splinter Review
Updated with all suggested changes.

r+ carried forward from second patch
Attachment #8792031 - Attachment is obsolete: true
Attachment #8793038 - Flags: review+
Here is a try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dbd54f424eb

Because I was pushing to try anyway, I also added an appropriate commit message. But for future reference, make sure patches have a commit message of the form:
Bug <bug id> - <message>, r=<reviewer nick>

Thanks again for your contribution! If the try run looks good, I'll land this on your behalf.
Attachment #8793038 - Attachment is obsolete: true
Attachment #8793307 - Flags: review+
Status: NEW → ASSIGNED
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1d144423657
Add testing/mochitest to the flake8 linter, r=ahal
Attached patch 1280571-4.patch (obsolete) — Splinter Review
I've updated the patch and it should now pass the test
Flags: needinfo?(prabhjyotsingh95)
Attachment #8793665 - Flags: review+
Thanks for the quick patch Prabhjyot! Sorry, I was away on vacation for a couple days and didn't have a chance to look at this yesterday. It appears your patch doesn't apply cleanly anymore, would you mind pulling in the latest mozilla-central changes and rebasing the patch on top? Let me know if you need a hand with this.
Flags: needinfo?(prabhjyotsingh95)
Attached patch 1280571-5.patchSplinter Review
Rebased patch
Attachment #8793665 - Attachment is obsolete: true
Flags: needinfo?(prabhjyotsingh95)
Attachment #8795613 - Flags: review+
Thanks! I pushed your patch to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2237b0b2c592

I'll make sure that we get a job with a leak in it to make sure the previous issue is fixed, then if everything looks good I'll land it.
Actually, I made a mistake pushing, this is the real try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c26227f15072
I think it looks good, the windows mda jobs are basically permafail, so not your problem. And no stack trace on the linux bc2 job after many retriggers.
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cce196d1d7b4
Add testing/mochitest to the flake8 linter, r=ahal
https://hg.mozilla.org/mozilla-central/rev/cce196d1d7b4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.