810347 - Better / more flexible test root support in sutAgent

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Geoff Brown [:gbrown]]

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)]

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 ) (UTC -8)]

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]]

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]]

+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]]

Attachment: 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]]

wip patch seems to do no harm: https://tbpl.mozilla.org/?tree=Try&rev=35f6dd4788ed

Comment 7

[Geoff Brown [:gbrown]]

Attachment: 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]]

Attachment: 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)]

Comment on attachment 693618 [details] [diff] [review]
remove special treatment of /data/local/tests from devicemanagerADB

Cool!

Comment 10

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

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 ) (UTC -8)]

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]]

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]]

(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]]

Attachment: add --remoteTestRoot to test scripts

Minor updates to avoid b2g test breakage. r=jmaher, ahal

Comment 15

[Geoff Brown [:gbrown]]

https://tbpl.mozilla.org/?tree=Try&rev=921d06aaa1a3

Comment 16

[Geoff Brown [:gbrown]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43dd443e1ba9

Leave open until 2nd (device manager) patch pushed.

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/43dd443e1ba9

Comment 18

[William Lachance (:wlach)]

Pushed dmADB patch to mozbase/mozdevice:

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