Closed
Bug 787203
Opened 12 years ago
Closed 9 years ago
Get marionette working on Fennec
Categories
(Remote Protocol :: Marionette, defect)
Tracking
(firefox50 fixed)
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: automatedtester, Assigned: impossibus)
References
(Blocks 1 open bug)
Details
(Whiteboard: [marionette=1.0])
Attachments
(9 files, 2 obsolete files)
190.09 KB,
text/x-log
|
Details | |
58 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
gps
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
ahal
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
ahal
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
ahal
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
ahal
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
WebApps need to have marionette working in Fennec so this is a tracking bug related to that
Reporter | ||
Updated•10 years ago
|
Whiteboard: [marionette=1.0]
Reporter | ||
Comment 1•10 years ago
|
||
Moving this to a Q3 goal.
* Finish off Marionette support on Fennec
* Stand up tests on buildbot/TC (whatever we decide)
dburns: it seems like you had some WIP on this. Did you post that somewhere? What's the status?
Flags: needinfo?(dburns)
Reporter | ||
Comment 5•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/39167/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39167/
Reporter | ||
Comment 6•9 years ago
|
||
Uploaded my WIP. We have a session but because I just hacked the tab code to "work" you can't do anything more than get a session.
Flags: needinfo?(dburns)
Review commit: https://reviewboard.mozilla.org/r/39643/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39643/
(In reply to Nick Alexander :nalexander from comment #7)
> Created attachment 8730043 [details]
> MozReview Request: Bug 787203: Get Marionette working in Fennec.
>
> Review commit: https://reviewboard.mozilla.org/r/39643/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/39643/
With this, I get good results with:
nalexander@chocho ~/M/g/t/m/h/marionette> python runtests.py --address=localhost:2828 --type=b2g tests/unit/unit-tests.ini > out
nalexander@chocho ~/M/g/t/m/h/marionette> pwd
/Users/nalexander/Mozilla/gecko/testing/marionette/harness/marionette
(Log attached.)
Reporter | ||
Comment 10•9 years ago
|
||
https://reviewboard.mozilla.org/r/39643/#review36315
Great work!
::: testing/marionette/driver.js:3058
(Diff revision 1)
> * identified within BrowserObject by a tab.
> */
> Object.defineProperty(BrowserObj.prototype, "curFrameId", {
> get() {
> let rv = null;
> - if (this.driver.appName != "Firefox") {
> + if (this.driver.appName != "Firefox" && this.driver.appName != "Fennec") {
this could just be `if (this.driver.appName == "B2G")` instead. This is not going to work in iOS so no need to be explicit about the "nots"
dburns: I'm not really plugged into the Marionette / A-Team world. What else needs to happen to land (some version) of this patch? Some automated testing would be nice, but shouldn't block getting these small fixes out the door.
My interest: I'm pursuing some additional Android-specific testing work that I might want to use Marionette for. I want to investigate an espresso-web style testing API. espresso-web is an ad-hoc wrapper around some WebDriver "Atoms", as far as I can tell; see https://google.github.io/android-testing-support-library/docs/espresso/web/. I'm investigating an espresso-style API wrapping WebDriver/WebElement.
Flags: needinfo?(dburns)
Reporter | ||
Comment 12•9 years ago
|
||
(In reply to Nick Alexander :nalexander from comment #11)
> dburns: I'm not really plugged into the Marionette / A-Team world. What
> else needs to happen to land (some version) of this patch? Some automated
> testing would be nice, but shouldn't block getting these small fixes out the
> door.
If we have this working locally then we should get them up for review ASAP and landed.
>
> My interest: I'm pursuing some additional Android-specific testing work that
> I might want to use Marionette for. I want to investigate an espresso-web
> style testing API. espresso-web is an ad-hoc wrapper around some WebDriver
> "Atoms", as far as I can tell; see
> https://google.github.io/android-testing-support-library/docs/espresso/web/.
> I'm investigating an espresso-style API wrapping WebDriver/WebElement.
Speaking to my Google contact that led the WebDriver team there he said that the espresso team had little interest in supporting web testing. As mentioned on IRC, I would prefer we used something like Selendroid/Appium as they would be much easier to support (and also don't use Fluent APIs!) but I have no skin in the fennec game so won't argue much on this as you know the workflow the mobile team want.
Flags: needinfo?(dburns)
(In reply to David Burns :automatedtester from comment #12)
> (In reply to Nick Alexander :nalexander from comment #11)
> > dburns: I'm not really plugged into the Marionette / A-Team world. What
> > else needs to happen to land (some version) of this patch? Some automated
> > testing would be nice, but shouldn't block getting these small fixes out the
> > door.
>
> If we have this working locally then we should get them up for review ASAP
> and landed.
>
> >
> > My interest: I'm pursuing some additional Android-specific testing work that
> > I might want to use Marionette for. I want to investigate an espresso-web
> > style testing API. espresso-web is an ad-hoc wrapper around some WebDriver
> > "Atoms", as far as I can tell; see
> > https://google.github.io/android-testing-support-library/docs/espresso/web/.
> > I'm investigating an espresso-style API wrapping WebDriver/WebElement.
>
> Speaking to my Google contact that led the WebDriver team there he said that
> the espresso team had little interest in supporting web testing.
Interesting, thanks. That explains why espresso-web is just a stub.
As
> mentioned on IRC, I would prefer we used something like Selendroid/Appium as
> they would be much easier to support (and also don't use Fluent APIs!) but I
> have no skin in the fennec game so won't argue much on this as you know the
> workflow the mobile team want.
I would be pleased to use either Selendroid or Appium, but both appear very tied to Android WebViews. I didn't spend too much time understanding the details of Selendroid and would be happy to dig into it further.
I would like to understand how you feel about Fluent APIs. I am interested in integrating with Espresso for timing and robustness purposes. I evaluated espresso-web as an API for driving Marionette with WebDriver and it's not a great fit: I ended up saying to myself, "why bother?" Any more context on the Espresso-style vs. imperative style API would be useful. Thanks!
Flags: needinfo?(dburns)
Reporter | ||
Comment 14•9 years ago
|
||
(In reply to Nick Alexander :nalexander from comment #13)
> (In reply to David Burns :automatedtester from comment #12)
> > (In reply to Nick Alexander :nalexander from comment #11)
> > > dburns: I'm not really plugged into the Marionette / A-Team world. What
> > > else needs to happen to land (some version) of this patch? Some automated
> > > testing would be nice, but shouldn't block getting these small fixes out the
> > > door.
> >
> > If we have this working locally then we should get them up for review ASAP
> > and landed.
> >
> > >
> > > My interest: I'm pursuing some additional Android-specific testing work that
> > > I might want to use Marionette for. I want to investigate an espresso-web
> > > style testing API. espresso-web is an ad-hoc wrapper around some WebDriver
> > > "Atoms", as far as I can tell; see
> > > https://google.github.io/android-testing-support-library/docs/espresso/web/.
> > > I'm investigating an espresso-style API wrapping WebDriver/WebElement.
> >
> > Speaking to my Google contact that led the WebDriver team there he said that
> > the espresso team had little interest in supporting web testing.
>
> Interesting, thanks. That explains why espresso-web is just a stub.
>
> As
> > mentioned on IRC, I would prefer we used something like Selendroid/Appium as
> > they would be much easier to support (and also don't use Fluent APIs!) but I
> > have no skin in the fennec game so won't argue much on this as you know the
> > workflow the mobile team want.
>
> I would be pleased to use either Selendroid or Appium, but both appear very
> tied to Android WebViews. I didn't spend too much time understanding the
> details of Selendroid and would be happy to dig into it further.
Both of these are not tied to webviews, they work against anything that Espresso can work against, they use the instrumentation APIs built into Android.
>
> I would like to understand how you feel about Fluent APIs.
Having spent far too many hours helping people debug fluent APIs I have learnt to hate them. Having to step through numerous methods to get what you want has just left me with a bad taste in my mouth.
Flags: needinfo?(dburns)
Sadly I won't be able to push this across the line. It's close, though, and worth polishing up. Perhaps gbrown can do the honours?
Flags: needinfo?(gbrown)
![]() |
||
Comment 16•9 years ago
|
||
Sorry, I don't think I can fit in another project just now. I'm happy to try to answer specific questions, and will try to circle back later in the quarter.
Flags: needinfo?(gbrown)
Assignee | ||
Comment 17•9 years ago
|
||
I'm going to dig into this. Expect questions from me in the near future!
Assignee | ||
Comment 18•9 years ago
|
||
Hi Nick, I'm trying to run your patch according to Comment 8 with an Android emulator. No luck yet. Maybe I'm missing some prerequisite step? Did you test with an emulator or a physical device?
> ./mach android-emulator
> adb forward tcp:2828 tcp:2828
> ./mach run # I've confirmed that prefs for marionette and port 2828 are set appropriately in browser intance
> cd testing/marionette/harness/marionette
> python runtests.py --address=localhost:2828 --type=b2g tests/unit/unit-tests.ini
At this point, socket happily connects at port, but then first packet received is empty, which is logged as "IOError: Connection to Marionette server is lost."
Flags: needinfo?(nalexander)
(In reply to Maja Frydrychowicz (:maja_zf) from comment #18)
> Hi Nick, I'm trying to run your patch according to Comment 8 with an Android
> emulator. No luck yet. Maybe I'm missing some prerequisite step? Did you
> test with an emulator or a physical device?
>
> > ./mach android-emulator
> > adb forward tcp:2828 tcp:2828
> > ./mach run # I've confirmed that prefs for marionette and port 2828 are set appropriately in browser intance
> > cd testing/marionette/harness/marionette
> > python runtests.py --address=localhost:2828 --type=b2g tests/unit/unit-tests.ini
>
> At this point, socket happily connects at port, but then first packet
> received is empty, which is logged as "IOError: Connection to Marionette
> server is lost."
You have installed and run Fennec on the emulator? You built Fennec with the ENABLE_MARIONETTE flag on, and the pref is enabled? (I think the patch includes that, but worth checking.)
Try to telnet to localhost:2828 after forwarding; if you don't get a handshake, you have other problems.
Finally, I only tried this on device. It's possible there's an emulator interaction.
Flags: needinfo?(nalexander)
![]() |
||
Comment 20•9 years ago
|
||
Is there a chance this is related to bug 1267925?
Comment 21•9 years ago
|
||
(In reply to Geoff Brown [:gbrown] from comment #20)
> Is there a chance this is related to bug 1267925?
I think that is unlikely.
Reporter | ||
Comment 22•9 years ago
|
||
Maja has this running, though not always reliably.
Assignee | ||
Comment 23•9 years ago
|
||
...and now I have it running reliably. For the curious, I think the flakiness came partly from not waiting long enough for Fennec to load (a long time after it "looks" loaded), and otherwise from how I sometimes happened to enable port-forwarding with adb. For example, if I start an emulator, then |./mach install| then |adb forward tcp:2828 tcp:2828| then |./mach run|, all is well. On the other hand, running |adb forward tcp:2828 tcp:2828| at any time from within |adb shell| is not equivalent (oops) -- it occupies the emulator's port 2828 and so interferes with Marionette.
Assignee | ||
Comment 24•9 years ago
|
||
(In reply to David Burns :automatedtester from comment #12)
> (In reply to Nick Alexander :nalexander from comment #11)
> > dburns: I'm not really plugged into the Marionette / A-Team world. What
> > else needs to happen to land (some version) of this patch? Some automated
> > testing would be nice, but shouldn't block getting these small fixes out the
> > door.
>
> If we have this working locally then we should get them up for review ASAP
> and landed.
The missing piece is managing emulator+fennec start-up in Marionette client, the way it's already done for desktop and for b2g. I'm going to address this in testing/marionette/client/marionette_driver/geckoinstance.py, with some substantial clean-up in Marionette harness and client while I'm at it.
For the automation side, we'll need to:
- Adapt the marionette.py mozharness script (and/or create an Android-specific config)
- Enable Marionette in Fennec builds -- I need to talk to someone on the Fennec team about that. :snorp, might that be you?
- Schedule the job in buildbot, or TaskCluster eventually. My understanding is that Fennec builds in TC are in the works (Bug 1267430), but I'm not aware of a timeline.
Assignee: dburns → mjzffr
Flags: needinfo?(snorp)
Assignee | ||
Comment 25•9 years ago
|
||
Margaret -- are you okay with us enabling Marionette in some Fennec builds? For example, debug only? I know one security concern is to make sure Marionette isn't enabled in any Fennec releases.
Greg & Margaret -- it's my understanding that nalexander handles a lot of the Fennec build stuff, but he's not available for now. Could you point me to where Fennec builds are configured and whom to ask for review?
Flags: needinfo?(snorp)
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(gps)
Comment 26•9 years ago
|
||
You are likely interested in mobile/android/confvars.sh. You can ask any build peer for review. :ted historically handles things involving test harness foo. But any of us are appropriate. chmanchester and I have also been touching a lot of code at the intersection of testing and the build system lately.
Flags: needinfo?(gps)
Comment 27•9 years ago
|
||
(In reply to Maja Frydrychowicz (:maja_zf) from comment #25)
> Margaret -- are you okay with us enabling Marionette in some Fennec builds?
> For example, debug only? I know one security concern is to make sure
> Marionette isn't enabled in any Fennec releases.
Sorry for the slow response. Sure, I think it's fine to enable Marionette in some builds.
Flags: needinfo?(margaret.leibovic)
Assignee | ||
Comment 28•9 years ago
|
||
Assignee | ||
Comment 29•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/61360/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61360/
Attachment #8766498 -
Flags: review?(dburns)
Attachment #8766499 -
Flags: review?(gps)
Attachment #8766500 -
Flags: review?(ahalberstadt)
Attachment #8766501 -
Flags: review?(ahalberstadt)
Attachment #8766502 -
Flags: review?(ahalberstadt)
Attachment #8766503 -
Flags: review?(ahalberstadt)
Attachment #8766504 -
Flags: review?(dburns)
Attachment #8766505 -
Flags: review?(dburns)
Assignee | ||
Comment 30•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/61362/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61362/
Assignee | ||
Comment 31•9 years ago
|
||
Allows Marionette client's FennecInstance to clean up properly
Review commit: https://reviewboard.mozilla.org/r/61364/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61364/
Assignee | ||
Comment 32•9 years ago
|
||
This is a refactor. |start_logcat| allows filtering by tag and is used for
recording gecko.log for Marionette tests on Fennec.
Review commit: https://reviewboard.mozilla.org/r/61366/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61366/
Assignee | ||
Comment 33•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/61368/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61368/
Assignee | ||
Comment 34•9 years ago
|
||
For adb forward --remove|remove-all
Review commit: https://reviewboard.mozilla.org/r/61370/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61370/
Assignee | ||
Comment 35•9 years ago
|
||
Add FennecInstance. Refactor GeckoInstance as well as other related clean-up.
Marionette runner can now launch both Android emulator and Fennec, or connect to a running
emulator and then launch Fennec. (The runner can also connect to an already running Fennec
instance provided that Marionette and adb port forwarding have previously been enabled.)
Review commit: https://reviewboard.mozilla.org/r/61372/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61372/
Assignee | ||
Comment 36•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/61374/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61374/
Assignee | ||
Comment 37•9 years ago
|
||
Next: schedule Marionette tests for Android in TaskCluster.
Reporter | ||
Comment 38•9 years ago
|
||
Comment on attachment 8766498 [details]
Bug 787203 - Get Marionette working in Fennec;
https://reviewboard.mozilla.org/r/61360/#review58228
::: testing/marionette/driver.js:1284
(Diff revision 1)
> * @param {string} name
> * Target name or ID of the window to switch to.
> */
> GeckoDriver.prototype.switchToWindow = function*(cmd, resp) {
> let switchTo = cmd.parameters.name;
> - let isB2G = this.appName == "B2G";
> + let isMobile = this.appName == "B2G" || this.appName == "Fennec";
we don't need the b2g part
::: testing/marionette/driver.js:1291
(Diff revision 1)
>
> let getOuterWindowId = function(win) {
> let rv = win.QueryInterface(Ci.nsIInterfaceRequestor)
> .getInterface(Ci.nsIDOMWindowUtils)
> .outerWindowID;
> - rv += isB2G ? "-b2g" : "";
> + rv += isMobile ? "-b2g" : "";
if we remove the b2g line I suspect we don't need this anymore
::: testing/marionette/harness/marionette/runner/base.py:767
(Diff revision 1)
> need_external_ip = True
> if not self.marionette:
> self.marionette = self.driverclass(**self._build_kwargs())
> # if we're working against a desktop version, we usually don't need
> # an external ip
> - if self.capabilities['device'] == "desktop":
> + if self.capabilities['device'] == "desktop" and self.capabilities['browserName'] != 'Fennec':
device has been removed so you will need to rebase
::: testing/marionette/harness/marionette/runner/base.py:820
(Diff revision 1)
>
> self._add_tests(tests)
>
> self.logger.info("running with e10s: {}".format(self.e10s))
> - version_info = mozversion.get_version(binary=self.bin,
> - sources=self.sources,
> + version_info = None
> + #version_info = mozversion.get_version(binary=self.bin,
should we remove this?
Attachment #8766498 -
Flags: review?(dburns) → review-
Reporter | ||
Comment 39•9 years ago
|
||
Comment on attachment 8766504 [details]
Bug 787203 - Add emulator and Fennec support to Marionette harness and client;
https://reviewboard.mozilla.org/r/61372/#review58234
Attachment #8766504 -
Flags: review?(dburns) → review+
Reporter | ||
Comment 40•9 years ago
|
||
Comment on attachment 8766505 [details]
Bug 787203 - Allow mach marionette-test to run with android build;
https://reviewboard.mozilla.org/r/61374/#review58236
Attachment #8766505 -
Flags: review?(dburns) → review+
Comment 41•9 years ago
|
||
Comment on attachment 8766499 [details]
Bug 787203 - Enable Marionette on debug Fennec build (android-api-15-debug);
https://reviewboard.mozilla.org/r/61362/#review58280
I'd have to look at the code history, but ENABLE_MARIONETTE=1 in the in-tree mozconfigs feels like a cargo cult that originated when someone didn't add an --enable-marionette flag to configure like they should have.
This will eventually need to get rewritten to play nice in a moz.configure world. But that is scope bloat for this commit series and I don't want to fault you for falling into a trap someone else created. This is fine to land.
Attachment #8766499 -
Flags: review?(gps) → review+
Assignee | ||
Comment 42•9 years ago
|
||
Comment on attachment 8766498 [details]
Bug 787203 - Get Marionette working in Fennec;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61360/diff/1-2/
Attachment #8766498 -
Flags: review- → review?(dburns)
Assignee | ||
Comment 43•9 years ago
|
||
Comment on attachment 8766499 [details]
Bug 787203 - Enable Marionette on debug Fennec build (android-api-15-debug);
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61362/diff/1-2/
Assignee | ||
Comment 44•9 years ago
|
||
Comment on attachment 8766500 [details]
Bug 787203 - [mozrunner] Update `disconnected` when cleaning up after emulator shutdown;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61364/diff/1-2/
Assignee | ||
Comment 45•9 years ago
|
||
Comment on attachment 8766501 [details]
Bug 787203 - [mozrunner] Expose a start_logcat method in Device;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61366/diff/1-2/
Assignee | ||
Comment 46•9 years ago
|
||
Comment on attachment 8766502 [details]
Bug 787203 - [mozrunner] Pass application arguments to FennecRunner;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61368/diff/1-2/
Assignee | ||
Comment 47•9 years ago
|
||
Comment on attachment 8766503 [details]
Bug 787203 - [mozdevice] Add remove_forward method to DeviceManagerADB;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61370/diff/1-2/
Assignee | ||
Comment 48•9 years ago
|
||
Comment on attachment 8766504 [details]
Bug 787203 - Add emulator and Fennec support to Marionette harness and client;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61372/diff/1-2/
Assignee | ||
Comment 49•9 years ago
|
||
Comment on attachment 8766505 [details]
Bug 787203 - Allow mach marionette-test to run with android build;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61374/diff/1-2/
Assignee | ||
Comment 50•9 years ago
|
||
https://reviewboard.mozilla.org/r/61360/#review58228
> device has been removed so you will need to rebase
Done. I also removed a couple of other left-over device bits.
> should we remove this?
This was removed later in the patch series, so I just included the change in this patch instead.
Assignee | ||
Comment 51•9 years ago
|
||
https://reviewboard.mozilla.org/r/61372/#review58434
::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:63
(Diff revision 2)
> @pytest.fixture()
> def mach_parsed_kwargs(logger):
Since the r+ carries over, here's a highlight of something new :I updated a harness-test fixture to reflect the changes in BaseMarionetteArguments.
Comment 52•9 years ago
|
||
Comment on attachment 8766500 [details]
Bug 787203 - [mozrunner] Update `disconnected` when cleaning up after emulator shutdown;
https://reviewboard.mozilla.org/r/61364/#review58462
Attachment #8766500 -
Flags: review?(ahalberstadt) → review+
Comment 53•9 years ago
|
||
Comment on attachment 8766501 [details]
Bug 787203 - [mozrunner] Expose a start_logcat method in Device;
https://reviewboard.mozilla.org/r/61366/#review58464
::: testing/mozbase/mozrunner/mozrunner/devices/base.py:144
(Diff revision 2)
> logcat_args = [self.app_ctx.adb, '-s', '%s' % serial,
> 'logcat', '-v', 'time', '-b', 'main', '-b', 'radio']
This is a dead variable now, please remove
Attachment #8766501 -
Flags: review?(ahalberstadt) → review+
Updated•9 years ago
|
Attachment #8766502 -
Flags: review?(ahalberstadt) → review-
Comment 54•9 years ago
|
||
Comment on attachment 8766502 [details]
Bug 787203 - [mozrunner] Pass application arguments to FennecRunner;
https://reviewboard.mozilla.org/r/61368/#review58468
::: testing/mozbase/mozrunner/mozrunner/base/device.py:177
(Diff revision 2)
> cmd.extend(['-s', self.app_ctx.dm._deviceSerial])
> cmd.append('shell')
> app = "%s/org.mozilla.gecko.BrowserApp" % self.app_ctx.remote_process
> cmd.extend(['am', 'start', '-a', 'android.activity.MAIN', '-n', app])
> params = ['-no-remote', '-profile', self.app_ctx.remote_profile]
> + params.extend([p for p in self.cmdargs if p not in params])
This could cause problems, for example if the cmdargs are ['-profile', 'abcxyz.default'], then -profile will not get added (because it already exists), but 'abcxyz.default' will.
I think it would be better to simply extend cmdargs without doing any filtering and leave it up to consumers to get it right.
Comment 55•9 years ago
|
||
Comment on attachment 8766503 [details]
Bug 787203 - [mozdevice] Add remove_forward method to DeviceManagerADB;
https://reviewboard.mozilla.org/r/61370/#review58470
I'd prefer if this was somehow integrated into the 'forward' method.. but I can't really think of a good way of doing that and it would probably require breaking backwards compatibility. So this looks like a good enough alternative. Thanks!
Attachment #8766503 -
Flags: review?(ahalberstadt) → review+
Assignee | ||
Comment 56•9 years ago
|
||
Comment on attachment 8766498 [details]
Bug 787203 - Get Marionette working in Fennec;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61360/diff/2-3/
Attachment #8766502 -
Flags: review- → review?(ahalberstadt)
Assignee | ||
Comment 57•9 years ago
|
||
Comment on attachment 8766499 [details]
Bug 787203 - Enable Marionette on debug Fennec build (android-api-15-debug);
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61362/diff/2-3/
Assignee | ||
Comment 58•9 years ago
|
||
Comment on attachment 8766500 [details]
Bug 787203 - [mozrunner] Update `disconnected` when cleaning up after emulator shutdown;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61364/diff/2-3/
Assignee | ||
Comment 59•9 years ago
|
||
Comment on attachment 8766501 [details]
Bug 787203 - [mozrunner] Expose a start_logcat method in Device;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61366/diff/2-3/
Assignee | ||
Comment 60•9 years ago
|
||
Comment on attachment 8766502 [details]
Bug 787203 - [mozrunner] Pass application arguments to FennecRunner;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61368/diff/2-3/
Assignee | ||
Comment 61•9 years ago
|
||
Comment on attachment 8766503 [details]
Bug 787203 - [mozdevice] Add remove_forward method to DeviceManagerADB;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61370/diff/2-3/
Assignee | ||
Comment 62•9 years ago
|
||
Comment on attachment 8766504 [details]
Bug 787203 - Add emulator and Fennec support to Marionette harness and client;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61372/diff/2-3/
Assignee | ||
Comment 63•9 years ago
|
||
Comment on attachment 8766505 [details]
Bug 787203 - Allow mach marionette-test to run with android build;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61374/diff/2-3/
Assignee | ||
Comment 64•9 years ago
|
||
https://reviewboard.mozilla.org/r/61360/#review58228
> Done. I also removed a couple of other left-over device bits.
...and now I've really removed the left-over device bits. (I had previously messed something up with my histedit, but fortunately those super awesome Marionette harness tests alerted me to the problem!)
Assignee | ||
Comment 65•9 years ago
|
||
https://reviewboard.mozilla.org/r/61368/#review58468
> This could cause problems, for example if the cmdargs are ['-profile', 'abcxyz.default'], then -profile will not get added (because it already exists), but 'abcxyz.default' will.
>
> I think it would be better to simply extend cmdargs without doing any filtering and leave it up to consumers to get it right.
Good point! Thanks.
Assignee | ||
Comment 66•9 years ago
|
||
Comment on attachment 8766501 [details]
Bug 787203 - [mozrunner] Expose a start_logcat method in Device;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61366/diff/3-4/
Assignee | ||
Comment 67•9 years ago
|
||
Comment on attachment 8766502 [details]
Bug 787203 - [mozrunner] Pass application arguments to FennecRunner;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61368/diff/3-4/
Assignee | ||
Comment 68•9 years ago
|
||
Comment on attachment 8766503 [details]
Bug 787203 - [mozdevice] Add remove_forward method to DeviceManagerADB;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61370/diff/3-4/
Assignee | ||
Comment 69•9 years ago
|
||
Comment on attachment 8766504 [details]
Bug 787203 - Add emulator and Fennec support to Marionette harness and client;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61372/diff/3-4/
Assignee | ||
Comment 70•9 years ago
|
||
Comment on attachment 8766505 [details]
Bug 787203 - Allow mach marionette-test to run with android build;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61374/diff/3-4/
Assignee | ||
Comment 71•9 years ago
|
||
Comment on attachment 8766504 [details]
Bug 787203 - Add emulator and Fennec support to Marionette harness and client;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61372/diff/4-5/
Assignee | ||
Comment 72•9 years ago
|
||
Comment on attachment 8766505 [details]
Bug 787203 - Allow mach marionette-test to run with android build;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61374/diff/4-5/
Reporter | ||
Comment 73•9 years ago
|
||
Comment on attachment 8766498 [details]
Bug 787203 - Get Marionette working in Fennec;
https://reviewboard.mozilla.org/r/61360/#review58496
Attachment #8766498 -
Flags: review?(dburns) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8728855 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8730043 -
Attachment is obsolete: true
Comment 74•9 years ago
|
||
Comment on attachment 8766502 [details]
Bug 787203 - [mozrunner] Pass application arguments to FennecRunner;
https://reviewboard.mozilla.org/r/61368/#review58922
Thanks!
Attachment #8766502 -
Flags: review?(ahalberstadt) → review+
Assignee | ||
Comment 75•9 years ago
|
||
Comment on attachment 8766498 [details]
Bug 787203 - Get Marionette working in Fennec;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61360/diff/3-4/
Assignee | ||
Comment 76•9 years ago
|
||
Comment on attachment 8766499 [details]
Bug 787203 - Enable Marionette on debug Fennec build (android-api-15-debug);
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61362/diff/3-4/
Assignee | ||
Comment 77•9 years ago
|
||
Comment on attachment 8766500 [details]
Bug 787203 - [mozrunner] Update `disconnected` when cleaning up after emulator shutdown;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61364/diff/3-4/
Assignee | ||
Comment 78•9 years ago
|
||
Comment on attachment 8766501 [details]
Bug 787203 - [mozrunner] Expose a start_logcat method in Device;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61366/diff/4-5/
Assignee | ||
Comment 79•9 years ago
|
||
Comment on attachment 8766502 [details]
Bug 787203 - [mozrunner] Pass application arguments to FennecRunner;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61368/diff/4-5/
Assignee | ||
Comment 80•9 years ago
|
||
Comment on attachment 8766503 [details]
Bug 787203 - [mozdevice] Add remove_forward method to DeviceManagerADB;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61370/diff/4-5/
Assignee | ||
Comment 81•9 years ago
|
||
Comment on attachment 8766504 [details]
Bug 787203 - Add emulator and Fennec support to Marionette harness and client;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61372/diff/5-6/
Assignee | ||
Comment 82•9 years ago
|
||
Comment on attachment 8766505 [details]
Bug 787203 - Allow mach marionette-test to run with android build;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61374/diff/5-6/
Comment 83•9 years ago
|
||
Pushed by mjzffr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0c4217a73078
Get Marionette working in Fennec; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/88891cd76c27
Enable Marionette on debug Fennec build (android-api-15-debug); r=gps
https://hg.mozilla.org/integration/autoland/rev/4d13156992bc
[mozrunner] Update `disconnected` when cleaning up after emulator shutdown; r=ahal
https://hg.mozilla.org/integration/autoland/rev/44be5559589e
[mozrunner] Expose a start_logcat method in Device; r=ahal
https://hg.mozilla.org/integration/autoland/rev/d7c92e7beefa
[mozrunner] Pass application arguments to FennecRunner; r=ahal
https://hg.mozilla.org/integration/autoland/rev/24964c4700e9
[mozdevice] Add remove_forward method to DeviceManagerADB; r=ahal
https://hg.mozilla.org/integration/autoland/rev/b6c77cd3400e
Add emulator and Fennec support to Marionette harness and client; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/1c7cb653a804
Allow mach marionette-test to run with android build; r=automatedtester
Comment 84•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0c4217a73078
https://hg.mozilla.org/mozilla-central/rev/88891cd76c27
https://hg.mozilla.org/mozilla-central/rev/4d13156992bc
https://hg.mozilla.org/mozilla-central/rev/44be5559589e
https://hg.mozilla.org/mozilla-central/rev/d7c92e7beefa
https://hg.mozilla.org/mozilla-central/rev/24964c4700e9
https://hg.mozilla.org/mozilla-central/rev/b6c77cd3400e
https://hg.mozilla.org/mozilla-central/rev/1c7cb653a804
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 85•9 years ago
|
||
This caused a small installer size increase (on debug builds only): https://treeherder.mozilla.org/perf.html#/alerts?id=1817
We should be careful about enabling this on opt builds, since .apk size is a concern on Android.
See Also: → 1520226
Blocks: 1527059
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•