936782 - devicemanagerSUT.installApp - check for success rather than failure

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: patch v1

As described in bug 936594, SUTAgentAndroid.InstallApp can fail in a variety of ways that do not involve the string 'Failure' occurring in the returned output. To make devicemanagerSUT.installApp more robust about detecting failures we should test for a successful return value from SUTAgentAndroid.InstallApp.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: patch v2

Comment 2

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

we would really benefit from this in automation, suttools and the mozharness scripts should use this as well.

Callek could help figure out what to do.

Comment 3

[William Lachance (:wlach)]

Comment on attachment 829790 [details] [diff] [review]
patch v2

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

I think the idea here is right however I think the implementation could be simplified. Please consider my alternate solution. :)

::: mozdevice/mozdevice/devicemanagerSUT.py
@@ +819,5 @@
>          data = self._runCmds([{ 'cmd': cmd }])
>  
> +        success = False
> +        for line in data.split('\n'):
> +            if ('installation complete [0]' in line):

Nit: Please no parentheses in conditionals in python.

@@ +822,5 @@
> +        for line in data.split('\n'):
> +            if ('installation complete [0]' in line):
> +                success = True
> +                break
> +        if (not success):

Same.

@@ +823,5 @@
> +            if ('installation complete [0]' in line):
> +                success = True
> +                break
> +        if (not success):
> +            raise DMError("Remove Device Error: Error installing app. Error message: %s" % data)

We could probably just replace this whole thing with something like:

if 'installation complete [0]' not in data:
    raise DMError("Remove Device Error: Error installing app. Error message: %s" % data)

Comment 4

[Bob Clary [:bc] (inactive)]

much better. I'll test as soon as I can disrupt a running test.

Comment 5

[Mark Côté [:mcote]]

Comment on attachment 829790 [details] [diff] [review]
patch v2

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

Agree with wlach on the simplification (and removal of extraneous parentheses).

Comment 6

[Justin Wood (:Callek)]

(In reply to Joel Maher (:jmaher) from comment #2)
> we would really benefit from this in automation, suttools and the mozharness
> scripts should use this as well.
> 
> Callek could help figure out what to do.

>if ('installation complete [0]' in line):

Is that output in the line in SUTAgent >= 1.17

And is the `f = re.compile('Failure')` still a decent aproximation in the current SUTAgent version?

aka: do we have a chance of regressing anything if we backport this in all devicemanager versions we currently use rather than taking a full snapshot?

Comment 7

[Bob Clary [:bc] (inactive)]

Attachment: patch v3

Comment 8

[Bob Clary [:bc] (inactive)]

(In reply to Justin Wood (:Callek) from comment #6)

> 
> >if ('installation complete [0]' in line):
> 
> Is that output in the line in SUTAgent >= 1.17

1.17 was created:

changeset:   130106:ec5be6e1bf45
user:        Geoff Brown <[e-mail]>
date:        Fri Apr 26 12:44:07 2013 -0600
summary:     Bug 865944 - Add activity command to sutagent and increase ver to 1.17; r=jmaher DONTBUILD

The output was introduced in the first version if I'm reading my hg logs correctly:

changeset:   43916:f8e71992be6b
user:        Clint Talbert <[e-mail]>
date:        Mon Jun 21 14:26:56 2010 -0700
summary:     Bug 567945 Import of sutagent code for Android


> 
> And is the `f = re.compile('Failure')` still a decent aproximation in the
> current SUTAgent version?
> 

I'm not sure what you mean here. I'll ping you on irc.

Comment 9

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

Callek- the success code should be the same on installation- sutagent 1.20 appears to be fine for installation changes (I assume everything else as well).  Regarding devicemanager changes, if we start picking stuff randomly it could lead to confusion. This is more isolated and would be safe, although I would prefer to use a real devicemanager package, preferably the latest.

Comment 10

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/mozbase/commit/10d1050395f2a73fdf3cd04ec5acc34111e1ff5e