Closed Bug 1287594 Opened 3 years ago Closed 3 years ago

Allow usage of marionette harness options through mach

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: whimboo, Assigned: clui, Mentored)

Details

(Whiteboard: [lang=py])

Attachments

(2 files)

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.
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
Mentor: mjzffr
Whiteboard: [lang=py]
Thanks for the guidance!
I'll start tackling the bug soon and reach out if I need more assistance.
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!
Flags: needinfo?(mjzffr)
(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
Flags: needinfo?(mjzffr)
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.
Flags: needinfo?(mjzffr)
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.
Flags: needinfo?(mjzffr)
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.
Attachment #8774280 - Flags: review-
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/
Attachment #8774280 - Flags: review-
Attachment #8774280 - Flags: review?(dburns)
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/
Attachment #8774280 - Attachment description: Fix (Bug 1287594) allow usage of marionette harness options through mach → Bug 1287594 - allow usage of marionette harness options through mach.
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.
Attachment #8774280 - Flags: review-
Assignee: nobody → clui159
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!
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 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/
Attachment #8774280 - Attachment description: Bug 1287594 - allow usage of marionette harness options through mach. → Bug 1287594 - Allow usage of marionette harness options through mach.
Attachment #8774280 - Flags: review- → review?(dburns)
Attachment #8774280 - Flags: review?(dburns)
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/
Attachment #8774280 - Flags: review?(dburns)
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/
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 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/
(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
David, would you mind to review the Marionette commit so that we can continue? Thanks.
Flags: needinfo?(dburns)
Flags: needinfo?(dburns)
Attachment #8774280 - Flags: review?(dburns) → review+
Comment on attachment 8774280 [details]
Bug 1287594 - Allow usage of marionette harness options through mach.

https://reviewboard.mozilla.org/r/66800/#review66938
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 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.
(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
Flags: needinfo?(mjzffr)
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.
Flags: needinfo?(mjzffr)
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.
Hi Calvin, please let us know if you have problems updating your patch, and if the reply from Maja answers the questions. Thanks.
Flags: needinfo?(clui159)
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
I have gone ahead and rebased along with only including marionette-test args in the runner file.
Flags: needinfo?(clui159)
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!
Attachment #8775019 - Flags: review?(hskupin) → review+
Wait, it's the mach command. So a try build will not help. :) It should be fine to get landed.
Flags: needinfo?(mjzffr)
Flags: needinfo?(mjzffr)
Pushed by mjzffr@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/563592a6f576
https://hg.mozilla.org/mozilla-central/rev/a2deef3b5ce0
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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.
You need to log in before you can comment on or make changes to this bug.