Closed Bug 1280573 Opened 8 years ago Closed 8 years ago

Add testing/mozbase to flake8 linter

Categories

(Testing :: Mozbase, defect)

defect
Not set
normal

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: ahal, Assigned: francesco.pischedda, Mentored)

Details

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

Attachments

(1 file, 2 obsolete files)

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

We should add testing/mozbase 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.
@Andrew, I'd like to work on this, it seems like a good starting point to get accustomed to the code base.

I've added testing/mozbase to flake8.lint, ran ./mach lint -l flake8 and got a nice report of 2037 errors, so I'm ready to work on this :)
Hi Francesco, thanks for your interest! Yes 2037 is a lot of errors. I'd recommend picking a single subdirectory under testing/mozbase and tackling that first. Then you can see if you want to continue, or work on something more interesting.

You could also look into a module called "autopep8" which might fix a lot of the errors for you. You could run it locally and generate a changeset from the results. The end result of that module can sometimes look ugly though, so it may still require a bit of fine-tuning by hand afterwards.
Hi Andrew,

I've already started fixing some files, it seems like working on a zen garden! :)

Here are the ones I've workend on:

$ hg status
M testing/mozbase/mozprofile/mozprofile/profile.py
M testing/mozbase/mozprofile/mozprofile/view.py
M testing/mozbase/mozprofile/mozprofile/webapps.py
M testing/mozbase/mozprofile/setup.py
M testing/mozbase/mozprofile/tests/addon_stubs.py
M testing/mozbase/mozprofile/tests/bug785146.py
M testing/mozbase/mozprofile/tests/permissions.py
M testing/mozbase/mozprofile/tests/server_locations.py
M testing/mozbase/mozprofile/tests/test_clone_cleanup.py
M testing/mozbase/mozprofile/tests/test_nonce.py
M testing/mozbase/mozprofile/tests/test_preferences.py
M testing/mozbase/mozprofile/tests/test_profile_view.py
M testing/mozbase/mozprofile/tests/test_webapps.py
M testing/mozbase/mozrunner/mozrunner/application.py
M testing/mozbase/mozrunner/mozrunner/base/device.py
M testing/mozbase/mozrunner/mozrunner/base/runner.py
M testing/mozbase/mozrunner/mozrunner/cli.py
M testing/mozbase/mozrunner/mozrunner/devices/android_device.py
M testing/mozbase/mozrunner/mozrunner/devices/autophone.py
M testing/mozbase/mozrunner/mozrunner/devices/base.py
M testing/mozbase/mozrunner/mozrunner/devices/emulator.py
M testing/mozbase/mozrunner/mozrunner/devices/emulator_battery.py
M testing/mozbase/mozrunner/mozrunner/devices/emulator_geo.py
M testing/mozbase/mozrunner/mozrunner/devices/emulator_screen.py
M testing/mozbase/mozrunner/mozrunner/errors.py
M testing/mozbase/mozrunner/mozrunner/runners.py
M testing/mozbase/mozrunner/mozrunner/utils.py
M testing/mozbase/mozrunner/setup.py
M testing/mozbase/mozrunner/tests/mozrunnertest.py
M testing/mozbase/mozscreenshot/setup.py
M testing/mozbase/mozsystemmonitor/mozsystemmonitor/resourcemonitor.py
M testing/mozbase/moztest/moztest/adapters/unit.py
M testing/mozbase/moztest/moztest/output/base.py
M testing/mozbase/moztest/moztest/results.py
M testing/mozbase/moztest/setup.py
M testing/mozbase/setup_development.py
M testing/mozbase/test.py
M testing/mozbase/versioninfo.py
M tools/lint/flake8.lint

running $ ./mach lint -l flake8 testing/mozbase now gives us a nice:
✖ 1741 problems (1741 errors, 0 warnings)

how should I have to proceed in order to send you the patches?
Wow, that was quick! You could hg export the changes to a patch file and upload it as an attachment to this bug, but the preferred way is to push the change to our code review tool called mozreview. Using mozreview requires a fair bit of setup, so if you want to simply upload the patch that's fine. But bonus points if you set it up and push there instead ;). If you do want to set it up, you can find instructions here:
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html

