1274408 - Remove emulator support from Marionette

Status: RESOLVED FIXED

(Testing :: Marionette Client and Harness, defect)

Description

[David Burns :automatedtester]

All B2G jobs have been disabled in buildbot and with that we should remove all emulator code.

Comment 1

[David Burns :automatedtester]

Attachment: 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/

Comment 2

[David Burns :automatedtester]

Attachment: 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/

Comment 3

[David Burns :automatedtester]

Attachment: 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/

Comment 4

[David Burns :automatedtester]

Attachment: 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/

Comment 5

[David Burns :automatedtester]

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

that's my cause to celebrate!

Comment 6

[David Burns :automatedtester]

This is working locally but try is closed due to backlog, will do a try push tomorrow

Comment 7

[David Burns :automatedtester]

(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

Comment 8

[Andreas Tolfsen ❲:ato❳]

aus, is Connected Devices still using/committed to B2G emulator support?

Comment 9

[Aus Lacroix [:aus]]

afaik the TV team is still using the emulator for tests. Mike, is this the case?

Comment 10

[Mike Lien[:mlien]]

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.

Comment 11

[David Burns :automatedtester]

Try pushes failed because --type has been removed and mozharness still uses that. This has been fixed in bug 1272414

Comment 12

[Jonathan Griffin (:jgriffin)]

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

https://reviewboard.mozilla.org/r/54038/#review50956

Comment 13

[Andreas Tolfsen ❲:ato❳]

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.

Comment 14

[Andreas Tolfsen ❲:ato❳]

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 15

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

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.

Comment 16

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

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.

Comment 17

[Andreas Tolfsen ❲:ato❳]

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.

Comment 18

[David Burns :automatedtester]

(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?

Comment 19

[David Burns :automatedtester]

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

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

This was fixed in a different bug

Comment 20

[David Burns :automatedtester]

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/

Comment 21

[David Burns :automatedtester]

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/

Comment 22

[David Burns :automatedtester]

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/

Comment 23

[David Burns :automatedtester]

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/

Comment 24

[David Burns :automatedtester]

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 25

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

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

Comment 26

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

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.)

Comment 27

[David Burns :automatedtester]

Attachment: 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/

Comment 28

[David Burns :automatedtester]

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/

Comment 29

[David Burns :automatedtester]

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/

Comment 30

[David Burns :automatedtester]

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/

Comment 31

[David Burns :automatedtester]

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 32

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

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

Comment 33

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

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

Comment 34

[Pulsebot]

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

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

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

Comment 36

[Andreas Tolfsen ❲:ato❳]

(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.

Comment 37

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

Moving bugs for Marionette client due to component changes.