Closed Bug 1407763 Opened 6 years ago Closed 5 years ago

Enable py2/py3 compat linters on testing/marionette

Categories

(Remote Protocol :: Marionette, enhancement, P3)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
enhancement

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: ahal, Assigned: iceman, Mentored)

References

Details

(Whiteboard: [lang=py])

User Story

To get started with contributing to Marionette please read through the following document:

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/doc/NewContributors.md

In case of questions just ask on the bug, or reach us on irc://irc.mozilla.org/#ateam.

Attachments

(2 files, 4 obsolete files)

Bug 1407763 - Enable py2/py3 compat linters on testing/marionette.
59 bytes, text/x-review-board-request
whimboo
: review-
Details
Bug 1407763 - Enable py2 and py3 compat linters for testing/marionette.
59 bytes, text/x-review-board-request
ahal
: review+
Details 
These linters can be enabled by removing 'testing/marionette' 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/marionette

Each of the errors will need to be fixed before this can land.
I'm happy to mentor this bug.
Mentor: hskupin
Whiteboard: [lang=py]
I'm ready to work on this, Please guide to get started on this?
Flags: needinfo?(vaibhavmule135)
User Story: (updated)
Hi Vaibhavmule. Sorry for being late here. But I missed your last comment, and you seem to have accidentally set yourself as needinfo. 

I just updated the user story, which you can find at the above section of this page. Go through the steps and get comfortable with Marionette. If you have questions around the task please let me know.
I'm take the ticket.
Not taking up this, anybody can take this one.
Flags: needinfo?(vaibhavmule135)
Assignee: nobody → freshjelly12
Status: NEW → ASSIGNED
Comment on attachment 8934927 [details]
Bug 1407763 - Enable py2/py3 compat linters on testing/marionette.

https://reviewboard.mozilla.org/r/205562/#review211504

::: commit-message-f54b0:2
(Diff revision 1)
> +Bug 1407763 - Enable py2/py3 compat linters on testing/marionette. r?whimboo
> +

You mentioned that this single change fixes all of the linter failures, right? If that is true then please also include the changes to the linter config in such that the py2/py3 linter is run against `testing/marionette`. Right now this is not the case and doesn't match your commit message.

::: testing/marionette/harness/marionette_harness/tests/unit/test_execute_sandboxes.py:5
(Diff revision 1)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # 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 __future__ import absolute_import

There has to be an empty line between this import and any others. You will have to make this change to all files.

::: testing/marionette/harness/marionette_harness/tests/unit/test_expected.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 urllib
>  
> +from __future__ import absolute_import 

Make sure to not introduce trailing white-spaces. Check your changes by running `mach lint testing/marionette`.

::: testing/marionette/mach_commands.py:6
(Diff revision 1)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # 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 __future__ import absolute_import, unicode_literals
> +from __future__ import print_function 

Don't import from `__future__` twice. Just add it to the list in alphabetical order.

::: testing/marionette/mach_test_package_commands.py:6
(Diff revision 1)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # 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 __future__ import absolute_import
> +from __future__ import print_function

A single import line please.
Attachment #8934927 - Flags: review?(hskupin) → review-
Comment on attachment 8935341 [details]
Bug 1407763 - Fix the flake erorrs, and add spaces between import statements

https://reviewboard.mozilla.org/r/206240/#review211836

