Last Comment Bug 810347 - Better / more flexible test root support in sutAgent
: Better / more flexible test root support in sutAgent
Status: RESOLVED FIXED
:
Product: Testing
Classification: Components
Component: General (show other bugs)
: unspecified
: x86 Android
: -- normal (vote)
: mozilla20
Assigned To: Geoff Brown [:gbrown] (pto May 28-June 13)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-09 09:08 PST by Geoff Brown [:gbrown] (pto May 28-June 13)
Modified: 2013-01-24 14:34 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
wip (17.64 KB, patch)
2012-12-04 17:33 PST, Geoff Brown [:gbrown] (pto May 28-June 13)
no flags Details | Diff | Review
add --remoteTestRoot to test scripts (18.77 KB, patch)
2012-12-18 14:03 PST, Geoff Brown [:gbrown] (pto May 28-June 13)
jmaher: review+
ahalberstadt: review+
Details | Diff | Review
remove special treatment of /data/local/tests from devicemanagerADB (1.36 KB, patch)
2012-12-18 14:07 PST, Geoff Brown [:gbrown] (pto May 28-June 13)
wlachance: review+
jmaher: feedback+
Details | Diff | Review
add --remoteTestRoot to test scripts (18.78 KB, patch)
2012-12-20 10:59 PST, Geoff Brown [:gbrown] (pto May 28-June 13)
gbrown: review+
Details | Diff | Review

Description Geoff Brown [:gbrown] (pto May 28-June 13) 2012-11-09 09:08:46 PST
Remote xpcshell tests cannot be run from /mnt/sdcard because the execute permissions on the xpcshell binary cannot be changed on /mnt/sdcard. 

devicemanagerADB uses a test root of /data/local/tests if that directory exists, or /mnt/sdcard/tests otherwise: https://hg.mozilla.org/mozilla-central/file/c39596b46863/testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py#l501  This allows xpcshell tests to be run via ADB - just create /data/local/tests first and away you go.

For SUT, the test root will be set to /data/local/tests only if the SD card is not mounted, or mounted read-only: https://hg.mozilla.org/mozilla-central/file/c39596b46863/build/mobile/sutagent/android/DoCommand.java#l1331. This is different and less convenient than the ADB behavior for enabling xpcshell tests. 

(And actually, the ADB behavior is arguably problematic: Some people don't know or forget about the mkdir /data/local/tests trick, and it's easy to forget if you have a /data/local/tests directory already created.)

Some ideas:
 1. use /data/local by default
 2. allow the test to specify a preference for test root
 3. change sutAgent to use /data/local if /data/local/tests exists

Looking ahead a little, I would like a solution that enables production runs of xpcshell tests, so idea #3 is probably not the best.
Comment 1 William Lachance (:wlach) 2012-11-09 09:51:10 PST
I think I prefer idea (2), as it's the least "magical". :) For dmSUT, we may need to modify the agent to allow setting the testroot to a different value.
Comment 2 Joel Maher (:jmaher) 2012-11-12 02:27:26 PST
On the apc.io boards, we would need to run all tbpl based automation in /data/local/... as the sdcard does not mount by default.

I would like to make the dev and test environments closer together if possible, this location of test data has been a source of confusion.  The only thing I have experienced as a problem with /data/local is the permissions.
Comment 3 Geoff Brown [:gbrown] (pto May 28-June 13) 2012-11-30 20:10:13 PST
I no longer think xpcshell tests should use /data/local/tests as a test root in the default case -- see bug 817235. Still, we should look at making the test root location more flexible.
Comment 4 Andrew Halberstadt [:ahal] 2012-12-03 06:42:51 PST
+1 to this bug.

If possible I think it would be nice to also make dmSUT and dmADB behave the same. The dmADB code for determining the default device root is magical. I would like to avoid creating a "tests" dir in order to use /data/local.
Comment 5 Geoff Brown [:gbrown] (pto May 28-June 13) 2012-12-04 17:33:58 PST
Created attachment 688534 [details] [diff] [review]
wip

devicemanagerADB and devicemanagerSUT already allow for the device root to be specified. This patch updates the xpcshell/mochitest/reftest/b2g-xpcshell/b2g-mochitest/b2g-reftest scripts to support a --remoteTestRoot=... option to specify the device manager device root.

This way you can do something like:

export EXTRA_TEST_ARGS="--remoteTestRoot=/data/local/tests"
make mochitest-robotium

Needs more testing. So far it seems to work, but I keep running into file permission problems on /data/local...probably best to deal with that in other bugs.

The b2g scripts were already manipulating deviceRoot; I've updated them to support --remoteTestRoot, but I'm not sure if that's useful.

I have not (yet?) changed the devicemanager behavior; if --remoteTestRoot is not specified, everything works the same as it always has. So there are still differences between using ADB and SUT, and the existence of /data/local/tests still makes a difference (comment 4).
Comment 6 Geoff Brown [:gbrown] (pto May 28-June 13) 2012-12-05 06:29:28 PST
wip patch seems to do no harm: https://tbpl.mozilla.org/?tree=Try&rev=35f6dd4788ed
Comment 7 Geoff Brown [:gbrown] (pto May 28-June 13) 2012-12-18 14:03:43 PST
Created attachment 693616 [details] [diff] [review]
add --remoteTestRoot to test scripts

