Closed Bug 902711 Opened 11 years ago Closed 10 years ago

Split the monolithic marionette Python package

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla31

People

(Reporter: jgriffin, Assigned: jgriffin)

References

Details

(Keywords: pi-marionette-task, Whiteboard: [u=marionette-user c=marionette-python p=2])

Attachments

(1 file, 4 obsolete files)

Split the transport into a separate package,
15.69 KB, patch
ato
: review+
Details | Diff | Splinter Review 
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.
This is awesome! (and very similar to what we do in the node js client)
(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).
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.
(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. :)
Whiteboard: [u=marionette-user c=marionette-python p=2]
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.
Attached patch WIP (obsolete) — — Splinter Review 
WIP - separates out the transport layer into a separate class.  Will separate out the api next.
Comment on attachment 816890 [details] [diff] [review]
WIP

quick scan this looks good!
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?
(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!
Attached patch WIP (obsolete) — — Splinter Review 
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.
Attachment #816890 - Attachment is obsolete: true
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.
Attachment #8382556 - Flags: review?(ato)
Attachment #817558 - Attachment is obsolete: true
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.
Attachment #8382556 - Flags: feedback?(mdas)
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 ../?
Attachment #8382556 - Flags: feedback?(mdas) → feedback+
(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.
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.
Fixed the packaging problem.
Attachment #8382658 - Flags: review?(ato)
Attachment #8382556 - Attachment is obsolete: true
Attachment #8382556 - Flags: review?(ato)
desktop try green, here's a B2G try run: https://tbpl.mozilla.org/?tree=Try&rev=11ffc70470d5
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)
Flags: needinfo?(jgriffin)
Attachment #8382658 - Attachment is obsolete: true
Attachment #8382658 - Flags: review?(ato)
Comment on attachment 8397457 [details] [diff] [review]
Split the transport into a separate package,

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

lgtm
Attachment #8397457 - Flags: review+
Flags: needinfo?(jgriffin)
Desktop try is green, here's one for B2G: https://tbpl.mozilla.org/?tree=Try&rev=05f5af1b4276
(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
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbe169081f1c
Status: NEW → ASSIGNED
Keywords: ateam-marionette-task
Target Milestone: --- → mozilla31
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?
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.
https://hg.mozilla.org/mozilla-central/rev/cbe169081f1c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
FYI, we can tackle splitting out other pieces of the marionette client in separate bugs.
(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.
Just run marionette-webapi tests and we got Bug 996477 now.
Blocks: 996477
No longer depends on: 996477
(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?
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: