1255064 - MarionetteHarness should internally use dicts for arguments and not Namespaces

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

While working on the mach command for firefox-ui-tests I stumbled again over the requirement to call the parser again:

http://mxr.mozilla.org/mozilla-central/source/testing/firefox-ui/mach_commands.py#38

The problem is simply that the MarionetteHarness expects a Namespace. It would be better if we could change that to a dict:

http://mxr.mozilla.org/mozilla-aurora/source/testing/marionette/harness/marionette/runtests.py#37

> self.kwargs = kwargs or vars(self.parse_args())

We even convert it in the `run()` method to a dict:

http://mxr.mozilla.org/mozilla-aurora/source/testing/marionette/harness/marionette/runtests.py#65

It would make it easier for other clients or unit tests to directly pass in a pre-defined dictionary without the need to setup a namespace first.

Comment 1

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

I see no problem with working with a dict instead of a Namespace in MarionetteHarness, and I think it would be a good mentored bug.

`parse_args` could return a dict instead: http://mxr.mozilla.org/mozilla-aurora/source/testing/marionette/harness/marionette/runtests.py#56

...and then we need to treat self.args as a dict everywhere else in MarionetteHarness.

Comment 2

[Rachel King (:rachel_bk)]

I'll take a look at this tonight or tomorrow, per Maja's recommendation for a good bug for a newbie.

Comment 3

[Rachel King (:rachel_bk)]

Attachment: MozReview Request: Bug 1255064 - MarionetteHarness should internally use dicts for arguments; r?maja_zf

Review commit: https://reviewboard.mozilla.org/r/39633/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39633/

Comment 4

[chaithanya]

Hi
I would like to work on this bug. Could you assign it to me please?

Comment 5

[Andreas Tolfsen ❲:ato❳]

(In reply to chaithanya from comment #4)
> I would like to work on this bug. Could you assign it to me please?

It looks like rachel_bk has already submitted a patch for this.

Comment 6

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8730018 [details]
MozReview Request: Bug 1255064 - MarionetteHarness should internally use dicts for arguments; r?maja_zf

https://reviewboard.mozilla.org/r/39633/#review36963

::: testing/firefox-ui/mach_commands.py:67
(Diff revision 1)
> -    # Bug 1255064 - Marionette requieres an argparse Namespace. So fake one for now.
> -    args = argparse.Namespace()
> -    for k, v in kwargs.iteritems():
> -        setattr(args, k, v)
> -
>      failed = test_types[testtype]['cli_module'].cli(args=args)

args is undefined here; pass in kwargs?

::: testing/marionette/harness/marionette/runtests.py:59
(Diff revision 1)
>  
>          args.logger = logger
> -        return args
> +        return vars(args)
>  
>      def process_args(self):
> -        if self.args.pydebugger:
> +        if self.args['pydebugger'] != None:

The 'pydebugger' key might not exist at this point, so use `args.get()` to deal with that safely.

::: testing/marionette/harness/marionette/runtests.py:70
(Diff revision 1)
> -            tests = args_dict.pop('tests')
> +            runner = self._runner_class(**self.args)
> -            runner = self._runner_class(**args_dict)
>              runner.run_tests(tests)
>              return runner.failed
>          except Exception:
> -            self.args.logger.error('Failure during test execution.',
> +            self.args['logger'].error('Failure during test execution.',

Same for 'logger' key. If no logger was provided, just raise.

Could you also rebase your patch onto a more recent revision from mozilla-central? When I push your patch to the Try Server for testing, it doesn't even build for reasons that don't seem related to your changes, which is odd.

Comment 7

[Rachel King (:rachel_bk)]

Comment on attachment 8730018 [details]
MozReview Request: Bug 1255064 - MarionetteHarness should internally use dicts for arguments; r?maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39633/diff/1-2/

Comment 8

[Rachel King (:rachel_bk)]

https://reviewboard.mozilla.org/r/39633/#review36963

I was having difficulties running `./mach marionette-test` locally, perhaps related? Couldn't successfully run it on the most recent changeset before mine (after rebasing), either. It would time out when waiting for a port.

Comment 9

[Rachel King (:rachel_bk)]

I addressed your comments and also found another spot where a Namespace was being passed in, so fixed that as well.

Comment 10

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/39633/#review36963

After pulling+updating, run ./mach build again (possibly preceded by ./mach clobber) to build from the new revision.

Comment 11

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

(In reply to Rachel King (:rachel_bk) from comment #9)
> I addressed your comments and also found another spot where a Namespace was
> being passed in, so fixed that as well.

Ok, let's see how things look on try. There will be other Namespaces to fixes as well (like dom/media/tests/external? several others, too. try results will point them out.)

Comment 12

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

...actually, try won't point the other Namespaces out because they come up in mach commands, which you can only test locally. The ones I know of are. You have fixed some already. I found these by searching for things like 'MarionetteHarness' and 'marionette.runtests' on dxr.mozilla.org:
./mach firefox-ui-functional
./mach marionette-test (https://dxr.mozilla.org/mozilla-central/source/testing/marionette/mach_commands.py)
./mach external-media-tests (https://dxr.mozilla.org/mozilla-central/source/dom/media/test/external/mach_commands.py)

Comment 13

[Rachel King (:rachel_bk)]

Comment on attachment 8730018 [details]
MozReview Request: Bug 1255064 - MarionetteHarness should internally use dicts for arguments; r?maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39633/diff/2-3/

Comment 14

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/39633/#review37307

::: dom/media/test/external/mach_commands.py:42
(Diff revision 3)
>      args = parser.parse_args(args=tests)
>  
>      for k, v in kwargs.iteritems():
>          setattr(args, k, v)
>  
>      parser.verify_usage(args)
>  
>      args.logger = commandline.setup_logging("Firefox External Media Tests",
>                                              args,
>                                              {"mach": sys.stdout})
>      failed = mn_cli(MediaTestRunner, MediaTestArguments, FirefoxMediaHarness,
> -                    args=args)
> +                    args=vars(args))

You should be able to cut out all the args processing here (parse_args, set_attr) etc. and just used kwargs, as in the firefox-ui/mach_commands.py.

Same for marionette/mach_commands.py

In other words, the arguments are being parsed twice here (once into kwargs by mach, and once into args by calling parse_args). That second parsing is unnecessary.

Comment 15

[Rachel King (:rachel_bk)]

Comment on attachment 8730018 [details]
MozReview Request: Bug 1255064 - MarionetteHarness should internally use dicts for arguments; r?maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39633/diff/3-4/

Comment 16

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/39633/#review37727

::: dom/media/test/external/mach_commands.py:39
(Diff revision 4)
> -    args = parser.parse_args(args=tests)
> -
> -    for k, v in kwargs.iteritems():
> +    kwargs['tests'] = tests
> +    kwargs['logger'] = commandline.setup_logging("Firefox External Media Tests",
> +                                            kwargs,

If you run ./mach external-media-tests you'll see that this is broken. I looked into it a bit, and I think you've uncovered a bug in either BaseMarionetteArguments or mach itself. I don't want to go down that rabbit-hole now, so I'll just file a separate bug to look into it later. 

Therefore, could you revert your most recent change to external/mach_commands.py and marionette/mach_commands.py? (Just submit what you can previously with `vars(args)`)

Please test locally with ./mach external-media-tests, ./mach marionette-test and ./mach firefox-ui-functional
Thanks!

::: testing/marionette/harness/marionette/runtests.py:59
(Diff revision 4)
>  
>          args.logger = logger
> -        return args
> +        return vars(args)
>  
>      def process_args(self):
> -        if self.args.pydebugger:
> +        if self.args.get('pydebugger') != None:

This can just be `if self.args.get('pydebugger'):`

Comment 17

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/39633/#review37307

> You should be able to cut out all the args processing here (parse_args, set_attr) etc. and just used kwargs, as in the firefox-ui/mach_commands.py.
> 
> Same for marionette/mach_commands.py
> 
> In other words, the arguments are being parsed twice here (once into kwargs by mach, and once into args by calling parse_args). That second parsing is unnecessary.

Just replying this previous issue I opened on MozReview to be clear that this is what I want to you revert.

Comment 18

[Rachel King (:rachel_bk)]

Comment on attachment 8730018 [details]
MozReview Request: Bug 1255064 - MarionetteHarness should internally use dicts for arguments; r?maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39633/diff/4-5/

Comment 19

[Rachel King (:rachel_bk)]

I ran all three mach tests, and they all worked.

Comment 20

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8730018 [details]
MozReview Request: Bug 1255064 - MarionetteHarness should internally use dicts for arguments; r?maja_zf

https://reviewboard.mozilla.org/r/39633/#review37801

Almost there! Thanks for sticking with this. :) I'm just waiting for you to revert one more piece.

Why the reverting? Although all three mach commands run without error when using dicts, if you provide options to the commands (like ./mach marionette --e10s), these options never get processed by BaseMarionetteArguments.parse_args or verify_usage (I discovered that mach doesn't call either of these methods when when provided with a parser!), so important configuration and error-handling is skipped.

Comment 21

[Rachel King (:rachel_bk)]

Comment on attachment 8730018 [details]
MozReview Request: Bug 1255064 - MarionetteHarness should internally use dicts for arguments; r?maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39633/diff/5-6/

Comment 22

[Rachel King (:rachel_bk)]

Oops, sorry! I completely missed the part about also reverting marionette/mach_commands.py. I believe I've now reverted everything I needed to, but please let me know if there's more!

Comment 23

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8730018 [details]
MozReview Request: Bug 1255064 - MarionetteHarness should internally use dicts for arguments; r?maja_zf

https://reviewboard.mozilla.org/r/39633/#review37893

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f3438a45052

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8f3438a45052

Comment 26

[David Burns :automatedtester]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e0b56d6d35de