Closed Bug 1194968 Opened 5 years ago Closed 5 years ago

Adjust Python bindings to match upcoming changes to the Marionette protocol

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(firefox43 fixed)

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: ato, Assigned: ato)

References

Details

(Keywords: pi-marionette-client)

Attachments

(1 file)

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@.
Blocks: 1153822
Assignee: nobody → ato
Status: NEW → ASSIGNED
Summary: Adjust Python bindings to match upcoming in the Marionette protocol → Adjust Python bindings to match upcoming changes to the Marionette protocol
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
Attachment #8649237 - Flags: review?(jgriffin)
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 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 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
Attachment #8649237 - Flags: review?(jgriffin) → review+
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.
(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.
https://hg.mozilla.org/mozilla-central/rev/92d63c478838
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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
Flags: needinfo?(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.
Flags: needinfo?(ato)
(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.
Depends on: 1197078
You need to log in before you can comment on or make changes to this bug.