Closed Bug 936782 Opened 8 years ago Closed 8 years ago

devicemanagerSUT.installApp - check for success rather than failure

Categories

(Testing :: Mozbase, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bc, Assigned: bc)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #829722 - Flags: review?(wlachance)
Attachment #829722 - Flags: feedback?(mcote)
Attached patch patch v2Splinter Review
Attachment #829722 - Attachment is obsolete: true
Attachment #829722 - Flags: review?(wlachance)
Attachment #829722 - Flags: feedback?(mcote)
Attachment #829790 - Flags: review?(wlachance)
Attachment #829790 - Flags: feedback?(mcote)
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.
Flags: needinfo?(bugspam.Callek)
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)
Attachment #829790 - Flags: review?(wlachance) → review+
much better. I'll test as soon as I can disrupt a running test.
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).
Attachment #829790 - Flags: feedback?(mcote) → feedback+
(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?
Flags: needinfo?(bugspam.Callek) → needinfo?(jmaher)
Attached patch patch v3Splinter Review
(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 <gbrown@mozilla.com>
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 <ctalbert@mozilla.com>
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.
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.
Flags: needinfo?(jmaher)
https://github.com/mozilla/mozbase/commit/10d1050395f2a73fdf3cd04ec5acc34111e1ff5e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.