If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Remove emulator support from Marionette

RESOLVED FIXED in Firefox 49

Status

Testing
Marionette
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: automatedtester, Assigned: automatedtester)

Tracking

({ateam-marionette-client, ateam-marionette-server})

unspecified
mozilla49
ateam-marionette-client, ateam-marionette-server
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

MozReview Requests

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

Attachments

(5 attachments)

(Assignee)

Description

a year ago
All B2G jobs have been disabled in buildbot and with that we should remove all emulator code.
(Assignee)

Comment 1

a year ago
Created attachment 8754551 [details]
MozReview Request: Bug 1274408: Remove emulator code from Marionette python client r?maja_zf

The emulator code has created a lot of hacky code in the Marionette and
by removing it, we are removing a lot of really bad technical debt.

Review commit: https://reviewboard.mozilla.org/r/54034/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54034/
Attachment #8754551 - Flags: review?(mjzffr)
Attachment #8754552 - Flags: review?(mjzffr)
Attachment #8754553 - Flags: review?(jgriffin)
Attachment #8754554 - Flags: review?(ato)
(Assignee)

Comment 2

a year ago
Created attachment 8754552 [details]
MozReview Request: Bug 1274408: Remove emulator support from Marionette Harness code r?maja_zf

The emulator code in Marionette harness makes it very hard to be able to
support other platforms. Since we no longer support B2G we should remove it.

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

Comment 3

a year ago
Created attachment 8754553 [details]
MozReview Request: Bug 1274408: Remove emulator mach support for Marionette r?jgriffin

Since we are not supporting the B2G Emulator, we should remove this dead code.

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

Comment 4

a year ago
Created attachment 8754554 [details]
MozReview Request: Bug 1274408: Remove emulator support from Marionette in Gecko r?ato

The emulator code was originally hacks to allow us to instrument the emulator
from JavaScript in the B2G world. Since we no longer support this it is being
removed.

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

Comment 5

a year ago
18 files changed, 23 insertions(+), 1211 deletions(-)

that's my cause to celebrate!
(Assignee)

Comment 6

a year ago
This is working locally but try is closed due to backlog, will do a try push tomorrow
(Assignee)

Comment 7

