705175 - devicemanager getAppRoot is broken

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Geoff Brown [:gbrown]]

make xpcshell-tests-remote uses devicemanager.getAppRoot(). This appears to be the only client of getAppRoot()...allowing for a sketchy implementation! 

There is no implementation of getAppRoot in devicemanagerSUT, so the base class implementation, devicemanager.getAppRoot() is used when invoked. 

devicemanager.getAppRoot() contains a typo which causes a Python AttributeError.

There is an (unused) SUT agent handler for the GETAPPROOT command: http://mxr.mozilla.org/mozilla-central/source/build/mobile/sutagent/android/DoCommand.java#305

I think devicemanagerSUT should implement getAppRoot() to use the GETAPPROOT command, and the base class implementation should be removed.

----

Traceback (most recent call last):
  File "/home/mozdev/src/config/pythonpath.py", line 52, in <module>
    main(sys.argv[1:])
  File "/home/mozdev/src/config/pythonpath.py", line 44, in main
    execfile(script, frozenglobals)
  File "/home/mozdev/src/testing/xpcshell/remotexpcshelltests.py", line 346, in <module>
    main()
  File "/home/mozdev/src/testing/xpcshell/remotexpcshelltests.py", line 341, in main
    **options.__dict__):
  File "/home/mozdev/src/testing/xpcshell/runxpcshelltests.py", line 511, in runTests
    stdout=pStdout, stderr=pStderr, env=self.env, cwd=testdir)
  File "/home/mozdev/src/testing/xpcshell/remotexpcshelltests.py", line 206, in launchProcess
    if (self.device.getAppRoot()):
  File "/home/mozdev/src/build/mobile/devicemanager.py", line 357, in getAppRoot
    elif (self.dirExsts('/data/data/org.mozilla.fennec')):
AttributeError: DeviceManagerSUT instance has no attribute 'dirExsts'

Comment 1

[Geoff Brown [:gbrown]]

xpcshell-tests-remote via adb is using getAppRoot to get the "application data" directory, to set GRE_HOME/sGREDir the same way that Fennec does. The adb getAppRoot typically returns /data/data/org.mozilla.fennec_<user>, and xpcshell seems to work well with that.
 
The existing GETAPPROOT command handler returns something rather different: the path to the apk, eg, /data/app/org.mozilla.fennec_<user>-1.apk...so a minor update to the SUT agent is also required.

Comment 2

[Geoff Brown [:gbrown]]

Attachment: patch to update SUT getAppRoot() to work with xpcshell-tests-remote

I am a little uncomfortable with this patch because:
 - it migrates the meaning of getAppRoot a little
 - package name determination seems brittle / error-prone
On the other hand:
 - only getAppRoot is affected, and getAppRoot is only used by xpcshell-test-remote, so it shouldn't cause too much trouble
 - the patch works for me
 - I cannot think of a better way to make this work.

Comment 3

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

Comment on attachment 577773 [details] [diff] [review]
patch to update SUT getAppRoot() to work with xpcshell-tests-remote

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

in general this patch seems good.  Can we make this work for the other branches.

::: build/mobile/devicemanagerSUT.py
@@ +861,5 @@
> +  def getAppRoot(self):
> +    if os.getenv('USER'):
> +      packageName = 'org.mozilla.fennec_' + os.getenv('USER')
> +    else:
> +      packageName = 'org.mozilla.fennec'

how will this work for org.mozilla.fennec_aurora?  I know the 'getapproot org.mozilla.fennec_aurora' command works fine in sutagent.

Comment 4

[Geoff Brown [:gbrown]]

(In reply to Joel Maher (:jmaher) from comment #3)
> how will this work for org.mozilla.fennec_aurora?  I know the 'getapproot
> org.mozilla.fennec_aurora' command works fine in sutagent.

Right, the sutagent will provide the correct directory for a given package name -- the hard part is getting the package name. This code will handle org.mozilla.fennec_<user> and org.mozilla.fennec, but not aurora or other variants.

Comment 5

[Geoff Brown [:gbrown]]

Attachment: patch to update SUT getAppRoot() to work with xpcshell-tests-remote

This is a more complex change, but allows for any package name:
 - change dm.getAppRoot to take the package name as a required parameter
 - change callers of dm.getAppRoot (remotexpcshelltests.py) to pass package name
 - in remotexpcshelltests.py, determine the package name by extracting "package-name.txt" from the APK

Comment 6

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

Comment on attachment 581548 [details] [diff] [review]
patch to update SUT getAppRoot() to work with xpcshell-tests-remote

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

r=me with the question/nit resolved.

::: build/mobile/devicemanagerADB.py
@@ +427,5 @@
>        return devroot + '/fennec'
>      elif (self.dirExists(devroot + '/firefox')):
>        return devroot + '/firefox'
> +    elif (packageName and self.dirExists('/data/data/' + packageName)):
> +      return '/data/data/' + packageName

why is this so different from devicemanagerSUT?  Specifically with the devroot + /fennec|/firefox.  We should remove those.  Also, should we be setting self.packageName here?  If we find that we have a successful packageName I would think we should set it.

Comment 7

[Geoff Brown [:gbrown]]

Oops - I lost track of this over the holidays.

I was being overly cautious about changing existing code -- I have removed the /fennec|/firefox cases now, and also set self.packageName as suggested. 

I re-tested (xpcshell-tests-remote and mochitest-robotium) and all looks good.

Comment 8

[Geoff Brown [:gbrown]]

Attachment: patch to update SUT getAppRoot() to work with xpcshell-tests-remote

Updated for review nits; r=jmaher.

Comment 9

[Geoff Brown [:gbrown]]

Try run looks okay: https://tbpl.mozilla.org/?tree=Try&rev=5f6a80b91792

Comment 10

[Ed Morley [:emorley]]

jmaher landed this, but his push was backed out for failures (presuming not due to this bug, given comment 9), so this still needs landing.

Comment 11

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

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33359378d8f8

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/33359378d8f8