Closed
Bug 1163797
Opened 9 years ago
Closed 9 years ago
Marionette mach command should use the same argument parser as the harness
Categories
(Remote Protocol :: Marionette, defect)
Remote Protocol
Marionette
Tracking
(firefox43 fixed)
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: ahal, Assigned: vaibhav1994)
References
Details
Attachments
(1 file)
The marionette mach command redefines all of the arguments [1] exposed by the marionette test harness [2]. This is bad for a few reasons:
* new options need to be defined twice
* makes command line for running locally different from command line when running in production
* encourages multiple entry points into the test harness and separation of features (i.e features that only work when using the mach command).
Instead of re-defining arguments, the mach command should just *use* the marionette argument parser directly. This is possible by using the 'parser' argument to the @Command decorator. This was already done for mochitest, so bug 1155338 can be used as a rough guideline.
A pre-requisite to this is getting the marionette harness to use argparse instead of optparse.
[1] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/mach_commands.py#119
[2] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/runner/base.py#247
Assignee | ||
Comment 1•9 years ago
|
||
Bug 1163797 - Removing CommandArguments decorators from marionette mach command and making it use argparse from test harness. r=ahal
Attachment #8656286 -
Flags: review?(ahalberstadt)
Assignee | ||
Comment 2•9 years ago
|
||
Andrew was testing through marionette mach command, and found that we can now shift it to argparse to use additional options that are available in marionette harness. This patch makes it possible to call in-harness arguments.
Assignee | ||
Comment 3•9 years ago
|
||
Reporter | ||
Updated•9 years ago
|
Attachment #8656286 -
Flags: review?(ahalberstadt)
Reporter | ||
Comment 4•9 years ago
|
||
Comment on attachment 8656286 [details]
MozReview Request: Bug 1163797 - Removing CommandArguments decorators from marionette-test mach command and making it use argparse from test harness. r=ahal
https://reviewboard.mozilla.org/r/18131/#review16301
Thanks for doing this, it's a lot simpler than I thought it would be! Just a few comments to fix.
::: testing/marionette/mach_commands.py:11
(Diff revision 1)
> from mozlog.structured import commandline
Remove the commandline import.
::: testing/marionette/mach_commands.py:12
(Diff revision 1)
> +from marionette.runner.base import BaseMarionetteArguments
Please lazy import this, otherwise base.py will be executed anytime anyone runs any mach command.
I'd recommend defining a function in this file that does the import, instantiates the BaseMarionetteArguments object and then returns it. Then you can pass that function in as the value to 'parser' in the @Command decorator.
::: testing/marionette/mach_commands.py:118
(Diff revision 1)
> - binary = self.get_binary_path('app')
> - return run_marionette(tests, binary=binary, topsrcdir=self.topsrcdir, **kwargs)
> + kwargs['binary'] = self.get_binary_path('app')
> + return run_marionette(tests, topsrcdir=self.topsrcdir, **kwargs)
Any particular reason for this change?
Assignee | ||
Comment 5•9 years ago
|
||
https://reviewboard.mozilla.org/r/18131/#review16389
::: testing/marionette/mach_commands.py:11
(Diff revision 1)
> from mozlog.structured import commandline
commandline is still required in: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/mach_commands.py?offset=0#68
I think run_marionette function also requires cleanup, but before that marionette-webapi will need to be cleaned up, I'll do that in another patch.
::: testing/marionette/mach_commands.py:118
(Diff revision 1)
> - binary = self.get_binary_path('app')
> - return run_marionette(tests, binary=binary, topsrcdir=self.topsrcdir, **kwargs)
> + kwargs['binary'] = self.get_binary_path('app')
> + return run_marionette(tests, topsrcdir=self.topsrcdir, **kwargs)
Yes, binary is required to be passed in since it is assigned here: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/mach_commands.py?offset=400#60
Assignee | ||
Comment 6•9 years ago
|
||
Comment on attachment 8656286 [details]
MozReview Request: Bug 1163797 - Removing CommandArguments decorators from marionette-test mach command and making it use argparse from test harness. r=ahal
Bug 1163797 - Removing CommandArguments decorators from marionette mach command and making it use argparse from test harness. r=ahal
Attachment #8656286 -
Flags: review?(ahalberstadt)
Assignee | ||
Comment 7•9 years ago
|
||
Successful try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c3b226530a0
Reporter | ||
Updated•9 years ago
|
Attachment #8656286 -
Flags: review?(ahalberstadt) → review+
Reporter | ||
Comment 8•9 years ago
|
||
Comment on attachment 8656286 [details]
MozReview Request: Bug 1163797 - Removing CommandArguments decorators from marionette-test mach command and making it use argparse from test harness. r=ahal
https://reviewboard.mozilla.org/r/18131/#review16417
Still looks like you haven't removed the commandline import. Please fix that before pushing.
Assignee | ||
Comment 9•9 years ago
|
||
https://reviewboard.mozilla.org/r/18131/#review16301
> Remove the commandline import.
commandline is still required in: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/mach_commands.py?offset=0#68
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8656286 [details]
MozReview Request: Bug 1163797 - Removing CommandArguments decorators from marionette-test mach command and making it use argparse from test harness. r=ahal
Bug 1163797 - Removing CommandArguments decorators from marionette-test mach command and making it use argparse from test harness. r=ahal
Attachment #8656286 -
Attachment description: MozReview Request: Bug 1163797 - Removing CommandArguments decorators from marionette mach command and making it use argparse from test harness. r=ahal → MozReview Request: Bug 1163797 - Removing CommandArguments decorators from marionette-test mach command and making it use argparse from test harness. r=ahal
Attachment #8656286 -
Flags: review+ → review?(ahalberstadt)
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8656286 [details]
MozReview Request: Bug 1163797 - Removing CommandArguments decorators from marionette-test mach command and making it use argparse from test harness. r=ahal
Carrying forward the r+
Attachment #8656286 -
Flags: review?(ahalberstadt) → review+
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → vaibhavmagarwal
Keywords: checkin-needed
Reporter | ||
Comment 12•9 years ago
|
||
Comment on attachment 8656286 [details]
MozReview Request: Bug 1163797 - Removing CommandArguments decorators from marionette-test mach command and making it use argparse from test harness. r=ahal
https://reviewboard.mozilla.org/r/18131/#review16421
Attachment #8656286 -
Flags: review+
Comment 13•9 years ago
|
||
Keywords: checkin-needed
Comment 14•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•