1216690 - Autophone - audit and pass root to adb when function contains a root argument.

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, defect)

Description

[Bob Clary [:bc] (inactive)]

I am still seeing failures due to the lack of consistency with passing root to adb methods. For example the webappstartup tests in:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=500dcb1f87f4&exclusion_profile=false&filter-searchStr=autophone

Find all functions with a default root= argument and make sure that all calls to self.dm.<method> also passes root=root.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: bug-1216690-pass-root-v1.patch

test run:

https://treeherder.allizom.org/#/jobs?repo=mozilla-inbound&revision=6dc0a7e6f791&exclusion_profile=false&filter-searchStr=autophone

Comment 2

[Geoff Brown [:gbrown]]

Comment on attachment 8678040 [details] [diff] [review]
bug-1216690-pass-root-v1.patch

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

This looks fine.

There are some additional instances that are missed - intentionally? https://github.com/mozilla/autophone/blob/master/worker.py#L577

I hate to see parameters passed around so much. Did you consider moving the issue into adb.py? You could introduce an always_root or default_root parameter, maybe?

Comment 3

[Bob Clary [:bc] (inactive)]

(In reply to Geoff Brown [:gbrown] from comment #2)
> Comment on attachment 8678040 [details] [diff] [review]
> bug-1216690-pass-root-v1.patch
> 
> Review of attachment 8678040 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine.
> 
> There are some additional instances that are missed - intentionally?
> https://github.com/mozilla/autophone/blob/master/worker.py#L577
> 

https://github.com/mozilla/autophone/blob/master/worker.py#L553 
 def install_build(self, job):

doesn't accept root as an argument.

The shell output pm list packages https://github.com/mozilla/autophone/blob/master/worker.py#L577
 self.dm.shell_output("pm list package org.mozilla").split()

isn't affected by whether the user is root or not as far as I understand it.

The idea in this bug was to find every method that did specify root as a default parameter and to make sure it passed that parameter to any adb methods that it called that took root as a parameter. If there are any methods that specify a default root parameter and which do not pass root to the adb methods it calls, then I missed something.


> I hate to see parameters passed around so much. Did you consider moving the
> issue into adb.py? You could introduce an always_root or default_root
> parameter, maybe?

I'm not sure how that would work. I keep hoping that we will be able to write a framework that doesn't depend on root and where we could just specify root=False at the top level of the call chain when we can get away without any root at all.

If you would like to pursue the idea, lets talk about how it might work.

Comment 4

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/autophone/commit/03a4d05cbc9a79f5d0cadf5242b0c1af1e20f2ac
deployed 2015-10-25 15:35:31,329

Comment 5

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #3)
> (In reply to Geoff Brown [:gbrown] from comment #2)
> > There are some additional instances that are missed - intentionally?
> > https://github.com/mozilla/autophone/blob/master/worker.py#L577
> The idea in this bug was to find every method that did specify root as a
> default parameter and to make sure it passed that parameter to any adb
> methods that it called that took root as a parameter. If there are any
> methods that specify a default root parameter and which do not pass root to
> the adb methods it calls, then I missed something.

I do not think that you missed anything. 

I have not followed root-related failures closely so am not clear on the scope of the problem. I was thinking that if you keep finding new cases of commands that need root, you might want to run every command with root. But certainly, there are commands that should never need root, and there is some risk and cost involved in using root, so it's probably best not to use root globally without cause.

> > I hate to see parameters passed around so much. Did you consider moving the
> > issue into adb.py? You could introduce an always_root or default_root
> > parameter, maybe?
> 
> I'm not sure how that would work.

It probably wouldn't! I was still thinking of global root, which would be easy-ish in adb.py, but overall, not a good idea.

Comment 6

[Bob Clary [:bc] (inactive)]

Ok. I was just negligent before in finding all of the potential cases which is why this additional bug/patch was required. Sounds good. Thanks!