Feel free to ping me or someone in #mozreview if you run into problems. Thanks again for tackling this!
Assignee: nobody → francesco.pischedda
Status: NEW → ASSIGNED
I'll try to setup mozreview, hope it will not be too much truble :)

A question, some files still do not pass the test because there are "import everything" statements and I don't know if removing them will break something (meybe they are import shortcuts), here is a cople of them:

/Users/francescopischedda/works/firefox/testing/mozbase/mozrunner/mozrunner/base/browser.py
  7:1  error  'platform' imported but unused  F401 (flake8)

/Users/francescopischedda/works/firefox/testing/mozbase/mozrunner/mozrunner/devices/__init__.py
   5:1  error  'emulator.BaseEmulator' imported but unused  F401 (flake8)
   5:1  error  'emulator.EmulatorAVD' imported but unused   F401 (flake8)
   5:1  error  'emulator.Emulator' imported but unused      F401 (flake8)
   6:1  error  'base.Device' imported but unused            F401 (flake8)
   8:1  error  'emulator_battery' imported but unused       F401 (flake8)
   9:1  error  'emulator_geo' imported but unused           F401 (flake8)
  10:1  error  'emulator_screen' imported but unused        F401 (flake8)

/Users/francescopischedda/works/firefox/testing/mozbase/moztest/moztest/__init__.py
  5:1  error  'adapters' imported but unused  F401 (flake8)

/Users/francescopischedda/works/firefox/testing/mozbase/moztest/moztest/adapters/__init__.py
  5:1  error  'unit' imported but unused  F401 (flake8)

/Users/francescopischedda/works/firefox/testing/mozbase/mozversion/mozversion/__init__.py
  5:1  error  'from .errors import *' used; unable to detect undefined names  F403 (flake8)
  5:1  error  '.errors.*' imported but unused                                 F401 (flake8)
  6:1  error  '.mozversion.get_version' imported but unused                   F401 (flake8)
  6:1  error  '.mozversion.cli' imported but unused                           F401 (flake8)

maybe I'll address those later but have you some suggestions to handle those cases ?

these instead are a bit different:

/Users/francescopischedda/works/firefox/testing/mozbase/mozrunner/mozrunner/utils.py
   13:1  error  undefined name 'uses_marionette' in __all__   F822 (flake8)
  254:9  error  do not assign a lambda expression, use a def  E731 (flake8)
  263:9  error  do not assign a lambda expression, use a def  E731 (flake8)
  272:9  error  do not assign a lambda expression, use a def  E731 (flake8)

I think that the first one can be removed safely and the other three may be replaced by a function returning a lambda but I'd like suggestions here too
pushed everything to review, see commit message for details, is there something that I can/have to do?
Comment on attachment 8795178 [details]
Bug 1280573 - Add testing/mozbase to flake8 linter:

https://reviewboard.mozilla.org/r/81304/#review80068

Whew, that's a lot of changes! I got through the first page, will have to tackle the 2nd and 3rd tomorrow. This looks great, just a few requests listed below. Also, did you use autopep8? Just want to double check that you set it to use 99 characters (instead of the default 80). Also, if you did use autopep8, please include the exact command you ran in the commit message.

Thanks again for doing this! I'll publish another review tomorrow.

::: testing/mozbase/manifestparser/tests/test_manifestparser.py:114
(Diff revision 6)
> -                         '[DEFAULT]\nfoo = bar\n\n[fleem]\nsubsuite = \n\n[include/flowers]\nblue = ocean\nred = roses\nsubsuite = \nyellow = submarine')
> +                         "[DEFAULT]\nfoo = bar"
> +                         "\n\n[fleem]\nsubsuite = "
> +                         "\n\n[include/flowers]"
> +                         "\nblue = ocean\nred = roses"
> +                         "\nsubsuite = "
> +                         "\nyellow = submarine")

Could you use triple quotes here instead please?

::: testing/mozbase/mozcrash/mozcrash/mozcrash.py:347
(Diff revision 6)
> -        # 01-30 20:15:41.937 E/GeckoAppShell( 1703): 	at android.os.Handler.handleCallback(Handler.java:587)
> +        # 01-30 20:15:41.937 E/GeckoAppShell( 1703):      at
> +        # android.os.Handler.handleCallback(Handler.java:587)

