902711 - Split the monolithic marionette Python package

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Jonathan Griffin (:jgriffin)]

We want to split the monolithic Marionette Python package into some components:

marionette_transport:  what's currently contained in client.py, mostly
marionette_api or marionette_protocol:  the pure API parts of marionette.py, errors.py, and related files
marionette_runner: the runner, plus the parts of the current Marionette class which have nothing to do with the API

I'm not the best at naming things, so if you have suggestions for names, let me know.

Comment 1

[James Lal [:lightsofapollo]]

This is awesome! (and very similar to what we do in the node js client)

Comment 2

[William Lachance (:wlach)]

(In reply to Jonathan Griffin (:jgriffin) from comment #0)

> marionette_api or marionette_protocol:  the pure API parts of marionette.py,
> errors.py, and related files

I think we should continue to call this marionette_client, both for backwards compatibility purposes as well as the fact that I think this name more intuitively describes what this package will provide (a client you can use to interact with something providing a marionette server).

Comment 3

[Jonathan Griffin (:jgriffin)]

For backwards compatibility, we'd actually want to call marionette_runner marionette_client, as that is what existing consumers expect when they import marionette_client.

Comment 4

[William Lachance (:wlach)]

(In reply to Jonathan Griffin (:jgriffin) from comment #3)
> For backwards compatibility, we'd actually want to call marionette_runner
> marionette_client, as that is what existing consumers expect when they
> import marionette_client.

Hmm, I guess it would depend on the consumer now that I think about it. Ok, so I guess backwards compatibility is not an argument though my comment about intuitive naming schemes still stands. :)

Comment 5

[Jonathan Griffin (:jgriffin)]

We're probably going to need a mozbase-style setup_development.py, in order to allow local installations to use the latest versions of all the marionette modules, rather than pulling potentially stale packages from pypi.

Comment 6

[Jonathan Griffin (:jgriffin)]

Attachment: WIP

WIP - separates out the transport layer into a separate class.  Will separate out the api next.

Comment 7

[David Burns :automatedtester]

Comment on attachment 816890 [details] [diff] [review]
WIP

quick scan this looks good!

Comment 8

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 816890 [details] [diff] [review]
WIP

Review of attachment 816890 [details] [diff] [review]:
-----------------------------------------------------------------

happy to see a WIP for this

::: testing/marionette/marionette_transport/marionette_transport/transport.py
@@ +5,5 @@
> +import json
> +import socket
> +
> +
> +class MarionetteClient(object):

Can this be renamed to MarionetteTransport?

Comment 9

[Jonathan Griffin (:jgriffin)]

(In reply to Malini Das [:mdas] from comment #8)
> Comment on attachment 816890 [details] [diff] [review]
> WIP
> 
> Review of attachment 816890 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> happy to see a WIP for this
> 
> ::: testing/marionette/marionette_transport/marionette_transport/transport.py
> @@ +5,5 @@
> > +import json
> > +import socket
> > +
> > +
> > +class MarionetteClient(object):
> 
> Can this be renamed to MarionetteTransport?

Done, for the next iteration!

Comment 10

[Jonathan Griffin (:jgriffin)]

Attachment: WIP

I've started moving the marionette api into it's own package; this patch is an incomplete but partially functional WIP.  We can sort out the names of the packages once we have a final patch, IMO.

Comment 11

[Jonathan Griffin (:jgriffin)]

Attachment: Split the transport into a separate package,

This does some moderately crazy stuff with setup.py in an attempt to provide a good developer experience and maintain compatbility with CI.  I'll run this all through try.

Comment 12

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8382556 [details] [diff] [review]
Split the transport into a separate package,

This changes our error handling a bit in the transport to be more consistent; the transport now throws IOError on failure to send/or receive, whereas before we threw InvalidResponseException when receiving a null response, vs an IOError when failing to send.  I'll have to check gaiatest to make sure this doesn't break anything downstream.

Comment 13

[Jonathan Griffin (:jgriffin)]

try: https://tbpl.mozilla.org/?tree=Try&rev=b9935ac2f1a8, https://tbpl.mozilla.org/?tree=Try&rev=ca10dd05bccc

Comment 14

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8382556 [details] [diff] [review]
Split the transport into a separate package,

Review of attachment 8382556 [details] [diff] [review]:
-----------------------------------------------------------------

crazy code makes sense for our crazy case. f+!

::: testing/marionette/client/requirements.txt
@@ +1,1 @@
> +../marionette_transport

why the ../?

Comment 15

[Jonathan Griffin (:jgriffin)]

(In reply to Malini Das [:mdas] from comment #14)
> 
> ::: testing/marionette/client/requirements.txt
> @@ +1,1 @@
> > +../marionette_transport
> 
> why the ../?

It's a relative path; it's in testing/marionette/marionette_transport.  Since it's a self-contained package, I don't think it makes sense to have it in a descendant directory.

Comment 16

[Malini Das [:mdas] - Away, not checking bugmail]

ah, right

Comment 17

[Jonathan Griffin (:jgriffin)]

The only change needed in gaiatest, AFAIK, is https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L858

Comment 18

[Jonathan Griffin (:jgriffin)]

15:29:26     INFO -    Could not find any downloads that satisfy the requirement marionette-transport==0.1 (from marionette-client->-r /builds/slave/test/build/tests/config/marionette_requirements.txt (line 2))
15:29:26     INFO -  No distributions at all found for marionette-transport==0.1 (from marionette-client->-r /builds/slave/test/build/tests/config/marionette_requirements.txt (line 2))

I need to make sure the new package ends up in tests.zip.

Comment 19

[Jonathan Griffin (:jgriffin)]

Attachment: Split the transport into a separate package,

Fixed the packaging problem.

Comment 20

[Jonathan Griffin (:jgriffin)]

New desktop try run: https://tbpl.mozilla.org/?tree=Try&rev=d491a0ef95a4

Comment 21

[Jonathan Griffin (:jgriffin)]

desktop try green, here's a B2G try run: https://tbpl.mozilla.org/?tree=Try&rev=11ffc70470d5

Comment 22

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8382658 [details] [diff] [review]
Split the transport into a separate package,

Review of attachment 8382658 [details] [diff] [review]:
-----------------------------------------------------------------

Overall I think this patch looks fine, but I have two concerns
which might might be nitpicks:

  * PyPI prefers using dashes to separate words in package names.
    So whilst "marionette_transport" follows "marionette_client",
    do we want to prefer "marionette-transport"?

  * The directory structure for the new marionette_transport
    module will be testing/marionette/marionette_transport/marionette_transport
    which seems a little excessive.  Could we potentially shorten
    it to testing/marionette/transport/marionette_transport?  Do
    you have a strong opinion on this?

::: testing/marionette/client/marionette/b2g_update_test.py
@@ +24,5 @@
>      SEND_TIMEOUT    = 60 * 5
>      MAX_RETRIES     = 24
>  
>      def __init__(self, addr, port, runner):
> +        MarionetteTransport.__init__(self, addr, port)

Since MarionetteTransport is a new style class, we should probably prefer to initialize it using the new style super class built-in:

    super(B2GUpdateMarionetteClient, self).__init__(self, addr, port)

Comment 23

[Jonathan Griffin (:jgriffin)]

Attachment: Split the transport into a separate package,

Addressed review comments.  Desktop try run:  https://tbpl.mozilla.org/?tree=Try&rev=7c27bb892a80

Comment 24

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8397457 [details] [diff] [review]
Split the transport into a separate package,

Review of attachment 8397457 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

Comment 25

[Jonathan Griffin (:jgriffin)]

Desktop try is green, here's one for B2G: https://tbpl.mozilla.org/?tree=Try&rev=05f5af1b4276

Comment 28

[Jonathan Griffin (:jgriffin)]

(In reply to Jonathan Griffin (:jgriffin) from comment #25)
> Desktop try is green, here's one for B2G:
> https://tbpl.mozilla.org/?tree=Try&rev=05f5af1b4276

sigh, packaging problems, let's try again:  https://tbpl.mozilla.org/?tree=Try&rev=faca98d53467

Comment 29

[Jonathan Griffin (:jgriffin)]

Once more, with feeling:  https://tbpl.mozilla.org/?tree=Try&rev=4f7ab401db41

Comment 30

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbe169081f1c

Comment 31

[Jonathan Griffin (:jgriffin)]

also: https://pypi.python.org/pypi/marionette-transport/0.1

Comment 32

[Dan Mosedale (:dmosedale, :dmose)]

Would it make sense to have to PyPi docs explicitly mention that the raw handles should be sharable with WebDriver so that one can do automated testing of content using a familiar API but still have the option of reaching up into Firefox chrome if necessary?

Comment 33

[Jonathan Griffin (:jgriffin)]

I believe the plan is to eventually add a plugin of some sort to the WebDriver Python bindings that would allow it to use the Marionette transport, but that work hasn't been done yet.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cbe169081f1c

Comment 35

[Jonathan Griffin (:jgriffin)]

FYI, we can tackle splitting out other pieces of the marionette client in separate bugs.

Comment 36

[Andreas Tolfsen ❲:ato❳]

(In reply to Dan Mosedale (:dmose) from comment #32)
> Would it make sense to have to PyPi docs explicitly mention that the raw
> handles should be sharable with WebDriver so that one can do automated
> testing of content using a familiar API but still have the option of
> reaching up into Firefox chrome if necessary?

So the WebDriver protocol explicitly caters for this sort of eventuality
by allowing driver implementations to provide vendor prefixed command
end-points specific to a certain browser.

How we expose that in the client API bindings is a good question, but as
jgriffin points out, an extension/mixin of sorts would make a lot of sense.

Comment 37

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Just run marionette-webapi tests and we got Bug 996477 now.

Comment 38

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Andreas Tolfsen (:ato) from comment #36)
> (In reply to Dan Mosedale (:dmose) from comment #32)
> > Would it make sense to have to PyPi docs explicitly mention that the raw
> > handles should be sharable with WebDriver so that one can do automated
> > testing of content using a familiar API but still have the option of
> > reaching up into Firefox chrome if necessary?
> 
> So the WebDriver protocol explicitly caters for this sort of eventuality
> by allowing driver implementations to provide vendor prefixed command
> end-points specific to a certain browser.
> 
> How we expose that in the client API bindings is a good question, but as
> jgriffin points out, an extension/mixin of sorts would make a lot of sense.

My expectation is that the Loop project is likely to want this before long.  Is there already a bug on file?