1011112 - Autophone - adb- uninstall_app should not raise ADBError if the app is not installed

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: uninstall.patch

I didn't catch this in testing until I reset a phone since fennec was already installed on my devices. This adds a method is_app_installed to check if the app is installed before trying to uninstall it. If the app isn't installed, uninstall_app will silently succeed. You may want us to log it, but I'm not sure it is necessary.

I also caught a typo in uninstall_app's docstring and a problem in worker.py where the attempt loop wasn't exited if the install succeeded.

Comment 1

[Mark Côté [:mcote]]

Comment on attachment 8423267 [details] [diff] [review]
uninstall.patch

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

r+ by inspection, with a couple nits.

::: adb.py
@@ +1023,4 @@
>  
>      def uninstall_app(self, app_name, timeout=None):
>          """
> +        UnInstalls an app on the device.

Should be "Uninstalls", no?

@@ +1038,2 @@
>          """
> +        if self.is_app_installed(app_name):

I'm thinking that 'timeout' should be passed here, since it's passed to the next command.

This raises an interesting point that the total time for this function to complete could actually be more than 'timeout', if both commands took almost the full timeout to complete.  Wonder how many other functions work like this.  Probably worth updating the docstrings, at least.

::: worker.py
@@ +400,4 @@
>                  self.dm.install_app(os.path.join(build_metadata['cache_build_dir'],
>                                                  'build.apk'))
>                  success = True
> +                break

Good catch.  I would actually prefer if those two lines were outside the try/except, since they can't generate ADBErrors, but it's not a big deal, for the same reason.

Comment 2

[Bob Clary [:bc] (inactive)]

(In reply to Mark Côté ( :mcote ) from comment #1)
> Comment on attachment 8423267 [details] [diff] [review]
> uninstall.patch
> 
> Review of attachment 8423267 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ by inspection, with a couple nits.
> 
> ::: adb.py
> @@ +1023,4 @@
> >  
> >      def uninstall_app(self, app_name, timeout=None):
> >          """
> > +        UnInstalls an app on the device.
> 
> Should be "Uninstalls", no?
> 

Sure.

> @@ +1038,2 @@
> >          """
> > +        if self.is_app_installed(app_name):
> 
> I'm thinking that 'timeout' should be passed here, since it's passed to the
> next command.
> 

Yeah, I actually caught that but forgot to refresh the patch. :-/

> This raises an interesting point that the total time for this function to
> complete could actually be more than 'timeout', if both commands took almost
> the full timeout to complete.  Wonder how many other functions work like
> this.  Probably worth updating the docstrings, at least.
> 

Yeah, the timeout isn't the total time for a function but for any adb process that might be called so the total could be much higher for a function that makes many calls. I'll try to think of a good explanation that isn't too confusing.

> ::: worker.py
> @@ +400,4 @@
> >                  self.dm.install_app(os.path.join(build_metadata['cache_build_dir'],
> >                                                  'build.apk'))
> >                  success = True
> > +                break
> 
> Good catch.  I would actually prefer if those two lines were outside the
> try/except, since they can't generate ADBErrors, but it's not a big deal,
> for the same reason.

I don't follow. install_app can raise ADBError and we don't want success = True in that case.

Comment 3

[Bob Clary [:bc] (inactive)]

(In reply to Bob Clary [:bc:] from comment #2)

> > > +        if self.is_app_installed(app_name):
> > 
> > I'm thinking that 'timeout' should be passed here, since it's passed to the
> > next command.
> > 
> 
> Yeah, I actually caught that but forgot to refresh the patch. :-/

Actually no, I caught a different one. Thanks for catching me on this.

Comment 4

[Bob Clary [:bc] (inactive)]

Thanks. Adjusted the timeout docstring and fixed the timeout issue.

https://github.com/mozilla/autophone/commit/dc7fa5538d3d80685a1f9ff4fbf5cfdc747f9f24

Comment 5

[Mark Côté [:mcote]]

> > ::: worker.py
> > @@ +400,4 @@
> > >                  self.dm.install_app(os.path.join(build_metadata['cache_build_dir'],
> > >                                                  'build.apk'))
> > >                  success = True
> > > +                break
> > 
> > Good catch.  I would actually prefer if those two lines were outside the
> > try/except, since they can't generate ADBErrors, but it's not a big deal,
> > for the same reason.
> 
> I don't follow. install_app can raise ADBError and we don't want success =
> True in that case.

Of course, you're totally right.