How come this line was changed but the one above (which is longer) wasn't? Because this is supposed to be a stack trace, maybe best to not break it up for now.

::: testing/mozbase/mozcrash/tests/test.py:227
(Diff revision 6)
> -               fatal_log[0] = "01-30 20:15:41.937 E/GeckoAppShell( 1703): >>> FATAL EXCEPTION FROM THREAD 9 (\"GeckoBackgroundThread\")"
> +        fatal_log[
> +            0] = "01-30 20:15:41.937 E/GeckoAppShell( 1703):" \
> +                 " >>> FATAL EXCEPTION FROM THREAD 9 (\"GeckoBackgroundThread\")"

This got broken up pretty weirdly.

::: testing/mozbase/mozcrash/tests/test.py:246
(Diff revision 6)
> -               passable_log[0] = "01-30 20:15:41.937 E/GeckoAppShell( 1703): >>> NOT-SO-BAD EXCEPTION FROM THREAD 9 (\"GeckoBackgroundThread\")"
> +        passable_log[
> +            0] = "01-30 20:15:41.937 E/GeckoAppShell( 1703):" \
> +                 " >>> NOT-SO-BAD EXCEPTION FROM THREAD 9 (\"GeckoBackgroundThread\")"

Same here

::: testing/mozbase/mozdevice/mozdevice/Zeroconf.py:86
(Diff revision 6)
> +__author__ = "Paul Scott-Murphy"
> +__email__ = "paul at scott dash murphy dot com"
> +__version__ = "0.12"
> +

This whole file is imported from an external source, so we shouldn't modify it. Please revert all the changes to it and we can exclude it in the flake8.lint file.

::: testing/mozbase/mozdevice/mozdevice/devicemanager.py:570
(Diff revision 6)
>      def _getLocalHash(filename):
>          """
>          Return the MD5 sum of a file on the host.
>          """
>          f = open(filename, 'rb')
> -        if (f == None):
> +        if (f is None):

Please remove brackets here
Comment on attachment 8795178 [details]
Bug 1280573 - Add testing/mozbase to flake8 linter:

https://reviewboard.mozilla.org/r/81304/#review80344

Thanks again for doing all this work, it's impressive!

Fyi, when you fix and re-push, normally it's up to the patch author (you) to mark the issue you've fixed as resolved. You can also reply to comments or questions I've asked directly in mozreview and they will be copied over to bugzilla after publishing.

I can't r+ this just yet because of the 90 or so remaining failures. We can either get those fixed as well and land everything together, or we can land this patch for now (assuming it passes try) but remove 'testing/mozbase' from flake8.lint until the other 90 are fixed.

::: testing/mozbase/manifestparser/tests/test_manifestparser.py:114
(Diff revisions 6 - 7)
> -                         "[DEFAULT]\nfoo = bar"
> -                         "\n\n[fleem]\nsubsuite = "
> -                         "\n\n[include/flowers]"
> -                         "\nblue = ocean\nred = roses"
> -                         "\nsubsuite = "
> -                         "\nyellow = submarine")
> +                         """[DEFAULT]\nfoo = bar
> +                         \n\n[fleem]\nsubsuite =
> +                         \n\n[include/flowers]
> +                         \nblue = ocean\nred = roses
> +                         \nsubsuite =
> +                         \nyellow = submarine""")

So this won't actually work, because it will insert extra newlines. You'll need to change it to something like:

    self.assertEqual(..., """
    [DEFAULT]
    foo = bar
    
    [fleem]
    subsuite =
    
    [include/flowers]
    blue = ocean
    red = roses
    subsuite =
    yellow = submarine""")
    
Possibly storing the string in a variable beforehand. It just makes it easier to see what the expected output is.

::: testing/mozbase/mozinstall/tests/test.py:90
(Diff revision 7)
>                  import pefile
> +                print pefile  # useless print for 'unused' import

Instead add the comment:

    import pefile  # noqa
    
and flake8 will ignore that line.

