Closed Bug 1397849 Opened 7 years ago Closed 7 years ago

Enable py2 and py3 linter on testing/mozbase

Categories

(Testing :: Mozbase, enhancement)

Product:
Testing
Component:
Mozbase
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: ahal, Assigned: stevea1, Mentored)

References

Details

(Whiteboard: [good-first-bug])

Attachments

(1 file)

Bug 1397849 - Enable py2 and py3 linter on testing/mozbase.
59 bytes, text/x-review-board-request
ahal
: review+
Details 
+++ 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.
Blocks: mozbase-py3
Assignee: nobody → stevea1
Status: NEW → ASSIGNED
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.
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)
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.
Correction: the tool's name is 2to3 (not ato3).
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
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
I believe I've made all the requested corrections. If not, please let me know and I'll take another run at it.
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+
(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.
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)
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
Depends on: 1407769
Hey Steve, I thought we needed to make six available in the test machine environments, but it turns out we already have it available in the internal pypi mirror. This means the only change we should need to make here is adding 'six' as a dependency to the setup.py files of mozbase modules that use it.

Could you push up a new change with the dependency added?
Can do. Essentially I'd need to add this to any existing setup.py files found under testing/mozbase?

install_requires=[
   'six'
]
Flags: needinfo?(stevea1) → needinfo?(ahalberstadt)
Yes, exactly. You might as well add 'six >= 1.10.0' as that is the version that is in-tree (and on the internal pypi mirror).

Also, it would be a good idea to bump the minor version number of each package you add this dependency to. So for example, in manifestparser, you'd bump the version to 1.2.
Flags: needinfo?(ahalberstadt)
I rebased the files and then updated the 3 setup.py files. Should be ready to try.
Flags: needinfo?(ahalberstadt)
Blocks: 1397853
I think it's good to re-land:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1564f2e297eaefbab150cab445e7867bec1155c8

Made sure some of the jobs that were previously failing now pass.
Flags: needinfo?(ahalberstadt)
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/02a070f1901a
Enable py2 and py3 linter on testing/mozbase. r=ahal
https://hg.mozilla.org/mozilla-central/rev/02a070f1901a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
This broke `./mach xpcshell-test --debugger=whatever`:

$ ./mach xpcshell-test --debugger=rr security/manager/ssl/tests/unit/test_signed_apps.js 
Elapsed: 2.28s; From _tests: Kept 32695 existing; Added/updated 0; Removed 0 files and 0 directories.
Error running mach:

    ['xpcshell-test', '--debugger=rr', 'security/manager/ssl/tests/unit/test_signed_apps.js']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

AttributeError: 'module' object has no attribute 'get_debugger_info'

  File "/home/keeler/mozilla-unified/testing/xpcshell/mach_commands.py", line 266, in run_xpcshell_test
    return xpcshell.run_test(**params)
  File "/home/keeler/mozilla-unified/testing/xpcshell/mach_commands.py", line 60, in run_test
    return self.run_suite(**kwargs)
  File "/home/keeler/mozilla-unified/testing/xpcshell/mach_commands.py", line 46, in run_suite
    return self._run_xpcshell_harness(**kwargs)
  File "/home/keeler/mozilla-unified/testing/xpcshell/mach_commands.py", line 136, in _run_xpcshell_harness
    result = xpcshell.runTests(filtered_args)
  File "/home/keeler/mozilla-unified/testing/xpcshell/runxpcshelltests.py", line 1215, in runTests
    self.debuggerInfo = mozdebug.get_debugger_info(options.get('debugger'),
Flags: needinfo?(stevea1)
Depends on: 1411776
(In reply to David Keeler [:keeler] (use needinfo?) from comment #25)
> This broke `./mach xpcshell-test --debugger=whatever`:

And './mach run --debugger=whatever'.
Hey Andrew, any insight on this?
Flags: needinfo?(stevea1) → needinfo?(ahalberstadt)
Looks like the mozdebug.__init__.py import needs to be relative. Let's fix this in bug 1411776
Flags: needinfo?(ahalberstadt)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: