1194968 - Adjust Python bindings to match upcoming changes to the Marionette protocol

Status: RESOLVED FIXED

(Testing :: Marionette Client and Harness, defect)

Description

[Andreas Tolfsen ❲:ato❳]

Due to a series of difficulties working on bug 1168396 and bug 1153822, I've decided to split out the Python bindings changes from bug 1153822 to a separate patch that additionally _will_ have backwards compatibility with the earlier Marionette protocol.

I've written about the deployment plan for this to tools-marionette@.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: MozReview Request: Bug 1194968: Support for Marionette protocol adjustments in Python client

Bug 1194968: Support for Marionette protocol adjustments in Python client

Whilst it is not a goal for the Marionette protocol to be fully compatible
with WebDriver due to the different transport mechanisms, bug 1153822
adjusts the Marionette protocol closer to what is prescribed in the
WebDriver specification.

This patch adds support for the new protocol (level or version 2)
to the Python bindings with backwards compatibility for the earlier,
existing protocol (1).

r=dburns

Comment 2

[Andreas Tolfsen ❲:ato❳]

jgriffin: You’ve reviewed an earlier version of this patch before, but the scope has since changed and it now also adds backwards-compatibility support for the existing protocol.

dburns isn’t accepting review requests, so I had to assign it to you.

Comment 3

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8649237 [details]
MozReview Request: Bug 1194968: Support for Marionette protocol adjustments in Python client

Bug 1194968: Support for Marionette protocol adjustments in Python client

Whilst it is not a goal for the Marionette protocol to be fully compatible
with WebDriver due to the different transport mechanisms, bug 1153822
adjusts the Marionette protocol closer to what is prescribed in the
WebDriver specification.

This patch adds support for the new protocol (level or version 2)
to the Python bindings with backwards compatibility for the earlier,
existing protocol (1).

r=jgriffin

Comment 4

[Andreas Tolfsen ❲:ato❳]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c97242c111f

Comment 5

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8649237 [details]
MozReview Request: Bug 1194968: Support for Marionette protocol adjustments in Python client

Bug 1194968: Support for Marionette protocol adjustments in Python client

Whilst it is not a goal for the Marionette protocol to be fully compatible
with WebDriver due to the different transport mechanisms, bug 1153822
adjusts the Marionette protocol closer to what is prescribed in the
WebDriver specification.

This patch adds support for the new protocol (level or version 2)
to the Python bindings with backwards compatibility for the earlier,
existing protocol (1).

r=jgriffin

Comment 6

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8649237 [details]
MozReview Request: Bug 1194968: Support for Marionette protocol adjustments in Python client

https://reviewboard.mozilla.org/r/16367/#review14745

Thanks for preserving backwards compatibility!  I think that's what your try run demonstrates, although I didn't look at what tip your try run was against.

Comment 7

[Andreas Tolfsen ❲:ato❳]

(In reply to Jonathan Griffin (:jgriffin) from comment #6)
> Thanks for preserving backwards compatibility!  I think that's what your try
> run demonstrates, although I didn't look at what tip your try run was
> against.

Yes, exactly.

The attached try run shows that the patch works against current tip, without bug 1153822 applied.  I’ve done testing locally to make sure it continues working when it lands.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92d63c478838

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/92d63c478838

Comment 10

[Edgar Chen [:edgar] (OOO untils 09/01, slow response)]

Hi :ato, I found this patch breaks the b2g marionette-webapi [1] :(. If I backout this patch, the tests backs to normal [2] (At least, I did not see same error). Looks like something wrong when handling emulator command? Could you help to take a look? 

Thank you.

==
19:26:25     INFO -  08-20 01:36:41.708 I/Gecko   (   44): 1440034601711	Marionette	DEBUG	conn7 emulator <- ({"emulator_cmd":"gsm clear","id":0})
19:26:25  WARNING -  08-20 01:36:41.918 E/GeckoConsole(   44): [JavaScript Error: "Error parsing incoming packet: {'r (SyntaxError: JSON.parse: expected property name or '}' at line 1 column 2 of the JSON data - JSONPacket.prototype.read@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/packets.js:164:20
19:26:25     INFO -  08-20 01:36:41.918 E/GeckoConsole(   44): DebuggerTransport.prototype._processIncoming@chrome://marionette/content/server.js -> resource://gre/modules/devtools/transport/transport.js:405:9
19:26:25     INFO -  08-20 01:36:41.918 E/GeckoConsole(   44): DebuggerTransport.prototype.onInputStreamReady<@chrome://marionette/content/server.js -> resource://gre/modules/devtools/transport/transport.js:356:13
19:26:25     INFO -  08-20 01:36:41.918 E/GeckoConsole(   44): makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
19:26:25     INFO -  08-20 01:36:41.918 E/GeckoConsole(   44): )" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/packets.js" line: 169}]
19:26:25     INFO -  08-20 01:36:42.008 I/Gecko   (   44): 1440034602016	Marionette	INFO	Closed connection conn7
===

[1] https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=29b2df16e961&exclusion_profile=false&filter-searchStr=mnw 
[2] https://treeherder.allizom.org/#/jobs?repo=try&revision=2d3f4a493f97

Comment 11

[Andreas Tolfsen ❲:ato❳]

(In reply to Edgar Chen [:edgar][:echen] from comment #10)
> Hi :ato, I found this patch breaks the b2g marionette-webapi [1] :(.

It’s hard to know if you break marionette-webapi it they are not run on Gecko’s try.  Why aren’t they anymore?

> Looks like something wrong when handling emulator command?

I changed some behaviour around emulator callbacks, which strictly speaking wasn’t necessary to change:

    https://hg.mozilla.org/mozilla-central/rev/92d63c478838#l5.368

Could you put the old functions back in place; _handle_emulator_cmd, _handle_emulator_shell, and the while loop in _send_message; and see if that helps?

If the marionette-webapi tests are blocking integration, can you try downgrade to the previous version in the mean time?  The previous version should continue to work against Gecko tip.

Generally I would recommend pinning dependencies so that you’re not surprised by this again in the future.

Comment 12

[Edgar Chen [:edgar] (OOO untils 09/01, slow response)]

(In reply to Andreas Tolfsen (:ato) from comment #11)
> (In reply to Edgar Chen [:edgar][:echen] from comment #10)
> > Hi :ato, I found this patch breaks the b2g marionette-webapi [1] :(.
> 
> It’s hard to know if you break marionette-webapi it they are not run on
> Gecko’s try.  Why aren’t they anymore?

marionette-webapi is run on emulator only and it is hidden on treeherder ui. You have to click square button to unhide the result.

> 
> > Looks like something wrong when handling emulator command?
> 
> I changed some behaviour around emulator callbacks, which strictly speaking
> wasn’t necessary to change:
> 
>     https://hg.mozilla.org/mozilla-central/rev/92d63c478838#l5.368
> 
> Could you put the old functions back in place; _handle_emulator_cmd,
> _handle_emulator_shell, and the while loop in _send_message; and see if that
> helps?

After digging into the code, I found the transport.send expects to get a json string:

https://hg.mozilla.org/mozilla-central/rev/92d63c478838#l6.115

but we didn't send result of emulator command as json string. Will file a follow-up bug and provide a fix there.
Thank you.

Comment 13

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

Moving bugs for Marionette client due to component changes.