::: testing/mozbase/mozleak/mozleak/__init__.py:9
(Diff revision 7)
>  from .leaklog import process_leak_log
> +
> +__all__ = ['process_leak_log']

I meant to ask, adding __all__ is ok I guess, but what rule is this fixing?

::: testing/mozbase/mozlog/mozlog/commandline.py:144
(Diff revision 7)
> -    for optname, (cls, help_str, formatters, action) in fmt_options.iteritems():
> -        for fmt in formatters:
> +    for optname, (cls, help_str, formatters_, action) in fmt_options.iteritems():
> +        for fmt in formatters_:

Why this change?

::: testing/mozbase/mozprocess/mozprocess/qijo.py:7
(Diff revision 7)
> -from ctypes import c_void_p, POINTER, sizeof, Structure, windll, WinError, WINFUNCTYPE, addressof, c_size_t, c_ulong
> +from ctypes import c_void_p, POINTER, sizeof, Structure, windll, WinError, WINFUNCTYPE
> +from ctypes import addressof, c_size_t, c_ulong

When there are a lot of imports our preferred style is:

    from ctypes import (
        c_void_p,
        POINTER,
        ...
    )

::: testing/mozbase/mozprocess/mozprocess/winprocess.py:458
(Diff revision 7)
> -    process = Popen(command)
> +    import subprocess
> +    process = subprocess.Popen(command)
>      process.kill()
>      code = process.returncode
>      print 'Child code: %s' % code
>      assert code == 127
> -        
> +
> +
>  def child():
>      print 'Starting child'
>      currentProc = GetCurrentProcess()
>      injob = IsProcessInJob(currentProc)
>      print "Is in a job?: %s" % injob
>      can_create = CanCreateJobObject()
>      print 'Can create job?: %s' % can_create
> -    process = Popen('c:\\windows\\notepad.exe')
> +    import subprocess
> +    process = subprocess.Popen('c:\\windows\\notepad.exe')

Please add the import at the top of the file.

::: testing/mozbase/mozprofile/mozprofile/profile.py:334
(Diff revision 7)
> +                                ',',
> +                                ',\n').splitlines())

Put these on the same line.

::: testing/mozbase/mozprofile/mozprofile/webapps.py
(Diff revision 7)
> -
>  from string import Template
>  import json
>  import os
>  import shutil
> -

The space here is on purpose to denote that mozfile is not part of the stdlib.

::: testing/mozbase/mozprofile/tests/addonid.py:54
(Diff revision 7)
> -     <em:description>A testing extension based on the Windmill Testing Framework client source</em:description>
> +     <em:description>A testing extension based on the
> +            Windmill Testing Framework client source</em:description>

This may cause test failures if this gets thrown in a string comparison later on. But we'll find out if it does or not when I push to try.

::: testing/mozbase/mozrunner/mozrunner/cli.py:94
(Diff revision 7)
>                            help="arguments to the debugger")
>          parser.add_option('--interactive', dest='interactive',
>                            action='store_true',
>                            help="run the program interactively")
>  
> -    ### methods for running
> +    # ## methods for running

Remove the other two #'s

::: testing/mozbase/mozrunner/mozrunner/devices/android_device.py:141
(Diff revision 7)
> -            err = 'environment variable MOZ_HOST_BIN is not set to a directory containing host xpcshell'
> +            err = """environment variable MOZ_HOST_BIN is not set to a directory
> +            containing host xpcshell"""

This will create a new line in the string, please use single quotes with \ instead.
Comment on attachment 8795178 [details]
Bug 1280573 - Add testing/mozbase to flake8 linter:

https://reviewboard.mozilla.org/r/81304/#review80344

> Why this change?

because it could shadow this import:
from . import formatters
Comment on attachment 8795178 [details]
Bug 1280573 - Add testing/mozbase to flake8 linter:

https://reviewboard.mozilla.org/r/81304/#review80344

> Instead add the comment:
> 
>     import pefile  # noqa
>     
> and flake8 will ignore that line.

nice, I didn't know about # noqa

> I meant to ask, adding __all__ is ok I guess, but what rule is this fixing?

