1245900 - Autophone - adb_android.py - improve is_device_ready

Status: RESOLVED WONTFIX

(Testing Graveyard :: Autophone, defect)

Description

[Bob Clary [:bc] (inactive)]

I believe I can use a combination of init.svc.bootanim, sys.boot_completed, dev.bootcomplete properties to improve the reliability of the detection when I device is ready for testing. I believe this will help reduce wait times after fennec installations and other reboots. I'll develop this against Autophone's adb*.py, but will be porting it to mozdevice's before long.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: job-elapsed.txt

I ran two tests. "Before" was master without any changes to is_device_ready while "After" is with the changes to is_device_ready in adb_android.py that I will attach in a moment.

There was approximately a 2% improvement in job times for the Samsung Galaxy S III and perhaps 2.5% improvement for Nexus 6P. The Nexus Ones showed no real change.

You can see that nexus-6p-5 is much slower than its counterparts. I'll try to investigate and possibly reflash it to factory and re-root it to see if this can be fixed.

Comment 2

[Bob Clary [:bc] (inactive)]

PS. The "After" test was with 

device_ready_retry_attempts = 12
device_ready_retry_wait = 15

which would check slightly more often than the default of 

device_ready_retry_attempts = 3
device_ready_retry_wait = 20

Comment 3

[Bob Clary [:bc] (inactive)]

Attachment: bug-1245900-adb_android-v1.patch

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: bug-1245900-autophone-v1.patch

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8716451 [details] [diff] [review]
bug-1245900-adb_android-v1.patch

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

where I commented about boot_complete assignment could use clarification or fixing.

::: adb_android.py
@@ +223,5 @@
> +                            # and sys report boot completed, then
> +                            # treat as finished booting.
> +                            boot_completed = True
> +                        elif boot_anim_complete == 'stopped':
> +                            boot_completed = True

could we do;
if not boot_anim_complete or boot_anim_complete == 'stopped':
    boot_completed = True

or did we mix up the boot_completed assignments?

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8716452 [details] [diff] [review]
bug-1245900-autophone-v1.patch

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

just one question.

::: autophone.py
@@ +786,5 @@
> +            if cfg.has_option(device_name, 'device_ready_retry_wait'):
> +                device_ready_retry_wait = cfg.getint(device_name,
> +                                                     'device_ready_retry_wait')
> +            else:
> +                device_ready_retry_wait = self.options.device_ready_retry_wait

in this case the data in the config will always hold true even if something is specified on the commandline?

Comment 7

[Bob Clary [:bc] (inactive)]

(In reply to Joel Maher (:jmaher) from comment #5)
> Comment on attachment 8716451 [details] [diff] [review]
> bug-1245900-adb_android-v1.patch
> 
> Review of attachment 8716451 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> where I commented about boot_complete assignment could use clarification or
> fixing.
> 
> ::: adb_android.py
> @@ +223,5 @@
> > +                            # and sys report boot completed, then
> > +                            # treat as finished booting.
> > +                            boot_completed = True
> > +                        elif boot_anim_complete == 'stopped':
> > +                            boot_completed = True
> 
> could we do;
> if not boot_anim_complete or boot_anim_complete == 'stopped':
>     boot_completed = True
> 
> or did we mix up the boot_completed assignments?

No, that would work. We just lose the ability to explicitly comment on that one condition where we have the sys.boot_completed and dev.bootcomplete but there is no init.svc.bootanim value. We could move the comment to that single if and add additional prose, but it wouldn't help readability I don't think. It seemed more readable breaking out the distinct possibilities to me, but I'm ok with changing it.

(In reply to Joel Maher (:jmaher) from comment #6)
> Comment on attachment 8716452 [details] [diff] [review]
> bug-1245900-autophone-v1.patch
> 
> Review of attachment 8716452 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> just one question.
> 
> ::: autophone.py
> @@ +786,5 @@
> > +            if cfg.has_option(device_name, 'device_ready_retry_wait'):
> > +                device_ready_retry_wait = cfg.getint(device_name,
> > +                                                     'device_ready_retry_wait')
> > +            else:
> > +                device_ready_retry_wait = self.options.device_ready_retry_wait
> 
> in this case the data in the config will always hold true even if something
> is specified on the commandline?

Yes. We want to be able to override the default or command line option value per device otherwise we wouldn't be able to specify a global default for most devices with override values for special cases like emulators.

Comment 8

[Bob Clary [:bc] (inactive)]

Attachment: bug-1245900-adb_android-v2.patch

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8716572 [details] [diff] [review]
bug-1245900-adb_android-v2.patch

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

thanks!

Comment 10

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/autophone/commit/b17576ce9855fca7ba9ccc6e742a24343dba30b6
https://github.com/mozilla/autophone/commit/3eef7b7feec91614b096bd72123a564dd64dc45e

Comment 11

[Bob Clary [:bc] (inactive)]

This caused errors with SELinux on Nexus 5 devices:

Phone nexus-5-5 requires intervention:

Attempt: 3, SELinux is not permissive

I'm reverting these changes and restarting the servers.

https://github.com/mozilla/autophone/commit/88ef91b33ff8f86d9d34b428b8601bd4a01113fd
https://github.com/mozilla/autophone/commit/7385abcfb947cbc0f4eeab7e935287bad27de1f9

Comment 12

[Bob Clary [:bc] (inactive)]

Autophone is going away. Resolving these to wontfix.