Open
Bug 1283216
Opened 8 years ago
Updated 1 year ago
Allow Marionette client object to be constructed with the build from the local environment as the default
Categories
(Testing :: Marionette Client and Harness, defect, P3)
Tracking
(Not tracked)
NEW
People
(Reporter: ato, Unassigned)
References
Details
(Keywords: pi-marionette-client)
Attachments
(1 file)
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.
Reporter | ||
Comment 1•8 years ago
|
||
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.
Whiteboard: [good first bug][lang=py]
Comment 2•8 years ago
|
||
We also do that for ./mach marionette-test.
Keywords: ateam-marionette-client
Comment 3•8 years ago
|
||
(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.
Flags: needinfo?(ato)
Reporter | ||
Comment 4•8 years ago
|
||
(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
Flags: needinfo?(ato)
Updated•8 years ago
|
Comment 5•8 years ago
|
||
Hi, I would like to work on this bug. Could I get some more background information? Thank You
Flags: needinfo?(ato)
Reporter | ||
Comment 6•8 years ago
|
||
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.
Flags: needinfo?(ato)
Comment 7•7 years ago
|
||
I'm new to this. Can you tell me how can I get started?
Flags: needinfo?(ato)
If you're looking for general setup instructions, you can start here: https://wiki.mozilla.org/User:Mjzffr/New_Contributors
Flags: needinfo?(ato)
Reporter | ||
Comment 10•7 years ago
|
||
Yes, please submit your patch to mozreview and r? me for a review.
Flags: needinfo?(ato)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 12•7 years ago
|
||
mozreview-review |
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
Attachment #8837027 -
Flags: review?(ato) → review+
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → anjul.ten
Status: NEW → ASSIGNED
Comment 13•7 years ago
|
||
Pushed by atolfsen@mozilla.com: 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
Reporter | ||
Comment 14•7 years ago
|
||
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f3cf063a5608
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 16•7 years ago
|
||
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?
Flags: needinfo?(ato)
Comment 17•7 years ago
|
||
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.
Flags: needinfo?(ato) → needinfo?(cbook)
Comment 18•7 years ago
|
||
Backout by cbook@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/0a7831d838f7 Backed out changeset f3cf063a5608 on request from whimboo
Updated•7 years ago
|
Status: RESOLVED → REOPENED
status-firefox54:
fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla54 → ---
Updated•7 years ago
|
Flags: needinfo?(cbook)
Comment 19•7 years ago
|
||
Can anyone figure out what's the problem with the patch code?
Comment 20•7 years ago
|
||
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•7 years ago
|
||
Maybe it could be done via another option like --find-binary, if we really need this feature.
Comment 22•7 years ago
|
||
Maybe, let the mentor decide and then If needed, I can continue working on it.
Flags: needinfo?(ato)
Comment 23•7 years ago
|
||
(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•7 years ago
|
||
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.
Flags: needinfo?(ted)
Reporter | ||
Comment 25•7 years ago
|
||
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?
Flags: needinfo?(ted)
Flags: needinfo?(ato)
Flags: needinfo?(ahalberstadt)
Reporter | ||
Comment 26•7 years ago
|
||
(Also, sorry for my late reply. I have been sick the past few days.)
Comment 27•7 years ago
|
||
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.
Flags: needinfo?(ahalberstadt)
Updated•7 years ago
|
Priority: -- → P3
Comment 28•7 years ago
|
||
Removing this bug from the mentored list given that it is not clear what to do here.
Assignee: anjul.ten → nobody
Mentor: ato
Status: REOPENED → NEW
Keywords: good-first-bug
Whiteboard: [lang=py]
Updated•2 years ago
|
Severity: normal → S3
Updated•1 year ago
|
Product: Testing → Remote Protocol
Updated•1 year ago
|
Component: Marionette → Marionette Client and Harness
Product: Remote Protocol → Testing
You need to log in
before you can comment on or make changes to this bug.
Description
•