Split the monolithic marionette Python package

RESOLVED FIXED in mozilla31

Status

Testing
Marionette
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jgriffin, Assigned: jgriffin)

Tracking

({ateam-marionette-task})

unspecified
mozilla31
ateam-marionette-task
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [u=marionette-user c=marionette-python p=2])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
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).
(Assignee)

Comment 3

5 years ago
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. :)
(Assignee)

Updated

5 years ago
Whiteboard: [u=marionette-user c=marionette-python p=2]
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
Created attachment 816890 [details] [diff] [review]
WIP

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

Comment 9

5 years ago
(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!
(Assignee)

Comment 10

5 years ago
Created attachment 817558 [details] [diff] [review]
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.
(Assignee)

Updated

5 years ago
Attachment #816890 - Attachment is obsolete: true
(Assignee)

Comment 11

4 years ago
Created attachment 8382556 [details] [diff] [review]
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.
Attachment #8382556 - Flags: review?(ato)
(Assignee)

Updated

4 years ago
Attachment #817558 - Attachment is obsolete: true
(Assignee)

Comment 12

4 years ago
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+
(Assignee)

Comment 15

4 years ago
(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.
(Assignee)

Comment 18

4 years ago
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.
(Assignee)

Comment 19

4 years ago
Created attachment 8382658 [details] [diff] [review]
Split the transport into a separate package,

Fixed the packaging problem.
Attachment #8382658 - Flags: review?(ato)
(Assignee)

Updated

4 years ago
Attachment #8382556 - Attachment is obsolete: true
Attachment #8382556 - Flags: review?(ato)
(Assignee)

Comment 21

4 years ago
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)
(Assignee)

Comment 23

4 years ago
Created attachment 8397457 [details] [diff] [review]
Split the transport into a separate package,

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

Updated

4 years ago
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)
(Assignee)

Comment 25

4 years ago
Desktop try is green, here's one for B2G: https://tbpl.mozilla.org/?tree=Try&rev=05f5af1b4276
Duplicate of this bug: 742811
Duplicate of this bug: 825284
(Assignee)

Comment 28

4 years ago
(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
(Assignee)

Comment 30

4 years ago
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?
(Assignee)

Comment 33

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 35

4 years ago
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?
You need to log in before you can comment on or make changes to this bug.