Closed Bug 1163801 Opened 9 years ago Closed 9 years ago

Marionette harness should use argparse instead of optparse

Categories

(Remote Protocol :: Marionette, defect)

defect
Not set
normal

Tracking

(firefox43 fixed)

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: ahal, Assigned: sehgalvibhor)

References

Details

Attachments

(3 files, 4 obsolete files)

In addition to being better than optparse (not to mention still supported), argparse for marionette is required to fix bug 1163797.
The wrinkle here is landing updates for gaia-ui-tests and firefox-ui-tests concurrently. Bot test harnesses subclass the marionette parser and depend on its API.
Assignee: nobody → sehgalvibhor
Attached patch marionette-argparse.patch (obsolete) — Splinter Review
chmanchester, you mentioned a need for updating the firefox-ui-tests and maybe other dependencies prior to doing this.  Could you elaborate here.
Flags: needinfo?(cmanchester)
Attachment #8622580 - Flags: review?(ahalberstadt)
The issue is things like [1] and [2]. In both cases a test harness out of tree imports and uses marionette's option parser as an option parser, so both need to be updated along with this change to avoid bustage.

[1] https://github.com/mozilla/firefox-ui-tests/blob/a8bd4576aa2f783bf513b5881de3c06902205240/firefox_ui_harness/options/base.py#L8
[2] https://github.com/mozilla-b2g/gaia/blob/3f98efee37194988908df33f87687ed725fb7974/tests/python/gaia-ui-tests/gaiatest/runtests.py#L8
Flags: needinfo?(cmanchester)
can we peg these instaqnces to a specific version?  thoughts on a protocol for solving this?  maybe accept small breakage if the race condition ends up as such?

firefox-ui-tests uses nightly builds on beta, could we have issues with branch versioning?

for mozilla-b2g/gaia, I imagine that is more straightforward.
I don't think we can start using specific marionette package versions on integration branches. For firefox-ui-tests this won't cause bustage on treeherder (yet!), so as long as we have a pull request ready with this landing that should be ok (this is purely a client change, so no incompatibilities across gecko versions will be introduced).

Gaia is similar, but I think we usually accept a short period of bustage on treeherder for these kinds of changes. RyanVM or another sheriff might have a suggestion for the least disruptive sequence of landings in this case.

A good first step would be to get patches written and tested for the two harnesses and then notify a sheriff when everything's ready to land.
Vibhor,

can you fork and clone:
1) https://github.com/mozilla/firefox-ui-tests
2) https://github.com/mozilla-b2g/gaia (warning, this is large!!)

then we can start updating these other repositories which use marionette and extend it.  In the meantime we can run some tests on try server (I can help with that tomorrow) so that we can verify all bits work well.

I know chmanchester can help with the firefox-ui-tests above, this is a smaller repo- you just submit a pull request and mention in this bug here that a review is needed.  pay attention to the commit messages and retain bug number/comment when doing the commit!

While this is a nasty bug, it will be great to get this finished.  Thanks for getting the first step done!
Joel,

Yeah Sure! I am on it !
Gimme a little time for the large repository.
While I clone it, We can start the other tasks side by side.

Sure thing!
Both of those links seem to peg marionette_client, so landing this shouldn't have an immediate impact on them. It would certainly be nice if we could patch them too, but it probably should be part of a separate bug called "Update marionette dependency in X" as there might be other issues related to the new version.
(In reply to Andrew Halberstadt [:ahal] from comment #9)
> Both of those links seem to peg marionette_client, so landing this shouldn't
> have an immediate impact on them. It would certainly be nice if we could
> patch them too, but it probably should be part of a separate bug called
> "Update marionette dependency in X" as there might be other issues related
> to the new version.

Gaia uses in-tree packages, so at least that will cause treeherder bustage, as I mentioned above.
Comment on attachment 8622580 [details] [diff] [review]
marionette-argparse.patch

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

Thanks for the patch! You'll need to at least fix the return values of parse_args(). Please do that and address the other comments and re-flag me for review. Let me know if you have any questions!

::: testing/marionette/client/marionette/__init__.py
@@ +18,5 @@
>          MarionetteTest,
>          MarionetteTestResult,
>          MarionetteTextTestRunner,
>          MemoryEnduranceTestCaseMixin,
> +        ArgumentParser,

I know you were just following the pre-existing convention here, but please import this directly from argparse so it's clear that this is not some sort of custom class.

::: testing/marionette/client/marionette/runner/__init__.py
@@ +4,5 @@
>  
>  from base import (
>          B2GTestResultMixin, BaseMarionetteOptions, BaseMarionetteTestRunner,
>          Marionette, MarionetteTest, MarionetteTestResult, MarionetteTextTestRunner,
> +        ArgumentParser, TestManifest, TestResult, TestResultCollection

Same as above.

::: testing/marionette/client/marionette/runner/base.py
@@ +295,2 @@
>                          dest='address',
>                          action='store',

Bonus points if you remove all |action='store'|, it's not necessary because it's the default :)

@@ +427,5 @@
>                               "used multiple times in which case the test must contain "
>                               "at least one of the given tags.")
>  
>      def parse_args(self, args=None, values=None):
> +        options, tests = ArgumentParser.parse_args(self, args, values)

ArgumentParser.parse_args() no longer returns a tuple. Instead you'll need to add a new call to add_argument that contains a positional argument called "tests". See https://docs.python.org/2.7/library/argparse.html#upgrading-optparse-code for more info.
Attachment #8622580 - Flags: review?(ahalberstadt)
Attached patch marionette_argparse1.patch (obsolete) — Splinter Review
Latest Patch!
Attachment #8636678 - Flags: review?(ahalberstadt)
Comment on attachment 8636678 [details] [diff] [review]
marionette_argparse1.patch

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

Thanks so much, this looks great! Give me a minute and I'll push this to try (our testing server) for you.
Attachment #8636678 - Flags: review?(ahalberstadt) → review+
Attachment #8622580 - Attachment is obsolete: true
Comment on attachment 8636678 [details] [diff] [review]
marionette_argparse1.patch

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

Your patch is actually bitrotted. Please pull in the latest changes, rebase your patch on top and resolve the merge conflicts. If you need help with this, please let me know :)

::: testing/marionette/client/marionette/runner/base.py
@@ -431,5 @@
> -        options, tests = OptionParser.parse_args(self, args, values)
> -        for handler in self.parse_args_handlers:
> -            handler(options, tests, args, values)
> -
> -        return (options, tests)

While I was rebasing this, I noticed a problem. It looks like self.parse_args_handlers is used by mixins/endurance.py, so you can't just remove this function.

ArgumentParser.parse_args returns a single 'args' variable instead of two. This means that you'll need to explicitly define a new positional argument above. You'll want something like:

self.add_argument('tests', nargs='*', default=[], help='Tests to run.')

as the top of the arguments list. Then you can access the tests by referencing args.tests. Thanks again, almost there.
Attachment #8636678 - Flags: review+
Note that the gaia tests mentioned earlier aren't sheriffed anymore, so updating those to get this landed is no longer a hard requirement. Updating firefox-ui-tests will just be a topic for the next time we want to update the marionette client used, so I can take care of it then.
(In reply to Andrew Halberstadt [:ahal] from comment #14)
> Comment on attachment 8636678 [details] [diff] [review]
> marionette_argparse1.patch
> 
> Review of attachment 8636678 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Your patch is actually bitrotted. Please pull in the latest changes, rebase
> your patch on top and resolve the merge conflicts. If you need help with
> this, please let me know :)
> 
> ::: testing/marionette/client/marionette/runner/base.py
> @@ -431,5 @@
> > -        options, tests = OptionParser.parse_args(self, args, values)
> > -        for handler in self.parse_args_handlers:
> > -            handler(options, tests, args, values)
> > -
> > -        return (options, tests)
> 
> While I was rebasing this, I noticed a problem. It looks like
> self.parse_args_handlers is used by mixins/endurance.py, so you can't just
> remove this function.
> 
> ArgumentParser.parse_args returns a single 'args' variable instead of two.
> This means that you'll need to explicitly define a new positional argument
> above. You'll want something like:
> 
> self.add_argument('tests', nargs='*', default=[], help='Tests to run.')
> 
> as the top of the arguments list. Then you can access the tests by
> referencing args.tests. Thanks again, almost there.