It fixes the "imported but unused" F402 error
Comment on attachment 8795178 [details]
Bug 1280573 - Add testing/mozbase to flake8 linter:

https://reviewboard.mozilla.org/r/81304/#review80622

Thanks again for the quick fixes, almost there :)

Also about the wildcard imports in the \_\_init\_\_.py, I would just add a file-level ignore directive to the top of the file like this:
    
    # flake8: noqa
    
This way the whole file gets skipped, which is fine. There isn't much logic in there anyway. The next step is to examine the remaining failures and decide whether to fix them first or land what we have. I'll see what there is later today.

::: testing/mozbase/manifestparser/tests/test_manifestparser.py:113
(Diff revisions 7 - 8)
> -        self.assertEqual(buffer.getvalue().strip(),
> +        expected_output = """[DEFAULT]\nfoo = bar
> -                         """[DEFAULT]\nfoo = bar
> -                         \n\n[fleem]\nsubsuite =
> +\n\n[fleem]\nsubsuite =
> -                         \n\n[include/flowers]
> +\n\n[include/flowers]
> -                         \nblue = ocean\nred = roses
> +\nblue = ocean\nred = roses
> -                         \nsubsuite =
> +\nsubsuite =
> -                         \nyellow = submarine""")
> +\nyellow = submarine"""

Sorry, this still isn't good enough. A triple quoted python string actually inserts newlines on line breaks. So for example:

    foo = "hello\nworld"
    bar = """hello
    world"""
    
    foo == bar
    > True

So you'll still need to remove the newline characters and insert the correct number of line breaks into the string (you'll need the exact string from my previous comment).

You can run `./mach python-test testing/mozbase/manifestparser` (after a build) to verify that the test passes correctly.

::: testing/mozbase/mozprocess/mozprocess/qijo.py:17
(Diff revisions 7 - 8)
> +    windll,
> +    WinError,
> +    WINFUNCTYPE,
> +    addressof,
> +    c_size_t,
> +    c_ulong)

Put the ')' on a newline please.
I was looking at the remaining failures, and I think we can get almost all the way there mostly by skipping:

1. Add "testing/mozbase/mozdevice/mozdevice/Zeroconf.py" to the 'exclude' list in flake8.lint
2. Add "# flake8: noqa" to all __init__.py files with * imports
3. Add "# flake8: noqa" to testing/mozbase/mozlog/mozlog/unstructured/logger.py (this file should probably be deleted anyway)
4. Add "# noqa" to lines that exceed 100 characters where appropriate.

I think this should get us down to just a handful of errors we can investigate one by one.
Thanks to these changes we are very very close to the end! :)

what's left is not very trivial and some knowledge of the code base is required, here is the list of the final issues:

/Users/francescopischedda/works/firefox/testing/mozbase/mozinfo/mozinfo/mozinfo.py
  203:8   error  undefined name 'isLinux'  F821 (flake8)
  203:19  error  undefined name 'isBsd'    F821 (flake8)

/Users/francescopischedda/works/firefox/testing/mozbase/mozlog/mozlog/formatters/tbplformatter.py
  244:9  error  undefined name 'message'  F821 (flake8)

/Users/francescopischedda/works/firefox/testing/mozbase/moznetwork/moznetwork/moznetwork.py
  159:5   error  undefined name 'structured'  F821 (flake8)
  161:28  error  undefined name 'structured'  F821 (flake8)
  165:5   error  undefined name 'structured'  F821 (flake8)
