1287594 - Allow usage of marionette harness options through mach

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Not sure if that is more a general mach bug or if we can customize it for our marionette mach command (incl. firefox-ui-tests and external media tests) for now.

Marionette has a lot of options like --log-html. All those are not accepted when running "mach marionette-test --log-html report.html".

Given that it is kinda hard to setup marionette via the source (not from pypi) a feature like the above would be fantastic to have.

Comment 1

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

I'm going to suggest this bug to a new contributor, so I'll add more details here to help them get started. This is maybe a good second bug, but it shouldn't be too heavy with some guidance.

To test whether the command is working you need a Firefox build first: ./mach build -- you only need to do this once.
Then you can run: ./mach marionette-test -h to see what options are available, as well as the example given by Henrik in Comment 0. To step through the command with a debugger you can use ./mach --debug marionette-test. The marionette-test command calls: http://searchfox.org/mozilla-central/rev/868b17897f7a7fcd7f6f67fd8185a7370db46604/testing/marionette/mach_commands.py#146

The problem is that the `setup_marionette_argument_parser` function doesn't use 'add_logging_group' to incorporate the logger options in the parser. 
* We need to add that behaviour to the function. 
* It would also be good to change it to use MarionetteArguments [1] instead of BaseMarionetteArguments -- then mach would better match the behaviour of runtests.py [2]. 
* Finally, `setup_marionette_argument_parser` should be used in mach_commands.run_marionette as well (instead of calling BaseMarionetteArguments directly).
* And then we'll need to update the harness tests to capture mach's new behaviour. http://searchfox.org/mozilla-central/rev/868b17897f7a7fcd7f6f67fd8185a7370db46604/testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py#64
* Something similar will have to be done with mach commands for external-media-tests and firefox-ui-functional, but let's focus on marionette-test first.

[1,2] https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/testing/marionette/harness/marionette/runtests.py#24

Comment 2

[Calvin Lui (:clui)]

Thanks for the guidance!
I'll start tackling the bug soon and reach out if I need more assistance.

Comment 3

[Calvin Lui (:clui)]

Maja, I have a few issues that I would appreciate clarification.

What is meant by having `setup_marionette_argument_parser` function use `add_logging_group`? Is that not handled after the function returns: http://searchfox.org/mozilla-central/rev/868b17897f7a7fcd7f6f67fd8185a7370db46604/testing/marionette/mach_commands.py#40

Also, I have made a minor attempt to address your 2nd & 3rd bullet points by importing MarionetteArguments from marionette.runtests and returning the call to it in place of here:
http://searchfox.org/mozilla-central/rev/868b17897f7a7fcd7f6f67fd8185a7370db46604/testing/marionette/mach_commands.py#27
And thenI call `setup_marionette_argument_parser() in line 39. Is this along the right idea?

Finally, I was hoping to get some more guidance on your bullet point about updating the harness tests. 
Am i supposed to add arguments to that list, and how do I know exactly which new arguments are allowed/captured?

Thanks!

Comment 4

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

(In reply to Calvin Lui (:clui) from comment #3)
> Maja, I have a few issues that I would appreciate clarification.
> 
> What is meant by having `setup_marionette_argument_parser` function use
> `add_logging_group`? Is that not handled after the function returns:
> http://searchfox.org/mozilla-central/rev/
> 868b17897f7a7fcd7f6f67fd8185a7370db46604/testing/marionette/mach_commands.
> py#40

Yes, we want to move that behaviour to the function `setup_marionette_argument_parser`. In other words, right now the arguments class is set up in a couple of different places and we want to consolidate that in `setup_marionette_argument_parser` and just call `setup_marionette_argument_parser` everywhere in mach_commands.

> 
> Also, I have made a minor attempt to address your 2nd & 3rd bullet points by
> importing MarionetteArguments from marionette.runtests and returning the
> call to it in place of here:
> http://searchfox.org/mozilla-central/rev/
> 868b17897f7a7fcd7f6f67fd8185a7370db46604/testing/marionette/mach_commands.
> py#27
> And thenI call `setup_marionette_argument_parser() in line 39. Is this along
> the right idea?

Yes

> 
> Finally, I was hoping to get some more guidance on your bullet point about
> updating the harness tests. 
> Am i supposed to add arguments to that list, and how do I know exactly which
> new arguments are allowed/captured?

Yes. Try looking at what args are passed into the call to MarionetteHarness: http://searchfox.org/mozilla-central/rev/868b17897f7a7fcd7f6f67fd8185a7370db46604/testing/marionette/mach_commands.py#58

Comment 5

[Calvin Lui (:clui)]

Attachment: Bug 1287594 - Allow usage of marionette harness options through mach.

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

Comment 6

[Calvin Lui (:clui)]