After adding the positional argument, wherever tests keyword is used will have to be replaced by args.tests right ? And do any changes have to be made to the parse_args function you mentioned above?

Thanks
I have updated the code and the bug.
However I need help with the parse_arg function which Andrew mentioned above.
A more elaborate solution for the tests argument will be great to finish this.
Thanks !
(In reply to Vibhor Sehgal from comment #16)
> After adding the positional argument, wherever tests keyword is used will
> have to be replaced by args.tests right ? And do any changes have to be made
> to the parse_args function you mentioned above?

I think the best way to fix this would be to modify the parse_args function to return (args, args.tests) instead of (options, args). That way consumers won't need to get updated anywhere.

(In reply to Vibhor Sehgal from comment #17)
> I have updated the code and the bug.

Not sure if you meant to upload a new attachment, but the code there is still the old patch.
Attached patch marionette_argparse1.patch (obsolete) — Splinter Review
Latest Patch !
Attached patch marionette_argparse1.patch (obsolete) — Splinter Review
Latest Patch!
Attachment #8636678 - Attachment is obsolete: true
Attachment #8642490 - Attachment is obsolete: true
Attachment #8642497 - Flags: review?(ahalberstadt)
Comment on attachment 8642497 [details] [diff] [review]
marionette_argparse1.patch

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

This looks great, thanks for sticking with it! I'll push this to the try server to make sure nothing breaks.

::: testing/marionette/client/marionette/runner/base.py
@@ +420,2 @@
>          for handler in self.parse_args_handlers:
>              handler(options, tests, args, values)

options is undefined here. But no worries, I'll fix it up when I push to try.
Attachment #8642497 - Flags: review?(ahalberstadt) → review+
Pushed to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=24e49feb8133

But the jobs fail with the following traceback:
Traceback (most recent call last):
File "/builds/slave/test/build/tests/marionette/marionette/runtests.py", line 63, in <module>
  cli()
File "/builds/slave/test/build/tests/marionette/marionette/runtests.py", line 47, in cli
  transport_version=transport_version)
File "/builds/slave/test/build/tests/marionette/marionette/runtests.py", line 28, in __init__
  BaseMarionetteOptions.__init__(self, **kwargs)
File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/marionette/runner/base.py", line 251, in __init__
  ArgumentParser.__init__(self, **kwargs)
File "/usr/lib/python2.7/argparse.py", line 1569, in __init__
  conflict_handler=conflict_handler)
File "/usr/lib/python2.7/argparse.py", line 1174, in __init__
  super(_ActionsContainer, self).__init__()
File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/marionette/runner/mixins/browsermob.py", line 17, in __init__
  group = self.add_argument_group('Browsermob Proxy')
File "/usr/lib/python2.7/argparse.py", line 1298, in add_argument_group
  group = _ArgumentGroup(self, *args, **kwargs)
File "/usr/lib/python2.7/argparse.py", line 1476, in __init__
  update('conflict_handler', container.conflict_handler)
AttributeError: 'MarionetteOptions' object has no attribute 'conflict_handler'
Return code: 1