(In reply to Francesco Pischedda from comment #23)
> /Users/francescopischedda/works/firefox/testing/mozbase/mozinfo/mozinfo/
> mozinfo.py
>   203:8   error  undefined name 'isLinux'  F821 (flake8)
>   203:19  error  undefined name 'isBsd'    F821 (flake8)

The line before this with globals() is actually defining these variables, but it is tricking the linter. The code is fine though, so I'd just add # noqa to this.

> 
> /Users/francescopischedda/works/firefox/testing/mozbase/mozlog/mozlog/
> formatters/tbplformatter.py
>   244:9  error  undefined name 'message'  F821 (flake8)

This one looks like an error. I think that whole line needs to be replaced with:
    return fmt.format(**data)

> /Users/francescopischedda/works/firefox/testing/mozbase/moznetwork/
> moznetwork/moznetwork.py
>   159:5   error  undefined name 'structured'  F821 (flake8)
>   161:28  error  undefined name 'structured'  F821 (flake8)
>   165:5   error  undefined name 'structured'  F821 (flake8)

These should all be "mozlog.commandline" instead of "structured.commandline"
and finally we obtained 
~/works/firefox ᐅ ./mach lint -l flake8 testing/mozbase
✖ 0 problems (0 errors, 0 warnings)

thank you very much for your help!
Awesome! \o/

There are still two issues on the review request. You can see the first one causes a test failure when you run:
./mach python-test --path-only testing/mozbase/manifestparser/tests/test_manifestparser.py

Once you've fixed those, I'll push your patch to try (or do you have level 1 commit access?). I expect there will be a bunch of other failures too, but we can fix them as they come up.
addressed the issue and now we have a nice:

~/works/firefox ᐅ ./mach python-test --path-only testing/mozbase/manifestparser/tests/test_manifestparser.py
 0:00.25 /Users/francescopischedda/works/firefox/obj-x86_64-apple-darwin16.0.0/_virtualenv/bin/python testing/mozbase/manifestparser/tests/test_manifestparser.py
 0:00.35 ...Included file '/Users/francescopischedda/works/firefox/testing/mozbase/manifestparser/tests/invalid.ini' does not exist
 0:00.36 ............
 0:00.36 ----------------------------------------------------------------------
 0:00.36 Ran 15 tests in 0.017s
 0:00.36
 0:00.36 OK

:)

> Once you've fixed those, I'll push your patch to try (or do you have level 1 commit access?). I expect there will be a bunch of other failures too, but we can fix them as they come up.

I don't think I have access to try, I've only setup review, have you some link I can read to get it?
So I pushed to try and there is a single error that is causing everything to fail:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1171896759d7d8ba50f4fcedc44a49f818dc3bbd

Here's the trace:
10:15:21     INFO -  Traceback (most recent call last):
10:15:21     INFO -    File "/tools/python27/lib/python2.7/threading.py", line 551, in __bootstrap_inner
10:15:21     INFO -      self.run()
10:15:21     INFO -    File "/tools/python27/lib/python2.7/threading.py", line 504, in run
10:15:21     INFO -      self.__target(*self.__args, **self.__kwargs)
10:15:21     INFO -    File "/builds/slave/test/build/venv/lib/python2.7/site-packages/mozprocess/processhandler.py", line 969, in _read
10:15:21     INFO -      callback(line.rstrip())
10:15:21     INFO -    File "/builds/slave/test/build/venv/lib/python2.7/site-packages/mozprocess/processhandler.py", line 887, in __call__
10:15:21     INFO -      e(*args, **kwargs)
10:15:21     INFO -    File "/builds/slave/test/build/tests/mochitest/runtests.py", line 2542, in processOutputLine
10:15:21     INFO -      message = handler(message)
10:15:21     INFO -    File "/builds/slave/test/build/tests/mochitest/runtests.py", line 2626, in fix_stack
10:15:21     INFO -      message['message'] = self.stackFixerFunction(message['message'])
10:15:21     INFO -    File "/builds/slave/test/build/venv/lib/python2.7/site-packages/mozrunner/utils.py", line 270, in <lambda>
10:15:21     INFO -      return lambda line: stack_fixer_module.fixSymbols(line)
10:15:21     INFO -  TypeError: fixSymbols() takes exactly 2 arguments (1 given)
There's also another manifestparser test failure I missed before, you can see it by running:
./mach python-test --path-only testing/mozbase/manifestparser/tests/test_convert_directory.py
fixed that with the latest push, please let me know for other issues
question: how can I run a complete test suite to shorten the ping-pong time ? (I'd like not to waste your time)
Mozbase is a bit tricky, because it is a series of low-level (in our python stack at least) libraries that are used all over the place throughout mozilla-central. So it's really hard to test it comprehensively on your local machine. You'd have to run every test suite on every platform (would take days of compute time) and even then would probably miss something.

Instead we have something called try server (or just "try"). It's basically like pushing to production except your changes don't actually land. All the same builds and tests happen though. To push to try you need level 1 commit access, so I don't mind pushing it on your behalf for now. If you plan to continue contributing to Mozilla for awhile, you could request commit access by following this procedure:
https://www.mozilla.org/en-US/about/governance/policies/commit/

It's up to you really. It wouldn't happen quickly enough for this bug, but could be useful for future bugs. In the meantime, it's no trouble at all for me to handle this. Would you be interested in starting work on something else in the meantime while we wait for try results to come back in?
Oh and it looks like something broke from comment 31 which needs to be fixed, and there's one last open issue on the review request :).

Thanks for sticking with this, I know it's a lot of work!
The latest mozreview request should hopefully contain a fix for the problem mentioned in comment31

Thank you for taking the time to push to try :)