::: npm-shrinkwrap.json
(Diff revision 1)
>  {
>    "name": "mozillaeslintsetup",
> -  "requires": true,

I'm not sure why you are modifying this file. It has nothing to do with this patch. Please revert those changes.

::: testing/marionette/harness/marionette_harness/tests/unit/test_execute_sandboxes.py:6
(Diff revision 1)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # 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 __future__ import absolute_import
> +

Please combine this commit with the one you pushed yesterday by using `hg histedit`.

::: testing/marionette/mach_test_package_commands.py:6
(Diff revision 1)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # 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 __future__ import absolute_import
> -from __future__ import print_function
> +from __future__ import (
> +    absolute_import, 

You are still adding trailing spaces.
Attachment #8935341 - Flags: review?(hskupin) → review-
Comment on attachment 8935348 [details]
Bug 1407763 - Fixed a part of lint errors, not finished yet

https://reviewboard.mozilla.org/r/206242/#review211842

Can you please explain what this commit is for? As mentioned in my review for the other new commit, please combine all into a single commit. And use the commit message as done for the very first commit. Thanks.
Attachment #8935348 - Flags: review?(hskupin) → review-
(In reply to Henrik Skupin (:whimboo) from comment #10)
> Comment on attachment 8935341 [details]
> Bug 1407763 - Fix the flake erorrs, and add spaces between import statements
> 
> https://reviewboard.mozilla.org/r/206240/#review211836
> 
> ::: npm-shrinkwrap.json
> (Diff revision 1)
> >  {
> >    "name": "mozillaeslintsetup",
> > -  "requires": true,
> 
> I'm not sure why you are modifying this file. It has nothing to do with this
> patch. Please revert those changes.
> 
> :::
> testing/marionette/harness/marionette_harness/tests/unit/
> test_execute_sandboxes.py:6
> (Diff revision 1)
> >  # This Source Code Form is subject to the terms of the Mozilla Public
> >  # 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 __future__ import absolute_import
> > +
> 
> Please combine this commit with the one you pushed yesterday by using `hg
> histedit`.
> 
> ::: testing/marionette/mach_test_package_commands.py:6
> (Diff revision 1)
> >  # This Source Code Form is subject to the terms of the Mozilla Public
> >  # 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 __future__ import absolute_import
> > -from __future__ import print_function
> > +from __future__ import (
> > +    absolute_import, 
> 
> You are still adding trailing spaces.


> I'm not sure why you are modifying this file. It has nothing to do with this
> patch. Please revert those changes.
Actually, I'm altogether didn't open the file, no ideas why the changes happened.
(In reply to Henrik Skupin (:whimboo) from comment #11)
> Comment on attachment 8935348 [details]
> Bug 1407763 - Fixed a part of lint errors, not finished yet
> 
> https://reviewboard.mozilla.org/r/206242/#review211842
> 
> Can you please explain what this commit is for? As mentioned in my review
> for the other new commit, please combine all into a single commit. And use
> the commit message as done for the very first commit. Thanks.

Okay, I'll combine all into one single commit. I thought it would be better to split into a couple of files, in order to make a review to easier, sorry.
(In reply to Mike Yusko(:iceman) from comment #14)
> Okay, I'll combine all into one single commit. I thought it would be better
> to split into a couple of files, in order to make a review to easier, sorry.

This is helpful for patches which fix different things on the same bug. But it's less helpful to add code, and then refactor that code in the next commit. The to land patches should be fine on their own.
Okay, got it! sorry for the little problem, I'll fix all things in 2-3 days.
Comment on attachment 8935360 [details]
Bug 1407763 - Fixed all lint errors, when the 'testing/marionette' section was removed

https://reviewboard.mozilla.org/r/206252/#review213378

I still don't see changes to tools/lint/py2.yml and tools/lint/py3.yml which would enable those checks by default. Further I wonder how you push the patches to mozreview, because each update ends-up as a separate commit. Please make sure to squash your changes (hg histedit).

::: testing/marionette/client/marionette_driver/geckoinstance.py:357
(Diff revision 1)
>                  self.runner.device.connect()
>              self.runner.start()
>          except Exception as e:
>              exc, val, tb = sys.exc_info()
>              message = "Error possibly due to runner or device args: {}"
> -            raise exc, message.format(e.message), tb
> +            raise (exc, message.format(e.message), tb)

Shouldn't this be `raise exc(message.format(e.message), tb)`?

::: testing/marionette/client/marionette_driver/marionette.py:798
(Diff revision 1)
>          exc, val, tb = sys.exc_info()
>  
>          # If the application hasn't been launched by Marionette no further action can be done.
>          # In such cases we simply re-throw the exception.
>          if not self.instance:
> -            raise exc, val, tb
> +            raise(exc, val, tb)

Same here for `raise exc(val, tb)h `raise exc(val, tb)`

::: testing/marionette/harness/marionette_harness/runner/base.py:922
(Diff revision 1)
>          finally:
>              self.cleanup()
>  
>              # reraise previous interruption now
>              if interrupted:
> -                raise interrupted[0], interrupted[1], interrupted[2]
> +                raise(interrupted[0], interrupted[1], interrupted[2])

Same here.

::: testing/marionette/harness/marionette_harness/runner/httpd.py:13
(Diff revision 1)
>  Marionette.
>  
>  """
> +from __future__ import (
> +    absolute_import,
> +    print_function

Please also update the call to print in this file to something like `print(msg, file=sys.stderr)`

::: testing/marionette/harness/marionette_harness/runner/serve.py:14
(Diff revision 1)
>  
>  """
>  
> +from __future__ import (
> +    absolute_import,
> +    print_function

Same here for the printing to stderr.

::: testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py:6
(Diff revision 1)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # 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 __future__ import (
> +    absolute_import, 

Please remove the trailing space.
Attachment #8935360 - Flags: review?(hskupin) → review-
(In reply to Henrik Skupin (:whimboo) [mostly away Dec 11th - Jan 3rd] from comment #17)
> Comment on attachment 8935360 [details]
> Bug 1407763 - Fixed all lint errors, when the 'testing/marionette' section
> was removed
> 
> https://reviewboard.mozilla.org/r/206252/#review213378
> 
> I still don't see changes to tools/lint/py2.yml and tools/lint/py3.yml which
> would enable those checks by default. Further I wonder how you push the
> patches to mozreview, because each update ends-up as a separate commit.
> Please make sure to squash your changes (hg histedit).
> 
> ::: testing/marionette/client/marionette_driver/geckoinstance.py:357
> (Diff revision 1)
> >                  self.runner.device.connect()
> >              self.runner.start()
> >          except Exception as e:
> >              exc, val, tb = sys.exc_info()
> >              message = "Error possibly due to runner or device args: {}"
> > -            raise exc, message.format(e.message), tb
> > +            raise (exc, message.format(e.message), tb)
> 
> Shouldn't this be `raise exc(message.format(e.message), tb)`?
> 
> ::: testing/marionette/client/marionette_driver/marionette.py:798
> (Diff revision 1)
> >          exc, val, tb = sys.exc_info()
> >  
> >          # If the application hasn't been launched by Marionette no further action can be done.
> >          # In such cases we simply re-throw the exception.
> >          if not self.instance:
> > -            raise exc, val, tb
> > +            raise(exc, val, tb)
> 
> Same here for `raise exc(val, tb)h `raise exc(val, tb)`
> 
> ::: testing/marionette/harness/marionette_harness/runner/base.py:922
> (Diff revision 1)
> >          finally:
> >              self.cleanup()
> >  
> >              # reraise previous interruption now
> >              if interrupted:
> > -                raise interrupted[0], interrupted[1], interrupted[2]
> > +                raise(interrupted[0], interrupted[1], interrupted[2])
> 
> Same here.
> 
> ::: testing/marionette/harness/marionette_harness/runner/httpd.py:13
> (Diff revision 1)
> >  Marionette.
> >  
> >  """
> > +from __future__ import (
> > +    absolute_import,
> > +    print_function
> 
> Please also update the call to print in this file to something like
> `print(msg, file=sys.stderr)`
> 
> ::: testing/marionette/harness/marionette_harness/runner/serve.py:14
> (Diff revision 1)
> >  
> >  """
> >  
> > +from __future__ import (
> > +    absolute_import,
> > +    print_function
> 
> Same here for the printing to stderr.
> 
> :::
> testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.
> py:6
> (Diff revision 1)
> >  # This Source Code Form is subject to the terms of the Mozilla Public
> >  # 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 __future__ import (
> > +    absolute_import, 
> 
> Please remove the trailing space.



Hello, sorry for the short delay, I had a small vacation. I fixed all things which you was mentioned and have a couple problems.

Problem one:
When I run ./mach marionette-test command, I got the error https://pastebin.mozilla.org/9074936

Problem two:
Okay, I fixed the problem above, and got the new error https://pastebin.mozilla.org/9074937

So stuck with the problems now.
(In reply to Mike Yusko(:iceman) from comment #18)
> Problem two:
> Okay, I fixed the problem above, and got the new error
> https://pastebin.mozilla.org/9074937

As the traceback shows this comes from `expected.py`. With the absolute import you will have to make sure to specify the full package hierarchy down to the errors module. Please see https://docs.python.org/2.5/whatsnew/pep-328.html
(In reply to Henrik Skupin (:whimboo) [mostly away Dec 11th - Jan 3rd] from comment #19)
> (In reply to Mike Yusko(:iceman) from comment #18)
> > Problem two:
> > Okay, I fixed the problem above, and got the new error
> > https://pastebin.mozilla.org/9074937
> 
> As the traceback shows this comes from `expected.py`. With the absolute
> import you will have to make sure to specify the full package hierarchy down
> to the errors module. Please see
> https://docs.python.org/2.5/whatsnew/pep-328.html

Okay, I think that I fixed all lint/import errors, results after the commands.

./mach lint -l py2 -l py3 testing/marionette
https://pastebin.mozilla.org/9075088

./mach marionette test
https://pastebin.mozilla.org/9075090

./mach lint testing/marionette
https://pastebin.mozilla.org/9075091
Comment on attachment 8938526 [details]
Bug 1407763 - Fix lint/import errors

https://reviewboard.mozilla.org/r/209188/#review215960

Mike, something goes really wrong with updating the patch. Each time you upload a change a new commit is created. As I said, this should not happen. As such I would propose that we really do the next update together via IRC. I would like to understand why the above is happening.

::: npm-shrinkwrap.json:3
(Diff revision 1)
>  {
>    "name": "mozillaeslintsetup",
> +  "requires": true,

As said in my last review this file should not be touched.

::: testing/marionette/client/marionette_driver/__init__.py:7
(Diff revision 1)
>  
> -__version__ = '2.5.0'
> -
>  from __future__ import absolute_import
>  
> +__version__ = '2.5.0'

Please reverse this so it keeps staying at the top.
Attachment #8938526 - Flags: review?(hskupin) → review-
Attachment #8935341 - Attachment is obsolete: true
Attachment #8935348 - Attachment is obsolete: true
Attachment #8935360 - Attachment is obsolete: true
Attachment #8938526 - Attachment is obsolete: true
Comment on attachment 8934927 [details]
Bug 1407763 - Enable py2/py3 compat linters on testing/marionette.

https://reviewboard.mozilla.org/r/205562/#review217522

Clearing the review quest until the remaining changes have been done.
Attachment #8934927 - Flags: review?(hskupin)
(In reply to Henrik Skupin (:whimboo) from comment #22)
> Comment on attachment 8938526 [details]
> Bug 1407763 - Fix lint/import errors
> 
> https://reviewboard.mozilla.org/r/209188/#review215960
> 
> Mike, something goes really wrong with updating the patch. Each time you
> upload a change a new commit is created. As I said, this should not happen.
> As such I would propose that we really do the next update together via IRC.
> I would like to understand why the above is happening.
> 
> ::: npm-shrinkwrap.json:3
> (Diff revision 1)
> >  {
> >    "name": "mozillaeslintsetup",
> > +  "requires": true,
> 
> As said in my last review this file should not be touched.
> 
> ::: testing/marionette/client/marionette_driver/__init__.py:7
> (Diff revision 1)
> >  
> > -__version__ = '2.5.0'
> > -
> >  from __future__ import absolute_import
> >  
> > +__version__ = '2.5.0'
> 
> Please reverse this so it keeps staying at the top.


Hello, seems like we can't reverse version and import statement, if we put version at the beginning of the file, 
and run the command ./mach marionette tests will crashed link on the error https://pastebin.mozilla.org/9076013

future import must be at the beginning of file. Any ideas?
(In reply to Mike Yusko(:iceman) from comment #25)
> Hello, seems like we can't reverse version and import statement, if we put
> version at the beginning of the file, 
> and run the command ./mach marionette tests will crashed link on the error
> https://pastebin.mozilla.org/9076013
> 
> future import must be at the beginning of file. Any ideas?

In that case please keep it in the proposed order. Unless Andrew has some additional info for that. But looks like that this is how we have to do it to prevent any breaking changes.
Flags: needinfo?(ahalberstadt)
Yeah, having __future__ at the top is enforced by python itself (this isn't something we control from the linter). This is done to prevent scenarios where some lines of the file have a __future__ feature enabled whereas others don't.
Flags: needinfo?(ahalberstadt)
(In reply to Andrew Halberstadt [:ahal] from comment #27)
> Yeah, having __future__ at the top is enforced by python itself (this isn't
> something we control from the linter). This is done to prevent scenarios
> where some lines of the file have a __future__ feature enabled whereas
> others don't.

Thank you for the information!
on your opinion, what is the best way to solve the problem?
Move the __version__ statement to below the __future__ import :). I think that's what :whimboo meant in comment 26.
(In reply to Andrew Halberstadt [:ahal] from comment #29)
> Move the __version__ statement to below the __future__ import :). I think
> that's what :whimboo meant in comment 26.

Okay, got it! for now the __version__ statement is below in my patch, so I need to fix only npm-shrinkwrap.json file, and the patch will be done for a merge.
[Mass Change 2018-01-15] Moving bugs to backlog
Priority: -- → P3
Comment on attachment 8934927 [details]
Bug 1407763 - Enable py2/py3 compat linters on testing/marionette.

https://reviewboard.mozilla.org/r/205562/#review219256

the patch still contains the node file, and also does not enable py2/py3 compat linting as it is the goal of this bug.

Given that Mike is out for the moment I will finish off this patch.
Attachment #8934927 - Flags: review-
So there is a general problem remaining here in how we re-raise exceptions. Formerly we had:

> raise ExceptionType, message, tb

This is not allowed anymore. Talking to Andrew on IRC and searching myself, we both came to the solution in using `six.reraise`.

I will have to update all of the places of the patch where we make use of it.
Attachment #8943288 - Flags: review?(ahalberstadt)
Comment on attachment 8943288 [details]
Bug 1407763 - Enable py2 and py3 compat linters for testing/marionette.

https://reviewboard.mozilla.org/r/213604/#review219810

::: testing/marionette/harness/marionette_harness/runner/base.py:941
(Diff revision 3)
>          finally:
>              self.cleanup()
>  
>              # reraise previous interruption now
>              if interrupted:
> -                raise interrupted[0], interrupted[1], interrupted[2]
> +                raise interrupted[0](interrupted[1], interrupted[2])

Doesn't this need to use reraise as well?

::: testing/marionette/puppeteer/firefox/firefox_puppeteer/puppeteer.py:7
(Diff revision 3)
>  # 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 decorators import use_class_as_property
> +from __future__ import absolute_import
> +
> +from . decorators import use_class_as_property

Space between . and decorators

::: testing/marionette/puppeteer/firefox/firefox_puppeteer/ui/update_wizard/__init__.py:7
(Diff revision 3)
>  # 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 dialog import UpdateWizardDialog
> +from __future__ import absolute_import
> +
> +from . dialog import UpdateWizardDialog

Ditto
Attachment #8943288 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8943288 [details]
Bug 1407763 - Enable py2 and py3 compat linters for testing/marionette.

https://reviewboard.mozilla.org/r/213604/#review219810

> Doesn't this need to use reraise as well?

Totally. Great spot! I was only searching for `tb`, which we used all over the place.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s b07fea6bc77e9e913e9ed0f369019bbad7db6236 -d 3cd74e25c015: rebasing 443122:b07fea6bc77e "Bug 1407763 - Enable py2 and py3 compat linters for testing/marionette. r=ahal" (tip)
merging testing/marionette/client/marionette_driver/geckoinstance.py
merging testing/marionette/harness/marionette_harness/tests/unit/test_prefs.py
merging testing/marionette/harness/marionette_harness/tests/unit/test_profile_management.py
warning: conflicts while merging testing/marionette/client/marionette_driver/geckoinstance.py! (edit, then use 'hg resolve --mark')
warning: conflicts while merging testing/marionette/harness/marionette_harness/tests/unit/test_profile_management.py! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/945b63ad5a0b
Enable py2 and py3 compat linters for testing/marionette. r=ahal
https://hg.mozilla.org/mozilla-central/rev/945b63ad5a0b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.