1168396 - Adjust Node.js bindings to match upcoming changes to the Marionette protocol

Status: RESOLVED FIXED

(Testing Graveyard :: JSMarionette, defect)

Description

[Andreas Tolfsen ❲:ato❳]

Bug 1153822 makes extensive changes to the Marionette protocol and the marionette-js-client needs an update to match that.  There also needs to be some synchronisation of the trees when this patch lands, as this is a _non-backwards compatible_ and _breaking_ change.

Comment 1

[James Lal [:lightsofapollo]]

NI? just for some visibility

Comment 2

[Andreas Tolfsen ❲:ato❳]

Attachment: PR

Comment 3

[Gareth Aye [:gaye] (back from PTO)]

Comment on attachment 8621722 [details] [review]
PR

This patch currently fails the linter check, the unit tests, and breaks all of gaia's integration tests. When we have those things settled, I'll be happy to do another pass.

Comment 4

[Andreas Tolfsen ❲:ato❳]

(In reply to Gareth Aye [:gaye] from comment #3)
> Comment on attachment 8621722 [details] [review]
> PR
> 
> This patch currently fails the linter check, the unit tests, and breaks all
> of gaia's integration tests. When we have those things settled, I'll be
> happy to do another pass.

Please see the PR description.

Comment 5

[Andreas Tolfsen ❲:ato❳]

Due to a series of challenges (I've written about some of these on tools-marionette@) I'm going to land this change with backwards compatibility.  This allows us to land the patch without coordination with the sheriffs, and should make the transition smoother.

Unfortunately it means that the patch will be slightly larger and not as clean as I would've hoped, but this will only be the case for a short period of time, until we've gotten all consumers (out-of-tree and in-tree) upgraded.

Comment 6

[Andreas Tolfsen ❲:ato❳]

try against m-c tip: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=93182c127542e92480dd622dd98afbc8d9eb6d05

I’ve run the patch against bug 1153822 locally and it seems to pass all the harness integration tests, as well as the harness unit tests.

Because of a series of issues running the remaining integration tests locally, I’ve not been able to test these with bug 1153822, but they pass against m-c as you can see in the try run above.

But any issues will show up when we attempt to land bug 1153822 on m-i.

Comment 7

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8621722 [details] [review]
PR

aus: Reassigning the review request to you since gaye is on PTO for a week.

Please let me know if you have any questions.  I’d be happy to help guide you through the review.

Comment 8

[Aus Lacroix [:aus]]

Comment on attachment 8621722 [details] [review]
PR

lgtm! there's a minor nit regarding order of arguments in client.js. See the note. I'm not bound to making it more node like, just something we wanted to raise. If you agree, could you change that single item before committing? Thanks! :)

Comment 9

[Andreas Tolfsen ❲:ato❳]

Regarding the argument ordering change in client.js, responses previously used to have an "ok" field, like this:

    {ok: true}

Now we just return {} which means we can’t specify the key to look for, since there is no "ok" field any longer.  This basically renders the need to specify a key lookup unnecessary for any non-value responses.

The alternative is to keep the original ordering of the arguments but pass in undefined and make that have special meaning.  But since I don’t particularly like attributing magic to types, I think this is the best option.

With aus’ approval, I will go ahead and merge this change.  If you later feel that I’ve made some incorrect assumptions or you prefer the original ordering, I’m happy  to go back and change it again.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/9e28030c7e33b6686e0a4c0dd203d7877ffc54be

Comment 11

[:Eli Perelman]

Can we also get this published to npm?

Comment 12

[:Eli Perelman]

ni? for comment 11.

Comment 13

[GitHub Autolander Bot]

Attachment: [gaia] gaye:bug-1168396-marionette-client-1.8.0 > mozilla-b2g:master

Comment 14

[Aus Lacroix [:aus]]

Going to go ahead and clear my NI? here. Took care of it over IRC. :)

Comment 15

[:Eli Perelman]

This was pushed to npm already, although @mozilla/raptor is pinned to the previous versions of marionette-client due to instability with the updated version.

Comment 16

[Andreas Tolfsen ❲:ato❳]

(In reply to :Eli Perelman from comment #15)
> This was pushed to npm already, although @mozilla/raptor is pinned to the
> previous versions of marionette-client due to instability with the updated
> version.

What sort of instability are we talking about here?  Caused by this change?