I'd really like to continue contributing and in fact, while waiting for results, I've started and almost finished working on another simple issue (https://bugzilla.mozilla.org/show_bug.cgi?id=1291687) :)

At this point I'd like to address more complicated issues, do you know about something you think I can work on?

Meanwhile I'll start the procedure to gain commit access level 1
Done my request to gain access level 1 https://bugzilla.mozilla.org/show_bug.cgi?id=1306575

Please vouch :)
Vouched. Keeping in the mozlint vein, maybe bug 1285555 would be a good fit. It's something that's important, but no one has had time to tackle. It shouldn't be too hard, but is also a lot more involved than fixing lint error ;). Let me know if that interests you. If not, I can find something in another area of the code base if you prefer.
Thank you!

I've had a look at 1285555 and it seems interesting, thanks for this too :)
Uh oh, looks like there are a bunch of new flake8 failures:
https://public-artifacts.taskcluster.net/VsVmP5_NTJ6XXcqGpvFD3Q/0/public/logs/live_backing.log

You may need to pull mozilla-central and rebase your patch on top to see some of them.
Hey Francesco, somehow your changes were pushed as three separate commits. Would you mind squashing them and re-pushing? You can use the 'hg histedit' command to accomplish this. I'll push the changes to try again in the meantime.
Flags: needinfo?(francesco.pischedda)
Attachment #8796555 - Attachment is obsolete: true
Attachment #8796555 - Flags: review?(ahalberstadt)
Attachment #8796556 - Attachment is obsolete: true
Attachment #8796556 - Flags: review?(ahalberstadt)
Hi Andrew,

in the last push review I have only one MozReview-Commit-ID because of a "mid air collision" detected by the system, hope to have choosen the correct one :)
Thanks Francesco! I don't think the MozReview-Commit-ID matters too much, either one should work.

Here are the results of the try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ffc25b5252844def045613b623d6257f9f94593

This is treeherder, our continuous integration reporting system. Every symbol there represents a "task". Green means the task passed, red or orange means the task failed. Before we can land this patch, we need to try and get (almost) all of those tasks to appear green. To see why the task failed, you can click the symbol, then in the bottom left hand side there will be a little "log" button. You can press this to open up the log which should contain the failure messages. In this try run, I see four general failures:

1. The f8 (flake8) task is orange. This means there are some new lint errors that will need to be fix.
2. The B (build) tasks are red. This is likely because a mozbase test is failing (they run as part of the build).
3. A bunch of Android tasks are orange, there is a traceback in the log but we'll need to investigate further.
4. Some windows C (crashtest) tasks are red, not sure why.. we'll have to investigate, it might be unrelated to your patch.

I'd recommend looking into the first two problems first, then we can push a new try run and see if the other two are still there. Once you have the first two fixed, please amend your commit, pull central and rebase it with:
$ hg commit --amend
$ hg pull central
$ hg rebase -r <revision> -d central

I know it's a lot to take in and probably somewhat overwhelming, but don't worry.. there's no time limit and we can take it one step at a time :). Please don't hestitate to ask if you have any questions.
Flags: needinfo?(francesco.pischedda)
Hello Andrew,

