1033402 - Listen for available port before starting marionette connection

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::PerformanceTest, defect, P1)

Description

[:Eli Perelman]

Our current initiative for starting the Marionette device host involves us sleeping instead of doing smarter polling. We should move this port polling into marionette-device-host to simplify the shell script being used for starting the tests in Gaia.

Comment 1

[:Eli Perelman]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/marionette-device-host/pull/1

Comment 2

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

Comment on attachment 8449532 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/marionette-device-host/pull/1

Looks good in general. A few nits on GH and I would like a test or two and also for the travis bits to be resurrected if that's not too much trouble. Reflag for review afterwards. Thanks!

Comment 3

[:Eli Perelman]

Comment on attachment 8449532 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/marionette-device-host/pull/1

Nits resolved, current test covers changes made to #start. Also moved B2G restart functionality into here with a flag to ensure that B2G only restarts on the first run.

Comment 4

[Hubert Figuiere [:hub]]

This was originally part of bug 1027232. I can't do bug 1027232 without that. The current PR doesn't work as it still requires a proper sleep value: the '2' used doesn't work with slower hardware like hamachi.

Comment 5

[Hubert Figuiere [:hub]]

Yup to check for the port, you need to actually connect to it and receive data.

In the first seconds, a connect lead to a "connection reset by peer".
Then when it is actually working, you get data sent upon "connect()" and that's when marionette is ready.

I have a basic implementation lifted from python. I am actually tempted to move it into the marionette-js-client.

Comment 6

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

Comment on attachment 8449532 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/marionette-device-host/pull/1

Pull request is closed. Reflag me if there's stuff to review :)

Comment 7

[Hubert Figuiere [:hub]]

Comment on attachment 8449532 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/marionette-device-host/pull/1

cancelling r?

Comment 8

[Hubert Figuiere [:hub]]

FWIW, a proposed patch for this in part of https://github.com/mozilla-b2g/marionette-device-host/pull/2

Comment 9

[Hubert Figuiere [:hub]]

At appears clear that we need to have this in the marionette-js-client now.

Comment 10

[Hubert Figuiere [:hub]]

This is no longer part of the pull request mentionned in comment 8

Comment 11

[Hubert Figuiere [:hub]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/marionette-js-client/pull/115

This time we do the check on marionette-js-client.

I will wait for Gaia try though. 

If you have feedback / comments

Comment 12

[Hubert Figuiere [:hub]]

This is the gaia try attempt:

https://tbpl.mozilla.org/?rev=37a00db615147cc1a6943bdc1f3e8060b7117694&tree=Gaia-Try

Comment 13

[Hubert Figuiere [:hub]]

It failed. Waiting a new one after fixing.

Comment 14

[Hubert Figuiere [:hub]]

bug 1040062 is why we haven't had a new gaia try attempt, likely.

Comment 15

[Hubert Figuiere [:hub]]

Second attempt:

https://tbpl.mozilla.org/?rev=b92e49f3efab8ba56a90bd3b2261a56e90506986&tree=Gaia-Try

Comment 16

[Hubert Figuiere [:hub]]

Look like Gaia Try is green. 

There is hope that Travis will be as the error in the previous run was just related to the tests itself because I use a custome gaia-node-module.

Comment 17

[Hubert Figuiere [:hub]]

Travis is green too. We pass all the test. Nits have been address. Can we push this?

I don't mind filing a bug so that we actually write proper unit tests for the drivers.

Comment 18

[Hubert Figuiere [:hub]]

Link to the Gaia Travis run with this https://travis-ci.org/mozilla-b2g/gaia/builds/30182352

Comment 19

[Hubert Figuiere [:hub]]

We have a test now in the tree for the waitForSocket().

Comment 20

[Gregor Wagner [:gwagner]]

This seems to be the last blocker to turn on Gij in tbpl and move off of travis.
Can we land this?

Comment 21

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

Once the patch is good to go, there's no reason not to :)

Comment 22

[Gregor Wagner [:gwagner]]

I guess the p in the whiteboard is for priority. Just want to make sure we have the right priority here.

Comment 23

[Hubert Figuiere [:hub]]

(In reply to Gregor Wagner [:gwagner] from comment #22)
> I guess the p in the whiteboard is for priority. Just want to make sure we
> have the right priority here.

No it is points. As used by Scrumbu.gs.

Actual priority value is being used.

Comment 24

[Hubert Figuiere [:hub]]

Comment on attachment 8456294 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/marionette-js-client/pull/115

Switching reviewer as suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=1027232#c30

James, your feedback and review would be highly appreciated.

Thank you kindly.

Comment 25

[Hubert Figuiere [:hub]]

Merged
https://github.com/mozilla-b2g/marionette-js-client/commit/be513c9e3decc75b28cc26401fa7a3daeda9eb47

Comment 26

[Hubert Figuiere [:hub]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia-node-modules/pull/57

Kevin,

Just updating the gaia-node-modules.

Thank you kindly.

Comment 27

[Hubert Figuiere [:hub]]

Comment on attachment 8456294 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/marionette-js-client/pull/115

jlal did r+ on github. We had a green tbpl.

Comment 28

[Kevin Grandon :kgrandon]

Comment on attachment 8467329 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia-node-modules/pull/57

Thanks Hub!

Comment 29

[Hubert Figuiere [:hub]]

Merged
https://github.com/mozilla-b2g/gaia-node-modules/commit/31071870f8037cd87bba3fa2b7a0614666457e64

Comment 30

[Hubert Figuiere [:hub]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/22498

Comment 31

[Kevin Grandon :kgrandon]

Comment on attachment 8467337 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/22498

Please wait for a green gaia-try build before landing. Thanks!

Comment 32

[Hubert Figuiere [:hub]]

Merged.

https://github.com/mozilla-b2g/gaia/commit/a2d748f55624021563ec998845c2426286e81f5d

Comment 33

[Hubert Figuiere [:hub]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/22524

This is the port to 2.0. It pulls the modules from the 2.0 branch of the gaia-node-modules repository.

Comment 34

[Hubert Figuiere [:hub]]

Comment on attachment 8467737 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/22524

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): We want bug 1027232 and bug 1033402 on 2.0 perf testing.
[User impact] if declined: less performance test
[Testing completed]: TBPL
[Risk to taking this patch] (and alternatives if risky): Might break integration test. We are testing making sure this does not happen. NPOTB otherwise.
[String changes made]: none

Comment 35

[Kevin Grandon :kgrandon]

Comment on attachment 8467737 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/22524

Pretty sure you can just land with a=npotb/testonly

Comment 36

[Hubert Figuiere [:hub]]

TBPL is green minus one test that is known to fail

Comment 37

[bhavana bajaj [:bajaj]]

Comment on attachment 8467737 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/22524

Given comment #36 approving here.

Comment 38

[Hubert Figuiere [:hub]]

Merged for 2.0

https://github.com/mozilla-b2g/gaia/commit/efa5c34186a5aea4761e0d6d43e743ffa1a5110c