Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Better / more flexible test root support in sutAgent

RESOLVED FIXED in mozilla20

Status

Testing
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gbrown, Assigned: gbrown)

Tracking

unspecified
mozilla20
x86
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.
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.
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.
Blocks: 807201
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.
+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.
Assignee: nobody → gbrown
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).
wip patch seems to do no harm: https://tbpl.mozilla.org/?tree=Try&rev=35f6dd4788ed
No longer blocks: 807201
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
Attachment #688534 - Attachment is obsolete: true
Attachment #693616 - Flags: review?(jmaher)
Attachment #693616 - Flags: review?(ahalberstadt)
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.
Attachment #693618 - Flags: review?(wlachance)
Attachment #693618 - Flags: feedback?(jmaher)
Comment on attachment 693618 [details] [diff] [review]
remove special treatment of /data/local/tests from devicemanagerADB

Cool!
Attachment #693618 - Flags: review?(wlachance) → review+
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?
Attachment #693616 - Flags: review?(jmaher) → review+
Comment on attachment 693618 [details] [diff] [review]
remove special treatment of /data/local/tests from devicemanagerADB

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

lgtm
Attachment #693618 - Flags: feedback?(jmaher) → feedback+
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.
Attachment #693616 - Flags: review?(ahalberstadt) → review+
(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.
Created attachment 694480 [details] [diff] [review]
add --remoteTestRoot to test scripts

Minor updates to avoid b2g test breakage. r=jmaher, ahal
Attachment #693616 - Attachment is obsolete: true
Attachment #694480 - Flags: review+
https://tbpl.mozilla.org/?tree=Try&rev=921d06aaa1a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/43dd443e1ba9

Leave open until 2nd (device manager) patch pushed.
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/43dd443e1ba9
Pushed dmADB patch to mozbase/mozdevice:

https://github.com/mozilla/mozbase/commit/160a573669ccb96aaccbd5b9985ea58f0ea68227
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.