1027232 - We should restart b2g before each app tests

Status: RESOLVED FIXED

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

Description

[Hubert Figuiere [:hub]]

We should restart b2g before each app tests

This is what b2gperf does.

See bug 1020557 comment 7.

Comment 1

[Hubert Figuiere [:hub]]

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

We restart b2g between each run


Note: this is the quick solution, I have one involving the device host but there is no way I can get it checked in before this evening.

Comment 2

[Hubert Figuiere [:hub]]

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

Gareth do you mind reviewing this?


I will have a follow up when I come back to do thing properly in the runner instead.

Comment 3

[Hubert Figuiere [:hub]]

Quick note:

There should be a second part to that bug that involve:

1. the restart code should be in the device host module and not the shell script
2. we should have an option to restart / not restart. We the current patch it is harder bug 1027649 is much harder to reproduce. I propose that restart be optout. NO_B2G_RESTART=1 as env should be enough.

Comment 4

[Hubert Figuiere [:hub]]

The marionette-device-host changes are on github.

See the diff 

https://github.com/hfiguiere/marionette-device-host/compare/bug1027232

I need to test it further, release a new npm module, update gaia-node-modules and set the new revision in gaia. Lot of moving pieces, lots of review.
Hence the [leave-open] whiteboard tag - fixing automation is a priority.

Comment 5

[Hubert Figuiere [:hub]]

Eli, if you can watch this. If it has a r+ make sure it is checked in. And then that bug 1020557 is no longer happening.

Don't close the bug though I have a definite followup for when I come back.

Thanks.

Comment 6

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

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

Comments on GH. Main concern is the sleeping to wait for the device b2g to come up. Also I would like for all of this code to move to marionette-device-host. Then after that I just have some js nits. Thanks hub and eli :)

Comment 7

[:Eli Perelman]

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

Comment 8

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

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

I want the adb shell stuff moved into the host so that we can really know for everything to be ready when we invoke the MarionetteDeviceHost#start callback. Flag for review again when we have that bit. Thanks!

Comment 9

[:Eli Perelman]

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

ADB code has been moved into marionette-device-host along with a tracking flag to ensure it only restarts B2G on the first run.

Comment 10

[:Eli Perelman]

Just a note that this patch should take care of point 1 from comment 3, but point 2 remains.

Comment 11

[Hubert Figuiere [:hub]]

The original goal what to get this quickly to resume automation - a 4 step landing would have been impossible before I left. #fail. Comment 3 and comment 4 said that. 

But at that point I'll just cancel the review and do it properly when I come back. I'm supposedly on vacation.

Comment 12

[Hubert Figuiere [:hub]]

taking it back from PTO.

Also tempted to actually merge bug 1033402 into this as it is needed anyway for that sole reason.

Comment 13

[Hubert Figuiere [:hub]]

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

Comment 14

[Hubert Figuiere [:hub]]

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

We should get that part in Gaia TestPerf first. I think Gareth comments have been addressed. This wait for lockscreen to be properly loaded before attempting anything. This actually improve the robustness of the tests.

Thanks.

The device-host part will be coming up next.

Comment 15

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

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

Other than a couple nits, this LGTM. Thanks!

Comment 16

[Hubert Figuiere [:hub]]

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

Gareth: pondering is waitForSocket() shouldn't be in marionette-js-client.

Otherwise this does work.

Comment 17

[Hubert Figuiere [:hub]]

And a further comment this waitForSocket() logic is lifted from the Python b2gperf, ie it is proven to be working in the long run.

Comment 18

[Hubert Figuiere [:hub]]

Merged the Gaia PerfTest part:
https://github.com/mozilla-b2g/gaia/commit/260a371cd07895f051bf96365342072902e7df1f

Comment 19

[Hubert Figuiere [:hub]]

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

I think it is safe to keep this localised to the device-host for now.

Switching to r?

Comment 20

[Hubert Figuiere [:hub]]

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

Cancelling the review request. I will split the PRs moving the resolution for bug 1033402 to the js-client.

Will update the pull request shortly and request review, but the blocker will need to be resolved first.

Comment 21

[Hubert Figuiere [:hub]]

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

This requires bug 1033402 to be landed. This is in the device-host module.

It will require: a new module release. the usual update of the gaia-node-module

Comment 22

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

Still need to figure out how we can tell that b2g is up without sleeping. If waiting for the tcp socket didn't work then perhaps waiting for the tcp socket and then trying to do things with it like executing a script in content and making sure that works suffices.

