Closed Bug 1052433 Opened 6 years ago Closed 6 years ago

[mozrunner] Remove the device reboot in B2GDeviceRunner

Categories

(Testing :: Mozbase, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: davehunt, Assigned: davehunt)

References

Details

Attachments

(1 file, 1 obsolete file)

The device is currently rebooted every time a B2GDeviceRunner is started on a device. This adds overhead to the tests and doesn't appear to be necessary. I have also seen occasions where the reboot fails and the device is left with a white screen.
I'm relying on this reboot at the moment. Why do you want to remove it?
Well I guess the answer to that is in the first comment. But I still rely on it to get the device into a clean state. I guess in theory it's possible to move out of mozrunner if it's only me that wants it.
(In reply to James Graham [:jgraham] from comment #2)
> I'm relying on this reboot at the moment. Why do you want to remove it?

How does rebooting get it into a clean state? For the UI tests we're stopping the b2g process, wiping out several locations, and starting it again. That's enough to give us a clean state, and the reboot along won't remove any persistent storage, etc.
For small values of "clean" i.e. not clearing out persistent storage, but resetting all the transient state, not just that of the b2g process itself. It's possible that just restarting the b2g process would be good enough, but to be honest I'm unconvinced this is true e.g. using the unreliable office wifi we seem to be even less likely to get a connection after restarting b2g than after a normal boot (I don't have data to back that up, but connecting under mozrunner does seem to be even more unreliable than connecting at other times and this seems like one of the main differences).
Perhaps we could make this conditional based on an argument? How often are you doing a reboot, and have you seen the white screen issue mentioned? It doesn't seem to take long for me to reproduce this with a reboot between each test. I can't speak for the connection in the office, but stop/starting the b2g process has been adequate for the Gaia functional UI tests.
I seem to remember that without the reboot, the new profile doesn't get picked up.. is that not the case anymore?

Otherwise, I think we need to reboot if we are installing a new profile. (I'm ok with removing the reboot if we *aren't* installing a new profile though).
cc'ing mdas as this will also affect her work with the JS gaia-integration.
Comment on attachment 8471579 [details] [diff] [review]
Remove the device reboot in B2GDeviceRunner. v1.0

I'd like confirmation from mdas and jgraham that this isn't going to break them before reviewing this.

It would be awesome to remove this if we could though, as that would speed the Gij tests up a lot.
Attachment #8471579 - Flags: review?(ahalberstadt)
(In reply to Andrew Halberstadt [:ahal] from comment #7)
> I seem to remember that without the reboot, the new profile doesn't get
> picked up.. is that not the case anymore?
> 
> Otherwise, I think we need to reboot if we are installing a new profile.
> (I'm ok with removing the reboot if we *aren't* installing a new profile
> though).

In runner.start() the profile is deleted and recreated, so I would expect that nothing would work if the subsequent start would not pick up the remote profile. That said, we should do further testing.
(In reply to Andrew Halberstadt [:ahal] from comment #9)
> Comment on attachment 8471579 [details] [diff] [review]
> Remove the device reboot in B2GDeviceRunner. v1.0
> 
> I'd like confirmation from mdas and jgraham that this isn't going to break
> them before reviewing this.
> 
> It would be awesome to remove this if we could though, as that would speed
> the Gij tests up a lot.

I have tested and can still influence the profile without the reboot. James: here's the files we clean up in gaiatest to get us into a clean state when performing a restart of the b2g process: https://github.com/mozilla-b2g/gaia/blob/16eec00c4cd1ef4c627b2103aac0b36334c439d0/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L771-L779

If we're still not sure about removing this reboot how about having a keyword argument 'reboot' which defaults to True, and then the gaiatest harness could skip the reboot between tests?
Flags: needinfo?(mdas)
Flags: needinfo?(james)
Flags: needinfo?(ahalberstadt)
So, I (think I) need a reboot at some point, but it probably doesn't have to be in this method. So if everything else works without a reboot here, I'm happy to seperate the concerns a bit.
Flags: needinfo?(james)
Blocks: 1056654
(In reply to Dave Hunt (:davehunt) from comment #11)
> If we're still not sure about removing this reboot how about having a
> keyword argument 'reboot' which defaults to True, and then the gaiatest
> harness could skip the reboot between tests?

Yeah, that would be ok. Though if the statement "We need to reboot iff we are modifying the profile" is true, then maybe we could just skip the reboot if no 'profile' argument was passed in. I'm not 100% sure that statement is true though.
Flags: needinfo?(ahalberstadt)
(In reply to Andrew Halberstadt [:ahal] from comment #9)
> Comment on attachment 8471579 [details] [diff] [review]
> Remove the device reboot in B2GDeviceRunner. v1.0
> 
> I'd like confirmation from mdas and jgraham that this isn't going to break
> them before reviewing this.
> 
> It would be awesome to remove this if we could though, as that would speed
> the Gij tests up a lot.

I ran some of the tests without it, and it's fine. In any case, if we need a reboot, we can add it to the device handler used by runner-service, since it has access to the B2GDeviceRunner's device object.
Flags: needinfo?(mdas)
Comment on attachment 8471579 [details] [diff] [review]
Remove the device reboot in B2GDeviceRunner. v1.0

Review of attachment 8471579 [details] [diff] [review]:
-----------------------------------------------------------------

Requesting review again based on recent feedback.
Attachment #8471579 - Flags: feedback?(james) → review?(ahalberstadt)
Comment on attachment 8471579 [details] [diff] [review]
Remove the device reboot in B2GDeviceRunner. v1.0

Review of attachment 8471579 [details] [diff] [review]:
-----------------------------------------------------------------

If it works for Malini and James, it wfm.
Attachment #8471579 - Flags: review?(ahalberstadt) → review+
Updated due to bitrot. Carrying r+
Attachment #8471579 - Attachment is obsolete: true
Attachment #8480000 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/82acd6f569ec
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.