Last Comment Bug 1211503 - Support for Marionette protocol level 3 in Marionette Python client
: Support for Marionette protocol level 3 in Marionette Python client
Status: RESOLVED FIXED
: ateam-marionette-client
Product: Testing
Classification: Components
Component: Marionette (show other bugs)
: unspecified
: Unspecified Unspecified
-- normal (vote)
: mozilla44
Assigned To: Andreas Tolfsen
:
:
Mentors:
Depends on:
Blocks: 1211489
  Show dependency treegraph
 
Reported: 2015-10-05 08:38 PDT by Andreas Tolfsen
Modified: 2015-11-11 04:51 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
MozReview Request: Bug 1211503: Support for Marionette protocol level 3 in the Python client (40 bytes, text/x-review-board-request)
2015-10-15 08:11 PDT, Andreas Tolfsen
dburns: review+
jgriffin: review+
Details | Review
A quick fix (3.09 KB, patch)
2015-10-26 04:09 PDT, Ben Hsu [:HoPang]
no flags Details | Diff | Splinter Review

Description User image Andreas Tolfsen 2015-10-05 08:38:31 PDT
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 User image Andreas Tolfsen 2015-10-14 10:51:21 PDT
try against level 2 server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=07608ba1a982
Comment 2 User image Andreas Tolfsen 2015-10-15 08:02:48 PDT
try with fixes against level 2 server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=636f6b920561
Comment 3 User image Andreas Tolfsen 2015-10-15 08:11:00 PDT
Created 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 4 User image David Burns :automatedtester 2015-10-16 03:05:51 PDT
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 User image Andreas Tolfsen 2015-10-16 04:57:21 PDT
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 User image Andreas Tolfsen 2015-10-17 07:12:13 PDT
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 User image Andreas Tolfsen 2015-10-17 07:12:28 PDT
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 User image Jonathan Griffin (:jgriffin) 2015-10-19 08:16:56 PDT
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 User image Andreas Tolfsen 2015-10-20 06:06:09 PDT
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 11 User image Carsten Book [:Tomcat] 2015-10-21 07:30:48 PDT
https://hg.mozilla.org/mozilla-central/rev/4cf388b1cf3a
Comment 12 User image Ben Hsu [:HoPang] 2015-10-26 04:09:58 PDT
Created attachment 8678780 [details] [diff] [review]
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 User image Hsin-Yi Tsai [:hsinyi] 2015-10-27 18:18:12 PDT
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 User image Jonathan Griffin (:jgriffin) 2015-10-29 09:49:00 PDT
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 User image Hsin-Yi Tsai [:hsinyi] 2015-10-29 19:36:54 PDT
(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 User image Hsin-Yi Tsai [:hsinyi] 2015-11-08 18:06:32 PST
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 User image Andreas Tolfsen 2015-11-09 07:54:39 PST
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 User image Ben Hsu [:HoPang] 2015-11-09 23:54:57 PST
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 User image Andreas Tolfsen 2015-11-10 03:55:43 PST
(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 User image Ben Hsu [:HoPang] 2015-11-11 04:21:27 PST
> 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 User image Andreas Tolfsen 2015-11-11 04:51:44 PST
(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!

Note You need to log in before you can comment on or make changes to this bug.