a year ago
(In reply to David Burns :automatedtester from comment #6)
> This is working locally but try is closed due to backlog, will do a try push
> tomorrow

I asked autoland to do that for me... will check results in the morning
aus, is Connected Devices still using/committed to B2G emulator support?
Status: NEW → ASSIGNED
Flags: needinfo?(aus)
Keywords: ateam-marionette-client, ateam-marionette-server

Comment 9

a year ago
afaik the TV team is still using the emulator for tests. Mike, is this the case?
Flags: needinfo?(aus) → needinfo?(mlien)

Comment 10

a year ago
Hi Aus, thank you for informing me. TV 2.6 already branched out and TV team use gaia branch v2.6 and gecko branch b2g48 to work thus I think this change won't affect TV team if it only land on m-c.
Flags: needinfo?(mlien)
(Assignee)

Updated

a year ago
Depends on: 1272414
(Assignee)

Comment 11

a year ago
Try pushes failed because --type has been removed and mozharness still uses that. This has been fixed in bug 1272414
Attachment #8754553 - Flags: review?(jgriffin) → review+
Comment on attachment 8754553 [details]
MozReview Request: Bug 1274408: Remove emulator mach support for Marionette r?jgriffin

https://reviewboard.mozilla.org/r/54038/#review50956
Attachment #8754554 - Flags: review?(ato) → review+
Comment on attachment 8754554 [details]
MozReview Request: Bug 1274408: Remove emulator support from Marionette in Gecko r?ato

https://reviewboard.mozilla.org/r/54040/#review50996

I have mixed feelings about removing this, mostly I think because I have spent an enormous amount of time and effort getting it right.  But it is not a sign of a good program to keep code paths that are not in use.

I’d quite like us to keep the asynchronous ideas in the protocol by retaining the `message.Command` and `message.Response` ideas.  I see your patch has not removed them, so I’m assuming we’re in agreement here.

::: testing/marionette/dispatcher.js:161
(Diff revision 1)
> - * Delegates message to client or emulator based on the provided
> + * Delegates message to client based on the provided
>   * {@code cmdId}.  The message is sent over the debugger transport socket.

Please fmt these two lines.

::: testing/marionette/dispatcher.js:177
(Diff revision 1)
>   */
>  Dispatcher.prototype.send = function(msg) {
>    msg.origin = MessageOrigin.Server;
>    if (msg instanceof Command) {
>      this.commands_.set(msg.id, msg);
>      this.sendToEmulator(msg);

I would add a warning or throw an error here, but I think it’s useful to keep the asynchronous principles here in Marionette.  It took a long time to get right.

::: testing/marionette/server.js
(Diff revision 1)
> -  if (bypassOffline) {
> -    logger.debug("Bypassing offline status");
> -    Preferences.set(MANAGE_OFFLINE_STATUS_PREF, false);
> -    Services.io.manageOfflineStatus = false;
> -    Services.io.offline = false;
> -  }

This should probably not be removed.
https://reviewboard.mozilla.org/r/54036/#review50998

::: testing/marionette/harness/marionette/runner/base.py:286
(Diff revision 1)
>          self.add_argument('--logcat-dir',
>                          dest='logdir',
>                          help='directory to store logcat dump files',
>                          type=dir_path)
>          self.add_argument('--logcat-stdout',
>                          action='store_true',
>                          default=False,
>                          help='dump adb logcat to stdout')

Is this needed for Android?
Comment on attachment 8754551 [details]
MozReview Request: Bug 1274408: Remove emulator code from Marionette python client r?maja_zf

https://reviewboard.mozilla.org/r/54034/#review51002

Overall, I think I found more things to remove! :)

::: testing/marionette/client/marionette_driver/marionette.py:542
(Diff revision 1)
> -                 profile=None, addons=None, emulator=None, sdcard=None, emulator_img=None,
> -                 emulator_binary=None, emulator_res=None, connect_to_running_emulator=False,
> -                 gecko_log=None, homedir=None, baseurl=None, no_window=False, logdir=None,
> +                 profile=None, addons=None,
> +                 gecko_log=None, baseurl=None,
> +                 symbols_path=None, timeout=None,
> -                 busybox=None, symbols_path=None, timeout=None,
>                   socket_timeout=DEFAULT_SOCKET_TIMEOUT, device_serial=None, adb_path=None,
>                   process_args=None, adb_host=None, adb_port=None, prefs=None,

You could remove the adb_port, ..., process_args, etc.

::: testing/marionette/client/marionette_driver/marionette.py:554
(Diff revision 1)
>          self.instance = None
>          self.session = None
>          self.session_id = None
>          self.window = None
>          self.chrome_window = None
>          self.runner = None

`self.runner` is only used for emulator, so let's remove.

::: testing/marionette/client/marionette_driver/marionette.py
(Diff revision 1)
>          if bin:
>              port = int(self.port)
>              if not Marionette.is_port_available(port, host=self.host):
>                  ex_msg = "%s:%d is unavailable." % (self.host, port)
>                  raise errors.MarionetteException(message=ex_msg)
> -            if app:

Currently `app` is also used to specify 'fxdesktop' (see geckoinstance.py) for Firefox UI Tests and external-media-tests to further customize browser prefs. There's also 'b2gdesktop' to consider.

::: testing/marionette/client/marionette_driver/marionette.py
(Diff revision 1)
>                          ConfigParser.NoSectionError,
>                          KeyError):
>                      instance_class = geckoinstance.GeckoInstance
>              self.instance = instance_class(host=self.host, port=self.port,
>                                             bin=self.bin, profile=self.profile,
> -                                           app_args=app_args,

`app_args` is used for jsdebugger and possibly more in GeckoInstance.

::: testing/marionette/client/marionette_driver/marionette.py:1073
(Diff revision 1)
>          body = {"capabilities": desired_capabilities, "sessionId": session_id}
>          resp = self._send_message("newSession", body)
>  
>          self.session_id = resp["sessionId"]
>          self.session = resp["value"] if self.protocol == 1 else resp["capabilities"]
>          self.b2g = "b2g" in self.session

Is this used anywhere if we don't have emulator support? I can't find any examples.
Attachment #8754551 - Flags: review?(mjzffr)
Attachment #8754552 - Flags: review?(mjzffr)
Comment on attachment 8754552 [details]
MozReview Request: Bug 1274408: Remove emulator support from Marionette Harness code r?maja_zf

https://reviewboard.mozilla.org/r/54036/#review51028

::: testing/marionette/harness/marionette/runner/base.py:236
(Diff revision 1)
>              def b2g_pre_run():
>                  dm_type = os.environ.get('DM_TRANS', 'adb')
>                  if dm_type == 'adb':
>                      self.b2g_pid = get_b2g_pid(get_dm(self.marionette))

Can this go away?

::: testing/marionette/harness/marionette/runner/base.py
(Diff revision 1)
>                          help='serial ID of a device to use for adb / fastboot')
>          self.add_argument('--adb-host',
>                          help='host to use for adb connection')
>          self.add_argument('--adb-port',
>                          help='port to use for adb connection')
> -        self.add_argument('--type',

There are lots of `self.type` references left in BaseMarionetteTestRunner.

::: testing/marionette/harness/marionette/runner/base.py
(Diff revision 1)
>          }
>          if self.bin:
>              kwargs.update({
>                  'host': 'localhost',
>                  'port': 2828,
> -                'app': self.app,

As in the previous patch in the series: we need to keep `app` to specify an instance class like 'fxdesktop' instead of the default GeckoInstance. Keep `app_args`, too.
https://reviewboard.mozilla.org/r/54034/#review51002

> Is this used anywhere if we don't have emulator support? I can't find any examples.

Mulet is still using Marionette AFAIK.
(Assignee)

Comment 18

a year ago
(In reply to Andreas Tolfsen ‹:ato› from comment #13)
> Comment on attachment 8754554 [details]
> MozReview Request: Bug 1274408: Remove emulator support from Marionette in
> Gecko r?ato
> 
> https://reviewboard.mozilla.org/r/54040/#review50996
> 
> I have mixed feelings about removing this, mostly I think because I have
> spent an enormous amount of time and effort getting it right.  But it is not
> a sign of a good program to keep code paths that are not in use.

This is not a negative against you, not in the slightest, this is just making sure we don't have to support code we don't need.

> 
> I’d quite like us to keep the asynchronous ideas in the protocol by
> retaining the `message.Command` and `message.Response` ideas.  I see your
> patch has not removed them, so I’m assuming we’re in agreement here.

This is something that I missed. While I can leave it there for now, if it is not being used then we really should see about removing it. 

> ::: testing/marionette/server.js
> (Diff revision 1)
> > -  if (bypassOffline) {
> > -    logger.debug("Bypassing offline status");
> > -    Preferences.set(MANAGE_OFFLINE_STATUS_PREF, false);
> > -    Services.io.manageOfflineStatus = false;
> > -    Services.io.offline = false;
> > -  }
> 
> This should probably not be removed.

Could you elaborate why just so we can leave a note for the future?
(Assignee)

Comment 19

a year ago
https://reviewboard.mozilla.org/r/54036/#review51028

> There are lots of `self.type` references left in BaseMarionetteTestRunner.

This was fixed in a different bug
(Assignee)

Comment 20

a year ago
Comment on attachment 8754551 [details]
MozReview Request: Bug 1274408: Remove emulator code from Marionette python client r?maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54034/diff/1-2/
Attachment #8754551 - Flags: review?(mjzffr)
Attachment #8754552 - Flags: review?(mjzffr)
(Assignee)

Comment 21

a year ago
Comment on attachment 8754552 [details]
MozReview Request: Bug 1274408: Remove emulator support from Marionette Harness code r?maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54036/diff/1-2/
(Assignee)

Comment 22

a year ago
Comment on attachment 8754553 [details]
MozReview Request: Bug 1274408: Remove emulator mach support for Marionette r?jgriffin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54038/diff/1-2/
(Assignee)

Comment 23

a year ago
Comment on attachment 8754554 [details]
MozReview Request: Bug 1274408: Remove emulator support from Marionette in Gecko r?ato

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54040/diff/1-2/
(Assignee)

Comment 24

a year ago
Comment on attachment 8754554 [details]
MozReview Request: Bug 1274408: Remove emulator support from Marionette in Gecko r?ato

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54040/diff/2-3/
Comment on attachment 8754551 [details]
MozReview Request: Bug 1274408: Remove emulator code from Marionette python client r?maja_zf

https://reviewboard.mozilla.org/r/54034/#review51482
Attachment #8754551 - Flags: review?(mjzffr) → review+
Attachment #8754552 - Flags: review?(mjzffr)
Comment on attachment 8754552 [details]
MozReview Request: Bug 1274408: Remove emulator support from Marionette Harness code r?maja_zf

https://reviewboard.mozilla.org/r/54036/#review51484

Looks like a few of the many, many emulator-related kwargs are still there. :)

::: testing/marionette/harness/marionette/runner/base.py:498
(Diff revisions 1 - 2)
> +        self.emulator = emulator
> +        self.emulator_binary = emulator_binary
> +        self.emulator_img = emulator_img
> +        self.emulator_res = emulator_res

There are emulator-related attributes in BaseMarionetteTestRunner than aren't in BaseMarionetteArguments. (See also logcat, etc.)

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:70
(Diff revision 2)
>           'adb_host': None,
>           'adb_port': None,
>           'addon': None,
>           'address': None,

More args can be removed here. (I originally just copied the kwargs dict I saw when stepping through ./mach marionette-test with pdb.)
(Assignee)

Comment 27

a year ago
Created attachment 8756076 [details]
MozReview Request: Bug 1274408: Remove B2G runners from Marionette Python Harness r?maja_zf

All tests were migrated over to MarionetteJS support before we stopped work on
B2G. This is now not used and am removing.

Review commit: https://reviewboard.mozilla.org/r/54940/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54940/
Attachment #8756076 - Flags: review?(mjzffr)
Attachment #8754552 - Flags: review?(mjzffr)
(Assignee)

Comment 28

a year ago
Comment on attachment 8754551 [details]
MozReview Request: Bug 1274408: Remove emulator code from Marionette python client r?maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54034/diff/2-3/
(Assignee)

Comment 29

a year ago
Comment on attachment 8754552 [details]
MozReview Request: Bug 1274408: Remove emulator support from Marionette Harness code r?maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54036/diff/2-3/
(Assignee)

Comment 30

a year ago
Comment on attachment 8754553 [details]
MozReview Request: Bug 1274408: Remove emulator mach support for Marionette r?jgriffin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54038/diff/2-3/
(Assignee)

Comment 31

a year ago
Comment on attachment 8754554 [details]
MozReview Request: Bug 1274408: Remove emulator support from Marionette in Gecko r?ato

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54040/diff/3-4/
Comment on attachment 8754552 [details]
MozReview Request: Bug 1274408: Remove emulator support from Marionette Harness code r?maja_zf

https://reviewboard.mozilla.org/r/54036/#review51626
Attachment #8754552 - Flags: review?(mjzffr) → review+
Comment on attachment 8756076 [details]
MozReview Request: Bug 1274408: Remove B2G runners from Marionette Python Harness r?maja_zf

https://reviewboard.mozilla.org/r/54940/#review51630
Attachment #8756076 - Flags: review?(mjzffr) → review+

Comment 34

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9935b7b2efb2
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b70e44e565f
https://hg.mozilla.org/integration/mozilla-inbound/rev/78bf598eed77
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ca92d57dac6
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a1948d999aa

Comment 35

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9935b7b2efb2
https://hg.mozilla.org/mozilla-central/rev/5b70e44e565f
https://hg.mozilla.org/mozilla-central/rev/78bf598eed77
https://hg.mozilla.org/mozilla-central/rev/5ca92d57dac6
https://hg.mozilla.org/mozilla-central/rev/8a1948d999aa
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(In reply to David Burns :automatedtester from comment #18)
> (In reply to Andreas Tolfsen ‹:ato› from comment #13)
> > I’d quite like us to keep the asynchronous ideas in the protocol by
> > retaining the `message.Command` and `message.Response` ideas.  I see your
> > patch has not removed them, so I’m assuming we’re in agreement here.
> 
> This is something that I missed. While I can leave it there for now, if it
> is not being used then we really should see about removing it. 

They are integral to how the Marionette protocol is designed.  The protocol is designed to allow for asynchronous messages and pipelining to limit chances of payload race conditions.

You can see the full explanation in https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/Protocol.

> > ::: testing/marionette/server.js
> > (Diff revision 1)
> > > -  if (bypassOffline) {
> > > -    logger.debug("Bypassing offline status");
> > > -    Preferences.set(MANAGE_OFFLINE_STATUS_PREF, false);
> > > -    Services.io.manageOfflineStatus = false;
> > > -    Services.io.offline = false;
> > > -  }
> > 
> > This should probably not be removed.
> 
> Could you elaborate why just so we can leave a note for the future?

As far as I understand this controls the “Work offline”/“Work online” setting in Firefox.  If the profile we start with has “Work offline” set, we need to override this so Marionette can function.
Depends on: 1281213
Depends on: 1276011
You need to log in before you can comment on or make changes to this bug.