1031008 - Autophone - let devices catch their breath

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, enhancement)

Description

[Bob Clary [:bc] (inactive)]

When Autophone overworks a device due to rapid fire jobs, it can drain the device battery even though the device is charged via usb. Autophone's worker.py should check the device's battery status and give it a break if the battery level drops too low.

$ adb -s 015d2109f533fa13 shell 'dumpsys battery'
Current Battery Service state:
  AC powered: false
  USB powered: true
  Wireless powered: false
  status: 2
  health: 2
  present: true
  level: 39
  scale: 100
  voltage:3802
  temperature: 290
  technology: Li-ion

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: bug-1031008-battery-status.patch

Mark, this incorporates the changes to adb.py already checked into mozdevice in Bug 1035254 with one addition of removing the extraneous pid = None in process_exist.

Comment 2

[Mark Côté [:mcote]]

Comment on attachment 8453844 [details] [diff] [review]
bug-1031008-battery-status.patch

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

This is great, thanks.  I'm going to r+ it because it should work as is, but I have one comment for your consideration.

::: worker.py
@@ +394,5 @@
> +                    self.phone_cfg['phoneid'],
> +                    phonetest.PhoneTestMessage.CHARGING,
> +                    build_metadata['blddate'],
> +                    repo))
> +                time.sleep(900)

I think it might be better to put this after a job runs instead of before.  If we're reasonably sure that our tests are the only things that can trigger battery drain, then recharging after a test means that we can potentially use idle time, whereas if we run it before, we'll always end up delaying a job if the battery is low.

Comment 3

[Bob Clary [:bc] (inactive)]

Good point. I think the original idea was to make sure the battery was charged before the test, but I don't think that is that big of a concern and using potential idle time for charging is a good use. In practical terms, the batteries only appear to drain quickly when doing a large number of builds, particularly when retesting ranges where we already have some results since we can do installs then find we already have results and bail to the next job. So in high drain scenarios there won't be much idle time anyway. But I agree and will make the change. Thanks!

Comment 4

[Bob Clary [:bc] (inactive)]

Hmmm. Looking at how I would do this means wrapping the call to run_tests in a try...finally and placing the battery check and delay in the finally clause. I need to do that in order to handle the case where an uncaught exception might terminate the test (such as an ADBTimeout). We wouldn't want a recurring exception to prevent the battery from recharging. But if I do that, it could delay the reporting of the error for several hours while the device recharges.

Comment 5

[Bob Clary [:bc] (inactive)]

I went ahead and pushed the original patch but will leave this open for a follow up to better use idle time for charging. I wanted to get the b2g-inbound stuff landed sooner than later.

https://github.com/mozilla/autophone/commit/50538162ab3962706b98a0e9db0606ffce977009

Comment 6

[Mark Côté [:mcote]]

(In reply to Bob Clary [:bc:] from comment #4)
> Hmmm. Looking at how I would do this means wrapping the call to run_tests in
> a try...finally and placing the battery check and delay in the finally
> clause. I need to do that in order to handle the case where an uncaught
> exception might terminate the test (such as an ADBTimeout). We wouldn't want
> a recurring exception to prevent the battery from recharging. But if I do
> that, it could delay the reporting of the error for several hours while the
> device recharges.

Charging at the end doesn't really change that, does it?  If we charge after every test, and idle periods don't drain the battery, then the battery should be roughly full at the beginning of each test run (at least above the min value), as it would be if we charged before the test.  The only problem then would be if a single test took the battery from roughly full to absolutely zero, but that could happen even if we kept the charging section at the top of the test.

Comment 7

[Mark Côté [:mcote]]

Regardless, yeah, this can be done (if we decide to) as a follow-up.

Comment 8

[Bob Clary [:bc] (inactive)]

Mark and I talked about this a bit and decided that the idle phone would be charging anyway and that making sure uncaught exceptions in the test do not cause an 'infinite restart' which would drain the battery is also important. I'll go ahead and mark this fixed.