Closed Bug 705175 Opened 9 years ago Closed 9 years ago
App Root is broken
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'
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.
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
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.
Try run looks okay: https://tbpl.mozilla.org/?tree=Try&rev=5f6a80b91792
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.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.