1227577 - Develop a new marionette-client promises driver

Status: RESOLVED FIXED

(Testing Graveyard :: JSMarionette, defect)

Description

[André Natal]

We have two existing marionette client "drivers" to handle tcp requests to the marionette server

    asynchronously with node-style callbacks [1]
    synchronously via sockit-to-me [2]

We'd now like to add an additional driver that handles request asynchronicity through promises.

[1] https://github.com/mozilla-b2g/gaia/blob/master/tests/jsmarionette/client/marionette-client/lib/marionette/drivers/tcp.js
[2] https://github.com/mozilla-b2g/gaia/blob/master/tests/jsmarionette/client/marionette-client/lib/marionette/drivers/tcp-sync.js

Comment 1

[Andreas Tolfsen ❲:ato❳]

Note that the different “drivers” are out of sync currently, and I doubt that they both work.  Are they in active use?  If they aren’t I would like to make a case for removing those that aren’t in use.

Comment 2

[GitHub Autolander Bot]

Attachment: [gaia] andrenatal:_promises_driver_patch > mozilla-b2g:master

Comment 3

[André Natal]

Hello,

the attached patch for the Promises driver is working with the corresponding unit test. Here [1] we have the packet flow of the test.

But when the integrated test is run,, after the handshake [2], after the client sends a 'newSession' [3] and the server replies with a new one [4], the client sends more than 10 messages with setContext [5], setSearchTimeout [6] and executeScript's with 'window.MARIONETTE_LOG_GRABBER [7].grabAndClearLogs()' rather than start the session like the test with the original driver. Some errors happens after the connection be established [8], but they also happens with the original driver [8.1].

The client with the Promises driver only follow up with the session like the tcp-sync driver does around message 9 [9] (sometimes even later, while TcpSync do it right after gets the session [10]), while keeps sending LOG_GRABBER messages in sequence [11] until the test fail with a 'TypeError: body.getAttribute is not a function' [12].

So my guess (fundamented on my still preliminary understanding of this), is that the client or the runner is not waiting the Promise or some error happens internally that is not outputted, then gets confused and sends these out-of-order packets. Or something else needs to be called by the Driver.

Another curious thing, is that analyzing the tcpdump and the log of the original driver, we realize he opens a connection and as soon receives the 'handshake' ({applicationType : gecko, marionetteProtocol:3}), he closes it [13][14]  and opens another one who then effectively starts the session [15]. I am not sure though if this is the right behavior and have something to do with the problem.

Here we have the link for the test log with the Promises Driver [16], with the TcpSync Driver [17], the tcpdump from the unit test [18], from the test with the Promises Driver [19], with the TcpSync Driver [20] and for the unit test [21]. 

I also see one of the tests failed on treeherder, but is passing locally [22]. I am using b2g-desktop built myself from master, and this test ('/apps/system/test/marionette/activity_chain_test.js') to run the driver.

I'd really appreciate if you guys could give some input.

Thanks

Andre


[1] https://www.dropbox.com/s/m2bf0dnb9a6fa1c/Screen%20Shot%202016-01-04%20at%2011.38.11%20PM.png?dl=0 
[2] https://gist.github.com/andrenatal/bc7eac5c735cd26f63a4#file-gistfile1-txt-L1682
[3] https://gist.github.com/andrenatal/bc7eac5c735cd26f63a4#file-gistfile1-txt-L1686
[4] https://gist.github.com/andrenatal/bc7eac5c735cd26f63a4#file-gistfile1-txt-L1700
[5] https://gist.github.com/andrenatal/bc7eac5c735cd26f63a4#file-gistfile1-txt-L1701
[6] https://gist.github.com/andrenatal/bc7eac5c735cd26f63a4#file-gistfile1-txt-L1703
[7] https://gist.github.com/andrenatal/bc7eac5c735cd26f63a4#file-gistfile1-txt-L1706
[8] https://gist.github.com/andrenatal/bc7eac5c735cd26f63a4#file-gistfile1-txt-L1683
[8.1] https://gist.github.com/andrenatal/8ab3b98f26ab74377956#file-gistfile1-txt-L1686
[9] https://gist.github.com/andrenatal/bc7eac5c735cd26f63a4#file-gistfile1-txt-L1716
[10] https://gist.github.com/andrenatal/8ab3b98f26ab74377956#file-gistfile1-txt-L1703
[11] https://gist.github.com/andrenatal/bc7eac5c735cd26f63a4#file-gistfile1-txt-L1727
[12] https://gist.github.com/andrenatal/bc7eac5c735cd26f63a4#file-gistfile1-txt-L1774
[13] https://www.dropbox.com/s/8m22utwntjg6ab3/Screenshot%202016-01-05%2000.39.27.png?dl=0
[14] https://gist.github.com/andrenatal/8ab3b98f26ab74377956#file-gistfile1-txt-L1683
[15] https://gist.github.com/andrenatal/8ab3b98f26ab74377956#file-gistfile1-txt-L1684
[16] https://gist.github.com/andrenatal/bc7eac5c735cd26f63a4
[17] https://gist.github.com/andrenatal/8ab3b98f26ab74377956
[18] https://www.dropbox.com/s/znmt6fjx8cat8kn/test_lo0.pcapng?dl=0
[19] https://www.dropbox.com/s/gi8r9m8upq7jcla/promises_lo0.pcapng?dl=0
[20] https://www.dropbox.com/s/qfoznduv69ls93d/tcpsync_lo0.pcapng?dl=0
[21] https://gist.github.com/andrenatal/2db697aa8083a117dd5d
[22] https://gist.github.com/andrenatal/2db697aa8083a117dd5d#file-gistfile1-txt-L86

Comment 4

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

