842633 - Allow subclasses of MarionetteTestRunner to add their own command line options

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

We'd like to add a command line option for GaiaTestRunner to specify if the b2g process should be restarted between tests. This is not ideal for the MarionetteTestRunner, as it's b2g specific. We therefore need a way for a subclass to extend the command line options.

Comment 1

[Jonathan Griffin (:jgriffin)]

There are many ways to do this, but one is to move all the options into a new class (MarionetteOptions, or some such) that extends OptionParser.  See https://bug799308.bugzilla.mozilla.org/attachment.cgi?id=739546 for a patch (not landed) that does this much in another context (getting mach commands for Marionette).

You could then adjust cli() to take a parser_class similar to how it currently takes a runner_class.

Any subclasses of Marionette could then create a subclass of MarionetteOptions which could add domain-specific options.  We'd probably want to move verify_parser_options (from the above patch) into MarionetteOptions, so that method could also be overridden in MarionetteOptions sublcasses.

Feel free to take that patch as a starting point and modify as needed; don't worry about bitrotting that.  I don't know when it will land and I will unbitrot it when it does.

Comment 2

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Allow subclasses of MarionetteTestRunner to specify their own command line options. v1.0

Thanks, this was a neater suggestion than my work in progress. The issue I have, though is providing my new command line option as a keyword argument to the runner class instantiation. I considered using **kwargs but my value is in the options object, which is not available to the subclass. Any suggestions?

Comment 3

[Jonathan Griffin (:jgriffin)]

Comment on attachment 741045 [details] [diff] [review]
Allow subclasses of MarionetteTestRunner to specify their own command line options. v1.0

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

I think we can still use **kwargs to pass values to the runner_class constructor; we can just call runner_class(**vars(options)) instead of passing in all the individual options members, like we do now.  This will allow the list to be flexible, depending on the contents of options.

::: testing/marionette/client/marionette/runtests.py
@@ +689,5 @@
> +                        action='store',
> +                        help='absolute path to directory containing breakpad symbols, or the url of a zip file containing symbols')
> +
> +
> +def verify_parser_result(tests, options):

I think we should move verify_parser_result so that it's a method of MarionetteTestOptions; this way, subclasses of that can add their own verification rules.

Comment 4

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Allow subclasses of MarionetteTestRunner to specify their own command line options. v1.1

That works well. I had to add as a keyword argument to MarionetteTestRunner but if you think it makes more sense I can simply discard this argument.

I've also add a test_kwargs to allow the TestCase subclass to provide its own arguments. This is necessary for me to be able to access the new options from the test case, however there may be a neater way to achieve this. My current working code for GaiaTestCase is:

    def __init__(self, **kwargs):
        restart = kwargs.pop('restart', False)
        MarionetteTestRunner.__init__(self, **kwargs)
        self.test_kwargs = {'restart':restart}

Comment 5

[Jonathan Griffin (:jgriffin)]

Comment on attachment 741270 [details] [diff] [review]
Allow subclasses of MarionetteTestRunner to specify their own command line options. v1.1

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

::: testing/marionette/client/marionette/runtests.py
@@ +187,5 @@
>                   es_server=None, rest_server=None, logger=None,
>                   testgroup="marionette", noWindow=False, logcat_dir=None,
>                   xml_output=None, repeat=0, perf=False, perfserv=None,
> +                 gecko_path=None, testvars=None, tree=None, type=None,
> +                 device=None, symbols_path=None):

We could add **kwargs to the end of this list, and then:

self.test_kwargs = kwargs

This way, any and all extra keyword arguments would get passed to the test class, and you wouldn't need to add that extra logic to GaiaTestCase's __init__.

@@ +733,1 @@
>      runner.run_tests(tests, testtype=options.type)

Since we're now passing type to __init__, I think run_tests() can use self.type, and we can remove the second parameter both in the declaration and all call sites.

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Allow subclasses of MarionetteTestRunner to specify their own command line options. v1.2

Great, thanks for the suggestions. This is working well with a locally patch GaiaTestCase that adds a 'restart' command line option.

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=a4e270617125

Comment 8

[Jonathan Griffin (:jgriffin)]

Comment on attachment 741537 [details] [diff] [review]
Allow subclasses of MarionetteTestRunner to specify their own command line options. v1.2

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

Nice, looks pretty clean, thanks for doing this.

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Try was successful. Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78bdcb813be1

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/78bdcb813be1

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/0d4f7fac1142

Comment 12

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/b0d487aa4369
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/c0199268e279