Get marionette working on Fennec

RESOLVED FIXED in Firefox 50

Status

Testing
Marionette
RESOLVED FIXED
5 years ago
9 months ago

People

(Reporter: automatedtester, Assigned: maja_zf)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla50
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [marionette=1.0])

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(9 attachments, 2 obsolete attachments)

190.09 KB, text/x-log
Details
58 bytes, text/x-review-board-request
automatedtester
: review+
Details | Review
58 bytes, text/x-review-board-request
gps
: review+
Details | Review
58 bytes, text/x-review-board-request
ahal
: review+
Details | Review
58 bytes, text/x-review-board-request
ahal
: review+
Details | Review
58 bytes, text/x-review-board-request
ahal
: review+
Details | Review
58 bytes, text/x-review-board-request
ahal
: review+
Details | Review
58 bytes, text/x-review-board-request
automatedtester
: review+
Details | Review
58 bytes, text/x-review-board-request
automatedtester
: review+
Details | Review
(Reporter)

Description

5 years ago
WebApps need to have marionette working in Fennec so this is a tracking bug related to that
(Reporter)

Updated

5 years ago
Depends on: 787280
(Reporter)

Updated

5 years ago
Depends on: 787545
(Reporter)

Updated

5 years ago
Depends on: 791793
(Reporter)

Updated

5 years ago
Blocks: 793661
(Reporter)

Updated

2 years ago
Whiteboard: [marionette=1.0]
(Reporter)

Comment 1

2 years ago
Moving this to a Q3 goal. 

* Finish off Marionette support on Fennec
* Stand up tests on buildbot/TC (whatever we decide)
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1180895

Updated

2 years ago
Blocks: 1194860
Duplicate of this bug: 642520
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

a year ago
Created attachment 8728855 [details]
MozReview Request: Bug 787203: Get Marionette working in Fennec

Review commit: https://reviewboard.mozilla.org/r/39167/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39167/
(Reporter)

Comment 6

a year 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)
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/
(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.)
Created attachment 8730044 [details]
marionette.log
(Reporter)

Comment 10

a year 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

a year 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

a year 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)
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)
I'm going to dig into this. Expect questions from me in the near future!
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)
Is there a chance this is related to bug 1267925?
(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

a year ago
Maja has this running, though not always reliably.
...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.
(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)
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)
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)
(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)
Depends on: 1278590
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dea125d659b6
Created attachment 8766498 [details]
Bug 787203 - Get Marionette working in Fennec;

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)
Created attachment 8766499 [details]
Bug 787203 - Enable Marionette on debug Fennec build (android-api-15-debug);

Review commit: https://reviewboard.mozilla.org/r/61362/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61362/
Created attachment 8766500 [details]
Bug 787203 - [mozrunner] Update `disconnected` when cleaning up after emulator shutdown;

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/
Created attachment 8766501 [details]
Bug 787203 - [mozrunner] Expose a start_logcat method in Device;

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/
Created attachment 8766502 [details]
Bug 787203 - [mozrunner] Pass application arguments to FennecRunner;

Review commit: https://reviewboard.mozilla.org/r/61368/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61368/
Created attachment 8766503 [details]
Bug 787203 - [mozdevice] Add remove_forward method to DeviceManagerADB;

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/
Created attachment 8766504 [details]
Bug 787203 - Add emulator and Fennec support to Marionette harness and client;

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/
Created attachment 8766505 [details]
Bug 787203 - Allow mach marionette-test to run with android build;

Review commit: https://reviewboard.mozilla.org/r/61374/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61374/
Next: schedule Marionette tests for Android in TaskCluster.
(Reporter)

Comment 38

11 months 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

11 months 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

11 months 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

11 months 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+
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)
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/
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/
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/
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/
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/
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/
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/
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.
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 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 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+
Attachment #8766502 - Flags: review?(ahalberstadt) → review-
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 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+
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)
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/
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/
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/
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/
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/
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/
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/
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!)
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.
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/
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/
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/
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/
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/
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/
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

11 months 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+
Attachment #8728855 - Attachment is obsolete: true
Attachment #8730043 - Attachment is obsolete: true
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+
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/
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/
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/
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/
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/
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/
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/
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

11 months 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

11 months 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
Last Resolved: 11 months ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Blocks: 1284874
Blocks: 1284960
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.

Updated

10 months ago
Blocks: 1294427

Updated

10 months ago
No longer blocks: 1294427
You need to log in before you can comment on or make changes to this bug.