1211503 - Support for Marionette protocol level 3 in Marionette Python client

Status: RESOLVED FIXED

(Testing :: Marionette Client and Harness, defect)

Description

[Andreas Tolfsen ❲:ato❳]

To future-proof the Marionette Python client it should have support for level 3 of the Marionette protocol outlined in bug 1211489.

The changes required on the client side should this time be minimal as the command result data structures are not changing.  However, the client should expect to receive "Response" messages and send "Command" messages as outlined in the message format description in bug 1211489.

Comment 1

[Andreas Tolfsen ❲:ato❳]

try against level 2 server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=07608ba1a982

Comment 2

[Andreas Tolfsen ❲:ato❳]

try with fixes against level 2 server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=636f6b920561

Comment 3

[Andreas Tolfsen ❲:ato❳]

Attachment: MozReview Request: Bug 1211503: Support for Marionette protocol level 3 in the Python client

Bug 1211503: Support for Marionette protocol level 3 in the Python client

Futures-proofs the Marionette Python client to have support for level
3 of the Marionette protocol outlined in bug 1211489.

This patch changes the marionette-transport API most notably by renaming
the MarionetteTransport class to TcpTransport and by splitting the
receive-capabilities of TcpTransport.send into a new function called
request.

Furthermore it introduces a message data structure for dealing with
incoming responses and commands, and for marshaling messages to send in
order to support all three protocol levels.

r=dburns
r=jgriffin

Comment 4

[David Burns :automatedtester]

Comment on attachment 8674276 [details]
MozReview Request: Bug 1211503: Support for Marionette protocol level 3 in the Python client

https://reviewboard.mozilla.org/r/22207/#review19891

::: testing/marionette/driver/marionette_driver/marionette.py:770
(Diff revision 1)
> -            error = resp["error"]
> +            error = obj["error"]

are we expecting these to be here when the code hits the point? If it's not do we want the unhandled exception?

::: testing/marionette/transport/marionette_transport/transport.py:143
(Diff revision 1)
> -        is prepended by len(message) + ":".
> +        # protocol 3 and above

nit: comment doesnt match code. if we want `and above` it should be `>= 3`

Comment 5

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/22207/#review19903

::: testing/marionette/transport/marionette_transport/transport.py:274
(Diff revision 1)
> +        to come back.  Allowed input is a ``Message`` instance or a JSON
> +        serialisable object.

Docs are wrong

Comment 6

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8674276 [details]
MozReview Request: Bug 1211503: Support for Marionette protocol level 3 in the Python client

Bug 1211503: Support for Marionette protocol level 3 in the Python client

Futures-proofs the Marionette Python client to have support for level
3 of the Marionette protocol outlined in bug 1211489.

This patch changes the marionette-transport API most notably by renaming
the MarionetteTransport class to TcpTransport and by splitting the
receive-capabilities of TcpTransport.send into a new function called
request.

Furthermore it introduces a message data structure for dealing with
incoming responses and commands, and for marshaling messages to send in
order to support all three protocol levels.

r=dburns
r=jgriffin

Comment 7

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/22207/#review19891

> nit: comment doesnt match code. if we want `and above` it should be `>= 3`

Well spotted, sir.

Comment 8

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8674276 [details]
MozReview Request: Bug 1211503: Support for Marionette protocol level 3 in the Python client

https://reviewboard.mozilla.org/r/22207/#review20001

Comment 9

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8674276 [details]
MozReview Request: Bug 1211503: Support for Marionette protocol level 3 in the Python client

Bug 1211503: Support for Marionette protocol level 3 in the Python client

Futures-proofs the Marionette Python client to have support for level
3 of the Marionette protocol outlined in bug 1211489.

This patch changes the marionette-transport API most notably by renaming
the MarionetteTransport class to TcpTransport and by splitting the
receive-capabilities of TcpTransport.send into a new function called
request.

Furthermore it introduces a message data structure for dealing with
incoming responses and commands, and for marshaling messages to send in
order to support all three protocol levels.

r=dburns
r=jgriffin

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cf388b1cf3a

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4cf388b1cf3a

Comment 12

[Ben Hsu [:HoPang]]

Attachment: A quick fix

Hi Andreas,

After the patches for this bug landed, marionette-webapi doesn't work anymore, and here is a short explanation. The first response we get after an “executeJSScript" or an “executeAsyncScript” is sent might be an emulator command instead of the actual response to the original request. Now, after marionette-client handle the emulator command received, it still takes the command as the actual response to the original request at the end of |_send_message(…)|, and thus an exception  is raised due to missing both |msg.result| and |msg.error| [1].

With the help from Edgar and John, I did some modification as this patch shows. Since both "executeJSScript" and "executeAsyncScript" start a new sub-session for running the testcase passed in, which means that multiple commands might be recieved since then. Thus, a loop is needed here to handle those commands and to wait for the actual response. To make things explicit, now the loop can only be entered when the origin request is either "executeJSScript" or “executeAsyncScript".

[1] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver/marionette_driver/marionette.py#711

Comment 13

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Andreas, Jonathan and David,
Sorry to flag you all as I have no idea who is available to check this issue at the moment. Could any of you please check Ben's comment 12? This bug causes regression that marionette-webapi doesn't work anymore that needs to be fixed to not blocking tests. Thank you very much.

Comment 14

[Jonathan Griffin (:jgriffin)]

This is something ato will need to look at; he's traveling this week but should be able to get to it early next week.

Comment 15

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Jonathan Griffin (:jgriffin) from comment #14)
> This is something ato will need to look at; he's traveling this week but
> should be able to get to it early next week.

Thank you Jonathan for the kind information.

Comment 16

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Andreas,
Sorry to pin you again. We are now managing to have a workaround in our working environments to not be blocked by this bug. However, ideally we need marionette-webapi getting back to work as soon as possible. Would you please take a look?

Comment 17

[Andreas Tolfsen ❲:ato❳]

bhsu, I don't really understand what you're trying to say.

From the patch you attached I understand that my patch removed the ability to run multiple emulator callbacks in a single invocation of executeScript/executeJSScript/executeAsyncScript.  Is this the only defect?

Basically we need the ability to run code such as:

    marionette.execute_async_script("""
        runEmulatorCmd("foo");
        runEmulatorCmd("bar");
        marionetteScriptFinished();
        """)

This doesn't work now because the second runEmulatorCmd will make the server return a command request, when the client is _actually_ expecting a response message with the "error" or "result" key.

I've filed bug 1223028 to follow up on this.  Please see the code I've attached there to see if this will resolve your problem.  My patch is essentially the same as the one you suggest, but I'm doubtful we need to move the loop into the try…catch block and test explicitly for the requested command name.

Comment 18

[Ben Hsu [:HoPang]]

Hi Andreas,

Thanks for your reply!

> From the patch you attached I understand that my patch removed the ability
> to run multiple emulator callbacks in a single invocation of
> executeScript/executeJSScript/executeAsyncScript.  Is this the only defect?

Yes, you're absolute right :)

> Basically we need the ability to run code such as:
> 
>     marionette.execute_async_script("""
>         runEmulatorCmd("foo");
>         runEmulatorCmd("bar");
>         marionetteScriptFinished();
>         """)
> 
> This doesn't work now because the second runEmulatorCmd will make the server
> return a command request, when the client is _actually_ expecting a response
> message with the "error" or "result" key.

Though it's not that important, in fact, it's the first |runEmulatorCmd(...)| that crashes marionette-client now, since it's mistaken as the response of the request command sent by |execute_async_script(...)| to start the test.

> I've filed bug 1223028 to follow up on this.  Please see the code I've
> attached there to see if this will resolve your problem.  My patch is
> essentially the same as the one you suggest, but I'm doubtful we need to
> move the loop into the try…catch block and test explicitly for the requested
> command name.

First, I think the loop should be put into the try...catch block, since marionette-client communicates with the the emulator via telent whose methods might raise certain errors according to [1]. Besides, both |_emulator_cmd(...)| and |_emulator_shell(...)| might raise an excption if some checks fail. Second, I don't have a strong preference for whether to place an explicit check if it's well documented. However, you listed three request commands which are invoked by |execute_script(...)|, |execute_js_script(...)| and |execute_async_script(...)| respectively, while there are only two of them listed in the comment in your patch. Do you mind checking it again? Thanks

[1] https://docs.python.org/2/library/telnetlib.html

Comment 19

[Andreas Tolfsen ❲:ato❳]

(In reply to Ben Hsu [:bhsu] from comment #18)
> Though it's not that important, in fact, it's the first
> |runEmulatorCmd(...)| that crashes marionette-client now, since it's
> mistaken as the response of the request command sent by
> |execute_async_script(...)| to start the test.

Hm, okay.  Would you mind testing the patch locally?

> However, you listed three
> request commands which are invoked by |execute_script(...)|,
> |execute_js_script(...)| and |execute_async_script(...)| respectively, while
> there are only two of them listed in the comment in your patch. Do you mind
> checking it again? Thanks

Will add it, thanks.

Comment 20

[Ben Hsu [:HoPang]]

> Hm, okay.  Would you mind testing the patch locally?

Sure, tests under telephony/test/marionette/ passed perfectly :)

Though some other tests for other components are still failed, I believe those failures have nothing to do with this issue anymore.

Comment 21

[Andreas Tolfsen ❲:ato❳]

(In reply to Ben Hsu [:bhsu] from comment #20)
> > Hm, okay.  Would you mind testing the patch locally?
> 
> Sure, tests under telephony/test/marionette/ passed perfectly :)
> 
> Though some other tests for other components are still failed, I believe
> those failures have nothing to do with this issue anymore.

Perfect, thank you so much for testing that!

Comment 22

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

Moving bugs for Marionette client due to component changes.