If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Enable py2 and py3 linter on testing/mozbase

ASSIGNED
Assigned to

Status

Testing
Mozbase
ASSIGNED
a month ago
5 days ago

People

(Reporter: ahal, Assigned: Steve Armand, Mentored, NeedInfo)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good-first-bug])

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a month ago
+++ This bug was initially created as a clone of Bug #1397423 +++

These linters can be enabled by removing 'testing/mozbase' from the exclude section in tools/lint/py2.yml and tools/lint/py3.yml.

You can then see all the errors by running:
./mach lint -l py2 -l py3 testing/mozbase

Each of the errors will need to be fixed before this can land.

To test changes, please be sure to run both:
./mach lint --outgoing
./mach python-test testing/mozbase

to be sure nothing else broke.
(Reporter)

Updated

a month ago
Blocks: 1093212
(Reporter)

Updated

a month ago
Assignee: nobody → stevea1
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 2

20 days ago
Fortunately I was able to get the lint and python-tests to run successfully. There are obviously a lot of files and edits here so let me know what's needed for cleanup, correction, etc.
(Reporter)

Comment 3

19 days ago
mozreview-review
Comment on attachment 8912427 [details]
Bug 1397849 - Enable py2 and py3 linter on testing/mozbase.

https://reviewboard.mozilla.org/r/183750/#review189810

Wow, what a huge patch, thanks so much! There are some minor issues to be fixed, mostly around reraising. Overall this is looking really good. In case you hadn't already, please also run the `flake8` linter on this directory, just in case some changes here flagged something.

Unrelated to this bug, are you using tools to automatically fix this? If so it would be awesome if we could get those hooked up to `mach lint --fix`. Might be a good follow-up bug to think about at some point.

::: commit-message-59315:1
(Diff revision 1)
> +Bug 1397849 - Enable py2 and py3 linter on testing/mochitest. r?ahal

s/mochitest/mozbase

::: testing/mozbase/manifestparser/manifestparser/expression.py:311
(Diff revision 1)
> -                             "%s\nexception: %svariables: %s" % (self.text,
> -                                                                 formatted,
> +                             "%s\nexception: %svariables: %s" %
> +                             (self.text, formatted, self.valuemapping)).with_traceback(tb)
> -                                                                 self.valuemapping)), None, tb

Unfortunately `with_traceback` is python 3 only. We're going to have to use the `six` library here.

At the top of the file, add `from six import reraise`. Then I think you just need to change the `raise` statement to:

    reraise(ParserError(...), None, tb)

::: testing/mozbase/mozcrash/mozcrash/mozcrash.py:56
(Diff revision 1)
>                        stackwalk_binary=None,
>                        dump_save_path=None,
>                        test_name=None,
>                        quiet=False):
>      """
> -    Print a stack trace for minidump files left behind by a crashing program.
> +    print(a stack trace for minidump files left behind by a crashing program.)

I think your tool for converting print statements accidentally changed this :)

::: testing/mozbase/mozcrash/mozcrash/mozcrash.py:330
(Diff revision 1)
>                               os.path.join(self.dump_save_path, os.path.basename(extra)))
>  
>  
>  def check_for_java_exception(logcat, test_name=None, quiet=False):
>      """
> -    Print a summary of a fatal Java exception, if present in the provided
> +    print a summary of a fatal Java exception, if present in the provided

Same here?

::: testing/mozbase/mozdevice/mozdevice/adb_android.py:13
(Diff revision 1)
>  import re
>  import time
>  
>  from abc import ABCMeta
>  
> -import version_codes
> +from mozdevice import version_codes

If you wanted to be consistent here, you could use:

    from . import version_codes

::: testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py:19
(Diff revision 1)
>  from distutils import dir_util
>  
> -from devicemanager import DeviceManager, DMError
> +from .devicemanager import DeviceManager, DMError
>  from mozprocess import ProcessHandler
>  import mozfile
> -import version_codes
> +from mozdevice import version_codes

ditto to above

::: testing/mozbase/mozinstall/mozinstall/mozinstall.py:112
(Diff revision 1)
> -                msg = "{} ({})".format(msg, val)
> -                raise InvalidSource, msg, tb
> +                msg = "{} ({}) {}".format(msg, val, tb)
> +                raise InvalidSource(msg)

You'll need to use the `six.reraise` trick here.

::: testing/mozbase/mozinstall/mozinstall/mozinstall.py:151
(Diff revision 1)
> -            raise InstallError, error, trbk
> +            raise InstallError(error).with_traceback(trbk)
>          # any other kind of exception like KeyboardInterrupt is just re-raised.
> -        raise cls, exc, trbk
> +        raise cls(exc).with_traceback(trbk)

You'll need to use the `six.reraise` trick here.

::: testing/mozbase/mozinstall/mozinstall/mozinstall.py:242
(Diff revision 1)
>                          raise Exception('Failure removing uninstall folder.')
>  
> -            except Exception, ex:
> +            except Exception as ex:
>                  cls, exc, trbk = sys.exc_info()
>                  error = UninstallError('Failed to uninstall %s (%s)' % (install_folder, str(ex)))
> -                raise UninstallError, error, trbk
> +                raise UninstallError(error).with_traceback(trbk)

`six.reraise`

::: testing/mozbase/mozlog/mozlog/__init__.py:20
(Diff revision 1)
> -from . import commandline
> -from . import structuredlog
> -from . import unstructured
> +from mozlog import commandline
> +from mozlog import structuredlog
> +from mozlog import unstructured

Is this change necessary? The previous format should be valid.

::: testing/mozbase/mozprofile/mozprofile/addons.py:320
(Diff revision 1)
>                          manifest = json.loads(f.read())
>                          is_webext = True
>              else:
>                  raise IOError('Add-on path is neither an XPI nor a directory: %s' % addon_path)
>          except (IOError, KeyError) as e:
> -            raise AddonFormatError(str(e)), None, sys.exc_info()[2]
> +            raise AddonFormatError(str(e))

`six.reraise`

::: testing/mozbase/mozprofile/mozprofile/addons.py:350
(Diff revision 1)
>                      # Remove the namespace prefix from the tag for comparison
>                      entry = node.nodeName.replace(em, "")
>                      if entry in details.keys():
>                          details.update({entry: get_text(node)})
>              except Exception as e:
> -                raise AddonFormatError(str(e)), None, sys.exc_info()[2]
> +                raise AddonFormatError(str(e))

`six.reraise`

::: testing/mozbase/mozprofile/mozprofile/cli.py:124
(Diff revision 1)
> -        print profile.summary()
> +        print (profile.summary())
>          return
>  
>      # if no profile was passed in print the newly created profile
>      if not cli.options.profile:
> -        print profile.profile
> +        print (profile.profile)

nit: spaces before the print function, this might cause the flake8 linter to fail too.

::: testing/mozbase/moztest/moztest/__init__.py:7
(Diff revision 1)
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> -import adapters
> +from __future__ import absolute_import
>  
> +# from moztest.adapters import *

Leftover comment?

::: testing/mozbase/mozversion/mozversion/__init__.py:8
(Diff revision 1)
>  # License, v. 2.0. If a copy of the MPL was not distributed with this file,
>  # You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> -from .errors import *
> +from __future__ import absolute_import
> +
> +import mozversion.errors

This might break some things. You'll need to leave the `from .errors import *` as is.
Attachment #8912427 - Flags: review?(ahalberstadt)
(Assignee)

Comment 4

19 days ago
The main tool I used at times was ato3. A mix of watching what it suggested and then hand-editing as well as letting it make changes (as I got more comfortable with it).

Thanks for the tip on six.reraise. I was definitely a little stuck getting past those instances you noted.

Also I was not clear (or consistent) with the import "style" as it seemed like there are different ways to get to the same end result. Sounds like using '.' (explicit relative?) is fine, so I'll plan to stick with that.
(Assignee)

Comment 5

18 days ago
Correction: the tool's name is 2to3 (not ato3).

Comment 6

18 days ago
mozreview-review
Comment on attachment 8912427 [details]
Bug 1397849 - Enable py2 and py3 linter on testing/mozbase.

https://reviewboard.mozilla.org/r/183750/#review190046

Nice job!

::: testing/mozbase/docs/_static/structured_example.py:1
(Diff revision 1)
> +from __future__ import absolute_import