Comment 23

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

(Or looking at the bootwatcher thing from marionette-apps)

Comment 24

[Hubert Figuiere [:hub]]

That setTimeout in the patch was no longer meant to be here. Really.

Comment 25

[Zac C (:zac)]

(In reply to Gareth Aye [:gaye] from comment #22)
> Still need to figure out how we can tell that b2g is up without sleeping. If
> waiting for the tcp socket didn't work then perhaps waiting for the tcp
> socket and then trying to do things with it like executing a script in
> content and making sure that works suffices.

Do you mean waiting for B2G or waiting for Gaia? Waiting for gecko/marionette server to be ready via the tcp socket is pretty well tested, I don't think you'll have problems with that (except when b2g has crashed). 
Waiting for Gaia, there are various options there but you'll need to handle at least 3 ready scenarios: lockscreen, ftu and no lockscreen or ftu, for various states of profiles.

Comment 26

[Hubert Figuiere [:hub]]

bug 1033402 is covering this. the homescreen it is already done (see first patch).

Comment 27

[Zac C (:zac)]

Also after stopping you can poll `ps` for the b2g process and check that it is no longer running before starting it again. (We do this on Python framework after the a bug caused b2g process to be un-stoppable).

Comment 28

[Hubert Figuiere [:hub]]

FYI, the pull request has been updated. The timeout is "gone". Actually set to 0 for an async call to the callback.

(In reply to Zac C (:zac) from comment #27)
> Also after stopping you can poll `ps` for the b2g process and check that it
> is no longer running before starting it again. (We do this on Python
> framework after the a bug caused b2g process to be un-stoppable).

I haven't had that issue though. I'll keep that in mind, the experience is valuable.

Comment 29

[Mike Lee [:mlee]]

Gareth, this is a critical blocking issue. Please prioritize doing this review. We need this resolved asap.

Comment 30

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

Hey Mike! I've been working incrementally with :hub to get this work in and I think we're making progress. I've always gotten feedback in within a day of his updates. I'm happy to keep providing feedback, but no amount of managerial compulsion is going to make me settle for a patches that aren't ready for prime time. This has been a longer than usual process because :hub is still in the process of learning how all of the bits are wired together.

If you're hoping for a less strict reviewer, James Lal is also a marionette js peer.

Comment 31

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

Oh! I just realized there was another pull request to marionette-device-host. That one looks okay. In general, I prefer one patch per bug since every time I've looked at this bug I've seen that I've already OK'd it. Sorry for the delay!

Comment 32

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

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

A couple of nits on GH but mostly looks good. Fix and merge when ready!

Comment 33

[Hubert Figuiere [:hub]]

(In reply to Gareth Aye [:gaye] from comment #31)
> Oh! I just realized there was another pull request to
> marionette-device-host. That one looks okay. In general, I prefer one patch
> per bug since every time I've looked at this bug I've seen that I've already
> OK'd it. Sorry for the delay!

Yes there was two commit in that other PR because I had no choice due to the dependency. Sorry about that. I thought I mentioned it.

Thanks !

Comment 34

[Hubert Figuiere [:hub]]

Merge
https://github.com/mozilla-b2g/marionette-device-host/commit/3c116cd509221f7e940e29b9f6932c9d6cb4dbb4

(technically it is fixed, but I'll need to do a new module release for that for it to be available. Will happen with the blocking bug)

Comment 35

[Hubert Figuiere [:hub]]

Actually keeping open until the released module.

Comment 36

[Hubert Figuiere [:hub]]

version 0.1.0 uploaded to npm.

The modules will be pulled into gaia-node-modules when we fix bug 1033402

Comment 37

[Hubert Figuiere [:hub]]

All in 

See
https://bugzilla.mozilla.org/show_bug.cgi?id=1033402#c32

Comment 38

[Hubert Figuiere [:hub]]

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

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 #): itself. we forgot to uplift
[User impact] if declined: perf test less reliable
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky): npotb. 
[String changes made]: none

Comment 39

[Hubert Figuiere [:hub]]

and by "perf test less reliable" I really mean "broken"

testing all done in master. backporting the rest of the feature require this.

Comment 40

[:Eli Perelman]

Agreed, perf tests cannot be run against v2.0 without this patch.

Comment 41

[Hubert Figuiere [:hub]]

Merged v2.0
https://github.com/mozilla-b2g/gaia/commit/8cc28fd31905a0ea2b2e15d13e80a0eab2feb1ba