Hey Andre. You can let the tcp driver handle all of the raw details. All we need is a tiny promises wrapper like I showed you in Orlando and an integration test to demonstrate it's doing the right thing.

Comment 5

[André Natal]

(In reply to Gareth Aye [:gaye] (back from PTO) from comment #4)
> Hey Andre. You can let the tcp driver handle all of the raw details. All we
> need is a tiny promises wrapper like I showed you in Orlando and an
> integration test to demonstrate it's doing the right thing.

That simple wrapper did not work properly. I'll revert the code and post the logs here.

Comment 6

[André Natal]

(In reply to Gareth Aye [:gaye] (back from PTO) from comment #4)
> Hey Andre. You can let the tcp driver handle all of the raw details. All we
> need is a tiny promises wrapper like I showed you in Orlando and an
> integration test to demonstrate it's doing the right thing.

Sorry, but by 'integration test' you mean a marionettejs test that uses the driver, or a specific integration only for the driver?

Comment 7

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

(In reply to André Natal from comment #6)
> (In reply to Gareth Aye [:gaye] (back from PTO) from comment #4)
> > Hey Andre. You can let the tcp driver handle all of the raw details. All we
> > need is a tiny promises wrapper like I showed you in Orlando and an
> > integration test to demonstrate it's doing the right thing.
> 
> Sorry, but by 'integration test' you mean a marionettejs test that uses the
> driver, or a specific integration only for the driver?

marionettejs test that uses the driver. Totally possible that the initial thing we hacked together was incomplete (I don't remember entirely), but the basic idea is that the thing that you are writing is only a very thin translation layer from the tcp driver's callbacks to promises.

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

(In reply to André Natal from comment #3)
> Another curious thing, is that analyzing the tcpdump and the log of the
> original driver, we realize he opens a connection and as soon receives the
> 'handshake' ({applicationType : gecko, marionetteProtocol:3}), he closes it
> [13][14]  and opens another one who then effectively starts the session
> [15]. I am not sure though if this is the right behavior and have something
> to do with the problem.

This seemed familiar so I skimmed the https://developer.mozilla.org/en-US/Firefox_OS/Automated_testing/Gaia_integration_tests#What_is_the_life-cycle_of_the_Gecko_processes section I wrote, and I think you're seeing suiteSetup()'s call to Host.create() creating a session and delete it.  Then the subsequent stuff is in setup().

Comment 9

[Aus Lacroix [:aus]]

(In reply to Andrew Sutherland [:asuth] from comment #8)
> (In reply to André Natal from comment #3)
> > Another curious thing, is that analyzing the tcpdump and the log of the
> > original driver, we realize he opens a connection and as soon receives the
> > 'handshake' ({applicationType : gecko, marionetteProtocol:3}), he closes it
> > [13][14]  and opens another one who then effectively starts the session
> > [15]. I am not sure though if this is the right behavior and have something
> > to do with the problem.
> 
> This seemed familiar so I skimmed the
> https://developer.mozilla.org/en-US/Firefox_OS/Automated_testing/
> Gaia_integration_tests#What_is_the_life-cycle_of_the_Gecko_processes section
> I wrote, and I think you're seeing suiteSetup()'s call to Host.create()
> creating a session and delete it.  Then the subsequent stuff is in setup().

This is correct. We first connect to the server to tell it which port we will effectively be using to run the tests, it will then restart and bind to that port, waiting for the client to reconnect. As soon as this happens, the session is considered created and connected and tests start running. This happens for each test() function.

I left some more comments in the github pull request. I think the underlying assumptions about the implementation originally talked about in Orlando are still good. Fixing some of the small issues with the tests and impl should solve the issues.

Comment 10

[Andreas Tolfsen ❲:ato❳]

Once this lands, should removing the old then unused drivers be considered separate bugs?

Comment 12

[GitHub Autolander Bot]

Attachment: [gaia] andrenatal:WIP > mozilla-b2g:master

Comment 13

[GitHub Autolander Bot]

Attachment: [gaia] andrenatal:promises_driver_patch_test > mozilla-b2g:master

Comment 14

[André Natal]

Here's the evidence for the integrated test [1] and for the unit test [2]. I was also able to run the integrated test without requiring comment all plugins in setup.js.

[1] https://gist.github.com/andrenatal/b1df7b3bce5e4522ac98

[2] 
mozillas-MacBook-Pro-8:marionette-client anatal$ ../../../../node_modules/.bin/mocha --require test/helper.js --ui tdd --recursive --reporter spec test/marionette/drivers/promises-test.js
  marionette/drivers/promises
    ✓ should return a promise on connect (484ms)
    ✓ should send an object and receive a promise (458ms)

  2 passing (951ms)

Comment 15

[André Natal]

Tests are passing on treeherder right now:


https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=191bc613ff009490b8408ce0e0dd75b41a84f0c1

Comment 16

[Aus Lacroix [:aus]]

Comment on attachment 8709248 [details] [review]
[gaia] andrenatal:promises_driver_patch_test > mozilla-b2g:master

OK, the tests have already been passing for a while, so the driver obviously works. I just wanted to make sure our assumptions were correct and based on review of underlying code paths, it looks like we're OK. Let's go ahead and land this today. :)

Comment 17

[André Natal]

Comment on attachment 8709248 [details] [review]
[gaia] andrenatal:promises_driver_patch_test > mozilla-b2g:master

r+ from aus

Comment 18

[Aus Lacroix [:aus]]

Commit (master): https://github.com/mozilla-b2g/gaia/commit/10df3a68103c4ae7bd033eb0a742deb59c01b3bf

Fixed.

We'll release a new client later on when we've updated the marionette-plugins to be aware of this new driver.