1283216 - Allow Marionette client object to be constructed with the build from the local environment as the default

Status: NEW

(Testing :: Marionette Client and Harness, defect, P3)

Description

[Andreas Tolfsen ❲:ato❳]

One of my annoyances when interacting with Marionette from the mach Python REPL (`./mach python`) is that I have to type out the full path to the Firefox binary.  This is tedious and I suspect we can do better.

If the bin kwarg is undefined and you have not selected to use an emulator (B2G/Fennec), we could probably use the binary of the current build as the default.

Comment 1

[Andreas Tolfsen ❲:ato❳]

ted suggests we can do:

    >>> from mozbuild.base import MozbuildObject
    >>> build = MozbuildObject.from_environment()
    >>> build.get_binary_path()
    u'/build/debug-mozilla-central/dist/bin/firefox'

… but that we have to be careful to use try…except when importing it because it doesn’t work outside of an objdir virtualenv.

We apparently do this for other test suites, such as https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/mochitest_options.py#22.

Comment 2

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

We also do that for ./mach marionette-test.

Comment 3

[Nelson João Morais (:njasm)]

(In reply to Andreas Tolfsen ‹:ato› from comment #0)
> One of my annoyances when interacting with Marionette from the mach Python
> REPL (`./mach python`) is that I have to type out the full path to the
> Firefox binary.  This is tedious and I suspect we can do better.

I suppose, that we want to add the logic to fallback to default build bin, when instantiating a Marionette object. I suggest to add a new method with the logic you describe below. in the Marionette's ctor, when self.bin = bin, we could self.bin = _get_default_bin_path(bin, ...) or something.

(In reply to Andreas Tolfsen ‹:ato› from comment #0)
> If the bin kwarg is undefined and you have not selected to use an emulator
> (B2G/Fennec), we could probably use the binary of the current build as the
> default.

how to know by the cli arguments that a user selected an emulator? what arg/s to take in consideration? I suspect more then one, since they're optional.

Comment 4

[Andreas Tolfsen ❲:ato❳]

(In reply to Nelson João Morais (:njasm) from comment #3)
> I suppose, that we want to add the logic to fallback to default build bin,
> when instantiating a Marionette object. I suggest to add a new method with
> the logic you describe below. in the Marionette's ctor, when self.bin = bin,
> we could self.bin = _get_default_bin_path(bin, ...) or something.

Yes.  Even a simple if condition would work:

    if bin is None:
        build = MozbuildObject.from_environment()
        self.bin = build.get_binary_path()
    else:
        self.bin = bin

> (In reply to Andreas Tolfsen ‹:ato› from comment #0)
> > If the bin kwarg is undefined and you have not selected to use an emulator
> > (B2G/Fennec), we could probably use the binary of the current build as the
> > default.
> 
> how to know by the cli arguments that a user selected an emulator? what
> arg/s to take in consideration? I suspect more then one, since they're
> optional.

You can probably look at the presence of the --emulator flag and check --app if it equals "fennec".  See the Marionette on Android/Fennec documentation:

    https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/Running_Tests

Comment 5

[Pranaydeep Singh [:theeviltwin]]

Hi, I would like to work on this bug. Could I get some more background information?
Thank You

Comment 6

[Andreas Tolfsen ❲:ato❳]

There’s a lot of information already in this bug on how to fix it.  Please reach out to me on IRC if you want to chat.

Comment 7

[Rahul Bhattacharjee]

I'm new to this. Can you tell me how can I get started?

Comment 8

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

If you're looking for general setup instructions, you can start here: https://wiki.mozilla.org/User:Mjzffr/New_Contributors

Comment 9

[Anjul Tyagi]

Can I work on this one?

Comment 10

[Andreas Tolfsen ❲:ato❳]

Yes, please submit your patch to mozreview and r? me for a review.

Comment 11

[Anjul Tyagi]

Attachment: Bug 1283216 - Fix by allowing the marionette client object to be constructed with the build from the local environment as default.

***
Bug 1283216 - Fix by allowing the marionette client object to be constructed with the build from the local environment as default. r?ato

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

Comment 12

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8837027 [details]
Bug 1283216 - Fix by allowing the marionette client object to be constructed with the build from the local environment as default.

https://reviewboard.mozilla.org/r/112296/#review113696

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f3cf063a5608
Fix by allowing the marionette client object to be constructed with the build from the local environment as default. r=ato

Comment 14

[Andreas Tolfsen ❲:ato❳]

Thanks Anjul!  I tested the patch locally and it works fine.  I have submitted it for integration into Firefox.  Expect it to land in a day or two after it passes CI tests.

On a personal note, I would also like to thank you for fixing this!  This makes interacting with `./mach python` so much easier:

% ./mach python
Python 2.7.13 (default, Jan 19 2017, 14:48:08) 
[GCC 6.3.0 20170118] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from marionette_driver.marionette import Marionette
>>> m = Marionette()
mozversion INFO | application_buildid: 20170214132131
mozversion INFO | application_display_name: Firefox
mozversion INFO | application_id: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}
mozversion INFO | application_name: Firefox
mozversion INFO | application_remotingname: firefox
mozversion INFO | application_vendor: Mozilla
mozversion INFO | application_version: 54.0a1
mozversion INFO | platform_buildid: 20170214132131
mozversion INFO | platform_version: 54.0a1
>>> m.start_session()
{u'rotatable': False, u'browserVersion': u'54.0a1', u'acceptInsecureCerts': False, u'specificationLevel': 0, u'moz:profile': u'/tmp/tmp3x8hLP.mozrunner', u'timeouts': {u'implicit': 0, u'page load': 300000, u'script': 30000}, u'browserName': u'firefox', u'platformVersion': u'4.9.0-1-amd64', u'moz:processID': 10278, u'pageLoadStrategy': u'normal', u'platformName': u'linux', u'moz:accessibilityChecks': False}
>>> m.quit()

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f3cf063a5608

Comment 16

[Andrew Swan [:aswan]]

This change has broken local "mach mochitest" for me, it makes the local Marionette client launch a new copy of Firefox, which then fights with the "real" copy over port 2828 and everything goes south from there...
From casually inspecting the code, I can't see anything guarding against this, was this an oversight or is something else going wrong in my local setup?

Comment 17

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

I think we should get this backed out. Looks like a couple of other people also have this problem now when running other test suites locally.

Comment 18

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/0a7831d838f7
Backed out changeset f3cf063a5608 on request from whimboo

Comment 19

[Anjul Tyagi]

Can anyone figure out what's the problem with the patch code?

Comment 20

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

Other harnesses which make use of Marionette do not pass in a binary on purpose. They start it on their own and only let Marionette connect via the given port, which is by default 2828. In your case here, the binary gets always set if not given.

The requested feature here is kinda in conflict with that. So not sure if it can actually be fixed.

Comment 21

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

Maybe it could be done via another option like --find-binary, if we really need this feature.

Comment 22

[Anjul Tyagi]

Maybe, let the mentor decide and then If needed, I can continue working on it.

Comment 23

[(not currently active) Ted Mielczarek]

(In reply to Henrik Skupin (:whimboo) from comment #20)
> Other harnesses which make use of Marionette do not pass in a binary on
> purpose. They start it on their own and only let Marionette connect via the
> given port, which is by default 2828. In your case here, the binary gets
> always set if not given.
> 
> The requested feature here is kinda in conflict with that. So not sure if it
> can actually be fixed.

I think it would be fine to provide an opt-out for those harnesses to use, and to just fix them in concert with this, something like `m = Marionette(run_app=False)`. That would allow the simple case to keep working.

Comment 24

[Anjul Tyagi]

Can you please elaborate how we can implement this using the idea in your last comment? I can give it a try to fix this one but I'm not able to understand how we can prevent the "bin" from being set to the local build for other harnesses.

Comment 25

[Andreas Tolfsen ❲:ato❳]

Because other test harnesses in the Firefox codebase uses Marionette for various utility tasks, such as installing unsigned add-ons at runtime and for accessing Gecko internals, and they don’t expect the binary to be started when the `bin` argument is left out, we need to make a way for them to instruct the Marionette harness _not_ to start the application.

We could do this by introducing a `run_app=True` flag like ted says:

What we need to do is to add a new ctor argument to the Marionette class in http://searchfox.org/mozilla-central/source/testing/marionette/client/marionette_driver/marionette.py#556 called run_app defaulting to True.  Then we need to add this argument to the mochitest harness in http://searchfox.org/mozilla-central/source/testing/mochitest/runtests.py#2435:

             marionette_args = {
                'symbols_path': options.symbolsPath,
                'socket_timeout': options.marionette_socket_timeout,
                'port_timeout': options.marionette_port_timeout,
                'run_app': False,
            }

I’m unsure which other harnesses use Marionette right now.  ahal, do we need to make similar changes elsewhere?

Comment 26

[Andreas Tolfsen ❲:ato❳]

(Also, sorry for my late reply.  I have been sick the past few days.)

Comment 27

[Andrew Halberstadt [:ahal]]

It'll at least need to be updated in reftest as well:
https://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/runreftest.py#623

Ideally I think the Marionette client wouldn't have any 'runner' logic in it at all (and instead we could make a mozrunner+marionette client abstraction, or else consumers would just use mozrunner directly). But I understand that's scope bloat for this bug.

Comment 28

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

Removing this bug from the mentored list given that it is not clear what to do here.