Haven't debugged it yet, but looks like a tricky error caused by the BrowserMobProxyOptionsMixin in mixins/browsermob.py
Any Idea or Suggestions , How should I go about fixing this error?
Little help maybe?
Thanks.
(In reply to Vibhor Sehgal from comment #23)
> Any Idea or Suggestions , How should I go about fixing this error?
> Little help maybe?
> Thanks.

It might be as simple as making the BrowserMobProxyOptionsMixin class inherit from ArgumentParser. But like I said, I haven't had time to debug this yet so I'm not sure what the problem is.

Here are some pointers that might help you debug:
* 'conflict_handler' is a property on the ArgumentParser class
* BrowserMobProxyOptionsMixin is "mixed in" via inheritance to the BaseMarionetteOptions class
* this didn't happen with optparse, so argparse must have some internal checks that assume the class is an ArgumentParser instance.

Other than that, look at the traceback carefully to try and understand the interactions happening here. I like to use the github clone for viewing python source (if you need/want to):
https://github.com/python/cpython/blob/2.7/Lib/argparse.py
Latest Patch!
Attachment #8642497 - Attachment is obsolete: true
Nice, thanks again for sticking with it! I pushed your latest patch to try (it ran fine for me locally):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0062639cfab6
Sure ! Yeah, this should probably work now. Was a tough bug though .
(In reply to Vibhor Sehgal from comment #27)
> Sure ! Yeah, this should probably work now. Was a tough bug though .

Oops, looks like the try run is still failing. Did you accidentally upload the wrong patch?

This is weird, because that patch worked fine for me locally. There's something weird happening, wonder if it depends what python version is being used.
Flags: needinfo?(sehgalvibhor)
(In reply to Andrew Halberstadt [:ahal] from comment #28)
> (In reply to Vibhor Sehgal from comment #27)
> > Sure ! Yeah, this should probably work now. Was a tough bug though .
> 
> Oops, looks like the try run is still failing. Did you accidentally upload
> the wrong patch?
> 
> This is weird, because that patch worked fine for me locally. There's
> something weird happening, wonder if it depends what python version is being
> used.

The patch uploaded is correct. I also tested it , it was running fine for me too.
What's the error it's showing now?
Flags: needinfo?(sehgalvibhor)
You can see the error if you click the try push link above and then click on the 'Mn' job. There's an error summary, and you can also click the 'show log' button to see the full traceback. It's the same as comment 22 though.

I figured out why this isn't reproducible locally. For some reason the |mach marionette-test| command instantiates a "BaseMarionetteOptions" class, while in automation a "MarionetteOptions" class is used. The latter is what includes the BrowserMobProxyOptionsMixin [1] (which is what is causing the failure).

So if you change this [1] from "BaseMarionetteOptions" to "MarionetteOptions", you'll see the error locally too by running |mach marionette-test|. I'll take a quick look here, maybe it's an easy fix.

[1] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/runtests.py#25
[2] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/mach_commands.py?offset=100#46
I think I see the problem, but unfortunately this is going to require a re-write of how marionette handles options mixins. I'll create a new commit to fix this based on top of your patch.
Marionette argparse 2
Bug 1163801 - Refactor marionette's options mixin system
Attachment #8650049 - Flags: review?(dburns)
David: Not sure who is best to do this review, please feel free to delegate.

The first commit is Vibhor's patch that I reviewed above. The second refactors marionette's system for mixing in different kinds of options, which wasn't compatible with argparse (see error in comment 22). This will break things like gaiatest, but looks like gaiatest now pegs marionette-client (currently on version 0.16), so not sure if I need to worry about that or not.
(In reply to Andrew Halberstadt [:ahal] from comment #34)
> This will break things like gaiatest, but looks like gaiatest now pegs
> marionette-client (currently on version 0.16), so not sure if I need to
> worry about that or not.

gaiatest isn’t even enabled on gaia-try, so we needn’t worry too much.
Comment on attachment 8650049 [details]
MozReview Request: Bug 1163801 - Refactor marionette's options mixin system

https://reviewboard.mozilla.org/r/16535/#review14793

Ship It!
Attachment #8650049 - Flags: review?(dburns) → review+
Blocks: 1196489
Blocks: 1197835
This breaks the hidden Gip tests on emulator. But they have been broken for a long time, and talking to :jlorenzo and :mwargers on irc, it seems they should be removed.

So given that they are A) already broken, B) might not be scheduled much longer, I'll land this already.

Here's a working try run with these patches, and the version bump patch from bug 1197835:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc94a9addc43

Thanks again Vibhor!
https://hg.mozilla.org/mozilla-central/rev/f1806bee0c8e
https://hg.mozilla.org/mozilla-central/rev/599e3e8eaa44
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Thanks Joel and Andrew for the help and guidance ! :)
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.