Hi Maja,

I have pushed my code to MozReview and have received the following review url:
https://reviewboard.mozilla.org/r/66798/

Please let me know if you have any comments or change is necessary.

Best.

Comment 7

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

Great, thanks for submitting to MozReview. I will publish comments shortly. Next time you push up for review, make sure your commit message is in the following format

>Bug 123 - some description; r?irc_nick_of_reviewer

For example, including r?maja_zf at the end will flag me for review so I get a notification and there will be no need to need-info me separately. Then I will respond with r- if more changes are needed, or r+ if the patch is accepted as is. 

I will be away for next two weeks, so you should flag AutomatedTester for review until I'm back. If you run into issues with anything along the way, feel free to ask for help in #ateam or #automation.

Comment 8

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

Comment on attachment 8774280 [details]
Bug 1287594 - Allow usage of marionette harness options through mach.

https://reviewboard.mozilla.org/r/66800/#review63688

::: testing/marionette/mach_commands.py:26
(Diff revision 1)
>  def setup_marionette_argument_parser():
> -    from marionette.runner.base import BaseMarionetteArguments
> -    return BaseMarionetteArguments()
> +    from marionette.runtests import MarionetteArguments
> +    from mozlog.structured import commandline
> +    
> +    parser = MarionetteArguments()
> +    commandline.add_logging_group(parser)
> +    return parser

This looks correct. However, I think MozReview is pointing out a whitespace issue here. Please make sure that each line is indented uses spaces only, not tabs. 

You can remove the blank lines in this function as well.

Comment 9

[Calvin Lui (:clui)]

Comment on attachment 8774280 [details]
Bug 1287594 - Allow usage of marionette harness options through mach.

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

Comment 10

[Calvin Lui (:clui)]

Comment on attachment 8774280 [details]
Bug 1287594 - Allow usage of marionette harness options through mach.

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

Comment 11

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

Comment on attachment 8774280 [details]
Bug 1287594 - Allow usage of marionette harness options through mach.

https://reviewboard.mozilla.org/r/66800/#review63970

Thanks for the quick follow-up, Calvin. I missed a couple of comments last time around: make sure to remove unused imports.

Also, please start the bug description with a capital (i.e. Bug 123 - Allow usage...)

::: testing/marionette/mach_commands.py:34
(Diff revision 3)
> +    parser = MarionetteArguments()
> +    commandline.add_logging_group(parser)
> +    return parser
>  
>  def run_marionette(tests, binary=None, topsrcdir=None, **kwargs):
>      from mozlog.structured import commandline

This line should be removed.