thank you for this detailed report and for the correct procedure to rebase (the other time I've messed up things a bit :) )

I'll try to address those test ASAP
Hi again,

I should have fixed the lint failing tests, I have rebased and pushed for review

A question, to rebase I've user hg rebase -d central without -r option, can this be a problem?
No that's fine. By default hg will rebase the current changeset, so as long as you are updated to the revision you want to rebase, it'll work the same.
Comment on attachment 8795178 [details]
Bug 1280573 - Add testing/mozbase to flake8 linter:

https://reviewboard.mozilla.org/r/81304/#review82214

::: testing/mozbase/manifestparser/tests/test_filters.py:20
(Diff revision 19)
> +def foo(x, y):
> +    return x
> +
> +a = b = c = d = e = f = bar = baz = foo

This is causing test failures if you look at the log:
https://treeherder.mozilla.org/logviewer.html#?job_id=28431321&repo=try#L38984

It looks like each variable needs to be a distinct function object. So you'll need to undo this change and put all the lambda's back. Please add # noqa to ignore them all.
Comment on attachment 8795178 [details]
Bug 1280573 - Add testing/mozbase to flake8 linter:

https://reviewboard.mozilla.org/r/81304/#review82272

There is still a failure in the Android tests (see try run in this review request), for some reason the tests aren't starting. I'll take a closer look at the diff when I get a chance to see if I can spot anything :/

::: testing/mozbase/manifestparser/tests/test_filters.py:24
(Diff revision 20)
> -        foo = lambda x, y: x
> -        bar = lambda x, y: x
> -        baz = lambda x, y: x
> +        foo = lambda x, y: x  # noqa
> +        bar = lambda x, y: x  # noqa
> +        baz = lambda x, y: x  # noqa

Hm, if you look at the try push attached to this review, you'll see the f8 task is still failing because of this. Apparently this is a bug in flake8 itself:
https://github.com/PyCQA/pycodestyle/issues/379

I guess we can just add a global:

    # flake: noqa

to the top of the file and remove all these line # noqa's instead.
Thanks for being so quick with your updates! Here is the latest try run I ran:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=876e3c7bfdd8ab893e9dd196abcd0f16a31a0140

So it looks like it's just the Android failures left. I'm still trying to figure out what is wrong, it's tricky. If you are looking for something else to do in the meantime, now would be a good time to start looking at something like bug 1285555.
Sorry for the delay, I got really busy and there was a holiday weekend here. I've narrowed the problem down to something in mozdevice with try bisection. I'll keep sending out try runs to see if I can find which file is causing problems.
As you told me before, we will solve one thing at a time without any rush, don't worry ;)
Comment on attachment 8795178 [details]
Bug 1280573 - Add testing/mozbase to flake8 linter:

https://reviewboard.mozilla.org/r/81304/#review83840

::: testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py:407
(Diff revision 21)
>          acmd.append(cmd[0:i] + "/org.mozilla.gecko.BrowserApp")
>          if args != "":
>              acmd.append("--es")
>              acmd.append("args")
>              acmd.append(args)
> -        if env != '' and env != None:
> +        if env != '' and env is None:

Ah, I think I found the bug! Should be "is not"
Comment on attachment 8795178 [details]
Bug 1280573 - Add testing/mozbase to flake8 linter:

https://reviewboard.mozilla.org/r/81304/#review83840

> Ah, I think I found the bug! Should be "is not"

so sorry about this, glad you tracked it :)
Comment on attachment 8795178 [details]
Bug 1280573 - Add testing/mozbase to flake8 linter:

https://reviewboard.mozilla.org/r/81304/#review83898

No worries, thanks for sticking through with this and for all the quick updates! I'll do one last try run just to make sure no new lint errors got added, then I'll land it for you if everything looks good.
Attachment #8795178 - Flags: review?(ahalberstadt) → review+
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f7b6a16eb1f6
Add testing/mozbase to flake8 linter: r=ahal
https://hg.mozilla.org/mozilla-central/rev/f7b6a16eb1f6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Congrats on getting your patch merged and thanks for your contribution! Btw, you got a shout out in our last team meeting ;)
https://wiki.mozilla.org/EngineeringProductivity/Meetings/2016-10-03
Wow that's a big honour!

I'm getting ready to address the other issue, can't wait to start this week end :)

Thank you very much for helping!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: