devicemanager getAppRoot is broken

RESOLVED FIXED in mozilla12

Status

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: gbrown, Assigned: gbrown)

Tracking

unspecified
mozilla12
x86
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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'
Assignee: nobody → 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.
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.
Attachment #577773 - Flags: review?(jmaher)
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.
(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.
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
Attachment #577773 - Attachment is obsolete: true
Attachment #577773 - Flags: review?(jmaher)
Attachment #581548 - Flags: review?(jmaher)
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.
Attachment #581548 - Flags: review?(jmaher) → review+
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.
Updated for review nits; r=jmaher.
Attachment #581548 - Attachment is obsolete: true
Attachment #586492 - Flags: review+
Keywords: checkin-needed
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.
https://hg.mozilla.org/mozilla-central/rev/33359378d8f8
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.