::: testing/marionette/mach_commands.py:38
(Diff revision 3)
>  def run_marionette(tests, binary=None, topsrcdir=None, **kwargs):
>      from mozlog.structured import commandline
>  
>      from marionette.runtests import (
>          MarionetteTestRunner,
>          BaseMarionetteArguments,

This line is also nolonger needed.

Comment 12

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

Now that you've made the necessary changes for |mach marionette-test|, could you also submit additional patches to fix |mach firefox-ui-functional|, |mach firefox-ui-update| and |mach external-media-tests|. You can ask whimboo to review those changes (whereas AutomatedTest will review |mach marionette-test|, as I mentioned earlier.

See https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/testing/firefox-ui/mach_commands.py
and https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/dom/media/test/external/mach_commands.py

Thanks!

Comment 13

[Calvin Lui (:clui)]

https://reviewboard.mozilla.org/r/66800/#review63970

> This line should be removed.

commandline is used in line 57
`args.logger = commandline.setup_logging("Marionette Unit Tests",
                                            args,
                                            {"mach": sys.stdout})`

Comment 14

[Calvin Lui (:clui)]

Comment on attachment 8774280 [details]
Bug 1287594 - Allow usage of marionette harness options through mach.

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

Comment 15

[Calvin Lui (:clui)]

Comment on attachment 8774280 [details]
Bug 1287594 - Allow usage of marionette harness options through mach.

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

Comment 16

[Calvin Lui (:clui)]

Comment on attachment 8774280 [details]
Bug 1287594 - Allow usage of marionette harness options through mach.

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

Comment 17

[Calvin Lui (:clui)]

Attachment: Bug 1287594 - Allow usage of marionette harness options through mach.

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

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://reviewboard.mozilla.org/r/67366/#review65002

I would like to wait with this review until David has reviewed the base Marionette changes. But one thing I want to mention is that you missed the mach commands for firefox-ui-tests which can be found under testing/firefox-ui/. Can you please check those too and get them added? Thanks.

Comment 19

[Calvin Lui (:clui)]

Comment on attachment 8775019 [details]
Bug 1287594 - Allow usage of marionette harness options through mach.

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

Comment 20

[Calvin Lui (:clui)]

(In reply to Henrik Skupin (:whimboo) from comment #18)
> https://reviewboard.mozilla.org/r/67366/#review65002
> 
> I would like to wait with this review until David has reviewed the base
> Marionette changes. But one thing I want to mention is that you missed the
> mach commands for firefox-ui-tests which can be found under
> testing/firefox-ui/. Can you please check those too and get them added?
> Thanks.

Sounds good, we will wait for David.
Also, I have made the changes for firefox-ui-tests. Please let me know if there is anything else

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

David, would you mind to review the Marionette commit so that we can continue? Thanks.

Comment 22

[David Burns :automatedtester]

Comment on attachment 8774280 [details]
Bug 1287594 - Allow usage of marionette harness options through mach.

https://reviewboard.mozilla.org/r/66800/#review66938

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8775019 [details]
Bug 1287594 - Allow usage of marionette harness options through mach.

https://reviewboard.mozilla.org/r/67366/#review68638

Generally it looks fine to me. Please check the comment for the marionette unit test, which doesn't seem to be right to me. Maybe also ask for a feedback from Maja, at least from my side it would be good if she could have look at those changes.

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:131
(Diff revision 2)
> +        'update_direct_only': None,
> +        'update_fallback_only': None,
> +        'update_mar_channels': None,
> +        'update_override_url': None,
> +        'update_target_buildid': None,
> +        'update_target_version': None,

All those command line arguments are not part of marionette-test which this test is actually covering, and I think those should be removed.

Comment 24

[Calvin Lui (:clui)]

Comment on attachment 8775019 [details]
Bug 1287594 - Allow usage of marionette harness options through mach.

https://reviewboard.mozilla.org/r/67366/#review68638

> All those command line arguments are not part of marionette-test which this test is actually covering, and I think those should be removed.

I added those arguments as a part of the firefox-ui-update test for mach.
I will notify Maja to also provide input.

Comment 25

[Calvin Lui (:clui)]

(In reply to Calvin Lui (:clui) from comment #24)
> Comment on attachment 8775019 [details]
> Bug 1287594 - Allow usage of marionette harness options through mach.
> 
> https://reviewboard.mozilla.org/r/67366/#review68638
> 
> > All those command line arguments are not part of marionette-test which this test is actually covering, and I think those should be removed.
> 
> I added those arguments as a part of the firefox-ui-update test for mach.
> I will notify Maja to also provide input.

@Maja, can you please take a look at this review?
Thanks

Comment 26

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

Comment on attachment 8775019 [details]
Bug 1287594 - Allow usage of marionette harness options through mach.

https://reviewboard.mozilla.org/r/67366/#review68638

> I added those arguments as a part of the firefox-ui-update test for mach.
> I will notify Maja to also provide input.

Only ./mach marionette-test arguments should be included in that list.

Comment 27

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

Note: some other changes have landed in this code over the past weeks, so you will need to rebase your commit onto mozilla-central and resolve any conflicts.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hi Calvin, please let us know if you have problems updating your patch, and if the reply from Maja answers the questions. Thanks.

Comment 29

[Calvin Lui (:clui)]

Comment on attachment 8774280 [details]
Bug 1287594 - Allow usage of marionette harness options through mach.

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

Comment 30

[Calvin Lui (:clui)]

Comment on attachment 8775019 [details]
Bug 1287594 - Allow usage of marionette harness options through mach.

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

Comment 31

[Calvin Lui (:clui)]

Comment on attachment 8775019 [details]
Bug 1287594 - Allow usage of marionette harness options through mach.

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

Comment 32

[Calvin Lui (:clui)]

Comment on attachment 8775019 [details]
Bug 1287594 - Allow usage of marionette harness options through mach.

https://reviewboard.mozilla.org/r/67366/#review68638

> Only ./mach marionette-test arguments should be included in that list.

I have gone ahead and only included marionette-test args

Comment 33

[Calvin Lui (:clui)]

I have gone ahead and rebased along with only including marionette-test args in the runner file.

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8775019 [details]
Bug 1287594 - Allow usage of marionette harness options through mach.

https://reviewboard.mozilla.org/r/67366/#review71020

r=me with a successful try build, which I will trigger now.

Thank you Calvin!

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Wait, it's the mach command. So a try build will not help. :) It should be fine to get landed.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/563592a6f576
Allow usage of marionette harness options through mach. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/a2deef3b5ce0
Allow usage of marionette harness options through mach. r=whimboo

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/563592a6f576
https://hg.mozilla.org/mozilla-central/rev/a2deef3b5ce0

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The landing of this patch actually reverted all of my changes for the --binary option for mach via bug 1295492. :( The last rebase as done here, was simply busted and slipped through the final review. I will reopen bug 1295492 to get my changes landed again.