Isn't this on by default in 2.7? We don't support anything but the latest python2.

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py:380
(Diff revision 1)
>              missing_paths = [test['path'] for test in missing]
>              if self.strict:
>                  raise IOError("Strict mode enabled, test paths must exist. "
>                                "The following test(s) are missing: %s" %
>                                json.dumps(missing_paths, indent=2))
> -            print >> sys.stderr, "Warning: The following test(s) are missing: %s" % \
> +            print ("Warning: The following test(s) are missing: %s" %

Another space too many here.

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py:729
(Diff revision 1)
>              if not absolute and relative_to:
>                  filenames = [relpath(filename, relative_to)
>                               for filename in filenames]
>  
>              # write to manifest
> -            print >> write, '\n'.join(['[%s]' % denormalize_path(filename)
> +            print ('\n'.join(['[%s]' % denormalize_path(filename)

And here
(Reporter)

Comment 7

18 days ago
mozreview-review-reply
Comment on attachment 8912427 [details]
Bug 1397849 - Enable py2 and py3 linter on testing/mozbase.

https://reviewboard.mozilla.org/r/183750/#review190046

> Isn't this on by default in 2.7? We don't support anything but the latest python2.

No, it's not default until 3.0
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

16 days ago
I believe I've made all the requested corrections. If not, please let me know and I'll take another run at it.
(Reporter)

Comment 11

14 days ago
mozreview-review
Comment on attachment 8912427 [details]
Bug 1397849 - Enable py2 and py3 linter on testing/mozbase.

https://reviewboard.mozilla.org/r/183750/#review191030

Thanks Steve, this is great! You'll need to mark Ms2ger's comments are resolved. For future reference, when you upload fixes you can just go though the issues you fixed and resolve them.
Attachment #8912427 - Flags: review?(ahalberstadt) → review+
(Reporter)

Comment 12

14 days ago
Looks good on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a16b493eae67c083859a7ea944f12c73aa0e828f
(Assignee)

Comment 13

13 days ago
(In reply to Andrew Halberstadt [:ahal] from comment #11)

> You'll need to mark Ms2ger's comments are
> resolved. For future reference, when you upload fixes you can just go though
> the issues you fixed and resolve them.

Thanks Andrew - I wasn't even aware this was possible (or recommended). I've done so and will be sure to do that going forward.

Comment 14

13 days ago
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9264d5789f76
Enable py2 and py3 linter on testing/mozbase. r=ahal
Sorry, this had to be backed out for import failures, e.g. in mochitests and xpcshell tests:

https://hg.mozilla.org/integration/autoland/rev/904d12098f33904e642bab4089c17700cd116e8c

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9264d5789f7622b60e2eeddddda328106aa2cfc8&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log mochitest: https://treeherder.mozilla.org/logviewer.html#?job_id=134897269&repo=autoland

[task 2017-10-04T13:22:57.683Z] 13:22:57     INFO -  Traceback (most recent call last):
[task 2017-10-04T13:22:57.684Z] 13:22:57     INFO -    File "/builds/worker/workspace/build/tests/mochitest/runtestsremote.py", line 16, in <module>
[task 2017-10-04T13:22:57.684Z] 13:22:57     INFO -      from runtests import MochitestDesktop, MessageLogger
[task 2017-10-04T13:22:57.684Z] 13:22:57     INFO -    File "/builds/worker/workspace/build/tests/mochitest/runtests.py", line 26, in <module>
[task 2017-10-04T13:22:57.687Z] 13:22:57     INFO -      import mozrunner
[task 2017-10-04T13:22:57.687Z] 13:22:57     INFO -    File "/builds/worker/workspace/build/venv/local/lib/python2.7/site-packages/mozrunner/__init__.py", line 8, in <module>
[task 2017-10-04T13:22:57.687Z] 13:22:57     INFO -      from .cli import *
[task 2017-10-04T13:22:57.687Z] 13:22:57     INFO -    File "/builds/worker/workspace/build/venv/local/lib/python2.7/site-packages/mozrunner/cli.py", line 10, in <module>
[task 2017-10-04T13:22:57.687Z] 13:22:57     INFO -      from mozprofile import MozProfileCLI
[task 2017-10-04T13:22:57.687Z] 13:22:57     INFO -    File "/builds/worker/workspace/build/venv/local/lib/python2.7/site-packages/mozprofile/__init__.py", line 16, in <module>
[task 2017-10-04T13:22:57.687Z] 13:22:57     INFO -      from mozprofile.addons import *
[task 2017-10-04T13:22:57.687Z] 13:22:57     INFO -    File "/builds/worker/workspace/build/venv/local/lib/python2.7/site-packages/mozprofile/addons.py", line 17, in <module>
[task 2017-10-04T13:22:57.687Z] 13:22:57     INFO -      from six import reraise
[task 2017-10-04T13:22:57.687Z] 13:22:57     INFO -  ImportError: No module named six
[task 2017-10-04T13:22:57.694Z] 13:22:57    ERROR - Return code: 1

Please also check the talos failures, e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=134896823&repo=autoland , and everything else on that push.
Flags: needinfo?(stevea1)
(Reporter)

Comment 16

13 days ago
Looks like this affects Android mochitest/reftests, and talos on all platforms. I guess we need to get six added to the pypi mirror? Or possibly in the docker image. I'm not sure how this works these days.

Steve, at the very least, we'll need to add "six" to the deps of each mozbase module that uses it and bump the version number. E.g: http://searchfox.org/mozilla-central/source/testing/mozbase/mozprofile/setup.py#14
(Reporter)

Updated

5 days ago
Depends on: 1407769
You need to log in before you can comment on or make changes to this bug.