1163797 - Marionette mach command should use the same argument parser as the harness

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Andrew Halberstadt [:ahal]]

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

Comment 1

[Vaibhav (:vaibhav1994)]

Attachment: 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

Comment 2

[Vaibhav (:vaibhav1994)]

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.

Comment 3

[Vaibhav (:vaibhav1994)]

Try Run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ddfc15a920d

Comment 4

[Andrew Halberstadt [:ahal]]

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?

Comment 5

[Vaibhav (:vaibhav1994)]

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

Comment 6

[Vaibhav (:vaibhav1994)]

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

Comment 7

[Vaibhav (:vaibhav1994)]

Successful try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c3b226530a0

Comment 8

[Andrew Halberstadt [:ahal]]

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.

Comment 9

[Vaibhav (:vaibhav1994)]

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

Comment 10

[Vaibhav (:vaibhav1994)]

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

Comment 11

[Vaibhav (:vaibhav1994)]

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+

Comment 12

[Andrew Halberstadt [:ahal]]

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

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/58b957a84ead

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/58b957a84ead