Closed Bug 1255064 Opened 4 years ago Closed 4 years ago

MarionetteHarness should internally use dicts for arguments and not Namespaces

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(firefox47 fixed, firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: whimboo, Assigned: rachel_bk, Mentored)

Details

(Keywords: pi-marionette-runner, Whiteboard: [lang=py][good first bug])

Attachments

(1 file)

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.
Flags: needinfo?(mjzffr)
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.
Mentor: mjzffr
Flags: needinfo?(mjzffr)
Whiteboard: [lang=py][good first bug]
I'll take a look at this tonight or tomorrow, per Maja's recommendation for a good bug for a newbie.
Hi
I would like to work on this bug. Could you assign it to me please?
Attachment #8730018 - Flags: review?(mjzffr)
(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.
Assignee: nobody → rachel.b.king
Status: NEW → ASSIGNED
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 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/
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.
I addressed your comments and also found another spot where a Namespace was being passed in, so fixed that as well.
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.
(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.)
...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 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/
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 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/
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'):`
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 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/
Attachment #8730018 - Attachment description: MozReview Request: Bug 1255064 - MarionetteHarness should internally use dicts for arguments and not Namespaces → MozReview Request: Bug 1255064 - MarionetteHarness should internally use dicts for arguments; r?maja_zf
Attachment #8730018 - Flags: review?(mjzffr)
I ran all three mach tests, and they all worked.
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.
Attachment #8730018 - Flags: review?(mjzffr)
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/
Attachment #8730018 - Flags: review?(mjzffr)
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 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
Attachment #8730018 - Flags: review?(mjzffr) → review+
https://hg.mozilla.org/mozilla-central/rev/8f3438a45052
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.