devicemanagerADB and devicemanagerSUT already allow for the device root to be specified. This patch updates the xpcshell/mochitest/reftest/b2g-xpcshell/b2g-mochitest/b2g-reftest scripts to support a --remoteTestRoot=... option to specify the device manager device root.

This way you can do something like:

export EXTRA_TEST_ARGS="--remoteTestRoot=/mnt/sdcard/mydir"
make mochitest-robotium

The b2g scripts were already manipulating deviceRoot; I have also updated them to support --remoteTestRoot.

I have (hopefully!) not changed any of the default behavior: if --remoteTestRoot is not specified, everything works as it always has.


Changing the test root to /data/local causes problems for some tests. For example, with sut, mochitest-robotium encounters permission problems on /data/local because files pushed via sut are owned by sut and have file permissions -rw------ -- they cannot be read by fennec. I would prefer to handle permission problems as follow-up bugs
Comment 8 Geoff Brown [:gbrown] (pto May 28-June 13) 2012-12-18 14:07:03 PST
Created attachment 693618 [details] [diff] [review]
remove special treatment of /data/local/tests from devicemanagerADB

With --remoteTestRoot support, there is no longer a need for the devicemanagerADB "hack" that switches the test root to /data/local if /data/local/tests exists. This patch removes the hack.
Comment 9 William Lachance (:wlach) 2012-12-18 16:25:58 PST
Comment on attachment 693618 [details] [diff] [review]
remove special treatment of /data/local/tests from devicemanagerADB

Cool!
Comment 10 Joel Maher (:jmaher) 2012-12-19 06:30:08 PST
Comment on attachment 693616 [details] [diff] [review]
add --remoteTestRoot to test scripts

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

::: layout/tools/reftest/runreftestb2g.py
@@ +109,5 @@
>                          type="string", dest="logcat_dir",
>                          help="directory to store logcat dump files")
>          defaults["logcat_dir"] = None
> +
> +        defaults["remoteTestRoot"] = "/data/local/tests"

why is this different from that of remotereftest.py?
Comment 11 Joel Maher (:jmaher) 2012-12-19 06:31:11 PST
Comment on attachment 693618 [details] [diff] [review]
remove special treatment of /data/local/tests from devicemanagerADB

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

lgtm
Comment 12 Andrew Halberstadt [:ahal] 2012-12-19 06:54:27 PST
Comment on attachment 693616 [details] [diff] [review]
add --remoteTestRoot to test scripts

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

Thanks for doing this!

Unfortunate that we call it "testRoot" in the harness code and "deviceRoot" in the devicemanager code. But I know these are conventions that have been around for awhile and probably aren't worth changing.
Comment 13 Geoff Brown [:gbrown] (pto May 28-June 13) 2012-12-19 08:57:51 PST
(In reply to Joel Maher (:jmaher) from comment #10)
> Comment on attachment 693616 [details] [diff] [review]
> add --remoteTestRoot to test scripts
> 
> Review of attachment 693616 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/tools/reftest/runreftestb2g.py
> @@ +109,5 @@
> >                          type="string", dest="logcat_dir",
> >                          help="directory to store logcat dump files")
> >          defaults["logcat_dir"] = None
> > +
> > +        defaults["remoteTestRoot"] = "/data/local/tests"
> 
> why is this different from that of remotereftest.py?

Because runreftestb2g.py was previously using /data/local/tests as its test root:

-        self.testDir = '/data/local/tests'
 
-    # Create /data/local/tests, to force its use by DeviceManagerADB;
-    # B2G won't run correctly with the profile installed to /mnt/sdcard.
-    dm.mkDirs(reftest.testDir)
 
(then, since /data/local/tests exists, dmADB will use that as test root).

...just trying not to change the default behavior.
Comment 14 Geoff Brown [:gbrown] (pto May 28-June 13) 2012-12-20 10:59:21 PST
Created attachment 694480 [details] [diff] [review]
add --remoteTestRoot to test scripts

Minor updates to avoid b2g test breakage. r=jmaher, ahal
Comment 15 Geoff Brown [:gbrown] (pto May 28-June 13) 2012-12-20 10:59:55 PST
https://tbpl.mozilla.org/?tree=Try&rev=921d06aaa1a3
Comment 16 Geoff Brown [:gbrown] (pto May 28-June 13) 2012-12-20 11:04:43 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/43dd443e1ba9

Leave open until 2nd (device manager) patch pushed.
Comment 18 William Lachance (:wlach) 2013-01-24 12:30:12 PST
Pushed dmADB patch to mozbase/mozdevice:

https://github.com/mozilla/mozbase/commit/160a573669ccb96aaccbd5b9985ea58f0ea68227

Note You need to log in before you can comment on or make changes to this bug.